[e10s] Enable devtools/framework tests with e10s

RESOLVED FIXED in Firefox 47

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
Firefox 47
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(e10sm6+, firefox47 fixed)

Details

(Whiteboard: [status:inflight])

Attachments

(6 attachments, 4 obsolete attachments)

1.05 KB, patch
jwalker
: review+
bgrins
: checkin+
Details | Diff | Splinter Review
11.43 KB, patch
bgrins
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
8.85 KB, patch
Details | Diff | Splinter Review
1.83 KB, patch
ejpbruel
: review+
bgrins
: checkin+
Details | Diff | Splinter Review
1.39 KB, patch
Details | Diff | Splinter Review
3.60 KB, patch
miker
: review+
Details | Diff | Splinter Review
They are currently all disabled in the browser.ini file
There is an error in browser_keybindings.js:

 0:19.83 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_keybindings.js | uncaught exception - NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIFocusManager.getFocusedElementForWindow] at chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:1528
 0:19.83 Stack trace:
 0:19.83     JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 1345
 0:19.83     JS frame :: chrome://mochikit/content/mochitest-e10s-utils.js :: e10s_init/< :: line 74
 0:19.83     native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
 0:19.83
All tests except for 1 worked without changes.  Pushed to try (but will this even run e10s for devtools on try?): https://tbpl.mozilla.org/?tree=Try&rev=fe822f88002d
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8446112 - Flags: review?(jwalker)
Attachment #8446112 - Flags: review?(jwalker) → review+
Keywords: checkin-needed
Whiteboard: [leave open]
I asked in #ateam how to push to try and have mochitest-dt run in e10s mode. AFAICT it's not possible using the syntax on trychooser (no reply yet).

Mochitest is run for some parts in e10s mode on linux. See the M-e10s(1 2 3 4 5) parts of tbpl.
https://hg.mozilla.org/integration/fx-team/rev/c387b619bf42
Keywords: checkin-needed
Whiteboard: [leave open] → [leave open][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c387b619bf42
Flags: in-testsuite+
Whiteboard: [leave open][fixed-in-fx-team] → [leave open]
Posted patch framework-e10s-pt2.patch (obsolete) — Splinter Review
So this test failed because of the waitForFocus call (Comment 1) and this patch 'fixes' it by removing the call.  It seems to work, but it isn't a great solution to the problem (it was waiting for focus for a reason).

This function is used pretty widely across our tests so I imagine this question will come up a lot - is there a good way to use it with e10s?
Flags: needinfo?(tomica+amo)
hey Brian, can you point me to where the waitForFocus() is implemented?
Flags: needinfo?(tomica+amo)
didn't intend do clear info..
Flags: needinfo?(tomica+amo)
(In reply to Tomislav Jovanovic [:zombie] from comment #7)
> hey Brian, can you point me to where the waitForFocus() is implemented?

Sure, it is here: http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#692.
And the issue can be seen by running:

mach mochitest-devtools --e10s browser/devtools/framework/test/browser_keybindings.js
so, at first look, i though this would be close to impossible to hack the helper waitForFocus() to work in chrome context, without moving all that logic to the frame script (where it would probably have to be reused via a JSM or something), because of the event listeners. 

but after some digging, it seems adding event listeners to DOM CPOWs now works, after bug 996785 was fixed (good to know! ;).

the second thing, that is actually throwing is the nsIFocusManager, obviously because it doesn't expect, and can't deal with CPOWs. unfortunately, and i'm not that familiar with focus-related APIs in gecko, but a quick look suggests it might be possible to get the similar information using the standard DOM APIs like document.hasFocus() and document.activeElement.
Flags: needinfo?(tomica+amo)
(In reply to Tomislav Jovanovic [:zombie] from comment #11)
> so, at first look, i though this would be close to impossible to hack the
> helper waitForFocus() to work in chrome context, without moving all that
> logic to the frame script (where it would probably have to be reused via a
> JSM or something), because of the event listeners. 
> 
> but after some digging, it seems adding event listeners to DOM CPOWs now
> works, after bug 996785 was fixed (good to know! ;).
> 
> the second thing, that is actually throwing is the nsIFocusManager,
> obviously because it doesn't expect, and can't deal with CPOWs.
> unfortunately, and i'm not that familiar with focus-related APIs in gecko,
> but a quick look suggests it might be possible to get the similar
> information using the standard DOM APIs like document.hasFocus() and
> document.activeElement.

Thanks for the info, I did some more searching around on the nsIFocusManager (getFocusedElementForWindow) issue and found Bug 921935.
Depends on: 921935
Whiteboard: [leave open] → [leave open][status:inflight]
Posted patch framework-e10s-pt2.patch (obsolete) — Splinter Review
Mike, this fixes the following tests:

browser/devtools/framework/test/browser_keybindings.js
browser/devtools/framework/test/browser_toolbox_options_disable_js.js
browser/devtools/framework/test/browser_target_events.js

And it starts trying to fix the disable cache one but there are still multiple problems.  Firstly, the assertions fail and as far as I can tell the cache actually isn't getting disabled.  Secondly, there is a hangup during the destroy process that causes the options panel destroy promise to never resolve (I think due to a call to activeTab.reconfigure).  So I've left a few of the basic fixes in there and disabled it for e10s in the browser.ini:
browser/devtools/framework/test/browser_toolbox_options_disable_cache.js
Attachment #8456377 - Attachment is obsolete: true
Attachment #8467781 - Flags: review?(mratcliffe)
Attachment #8467781 - Flags: review?(mratcliffe) → review+
Had to move the pickerButton cleanup into then() inside of destroyInspector() to resolve a few oranges outside of framework.  Pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6ca4b6d17eb4
Attachment #8467781 - Attachment is obsolete: true
Attachment #8468444 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/977a99c032e7
Keywords: checkin-needed
Whiteboard: [leave open][status:inflight] → [leave open][fixed-in-fx-team]
Comment on attachment 8468444 [details] [diff] [review]
framework-e10s-pt2.patch

Not sure if the [leave open] is needed anymore or not. Please remove it if it isn't.
Attachment #8468444 - Flags: checkin+
Flags: needinfo?(bgrinstead)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> Comment on attachment 8468444 [details] [diff] [review]
> framework-e10s-pt2.patch
> 
> Not sure if the [leave open] is needed anymore or not. Please remove it if
> it isn't.

Still needed for one more test.
Flags: needinfo?(bgrinstead)
Mike, can you take a look at the disable caching feature and let me know if it seems to be failing in e10s mode?  To me, the assertions indicate that maybe it is not actually disabling the cache.  With the latest fx-team you can run:

mach mochitest-devtools --e10s browser/devtools/framework/test/browser_toolbox_options_disable_cache.js

Here are the errors I see:

139 INFO Passed:  11
140 INFO Failed:  4
141 INFO Todo:    1

151 WARNING The following tests failed:
152 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js | Tab 0 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7 - Tab 0 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7
TEST-INFO | expected PASS
153 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js | Tab 1 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7 - Tab 1 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7
TEST-INFO | expected PASS
154 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js | Tab 2 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7 - Tab 2 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7
TEST-INFO | expected PASS
155 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js | Tab 2 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7 - Tab 2 cache is not enabled - Didn't expect 512b65d2-8adc-8810-951f-4e68901e4794, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:775
    chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js:checkCacheEnabled:97
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7
Flags: needinfo?(mratcliffe)
https://hg.mozilla.org/mozilla-central/rev/977a99c032e7
Whiteboard: [leave open][fixed-in-fx-team] → [leave open]
Whiteboard: [leave open] → [leave open][status:inflight]
The test is failing just because scrollIntoView needs at least two executeSoons when using e10s... not sure why because we are not touching content there.

I have logged 1052109 for the platform guys to look into this and we can use whichever API they come up with.
Comment on attachment 8485734 [details] [diff] [review]
framework-e10s-pt2.patch

Review of attachment 8485734 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like some good refactors overall.  Unfortunately I'm still seeing errors on the try push with e10s enabled for options-disable-cache.js: https://tbpl.mozilla.org/php/getParsedLog.php?id=47602373&tree=Try&full=1

::: browser/devtools/framework/test/head.js
@@ +137,5 @@
> +  return def.promise;
> +}
> +
> +/**
> + * element.scrollIntoView() does not fire an event on scroll... event the scroll

looks like a typo with 'event the scroll event'
Attachment #8485734 - Flags: review?(bgrinstead)
Applying this patch in the queue will run tests on try in e10s mode: https://bug1061904.bugzilla.mozilla.org/attachment.cgi?id=8482977
(In reply to Brian Grinstead [:bgrins] from comment #24)
> Applying this patch in the queue will run tests on try in e10s mode:
> https://bug1061904.bugzilla.mozilla.org/attachment.cgi?id=8482977

It will if "--e10" is corrected to "--e10s"
Moving DevTools test bugs to e10s milestone M7 (i.e. not blocking e10s merge to Aurora).
Whiteboard: [leave open][status:inflight] → [leave open][status:inflight][e10s-m7]
Hey Brian, how are we doing with this bug? Is there anything blocking you from finishing this up?
Flags: needinfo?(bgrinstead)
Depends on: 1070837
Whiteboard: [leave open][status:inflight][e10s-m7] → [leave open][status:inflight][e10s-m6]
Whiteboard: [leave open][status:inflight][e10s-m6] → [leave open][status:inflight][e10s-m7]
Posted patch framework-e10s-pt3.patch (obsolete) — Splinter Review
This is just enabling tests that are now working in e10s in the browser.ini.  Bug 1070837 is no longer a problem, but here are the tests that still need to be fixed after this patch:

browser_toolbox_options_disable_cache-02.js
browser_toolbox_options_disable_js.js
browser_toolbox_select_event.js

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f33b0b6d36a
Flags: needinfo?(bgrinstead)
Attachment #8549899 - Flags: review?(ejpbruel)
(In reply to Brian Grinstead [:bgrins] from comment #28)
> 
> here are the tests that still need
> to be fixed after this patch:
> 
> browser_toolbox_options_disable_cache-02.js
> browser_toolbox_options_disable_js.js
> browser_toolbox_select_event.js

Mike, I believe the scrollIntoView thing you mentioned in Comment 20 is no longer an issue for browser_toolbox_options_disable_cache.js.  Can you check if it is still an issue in browser_toolbox_options_disable_cache-02.js, or if this is a different problem?  Here is the error I see locally:

142 INFO TEST-UNEXPECTED-FAIL | browser/devtools/framework/test/browser_toolbox_options_disable_cache-02.js | Tab 2 cache is not enabled - Didn't expect c69482c1-e04d-8118-89d7-a23359471b3f, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:855
    chrome://mochitests/content/browser/browser/devtools/framework/test/helper_disable_cache.js:checkCacheEnabled:64
    self-hosted:InterpretGeneratorResume:677
    self-hosted:next:585
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:870:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:749:7
    this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:691:37
143 INFO TEST-UNEXPECTED-FAIL | browser/devtools/framework/test/browser_toolbox_options_disable_cache-02.js | Tab 2 cache is not enabled - Didn't expect c69482c1-e04d-8118-89d7-a23359471b3f, but got it
Stack trace:
    chrome://mochikit/content/browser-test.js:test_isnot:855
    chrome://mochitests/content/browser/browser/devtools/framework/test/helper_disable_cache.js:checkCacheEnabled:64
    self-hosted:InterpretGeneratorResume:677
    self-hosted:next:585
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:870:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:749:7
    this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:691:37
SUITE-END | took 10s
Flags: needinfo?(mratcliffe)
OK, so apparently browser_toolbox_options_disable_cache-01.js wasn't safe to enable for linux (failures here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f33b0b6d36a).

Keeping that disabled for now - here is a new try push with this version: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8b2c3fe0321
Attachment #8549899 - Attachment is obsolete: true
Attachment #8549899 - Flags: review?(ejpbruel)
Attachment #8550048 - Flags: review?(ejpbruel)
Comment on attachment 8550048 [details] [diff] [review]
framework-e10s-pt3.patch

Review of attachment 8550048 [details] [diff] [review]:
-----------------------------------------------------------------

Do we really need reviews for patches like this? I would be fine with r=me for similar patches in the future (i.e. trivial patches that only touch a configuration file).
Attachment #8550048 - Flags: review?(ejpbruel) → review+
https://hg.mozilla.org/integration/fx-team/rev/700f4ab5423a
Whiteboard: [leave open][status:inflight][e10s-m7] → [leave open][status:inflight][e10s-m7][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/700f4ab5423a
Whiteboard: [leave open][status:inflight][e10s-m7][fixed-in-fx-team] → [leave open][status:inflight][e10s-m7]
Note that bug 1083269 fixed waitForFocus to work with content process windows. Does this affect any on the tests disabled or changed here?
Flags: needinfo?(bgrinstead)
(In reply to Neil Deakin from comment #34)
> Note that bug 1083269 fixed waitForFocus to work with content process
> windows. Does this affect any on the tests disabled or changed here?

I don't think so.. In Attachment 8485734 [details] [diff] I changed a call from waitForFocus(setupKeyBindingsTest, content) to waitForFocus(setupKeyBindingsTest) and everything seems to work, so unless if there is a reason to undo that change we should be fine here.
Flags: needinfo?(bgrinstead)
See Also: → 921935
Flags: needinfo?(mratcliffe)
Mike, I think the ni? from earlier got cleared.  See Comment 29 - the following 3 tests are still failing in e10s (and don't seem related to the scrollIntoView issue).  AFAICT from the error messages, the disable cache/js features may not be working in e10s.  Since you've worked on these features, can you take a look and see what is going on, at least to help narrow down where things are breaking here?

browser_toolbox_options_disable_cache-01.js
browser_toolbox_options_disable_cache-02.js
browser_toolbox_options_disable_js.js
Flags: needinfo?(mratcliffe)
Summary: Enable devtools/framework tests with e10s → [e10s] Enable devtools/framework tests with e10s
I think we should try to get these tests into the Aurora 40 uplift, so I propose upgrading this bug to M6.
(In reply to Brian Grinstead [:bgrins] from comment #36)
> Created attachment 8576965 [details] [diff] [review]
> framework-e10s-pt4.patch
> 
> Mike, I think the ni? from earlier got cleared.  See Comment 29 - the
> following 3 tests are still failing in e10s (and don't seem related to the
> scrollIntoView issue).  AFAICT from the error messages, the disable cache/js
> features may not be working in e10s.  Since you've worked on these features,
> can you take a look and see what is going on, at least to help narrow down
> where things are breaking here?
> 
> browser_toolbox_options_disable_cache-01.js
> browser_toolbox_options_disable_cache-02.js
> browser_toolbox_options_disable_js.js

I will take a look.
Flags: needinfo?(mratcliffe)
Hey brian, how's this work coming along? We're about two weeks away from uplift.
Flags: needinfo?(bgrinstead)
Whiteboard: [leave open][status:inflight][e10s-m7] → [leave open][status:inflight]
(In reply to Jim Mathies [:jimm] from comment #39)
> Hey brian, how's this work coming along? We're about two weeks away from
> uplift.

It's just the disable cache / disable js tests remaining.  I can't tell if the feature itself isn't working or if it's a test-only issue.  Mike knows this feature well so I think he was going to take a look at it, forwarding the needinfo.
Flags: needinfo?(bgrinstead) → needinfo?(mratcliffe)
(In reply to Brian Grinstead [:bgrins] from comment #36)
> Created attachment 8576965 [details] [diff] [review]
> framework-e10s-pt4.patch
> 
> Mike, I think the ni? from earlier got cleared.  See Comment 29 - the
> following 3 tests are still failing in e10s (and don't seem related to the
> scrollIntoView issue).

This patch ads a 500ms wait between cbx.scrollintoview() and cbx.click()... suddenly the tests pass locally in e10s mode.

It may be that we will need to up this number on try but 100ms works locally.

In bug 1052109 we request a way to reliably wait for scrollintoview but it is hard, especially inside nested iframes. For the moment adding the pause seems like the only answer we have.

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03a2341cd650
Flags: needinfo?(mratcliffe)
Attachment #8605749 - Flags: review?(bgrinstead)
Comment on attachment 8605749 [details] [diff] [review]
1030318-scrollintoview-needs-wait.patch

Review of attachment 8605749 [details] [diff] [review]:
-----------------------------------------------------------------

I've been bit by this bug before also, but I thought calling el.click() was safe regardless of whether it was in view (as opposed to synthesizemouse).  Can you test locally and see if putting the `wait` after the checkbox click (instead of before) still fixes it?  Because I suspect we can actually remove the call to scrollIntoView completely and the tests would still work.

r=me if try is green and if the problem is in fact the scrollIntoVeiw issue.  Make sure to trigger a bunch of re-runs on the e10s tests so we can catch any likely intermittents

::: browser/devtools/framework/test/browser_toolbox_options_disable_js.js
@@ +62,5 @@
>    let panel = toolbox.getCurrentPanel();
>    let cbx = panel.panelDoc.getElementById("devtools-disable-javascript");
>  
>    cbx.scrollIntoView();
> +  waitFor(500).then(() => {

Please add a comment linking to the relevant bug that will let us remove this.

::: browser/devtools/framework/test/helper_disable_cache.js
@@ +73,5 @@
>  
>    cbx.scrollIntoView();
>  
>    // After uising scrollIntoView() we need to wait for the browser to scroll.
> +  yield waitFor(500);

Ditto
Attachment #8605749 - Flags: review?(bgrinstead) → review+
- No longer using scrollIntoView().
- Uses cbx.click() as requested.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=673f76cd2085
Attachment #8605749 - Attachment is obsolete: true
Attachment #8607547 - Flags: review+
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #43)
> Created attachment 8607547 [details] [diff] [review]
> 1030318-scrollintoview-needs-wait.patch
> 
> - No longer using scrollIntoView().
> - Uses cbx.click() as requested.
> 
> Try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=673f76cd2085

Looks like you have a couple failures here, but nothing that looks too serious - 

695 INFO TEST-UNEXPECTED-FAIL | browser/devtools/framework/test/browser_toolbox_options_disable_js.js | Output is "JavaScript Enabled" - Got No output, expected JavaScript Enabled
697 INFO TEST-UNEXPECTED-FAIL | browser/devtools/framework/test/browser_toolbox_options_disable_js.js | Output is "JavaScript Enabled" in iframe - Got No output, expected JavaScript Enabled 

Curious, what's the current eta on finishing this bug up?
(In reply to Jim Mathies [:jimm] from comment #44)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #43)
> > Created attachment 8607547 [details] [diff] [review]
> > 1030318-scrollintoview-needs-wait.patch
> > 
> > - No longer using scrollIntoView().
> > - Uses cbx.click() as requested.
> > 
> > Try:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=673f76cd2085
> 
> Looks like you have a couple failures here, but nothing that looks too
> serious - 
> 
> 695 INFO TEST-UNEXPECTED-FAIL |
> browser/devtools/framework/test/browser_toolbox_options_disable_js.js |
> Output is "JavaScript Enabled" - Got No output, expected JavaScript Enabled
> 697 INFO TEST-UNEXPECTED-FAIL |
> browser/devtools/framework/test/browser_toolbox_options_disable_js.js |
> Output is "JavaScript Enabled" in iframe - Got No output, expected
> JavaScript Enabled 
> 
> Curious, what's the current eta on finishing this bug up?

I have no time at the moment to investigate other approaches so I guess we have three options:
1. Leave it for now as the tests are currently disabled and in real life the features work perfectly.
2. Wait for me to finish the storage inspector when I can take another look.
3. Somebody else can take a look.

We probably just need to add a delay but it would be good to know exactly what is making the pause necessary. Maybe we should be waiting a little longer after toolbox init?
I've got a try push here that enables the disable cache / js tests originally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6074ad0bd6fc.  There are two others I'll leave disabled but since we have bugs already for them (bug 1231869 and bug 1069044) I'm going to land that part.
No longer blocks: 1069044
Depends on: 1231869, 1069044
Whiteboard: [leave open][status:inflight] → [status:inflight]
hi, sorry had to back this out for causing perma failures on linux x64 opt like https://treeherder.mozilla.org/logviewer.html#?job_id=6742280&repo=fx-team
Flags: needinfo?(bgrinstead)
Looks like 1240509 would fix the failures.
Or I should say 1240368, checked in yesterday.
(In reply to Neil Deakin from comment #51)
> Or I should say 1240368, checked in yesterday.

I think this was checked in after that bug.  But here's a new try push anyway just to confirm: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13759e8ff5b6.  Guessing we'll need to wait for a resolution in Bug 1240509 before relanding
Depends on: 1240509
Flags: needinfo?(bgrinstead)
New try push on top of Bug 1204174.. maybe this will fix the perf perma orange problem: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ca792e34456
Depends on: 1204174
https://hg.mozilla.org/mozilla-central/rev/d23494dca1d5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.