Closed Bug 1243736 Opened 4 years ago Closed 4 years ago

Re-enable the browser_rules_pseudo-element_02.js with e10s

Categories

(DevTools :: Inspector: Rules, defect)

defect
Not set
normal

Tracking

(e10s+, firefox46 fixed, firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
e10s + ---
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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).
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.
Blocks: e10s-tests
tracking-e10s: --- → +
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.
https://hg.mozilla.org/mozilla-central/rev/223de53aa2dc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1249888
Depends on: 1260510
[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
Depends on: 1263439
Depends on: 1255787
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.