Remove BranchManager in favor of just using currentRevision The Branch concept never got fleshed out, and would likely be too complex for the average case. Local Overrides are simpler.
Created attachment 379158 [details] [PATCH] Proposed Fix
One thing I ran into when I tried to remove the branch manager almost a year ago was that it broke CSS editing. Just a thing to consider. I haven't looked yet to see if that was handled, but that was the main thing I ran into.
Comment on attachment 379158 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=379158&action=review r=me > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:-1016 > -localizedStrings["Show More"] = "Show More"; Where did this come from? o.0 Looks like this came from <https://trac.webkit.org/r250087>. Odd :| > Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:658 > + let revision = styleSheet.currentRevision; We could just inline this. > Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:702 > + let revision = representedObject.currentRevision; Ditto > Source/WebInspectorUI/UserInterface/Models/Branch.js:-63 > - revisionForRepresentedObject(representedObject, doNotCreateIfNeeded) One thing that's different about this vs what you have above is that each time this is called, it can potentially generate a new `WI.SourceCodeRevision`, whereas you only do it when processing the content from the backend. It does look like you've covered all the bases, especially for `WI.CSSStyleSheet`, but I wonder if this could be done at a "lower" level, like inside `WI.SourceCode.prototype.get revisionForRequestedContent`, or even removing the concept of revisions altogether (or make it `WI.CSSStyleSheet` and `WI.LocalResourceS` only).
Comment on attachment 379158 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=379158&action=review >> Source/WebInspectorUI/UserInterface/Models/Branch.js:-63 >> - revisionForRepresentedObject(representedObject, doNotCreateIfNeeded) > > One thing that's different about this vs what you have above is that each time this is called, it can potentially generate a new `WI.SourceCodeRevision`, whereas you only do it when processing the content from the backend. > > It does look like you've covered all the bases, especially for `WI.CSSStyleSheet`, but I wonder if this could be done at a "lower" level, like inside `WI.SourceCode.prototype.get revisionForRequestedContent`, or even removing the concept of revisions altogether (or make it `WI.CSSStyleSheet` and `WI.LocalResourceS` only). I've moved us to a model where a SourceCode has just two revisions: `originalRevision` and `currentRevision`. • `originalRevision` contains the original contents (typically from the backend, with the exception of LocalResource / SourceMapResource). • `currentRevision` is always the current contents in the frontend and is allowed to be updated/modified by clients. This keeps the existing but unused functionality to `revert()` a Resource back to its original contents. I played around with that (for Override Images in particular) and it works, but I'm not sure the best way to expose that in the UI yet, so I left it unused. I toyed with the idea of creating a series of revisions (original ([0]), revisions[1], ... current ([n])) but found that to be overkill and at this point we never use anything other than current contents. We'd need to have a compelling reason to go that route. I still think the original vs current has merit. A HAR Export for example would want to export the original contents and not the current contents in case of user modifications.
Comment on attachment 379158 [details] [PATCH] Proposed Fix Clearing flags on attachment: 379158 Committed r250149: <https://trac.webkit.org/changeset/250149>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55570883>