1086492 - Disable tilt for E10S

Status: RESOLVED FIXED

(Firefox Graveyard :: Developer Tools: 3D View, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: disable-tilt-for-e10s-1086492.patch

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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.

Comment 6

[Joe Walker [: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.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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

Comment 8

[Victor Porof [:vporof][:vp]]

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.

Comment 9

[Joe Walker [: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.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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

Comment 11

[Joe Walker [: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?

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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

Comment 13

[Joe Walker [: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.

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: disable-tilt-for-e10s-1086492.patch

Updated test as requested.

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cab7db08efb4