Closed Bug 1224558 Opened 9 years ago Closed 7 years ago

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

Categories

(DevTools :: Style Editor, defect, P2)

defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: pbro, Assigned: tromey)

References

Details

Attachments

(1 file)

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.
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
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
See Also: → 1213713
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.
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.
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);
Blocks: 1388497
Sorry for keeping the bug without touching it, unassigning.
Assignee: jdescottes → nobody
See Also: → 1337330
Assignee: nobody → ttromey
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 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+
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.
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.
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.