Closed Bug 1452143 Opened 7 years ago Closed 7 years ago

Consider disabling CSS error reporting by default

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: dev-doc-complete)

Attachments

(7 files)

Bug 1452143: Expose and honor a cssErrorReportingEnabled in the docshell.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1452143: Hook the filters and enable error reporting on demand.
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1452143: Reparse doc sheets after enabling error reporting on a docshell.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1452143: Make InspectorUtils.getAllStyleSheets handle Shadow DOM, and also optionally not return UA / User sheets.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1452143: Don't test content CSS error reporting in the browser console.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1452143: Fix webconsole tests.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1452143: Fix dom / style tests.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Error reporting is slow (it was a decent perf hit when implementing it on stylo), and the relevant errors are hidden by default for devtools. It'd be fine to not do all the error reporting stuff if we're not showing them, and maybe reparse the sheets afterwards if you toggle the CSS errors checkbox?
Patrick, would you be fine with something like comment 0? That is, making CSS error reporting, let's say, a pref (or some document-dependent bit or similar), and disable it by default? Then devtools would enable it and reparse the sheets of the document. This would also make bug 1346988 a bit simpler.
Flags: needinfo?(pbrosset)
Is there a clear API DevTools would use to trigger "reparse the sheets" without other side effects?
Flags: needinfo?(emilio)
We have InspectorUtils::ParseStyleSheet which we already use for the stylesheet editor. Would that not work? Not sure which side effects are you thinking about.
Flags: needinfo?(emilio)
Emilio and I discussed a bit. I think it's probably worth blocking parallel CSS parsing on this, since we can just disable parallel parsing if the pref is set, and then avoid writing a bunch of thread-safe error reporting machinery.
Blocks: 1346988
Overall, I feel like this is worth trying so we can obtain the perf benefit here. I would suggest some kind of per-document state that the console could read and write to achieve this. Since the CSS console filter is a UI-side toggle, it would be nice to avoid multiple parses if the user toggles the filter off and on several times. So, the console would do something like: 1. If CSS error filter is off, wait for it to toggle on, then go to 2. 2. If CSS error filter is on, check per-document state. 3. If document sheets were parsed without errors, flip that on and reparse. Switching ni? to :bgrins and :nchevobbe who are more involved with Console work so they can provide feedback as well. (In reply to Emilio Cobos Álvarez [:emilio] from comment #3) > We have InspectorUtils::ParseStyleSheet which we already use for the > stylesheet editor. > > Would that not work? Not sure which side effects are you thinking about. Okay, I suppose that also rebuilds style contexts, restyles the page, etc.? I was just thinking that for this situation, the sheets aren't actually changing, so if there was some more minimal path to "just get the errors", perhaps that's nice to have. Not a huge deal though.
Flags: needinfo?(pbrosset)
Flags: needinfo?(nchevobbe)
Flags: needinfo?(bgrinstead)
It seems like a reasonable idea to me, if we do have a convenient way of getting those messages. The process described by Ryan in Comment 5 seems the right thing to do. We should also make sure we get those messages when the CSS filter is on and the user navigates to a new URL (or reload the page).
Flags: needinfo?(nchevobbe)
Depends on: 1452916
Comment on attachment 8966544 [details] Bug 1452143: Expose and honor a cssErrorReportingEnabled in the docshell. Err, these were supposed to go in bug 1452916.
Attachment #8966544 - Attachment is obsolete: true
Attachment #8966544 - Flags: review?(bobbyholley)
Attachment #8966545 - Attachment is obsolete: true
Attachment #8966545 - Flags: review?(bobbyholley)
Hi Nicolas, Could you point me to where the devtools code for handling / filtering the messages lives? I suspect GitHub, right? The only bit I see outside of tests dealing with this is [1], which doesn't seem particularly related. I will take a look tomorrow, but if you could point me to the right place that'd be appreciated. Thanks! [1]: https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/devtools/client/shared/developer-toolbar.js#690
Flags: needinfo?(nchevobbe)
So the actual filter toggle is handled in [1]. This is a Redux reducer, where we loop through all the messages we **already** received and check if they match the current filters. Now, if we want to wait for the first "CSS filter toggle" to actually call a server method that would trigger the warning messages to be sent, I think we could: - Have a `serverCssReportingEnabled` flag property in the message state ([2]) - Create a middleware in [3] where we would check that the dispatched action is FILTER_TOGGLE for css. If it is, and state.serverCssReportingEnabled is false, then call the server function that will log send those warnings, and set serverCssReportingEnabled to true Then we would need to handle the initial state where the user has the CSS filter on. Do not hesitate to ping me here or on Slack/IRC if there is something unclear --- [1] https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/client/webconsole/reducers/messages.js#328,335,339 [2] https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/client/webconsole/reducers/messages.js#31-57 [3] https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/client/webconsole/store.js#76-78
Flags: needinfo?(nchevobbe)
On the 'server' side (i.e. chrome JS code running in the window), CSS warnings are received by the ConsoleServiceListener [0]. This is setup by the ConsoleActor [1] which gets attached when the tools are opened. [0]: https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/server/actors/webconsole/listeners.js#37 [1]: https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/server/actors/webconsole.js#576
Flags: needinfo?(bgrinstead)
We also need to consider the Browser Console case, where the actor isn't associated with a single window but instead "all" windows (including content pages although we'll be able to filter those out as of Bug 1260877). For this case we could either: (a) loop through *all* the windows (possibly also in the content process) and call InspectorUtils::ParseStyleSheet on all the sheets. We could simplify this by never showing CSS warnings for content pages in the browser console. (b) remove the button entirely and just not ever show any CSS warnings in the BC (c) allow you to press the button and don't attempt to dynamically re-populate but rather send a message to the BC saying 'restart your browser to start seeing css warnings'
I'd be generally OK with removing the messages from the BC and filtering out any content CSS warnings for the BC in the ConsoleServiceListener, but we should check on firefox-dev to see if people are using that feature (it can always be accessed from the Browser Toolbox).
(In reply to Brian Grinstead [:bgrins] from comment #14) > I'd be generally OK with removing the messages from the BC and filtering out > any content CSS warnings for the BC in the ConsoleServiceListener, but we > should check on firefox-dev to see if people are using that feature (it can > always be accessed from the Browser Toolbox). Posted this to https://groups.google.com/d/msg/firefox-dev/p91D2kjbIAw/gZhfKKMpBgAJ
(In reply to Brian Grinstead [:bgrins] from comment #15) > (In reply to Brian Grinstead [:bgrins] from comment #14) > > I'd be generally OK with removing the messages from the BC and filtering out > > any content CSS warnings for the BC in the ConsoleServiceListener, but we > > should check on firefox-dev to see if people are using that feature (it can > > always be accessed from the Browser Toolbox). > > Posted this to > https://groups.google.com/d/msg/firefox-dev/p91D2kjbIAw/gZhfKKMpBgAJ Thank you very much Brian!
Probably needs a docs mention.
Keywords: dev-doc-needed
Another option for the Browser Console I didn't think of originally (sort of combining some of the others): - Don't ever show content CSS warnings in BC - Don't bother repopulating old CSS warnings when the filter is clicked on the BC (i.e. just start getting new ones once the filter happens)
By the way, we shouldn't key the platform support for this off of the client side pref (devtools.webconsole.filter.css), since that wouldn't work for remote debugging case when the frontend is running in a different firefox process than the server (including the Browser Toolbox). So I guess we'll need to deal with this more like how we do for NetworkMonitor.saveRequestAndResponseBodies - have the client send a request that toggles a different pref on the server. We could probably even reuse the same onSetPreferences / onGetPreferences function here but add a new case for this: https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/server/actors/webconsole.js#1145-1187. Nicolas' outline in Comment 11 still holds, it's just that the 'server function' would be called via `this.webConsoleClient.setPreferences` (for example, https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/client/webconsole/new-webconsole.js#160-165).
I still believe it would be better to use a per-document state here, rather than a global pref. The advantage of per-document state is that only tabs you are inspecting can ever be affected. This ensures that even the users who _do_ enable the CSS console filter can get fast CSS parsing without error reporting for all normal tabs where DevTools are never used. One example of per-document state would be a new flag on the docshell: similar to things like `allowJavascript` and `customUserAgent`, we could add a new flag `logCSSErrors` or similar. So, something like this: 1. DevTools console checks if client-side CSS filter pref is enabled * If CSS filter is disabled, watch for it to change, and re-enter these steps. Abort for now. 2. DevTools console sends message to server-side actor to check the `logCSSErrors` flag on the tab's docshell 3. If the per-document flag is off, flip it on and trigger a reparse.
yeah, that sounds good, I wasn't aware of the customUserAgent and allowJavascript stuff in the docshell (only looked at nsIDocument :)
Assignee: nobody → emilio
Comment on attachment 8966545 [details] Bug 1452143: Hook the filters and enable error reporting on demand. https://reviewboard.mozilla.org/r/235262/#review242034 Code analysis found 5 defects in this patch: - 5 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/webconsole/store.js:179 (Diff revision 2) > + */ > +function ensureCSSErrorReportingEnabled(hud) { > + return next => (reducer, initialState, enhancer) => { > + function ensureErrorReportingEnhancer(state, action) { > + let proxy = hud ? hud.proxy : null; > + if (!proxy) Error: Expected { after 'if' condition. [eslint: curly] ::: devtools/client/webconsole/store.js:183 (Diff revision 2) > + let proxy = hud ? hud.proxy : null; > + if (!proxy) > + return reducer(state, action); > + > + state = reducer(state, action); > + if (!state.filters.css) Error: Expected { after 'if' condition. [eslint: curly] ::: devtools/client/webconsole/store.js:188 (Diff revision 2) > + if (!state.filters.css) > + return state; > + > + let cssFilterToggled = > + action.type == FILTER_TOGGLE && action.filter == "css"; > + if (cssFilterToggled || action.type == INITIALIZE) Error: Expected { after 'if' condition. [eslint: curly] ::: devtools/client/webconsole/store.js:193 (Diff revision 2) > + if (cssFilterToggled || action.type == INITIALIZE) > + proxy.target.activeTab.ensureCSSErrorReportingEnabled(); > + return state; > + } > + return next(ensureErrorReportingEnhancer, initialState, enhancer); > + } Error: Missing semicolon. [eslint: semi] ::: devtools/server/actors/tab.js:1022 (Diff revision 2) > + > + this.docShell.cssErrorReportingEnabled = true; > + // FIXME(emilio): Reparse sheets. > + return {}; > + }, > + Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines]
Comment on attachment 8967571 [details] Bug 1452143: Reparse doc sheets after enabling error reporting on a docshell. https://reviewboard.mozilla.org/r/236248/#review242038 Code analysis found 3 defects in this patch: - 3 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/server/actors/stylesheets.js:197 (Diff revision 1) > + if (!excludedProtocolsRe.test(href)) { > + // Stylesheets using other protocols should use the content principal. > + // eslint-disable-next-line mozilla/use-ownerGlobal > + // charset of referring document. > + if (sheet.ownerNode) { > + options.window = sheet.ownerNode.ownerDocument.defaultView; Error: Use .ownerglobal instead of .ownerdocument.defaultview [eslint: mozilla/use-ownerGlobal] ::: devtools/server/actors/tab.js:1023 (Diff revision 1) > } > > this.docShell.cssErrorReportingEnabled = true; > - // FIXME(emilio): Reparse sheets. > + for (let sheet of this.docShell.document.styleSheets) { > + getSheetText(sheet).then(text => { > + InspectorUtils.parseStyleSheet(sheet, text) Error: Missing semicolon. [eslint: semi] ::: devtools/server/actors/tab.js:1029 (Diff revision 1) > + }); > + } > + > return {}; > }, > Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines]
Take the review requests as feedback requests mostly, since it hasn't run through CI yet. :)
Comment on attachment 8966544 [details] Bug 1452143: Expose and honor a cssErrorReportingEnabled in the docshell. https://reviewboard.mozilla.org/r/235260/#review242030 So, I don't think we need to do any off-main-thread checking. The idea is just that we'd just modify StreamLoader.cpp to pass aAllowAsync=false if the current document needs error reporting. Given that, I think this code would be clearer if we ditched all the existing-overloads except for an nsIDocument static helper. The no-argument case would just hunt down the appropriate document then invoke the helper.
Attachment #8966544 - Flags: review?(bobbyholley) → review-
Attachment #8967571 - Flags: review?(bgrinstead) → review?(jryans)
(In reply to Bobby Holley (:bholley) from comment #28) > Comment on attachment 8966544 [details] > Bug 1452143: Expose and honor a cssErrorReportingEnabled in the docshell. > > https://reviewboard.mozilla.org/r/235260/#review242030 > > So, I don't think we need to do any off-main-thread checking. The idea is > just that we'd just modify StreamLoader.cpp to pass aAllowAsync=false if the > current document needs error reporting. > > Given that, I think this code would be clearer if we ditched all the > existing-overloads except for an nsIDocument static helper. The no-argument > case would just hunt down the appropriate document then invoke the helper. The no-argument case is called from actual parsing. What's the deal with that? I'd assume that we'd need to add something like: if (!NS_IsMainThread()) return false; there or what not.
Flags: needinfo?(bobbyholley)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #29) > The no-argument case is called from actual parsing. What's the deal with > that? I'd assume that we'd need to add something like: > > if (!NS_IsMainThread()) > return false; > > there or what not. You mean from Gecko_ReportUnexpectedCSSError? The async parse path just never invokes that function, error reporting is just a no-op there.
Flags: needinfo?(bobbyholley)
So the basic idea is that the async parsing code just ignores error reporting entirely, and so when Gecko wants error reports it uses the sync path.
(In reply to Bobby Holley (:bholley) from comment #31) > So the basic idea is that the async parsing code just ignores error > reporting entirely, and so when Gecko wants error reports it uses the sync > path. Alright, this version should work then.
I just noticed while fixing up tests that we also report CSS errors from style attribute parsing and CSSOM. This patch doesn't preserve those, and I can't think a good way of doing that. For inline style it is doable (is it worth it though?), you'd just need to do `element.setAttribute("style", element.getAttribute("style")` for every element in the doc. But we do definitely lose CSSOM info, like if some script did `element.style.color = "foo";`. I think it's not a huge deal though.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #36) > I just noticed while fixing up tests that we also report CSS errors from > style attribute parsing and CSSOM. This patch doesn't preserve those, and I > can't think a good way of doing that. > > For inline style it is doable (is it worth it though?), you'd just need to > do `element.setAttribute("style", element.getAttribute("style")` for every > element in the doc. > > But we do definitely lose CSSOM info, like if some script did > `element.style.color = "foo";`. I think it's not a huge deal though. Is this feature loss important? Tagging :bgrins for Console work and :bz as a user of the CSS error reporting info (if my memory is correct).
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bgrinstead)
(Note that we'd only lose it when you open the tab / toggle the filter and it was disabled). Loading the page with the filter already enabled works.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #42) > (Note that we'd only lose it when you open the tab / toggle the filter and > it was disabled). Loading the page with the filter already enabled works. Ah, okay. It's probably a reasonable trade off and hopefully not too mysterious...?
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #41) > (In reply to Emilio Cobos Álvarez [:emilio] from comment #36) > > I just noticed while fixing up tests that we also report CSS errors from > > style attribute parsing and CSSOM. This patch doesn't preserve those, and I > > can't think a good way of doing that. > > > > For inline style it is doable (is it worth it though?), you'd just need to > > do `element.setAttribute("style", element.getAttribute("style")` for every > > element in the doc. > > > > But we do definitely lose CSSOM info, like if some script did > > `element.style.color = "foo";`. I think it's not a huge deal though. > > Is this feature loss important? Tagging :bgrins for Console work and :bz as > a user of the CSS error reporting info (if my memory is correct). I don't think it's that important, and I don't think we need to go through and reset styles on elements in the DOM to try and preserve it.
Flags: needinfo?(bgrinstead)
Comment on attachment 8966544 [details] Bug 1452143: Expose and honor a cssErrorReportingEnabled in the docshell. https://reviewboard.mozilla.org/r/235260/#review242258 r=me with those things addressed. Thanks for working on this! ::: commit-message-25dd1:3 (Diff revision 4) > +The idea would be for parallel CSS parsing to check the static methods in > +ErrorReporter before loading the sheet, and unconditionally return false for the > +method if off-main-thread, since technically the owner node, document, and such > +can go away while you're parsing a sheet or what not (plus, sheets are not > +refcounted so not sure you can even poke at them OMT). > + > +But I guess this needs more changes than that for @import and what not, so maybe > +some of this needs to be changed further. This commit message is no longer accurate. ::: layout/style/ErrorReporter.cpp:195 (Diff revision 4) > + if (mSheet) { > + nsINode* owner = mSheet->GetOwnerNode() > + ? mSheet->GetOwnerNode() > + : mSheet->GetAssociatedDocument(); > + > + if (owner && ShouldReportErrors(*owner->OwnerDoc())) { > + return true; > + } > + } > + > + if (mLoader && mLoader->GetDocument() && > + ShouldReportErrors(*mLoader->GetDocument())) { > + return true; > + } I think it would be better to separate this into (1) find the document, and then (2) invoke the helper. The current setup seems like it might invoke ShouldReportErrors twice in the case where it returns false. Even if there's an invariant that only one of mSheet or mLoader is ever non-null (I'm not sure if there is), it's not clear from reading this here. So I think it would be clearer to do the above. ::: layout/style/ErrorReporter.cpp:196 (Diff revision 4) > + nsINode* owner = mSheet->GetOwnerNode() > + ? mSheet->GetOwnerNode() > + : mSheet->GetAssociatedDocument(); Why can't we just unconditionally call GetAssociatedDocument here, and then drop the OwnerDoc() call below?
Attachment #8966544 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) from comment #45) > Comment on attachment 8966544 [details] > Bug 1452143: Expose and honor a cssErrorReportingEnabled in the docshell. > > https://reviewboard.mozilla.org/r/235260/#review242258 > > r=me with those things addressed. Thanks for working on this! > > ::: commit-message-25dd1:3 > (Diff revision 4) > > +The idea would be for parallel CSS parsing to check the static methods in > > +ErrorReporter before loading the sheet, and unconditionally return false for the > > +method if off-main-thread, since technically the owner node, document, and such > > +can go away while you're parsing a sheet or what not (plus, sheets are not > > +refcounted so not sure you can even poke at them OMT). > > + > > +But I guess this needs more changes than that for @import and what not, so maybe > > +some of this needs to be changed further. > > This commit message is no longer accurate. Will reword. > ::: layout/style/ErrorReporter.cpp:195 > (Diff revision 4) > > + if (mSheet) { > > + nsINode* owner = mSheet->GetOwnerNode() > > + ? mSheet->GetOwnerNode() > > + : mSheet->GetAssociatedDocument(); > > + > > + if (owner && ShouldReportErrors(*owner->OwnerDoc())) { > > + return true; > > + } > > + } > > + > > + if (mLoader && mLoader->GetDocument() && > > + ShouldReportErrors(*mLoader->GetDocument())) { > > + return true; > > + } > > I think it would be better to separate this into (1) find the document, and > then (2) invoke the helper. The current setup seems like it might invoke > ShouldReportErrors twice in the case where it returns false. > > Even if there's an invariant that only one of mSheet or mLoader is ever > non-null (I'm not sure if there is), it's not clear from reading this here. > So I think it would be clearer to do the above. This is consistent with what we do to get the window ID to report to, so I'd rather not change it unnecessarily... But I'll dig a bit more to see. > ::: layout/style/ErrorReporter.cpp:196 > (Diff revision 4) > > + nsINode* owner = mSheet->GetOwnerNode() > > + ? mSheet->GetOwnerNode() > > + : mSheet->GetAssociatedDocument(); > > Why can't we just unconditionally call GetAssociatedDocument here, and then > drop the OwnerDoc() call below? Yes, sheets in a shadow root are not associated to the document.
Comment on attachment 8967571 [details] Bug 1452143: Reparse doc sheets after enabling error reporting on a docshell. https://reviewboard.mozilla.org/r/236248/#review242244 Thanks for working on this! :) Overall, it looks reasonable to me, but we should try to preserve the fetch from network monitor bit. ::: devtools/server/actors/stylesheets.js:162 (Diff revision 3) > + if (sheet.ownerNode && sheet.ownerNode.ownerDocument.characterSet) { > + return sheet.ownerNode.ownerDocument.characterSet; > + } > + } > + > + // step 5: default to utf-8. Probably "step 5" here can be removed now that there's no spec reference and also none of the other blocks have step numbers. ::: devtools/server/actors/stylesheets.js (Diff revision 3) > - * - contentType: the content type of the document > - * If an error occurs, the promise is rejected with that error. > - */ > - async fetchStylesheet(href) { > - // Check if network monitor observed this load, and if so, use that. > - let result = this.fetchStylesheetFromNetworkMonitor(href); It appears we lost this block that fetches from the network monitor, which I am pretty sure we want to keep to avoid double fetching in Style Editor, etc. Was it removed on purpose? It should be covered by a test[1]. Maybe you avoided it because it uses some state from the sheet actor that you don't have anymore? You could have `fetchStylesheetFromNetworkMonitor` take the console actor as an arg... Not the prettiest, but we hope to refactor this stuff to work differently in the future, so I think we can live with it for now. Your code in the tab actor will need to retrieve the console actor to pass it in. The tab actor _is_ the `parentActor` referenced from `StyleSheetActor#get _consoleActor`[2], so for your tab actor version, something like: ``` get _consoleActor() { if (this.exited) { return null; } let form = this.form(); return this.conn._getOrCreateActor(form.consoleActor); }, ``` should work. [1]: https://searchfox.org/mozilla-central/source/devtools/client/styleeditor/test/browser_styleeditor_fetch-from-netmonitor.js [2]: https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/devtools/server/actors/stylesheets.js#450 ::: devtools/server/actors/stylesheets.js:614 (Diff revision 3) > this.emit("style-applied", kind, this); > } > }); > > exports.StyleSheetActor = StyleSheetActor; > +exports.getSheetText = getSheetText; This module appears to place exports after their definition... (Other modules keep them all together near the end of the file; there's currenty no consistent style in DevTools for this.) I'd say place this after the `getSheetText` function. ::: devtools/server/actors/tab.js:1015 (Diff revision 3) > }, > > /** > * Ensure that CSS error reporting is enabled. > */ > ensureCSSErrorReportingEnabled(request) { If you like, you could mark this function async... ::: devtools/server/actors/tab.js:1022 (Diff revision 3) > return {}; > } > > this.docShell.cssErrorReportingEnabled = true; > - // FIXME(emilio): Reparse sheets. > + for (let sheet of this.docShell.document.styleSheets) { > + getSheetText(sheet).then(text => { ...and use `await` instead of `.then()` to get the text here.
Attachment #8967571 - Flags: review?(jryans) → review-
Comment on attachment 8967571 [details] Bug 1452143: Reparse doc sheets after enabling error reporting on a docshell. https://reviewboard.mozilla.org/r/236248/#review242276 ::: devtools/server/actors/stylesheets.js (Diff revision 3) > - * - contentType: the content type of the document > - * If an error occurs, the promise is rejected with that error. > - */ > - async fetchStylesheet(href) { > - // Check if network monitor observed this load, and if so, use that. > - let result = this.fetchStylesheetFromNetworkMonitor(href); yeah, I did that on purpose, but I hadn't got to that test yet... I'll do that, thanks! ::: devtools/server/actors/stylesheets.js:614 (Diff revision 3) > this.emit("style-applied", kind, this); > } > }); > > exports.StyleSheetActor = StyleSheetActor; > +exports.getSheetText = getSheetText; Alright, will do that. ::: devtools/server/actors/tab.js:1022 (Diff revision 3) > return {}; > } > > this.docShell.cssErrorReportingEnabled = true; > - // FIXME(emilio): Reparse sheets. > + for (let sheet of this.docShell.document.styleSheets) { > + getSheetText(sheet).then(text => { Not the best at Javascript, but wouldn't that mean that we wait until we fetched one sheet to fetch the next one? I can do that I guess, to guarantee the order of the messages, but if it's not a big deal I'd rather leave it like this, or stash all the promises in an array and await promise.all or something.
Comment on attachment 8967571 [details] Bug 1452143: Reparse doc sheets after enabling error reporting on a docshell. https://reviewboard.mozilla.org/r/236248/#review242276 > Not the best at Javascript, but wouldn't that mean that we wait until we fetched one sheet to fetch the next one? > > I can do that I guess, to guarantee the order of the messages, but if it's not a big deal I'd rather leave it like this, or stash all the promises in an array and await promise.all or something. Ah right! Yeah, no reason to actually wait here. Fine to leave as is or use Promise.all.
For my use cases, just reloading after toggling the switch in devtools is fine.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #50) > For my use cases, just reloading after toggling the switch in devtools is > fine. I wonder if that would even be OK generally instead of re-parsing sheets. If we wanted to make it obvious we could print a message to the console like 'reload the page to see css warnings' or something like that. That said, it looks like we already have an approach here that does support dynamically loading the warnings on change which does seem strictly better if we can do it without too much complexity.
Comment on attachment 8967571 [details] Bug 1452143: Reparse doc sheets after enabling error reporting on a docshell. https://reviewboard.mozilla.org/r/236248/#review242746 Thanks, this version looks good to me! :) ::: commit-message-5d3cd:6 (Diff revision 4) > +Bug 1452143: Reparse doc sheets after enabling error reporting on a docshell. r?jryans > + > +While at it, remove useless charset rule lookups, since charset rules aren't > +part of the OM, and have no effect at all anymore. > + > +I suspect I need to go through InspectorUtils.getAllSheets for the content Nit: Update comment to match what happened
Attachment #8967571 - Flags: review?(jryans) → review+
Comment on attachment 8968265 [details] Bug 1452143: Don't test content CSS error reporting in the browser console. https://reviewboard.mozilla.org/r/236946/#review242748 Agreed, this seems acceptable given the discussion on the list.
Attachment #8968265 - Flags: review?(jryans) → review+
Comment on attachment 8968266 [details] Bug 1452143: Fix webconsole tests. https://reviewboard.mozilla.org/r/236948/#review242756 ::: devtools/client/webconsole/test/mochitest/browser_webconsole_cached_messages.js:55 (Diff revision 1) > + // cached. > + let getCSSMessage = () => findMessage(hud, "cssColorBug611032", ".message.warn.css"); > + message = getCSSMessage(); > + while (!message && waitForCSSMessage) { > + // eslint-disable-next-line mozilla/no-arbitrary-setTimeout > + await new Promise(resolve => window.setTimeout(resolve, 50)); Hmm, it would be nice to avoid `setTimeout` unless it's really the only way here... What about the `waitForMessage` helper[1], does that work? [1]: https://searchfox.org/mozilla-central/rev/bd326a2a6b729dc62a5aee57354a97ceac4d1dc0/devtools/client/webconsole/test/mochitest/head.js#173
Attachment #8968266 - Flags: review?(jryans) → review-
Comment on attachment 8967706 [details] Bug 1452143: Make InspectorUtils.getAllStyleSheets handle Shadow DOM, and also optionally not return UA / User sheets. https://reviewboard.mozilla.org/r/236422/#review242758 ::: layout/inspector/InspectorUtils.cpp:89 (Diff revision 2) > AutoTArray<StyleSheet*, 32> xblSheetArray; > - styleSet->AppendAllXBLStyleSheets(xblSheetArray); > + styleSet->AppendAllNonDocumentAuthorSheets(xblSheetArray); > > // The XBL stylesheet array will quite often be full of duplicates. Cope: > + // > + // FIXME(emilio): I think this is not true since bug 1452525. File a followup to fix this?
Attachment #8967706 - Flags: review?(bobbyholley) → review+
Comment on attachment 8968267 [details] Bug 1452143: Fix dom / style tests. https://reviewboard.mozilla.org/r/236950/#review242760
Attachment #8968267 - Flags: review?(bobbyholley) → review+
Comment on attachment 8967706 [details] Bug 1452143: Make InspectorUtils.getAllStyleSheets handle Shadow DOM, and also optionally not return UA / User sheets. https://reviewboard.mozilla.org/r/236422/#review242758 > File a followup to fix this? Filed bug 1452525.
Comment on attachment 8968266 [details] Bug 1452143: Fix webconsole tests. https://reviewboard.mozilla.org/r/236948/#review242780 ::: devtools/client/webconsole/test/mochitest/browser_webconsole_cached_messages.js:55 (Diff revision 1) > + // cached. > + let getCSSMessage = () => findMessage(hud, "cssColorBug611032", ".message.warn.css"); > + message = getCSSMessage(); > + while (!message && waitForCSSMessage) { > + // eslint-disable-next-line mozilla/no-arbitrary-setTimeout > + await new Promise(resolve => window.setTimeout(resolve, 50)); That seems to work, thanks! Sorry for missing that one.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #65) > Comment on attachment 8967706 [details] > Bug 1452143: Make InspectorUtils.getAllStyleSheets handle Shadow DOM, and > also optionally not return UA / User sheets. > > https://reviewboard.mozilla.org/r/236422/#review242758 > > > File a followup to fix this? > > Filed bug 1452525. Err, bug 1454467 that is.
Comment on attachment 8968266 [details] Bug 1452143: Fix webconsole tests. https://reviewboard.mozilla.org/r/236948/#review242786 Looks great, thanks! :)
Attachment #8968266 - Flags: review?(jryans) → review+
Comment on attachment 8966545 [details] Bug 1452143: Hook the filters and enable error reporting on demand. Clearing the flag since there's still a FIXME in the actor, and we are missing tests (which I think emilio is working on). Aside from that, the client code looks good to me
Attachment #8966545 - Flags: review?(nchevobbe)
Comment on attachment 8966545 [details] Bug 1452143: Hook the filters and enable error reporting on demand. Re-requesting review since FIXME goes away in the next commit (it was a bunch of code unrelated to this thus I submitted it as a separate patch), and tests cover this (see the test modifications in later patches).
Attachment #8966545 - Flags: review?(nchevobbe)
Comment on attachment 8966545 [details] Bug 1452143: Hook the filters and enable error reporting on demand. https://reviewboard.mozilla.org/r/235262/#review242896 The client code seems good to me. We should have a test ::: devtools/server/actors/webconsole/worker-listeners.js:2 (Diff revision 6) > /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ > -/* vim: set ft= javascript ts=2 et sw=2 tw=80: */ > +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ changes to this file should be reverted
Attachment #8966545 - Flags: review?(nchevobbe) → review+
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3f214ca585db Expose and honor a cssErrorReportingEnabled in the docshell. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/8c85b98829b2 Make InspectorUtils.getAllStyleSheets handle Shadow DOM, and also optionally not return UA / User sheets. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/fabc60b735a6 Hook the filters and enable error reporting on demand. r=nchevobbe https://hg.mozilla.org/integration/mozilla-inbound/rev/6b3425746bfd Reparse doc sheets after enabling error reporting on a docshell. r=jryans https://hg.mozilla.org/integration/mozilla-inbound/rev/5520fcaf93d2 Don't test content CSS error reporting in the browser console. r=jryans https://hg.mozilla.org/integration/mozilla-inbound/rev/7b130a4f638a Fix webconsole tests. r=jryans https://hg.mozilla.org/integration/mozilla-inbound/rev/20b1c2e6fceb Fix dom / style tests. r=bholley
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a5aa0c6be992 Fix two more tests which don't run on debug try on a CLOSED TREE. r=me
Depends on: 1470035
I have documented this in the following places: https://developer.mozilla.org/en-US/docs/Tools/Web_Console/Console_messages#CSS https://developer.mozilla.org/en-US/Firefox/Releases/61#Developer_tools Let me know if I've got this right. Thanks!
Flags: needinfo?(emilio)
Keywords: dev-doc-needed → dev-doc-complete
Hmm, they were already not shown by default, we just stopped sending them to devtools when they weren't enabled. So I'm not sure this is worth pointing out in the relnotes.
Flags: needinfo?(emilio)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: