1243736 - Re-enable the browser_rules_pseudo-element_02.js with e10s

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect)

Description

[Patrick Brosset <:pbro>]

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

Comment 1

[Patrick Brosset <:pbro>]

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).

Comment 2

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1243736 - Enable browser_rules_content_02.js with e10s and make rule-view wait for updateSourceLink; r=bgrins

Review commit: https://reviewboard.mozilla.org/r/32767/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32767/

Comment 3

[Patrick Brosset <:pbro>]

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 4

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=baec6a0af600

Comment 5

[Brian Grinstead [:bgrins]]

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")

Comment 6

[Patrick Brosset <:pbro>]

(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.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/223de53aa2dc

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/223de53aa2dc

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fe917186b983

Comment 10

[Mayur Patil]

[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

Comment 11

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.