Closed Bug 1243736 Opened 4 years ago Closed 4 years ago
Re-enable the browser
_rules _pseudo-element _02 .js with e10s
MozReview Request: Bug 1243736 - Enable browser_rules_content_02.js with e10s and make rule-view wait for updateSourceLink; r=bgrins
58 bytes, text/x-review-board-request
This test is currently disabled when tests are run with e10s and as such, is part of this list of tests we want to re-enable: https://docs.google.com/spreadsheets/d/10UeyRoiWV2HjkWwAU51HXyXAV7YLi4BjDm55mr5Xv6c/edit#gid=1777180571
I have a pretty simple fix for this, but while investigating, I realized that this test was generating an exception while ending. This is bad because checking this fix in like this would introduce exceptions in the logs. I don't believe it would fail the test though because this comes from an exception being logged in an handled promise rejection. But bad nonetheless. So, as I started investigating this, I found out that the code doesn't wait for one specific part of the async rule-view refresh process (when you select a new node). This, therefore, causes exceptions when selecting a new element right before a test ends because, typically, devtools has been destroyed before the request/response can be handled. The specific request is the one made by RuleEditors to retrieve original source links (which is useful when there are source maps). Making the rule-view code wait for this to be done before signaling the inspector-panel that it is done refreshing gets rid of a this exception (which most probably occurs in a lot of tests).
Review commit: https://reviewboard.mozilla.org/r/32767/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/32767/
Attachment #8713191 - Flags: review?(bgrinstead)
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Summary: Re-enable the browser_rules_content_02.js with e10s → Re-enable the browser_rules_pseudo-element_02.js with e10s
I got confused when naming this bug and commit message, this isn't about browser_rules_content_02.js, but about browser_rules_pseudo-element_02.js I'll rename the commit before pushing, for now, it doesn't matter.
Comment on attachment 8713191 [details] MozReview Request: Bug 1243736 - Enable browser_rules_content_02.js with e10s and make rule-view wait for updateSourceLink; r=bgrins https://reviewboard.mozilla.org/r/32767/#review29833 Looks fine to me - don't forget to update the commit message as in Comment 3 ::: devtools/client/inspector/rules/rules.js:1163 (Diff revision 1) > + let onEditorReady = promise.defer(); Doesn't 'once' already return a Promise? If so could you do: editorReadyPromises.push(rule.editor.once("source-link-updated")); ::: devtools/client/inspector/rules/views/rule-editor.js:30 (Diff revision 1) > +loader.lazyRequireGetter(this, "EventEmitter", Curious, is there any reason to use this over: const EventEmitter = require("devtools/shared/event-emitter")
Attachment #8713191 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #5) > ::: devtools/client/inspector/rules/rules.js:1163 > (Diff revision 1) > > + let onEditorReady = promise.defer(); > > Doesn't 'once' already return a Promise? If so could you do: > > editorReadyPromises.push(rule.editor.once("source-link-updated")); Right, I missed that somehow. Fixed now > ::: devtools/client/inspector/rules/views/rule-editor.js:30 > (Diff revision 1) > > +loader.lazyRequireGetter(this, "EventEmitter", > > Curious, is there any reason to use this over: > > const EventEmitter = require("devtools/shared/event-emitter") Hm, you're right, EventEmitter is used right away anyway, so there's no reason to lazy load it. Fixed.
[bugday-20160323] Status: RESOLVED,FIXED -> UNVERIFIED Comments: STR: Not clear. Developer specific testing Component: Name Firefox Version 46.0b9 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Expected Results: Developer specific testing Actual Results: As expected
This has caused a few regressions: bug 1249888 (fixed in 46+), bug 1255787 (only fixed in 48) and bug 1263439. Noting in case we see a pattern of more problems or in case they can be fixed in a more general way.
You need to log in before you can comment on or make changes to this bug.