Skip to content

Instantly share code, notes, and snippets.

@juliandescottes

juliandescottes/parentStyleSheet_callsites.md

Last active September 24, 2020 17:37
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save juliandescottes/7b6719b72a3c62968f67c6278171da27 to your computer and use it in GitHub Desktop.
Save juliandescottes/7b6719b72a3c62968f67c6278171da27 to your computer and use it in GitHub Desktop.
Download ZIP
Raw
parentStyleSheet_callsites.md

First, let's look at all the call sites that don't reassign/rename anything.

StyleRuleFront::get href()

Could be replaced be returning parentStylesheet.href in the actor's form.

StyleRuleFront::get nodeHref()

Same comment as for href().

StyleRuleFront::get location()

Here we set this.parentStylesheet as the source (generic property names :'( ) of a new object. What are the call sites of this location getter? According to our coverage tools, we never hit this code in our test suite [1]. It is locally used by getOriginalLocation, but again according to our coverage tools, this is not used.

[1] https://coverage.moz.tools/#revision=latest&path=devtools/client/fronts/styles.js&view=file

StyleRuleFront::getOriginalLocation()

See previous comment, seems unused.

RuleRewriter::getDefaultIndentation()

We could add a new API on StyleRuleActor in order to get the indentation.

SelectorView::constructor() (from devtools/client/inspector/computed/computed.js)

Several uses. First it is passed to CssLogic.shortSource(sheet). Then it is used to create a generatedLocation object. For this object we set the stylesheet (resource/front) as the sheet property, and we also set href to its href || nodeHref. Finally we call sourceMapURLService.subscribeByID with either its actorID (if it's a front) or the resourceId (if it's a resource).

Regarding generatedLocation, I don't think it is used anywhere outside this file (should be renamed _generatedLocation). Locally it is used with CssLogic.shortSource (again). shortSource only relies on the href property of the provided object. I suggest we refactor the method in order to directly take a href string. This would make the callsites much more explicit. generatedLocation is also used with toolbox.viewSourceInStyleEditorByFront.

Let's explore this code path.

ViewSource::viewSourceInStyleEditorByFront()

As the name suggests, this method strongly implies "front" but here we will pass it a resource when server side resources are enabled. It should probably be updated to reflect this. Then it forwards the front/resource to viewSourceInStyleEditor.

ViewSource::viewSourceInStyleEditor()

This method accepts a stylesheetFrontOrGeneratedURL (this should also be updated to reflect that we will now have resources). If stylesheetFrontOrGeneratedURL happens to be a URL, we will call getStylesheetFrontForGeneratedURL. This method simply works by looping on the stylesheets known by the StyleEditor and returning the one that matches the URL. Since the StyleEditor stores resources, this method actually returns resources and should be renamed to getStylesheetResourceForGeneratedURL. At that point, we have a stylesheet resource (assigned to a stylesheetFront variable in this method, should be renamed). Now we call selectStyleSheet.

StyleEditorUI::selectStyleSheet()

Here we store our stylesheet resource in this._styleSheetToSelect.stylesheet. But the only thing we will use it for is to find the editor that corresponds to this stylesheet and display it. In other words, if we simply passed the resource id here, it would be the same. We could try that: update the whole code path from viewSourceInStyleEditorByFront to pass a resourceId instead of a front/resource. There is one catch with this approach, see next paragraph.

ViewSource::viewSourceInStyleEditor() (again)

If this method cannot find a stylesheet to display in the styleeditor, it falls back on gViewSourceUtils.viewSource to open the URL (which will by default open the URL in a new tab I think?). So if we change viewSourceInStyleEditorByFront to use a resourceId instead of a front/resource, we have no way of getting the href/nodeHref. We could pass the resourceId and the href/nodeHref as an object ...

That's where I think I hit a wall. We cannot just use the URL because it would fail in scenarios where we have several stylesheet instances for a single URL. We cannot just use the unique id because we need the URL for fallback scenarios. We need an object with both, so why not the resource...

Rule::get sheet() (devtools/client/inspector/rules/models/rule.js)

This getter is used in various spots. First in the title getter, with CssLogic.shortSource (so here it only needs the href).

Then in the generatedLocation getter. This getter seems to have 1 external call site, in new-rules.js (https://searchfox.org/mozilla-central/rev/89d33e1c3b0a57a9377b4815c2f4b58d933b7c32/devtools/client/inspector/rules/new-rules.js#448), otherwise it's only used within the file. It is used in subscribeToLocationChange with the sourceMap service. Here we use only the actorID from the parentStyleSheet (should be updated to actorID || resourceId I suppose?). It is also used in _getSourceText, and here we only need the href || nodeHref.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment