StyleSheetsActor should use the parent tabActor to retrieve the list of windows and react to new/removed windows

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Developer Tools: Style Editor
P2
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: pbro, Assigned: tromey)

Tracking

unspecified
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
The StyleSheetsActor currently walks the DOM to retrieve all windows in the current window in order to get all stylesheets.

It does this by searching for nested iframe, browser, frame elements in the current window and recursing:
https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/stylesheets.js#111

Instead, it could use the parent tabActor's windows getter which gives the same list without having to query the DOM or recursing.

On top of this, the tabActor also emits 'window-ready' and 'window-destroyed' events that the StyleSheetsActor should use to react to when windows are added or removed and therefore update the list of style-sheets displayed in the style-editor.
(Reporter)

Comment 1

2 years ago
It'd be great to take this opportunity to also change the way the style-editor initializes. For now, it does one call to the actor that retrieves all stylesheets, and this call only returns when all windows have loaded. We should instead try to return something quickly and then add stylesheets with events.
I have seen occurrences of Bug 1222991 in several websites : youtube, engadget, techcrunch, yahoo (everytime with about:blank urls). 

Even when no about:blank URL blocks the load of the stylesheets, simply waiting for all frames to be loaded is bad for the user experience.
Assignee: nobody → jdescottes
(Reporter)

Comment 3

2 years ago
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2

Comment 4

a year ago
I am experiencing this same issue with SharePoint 2013 Publishing sites that contain lists or libraries.  Here is a page on our site that shows the problem: http://info2.scdot.org/StormReports/Pages/default.aspx

This type of web part generates iframes.  This is a similar page but with a different web part that does not generate iframes, so it works: http://info2.scdot.org/ED/Pages/engineering_directives.aspx.  I have added this comment to the similar bugs also.

Comment 5

a year ago
I can get the style editor working by using the inspector to remove the iframe nodes, closing the developer tools, reopen, then going to style editor.

Comment 6

a year ago
Confirmed (i.e. Firefox Nightly 52.0a1 (2016-11-06)).

Tested with:
Menu: "Tools"-"Options"-"Privacy"-"History"-"Clear history when Nightly closes"(check-on)

e.g. also https://www.seznam.cz/

Ad http://hg.mozilla.org/mozilla-central/file/673b5327afe1/devtools/server/actors/stylesheets.js#l872
IMHO/AFAIK:
After: doc.readyState == "uninitialized"
...this event will never/sometimes happen (for some reason):
yield listenOnce(win, "DOMContentLoaded", true);
Duplicate of this bug: 1327344
(Assignee)

Updated

5 months ago
Blocks: 1388497
Sorry for keeping the bug without touching it, unassigning.
Assignee: jdescottes → nobody
(Assignee)

Updated

5 months ago
See Also: → bug 1337330
(Assignee)

Updated

5 months ago
Assignee: nobody → ttromey
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1222991
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1213713
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1337330
Comment hidden (mozreview-request)
(Assignee)

Comment 14

5 months ago
Comment on attachment 8898782 [details]
Bug 1224558 - change style editor to notice stylesheet additions;

This got obsoleted in the meantime by the event refactoring.
Attachment #8898782 - Flags: review?(gl)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

4 months ago
mozreview-review
Comment on attachment 8898782 [details]
Bug 1224558 - change style editor to notice stylesheet additions;

https://reviewboard.mozilla.org/r/170172/#review176604

::: devtools/client/styleeditor/StyleEditorUI.jsm:94
(Diff revision 1)
>  
>    this._prefObserver = new PrefObserver("devtools.styleeditor.");
>    this._prefObserver.on(PREF_ORIG_SOURCES, this._onNewDocument);
>    this._prefObserver.on(PREF_MEDIA_SIDEBAR, this._onMediaPrefChanged);
> +
> +  this._addStyleSheet = this._addStyleSheet.bind(this);

Move this up to the group above where we do all the bindings as well.

::: devtools/client/styleeditor/StyleEditorUI.jsm:368
(Diff revision 1)
> -  _addStyleSheetEditor: Task.async(function* (styleSheet, file, isNew) {
> +  _addStyleSheetEditor: Task.async(function* (styleSheet, isNew) {
>      // recall location of saved file for this sheet after page reload
> +    let file = null;
>      let identifier = this.getStyleSheetIdentifier(styleSheet);
>      let savedFile = this.savedLocations[identifier];
>      if (savedFile && !file) {

This could just be if (savedFile) {}

::: devtools/client/styleeditor/StyleEditorUI.jsm:1034
(Diff revision 1)
>    _jumpToLocation: function (location) {
>      let source = location.styleSheet || location.source;
>      this.selectStyleSheet(source, location.line - 1, location.column - 1);
>    },
>  
>    destroy: function () {

We should clear the map in this._seenSheets, and set both this._seenSheets and this._supressAdd to null.

::: devtools/server/actors/stylesheets.js:825
(Diff revision 1)
>  
>      this.parentActor = tabActor;
> +
> +    this._onSheetAdded = this._onSheetAdded.bind(this);
> +    this._onWindowReady = this._onWindowReady.bind(this);
> +    this._onNewStyleSheetActor = this._onNewStyleSheetActor.bind(this);

Let's sort these bindings alphabetically and move this._onNewStyleSheetActor on top of this._onSheetAdded

::: devtools/server/actors/stylesheets.js:912
(Diff revision 1)
>    },
>  
>    /**
> +   * Event handler that is called when a new style sheet is added to
> +   * a document.
> +   * @param {Event} evt

Add a new line to separate the @param from the JSDoc to be consistent with the formating in this file.

::: devtools/client/styleeditor/StyleEditorUI.jsm:74
(Diff revision 3)
>    this._root = this._panelDoc.getElementById("style-editor-chrome");
>  
>    this.editors = [];
>    this.selectedEditor = null;
>    this.savedLocations = {};
> +  this._seenSheets = new Map();

Can you comment on what goes into the map? What is the key and what is the value.

::: devtools/client/styleeditor/StyleEditorUI.jsm:76
(Diff revision 3)
>    this.editors = [];
>    this.selectedEditor = null;
>    this.savedLocations = {};
> +  this._seenSheets = new Map();
> +
> +  // Don't add any style sheets that might arrive via events, until

Can you add to the comment which kind of events can trigger stylesheet additions.

::: devtools/client/styleeditor/StyleEditorUI.jsm:94
(Diff revision 3)
>  
>    this._prefObserver = new PrefObserver("devtools.styleeditor.");
>    this._prefObserver.on(PREF_ORIG_SOURCES, this._onNewDocument);
>    this._prefObserver.on(PREF_MEDIA_SIDEBAR, this._onMediaPrefChanged);
> +
> +  this._addStyleSheet = this._addStyleSheet.bind(this);

Move this binding up with the all the other functions that are being binded.

::: devtools/client/styleeditor/StyleEditorUI.jsm:242
(Diff revision 3)
>     *        Event name
>     * @param {StyleSheet} styleSheet
>     *        StyleSheet object for new sheet
>     */
>    _onNewDocument: function () {
> +    this._suppressAdd = true;

Add a comment here for why we supressAdd. For the most part, this is because we are already gonna be fetching stylesheets and reseting the stylesheet list, and want to make sure stylesheets aren't added during this time through events.

::: devtools/client/styleeditor/StyleEditorUI.jsm:322
(Diff revision 3)
> +   *         is enabled, then the promise resolves to null.
> +   */
> +  _addStyleSheet: function (styleSheet, isNew) {
> +    if (this._suppressAdd) {
> +      return null;
> +    }

After a new line after this if statement block.

::: devtools/client/styleeditor/StyleEditorUI.jsm:332
(Diff revision 3)
> -    if (!Services.prefs.getBoolPref(PREF_ORIG_SOURCES)) {
> +        if (!Services.prefs.getBoolPref(PREF_ORIG_SOURCES)) {
> -      return;
> +          return editor;
> -    }
> +        }
>  
> -    let sources = yield styleSheet.getOriginalSources();
> +        let sources = await styleSheet.getOriginalSources();
> -    if (sources && sources.length) {
> +        if (sources && sources.length) {

This if statement can probably use a comment to explain what is happening.

::: devtools/client/styleeditor/StyleEditorUI.jsm:368
(Diff revision 3)
> -  _addStyleSheetEditor: Task.async(function* (styleSheet, file, isNew) {
> +  _addStyleSheetEditor: Task.async(function* (styleSheet, isNew) {
>      // recall location of saved file for this sheet after page reload
> +    let file = null;
>      let identifier = this.getStyleSheetIdentifier(styleSheet);
>      let savedFile = this.savedLocations[identifier];
>      if (savedFile && !file) {

We probably can just change this to: 
if (savedFile) since !file is always true

::: devtools/client/styleeditor/test/browser_styleeditor_add_stylesheet.js:21
(Diff revision 3)
> +  // be seeing events from the initial open.
> +  let added = new Promise(resolve => {
> +    let handler = () => {
> +      if (ui.editors.length === 3) {
> +        resolve();
> +        ui.off("editor-added", handler);

Doesn't this need to happen before resolve?

::: devtools/server/actors/stylesheets.js:814
(Diff revision 3)
> +    // the sheet is enabled) ensures that the sheet is ready before we
> +    // try to make an actor for it.
> +    this.parentActor.chromeEventHandler
> +      .addEventListener("StyleSheetApplicableStateChanged", this._onSheetAdded, true);
> +
> +    this._nextStyleSheetIsNew = false;

I think we can repeat or move the comment of this variable that we mention in addStyleSheet up here.

::: devtools/server/actors/stylesheets.js:842
(Diff revision 3)
> +    this._addStyleSheets(evt.window);
> +  },
> +
> +  /**
> +   * Event handler that is called when a the tab actor emits stylesheet-added.
> +   * @param {Actor} actor

Add a new line before @param

::: devtools/server/actors/stylesheets.js:843
(Diff revision 3)
> +  },
> +
> +  /**
> +   * Event handler that is called when a the tab actor emits stylesheet-added.
> +   * @param {Actor} actor
> +   *        The new actor.

Should be explicit about the type of actor in this comment.

::: devtools/server/actors/stylesheets.js:886
(Diff revision 3)
>  
>      return true;
>    },
>  
>    /**
> +   * Event handler that is called when a new style sheet is added to

I think we should be explicit about what the event name that triggers this event handler "StyleSheetApplicableStateChanged" in the comment. This is similar to your previous event handler JSDoc.

::: devtools/server/actors/stylesheets.js:888
(Diff revision 3)
>    },
>  
>    /**
> +   * Event handler that is called when a new style sheet is added to
> +   * a document.
> +   * @param {Event} evt

Add a new line before @param.

::: devtools/server/actors/stylesheets.js:911
(Diff revision 3)
>     *         Promise that resolves to an array of StyleSheetActors
>     */
>    _addStyleSheets: function (win) {
>      return Task.spawn(function* () {
>        let doc = win.document;
> -      // readyState can be uninitialized if an iframe has just been created but
> +      doc.styleSheetChangeEventsEnabled = true;

This could use a comment about what it is doing and what it triggers.
Attachment #8898782 - Flags: review?(gl) → review+
(Assignee)

Comment 18

4 months ago
mozreview-review-reply
Comment on attachment 8898782 [details]
Bug 1224558 - change style editor to notice stylesheet additions;

https://reviewboard.mozilla.org/r/170172/#review176604

> This if statement can probably use a comment to explain what is happening.

This was pre-existing but I stuck in a comment that I think is correct.
Comment hidden (mozreview-request)
(Assignee)

Comment 20

4 months ago
Not sure what is up with that try run, but when I do it with a clean branch
(just this patch rebased onto incoming) I get:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30f250de8c6639c73270d8f8d6d0e8e785989d1d

... which is more what I'd expect.
Comment hidden (mozreview-request)

Comment 22

4 months ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/572f37089603
change style editor to notice stylesheet additions; r=gl
https://hg.mozilla.org/mozilla-central/rev/572f37089603
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Updated

4 months ago
Duplicate of this bug: 974827
You need to log in before you can comment on or make changes to this bug.