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)
DevTools
Style Editor
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.
Reporter | ||
Comment 1•9 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.
Comment 2•9 years ago
|
||
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
Updated•8 years ago
|
Comment 4•8 years 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•8 years 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•8 years 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);
Comment 7•8 years ago
|
||
I'm sorry, see also https://bugzilla.mozilla.org/show_bug.cgi?id=1222991#c11 , etc.
Comment 9•7 years ago
|
||
Sorry for keeping the bug without touching it, unassigning.
Assignee: jdescottes → nobody
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ttromey
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/572f37089603
change style editor to notice stylesheet additions; r=gl
Comment 23•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•