Bug 202000 – Web Inspector: Remove BranchManager in favor of just using currentRevision

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 379158 [details]
[PATCH] Proposed Fix

[Devin Rousso]

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.

[Devin Rousso]

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).

[Joseph Pecoraro]

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.

[WebKit Commit Bot]

Comment on attachment 379158 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 379158

Committed r250149: <https://trac.webkit.org/changeset/250149>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55570883>