Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Description • 10 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 1 • 10 years ago

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

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 3 • 10 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 4 • 10 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 5 • 10 years ago

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.

	Joe Walker [:jwalker]
	Comment 6 • 10 years ago

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

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 7 • 10 years ago

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

	Victor Porof [:vporof][:vp]
	Comment 8 • 10 years ago

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.

	Joe Walker [:jwalker]
	Comment 9 • 10 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 10 • 10 years ago

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

	Joe Walker [:jwalker]
	Comment 11 • 10 years ago

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

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 12 • 10 years ago

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

	Joe Walker [:jwalker]
	Comment 13 • 10 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 14 • 10 years ago

Updated test as requested.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 15 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

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