If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Disable tilt for E10S

RESOLVED FIXED in Firefox 36

Status

Firefox Graveyard
Developer Tools: 3D View
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: miker, Assigned: miker)

Tracking

unspecified
Firefox 36
Dependency tree / graph
Bug Flags:
in-testsuite -

Details

Attachments

(1 attachment, 4 obsolete attachments)

Because we will not be able to make tilt remote-able in time for E10S we need to disable it. This includes buttons, options and GCLI commands.
Blocks: 937166
I am not convinced that I have disabled all of our tilt testing so we need a try run in e10s mode to check that.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=5f375915e68f
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5f375915e68f
Created attachment 8508709 [details] [diff] [review]
disable-tilt-for-e10s-1086492.patch
Comment on attachment 8508709 [details] [diff] [review]
disable-tilt-for-e10s-1086492.patch

No errors in try run relating to tilt so I must have disabled everything I needed to.
Attachment #8508709 - Flags: review?(vporof)
Comment on attachment 8508709 [details] [diff] [review]
disable-tilt-for-e10s-1086492.patch

Actually... let's leave tilt enabled in non-e10s mode if we can.
Attachment #8508709 - Flags: review?(vporof)
tracking-e10s: --- → +
jwalker suggested that we use the hidden attribute of the command spec to flag them as hidden when in E10S mode. Unfortunately the commands are cached.

We also have no access to the content window or toolbox.target from within the command spec or e.g. api.js in order to check whether the tab is in E10S mode.

Joe, is there anywhere in GCLI that we can filter the commands by the state of the current content window? Currently the best solution I can find is to return "Not available in E10S mode" from a command if context.window === null.
Flags: needinfo?(jwalker)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #5)
> jwalker suggested that we use the hidden attribute of the command spec to
> flag them as hidden when in E10S mode. Unfortunately the commands are cached.
> 
> We also have no access to the content window or toolbox.target from within
> the command spec or e.g. api.js in order to check whether the tab is in E10S
> mode.
> 
> Joe, is there anywhere in GCLI that we can filter the commands by the state
> of the current content window? Currently the best solution I can find is to
> return "Not available in E10S mode" from a command if context.window ===
> null.

I'm surprised that they're cached, but we can always just do hidden:true for now. Keeps it simple.
Flags: needinfo?(jwalker)
Created attachment 8511011 [details] [diff] [review]
disable-tilt-for-e10s-1086492.patch

Pretty simple:

1. Comment out tilt stuff from browser/devtools/commandline/test/browser_cmd_settings.js
2. Add target.isMultiProcess.
3. Hide Options Panel 3D View checkbox only in E10S mode.
4. Hide Tilt toolbox button only in E10S mode.
5. Disable tilt tests.
6. Hide tilt commands in GCLI for both single and multi-process modes (no other choice here)
7. Because hidden GCLI commands are still executable we display e.g. "The command '%1$S' is not available in multiprocess mode (E10S)" if an attempt is made to execute the commands in E10S mode.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=38e92369c267
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=38e92369c267
Attachment #8508709 - Attachment is obsolete: true
Attachment #8511011 - Flags: review?(vporof)
Comment on attachment 8511011 [details] [diff] [review]
disable-tilt-for-e10s-1086492.patch

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

LGTM, but I think Joe would be better suited for this review.
Attachment #8511011 - Flags: review?(vporof) → review+
Attachment #8511011 - Flags: review?(jwalker)
Attachment #8511011 - Flags: review?(jwalker)
Comment on attachment 8511011 [details] [diff] [review]
disable-tilt-for-e10s-1086492.patch

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

r+

::: browser/devtools/commandline/test/browser_cmd_settings.js
@@ +24,5 @@
>    let gcli = require("gcli/index");
>    yield gcli.load();
>    let settings = gcli.settings;
>  
> +  // let tiltEnabled = settings.get("devtools.tilt.enabled");

This is just some random boolean pref, not really anything to do with tilt. Maybe a better fix would be to use some other random boolean pref instead.
Created attachment 8513610 [details] [diff] [review]
disable-tilt-for-e10s-1086492.patch

Asking jwalker for review at vporof's request.

New try run (fixed test):
https://tbpl.mozilla.org/?tree=Try&rev=e45b460b23bc
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e45b460b23bc
Attachment #8511011 - Attachment is obsolete: true
Attachment #8513610 - Flags: review?(jwalker)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #10)
> Created attachment 8513610 [details] [diff] [review]
> disable-tilt-for-e10s-1086492.patch
> 
> Asking jwalker for review at vporof's request.

I'm not clear what's changed?
Created attachment 8514246 [details] [diff] [review]
disable-tilt-for-e10s-1086492.patch

The only things that need review are:
1. browser/devtools/framework/test/browser_toolbox_options_disable_buttons.js
2. browser/devtools/framework/toolbox.js

In the test we have just disabled tilt checks in E10S mode.

In toolbox.js we have removed tilt from toolboxButtons (allows a bunch of tests to pass without modification) and deal with show / hide separately.

This try should be green:
https://tbpl.mozilla.org/?tree=Try&rev=bb72d67ec6f3
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bb72d67ec6f3
Attachment #8513610 - Attachment is obsolete: true
Attachment #8513610 - Flags: review?(jwalker)
Attachment #8514246 - Flags: review?(jwalker)
Comment on attachment 8514246 [details] [diff] [review]
disable-tilt-for-e10s-1086492.patch

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

::: browser/devtools/commandline/test/browser_cmd_settings.js
@@ +24,5 @@
>    let gcli = require("gcli/index");
>    yield gcli.load();
>    let settings = gcli.settings;
>  
> +  // let tiltEnabled = settings.get("devtools.tilt.enabled");

Couldn't we just s/devtools.tilt.enabled/devtools.gcli.hideIntro/g rather than comment this out?
See comment 9.
Seems a shame to turn tests off when there is such an easy fix.
Attachment #8514246 - Flags: review?(jwalker) → review+
Created attachment 8514351 [details] [diff] [review]
disable-tilt-for-e10s-1086492.patch

Updated test as requested.
Attachment #8514246 - Attachment is obsolete: true
Attachment #8514351 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/cab7db08efb4
Whiteboard: [fixed-in-fx-team]
Attachment #8514351 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/cab7db08efb4
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Depends on: 1094968
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.