Closed
Bug 1086492
Opened 9 years ago
Closed 9 years ago
Disable tilt for E10S
Categories
(Firefox Graveyard :: Developer Tools: 3D View, defect)
Firefox Graveyard
Developer Tools: 3D View
Tracking
(e10s+)
RESOLVED
FIXED
Firefox 36
Tracking | Status | |
---|---|---|
e10s | + | --- |
People
(Reporter: miker, Assigned: miker)
References
Details
Attachments
(1 file, 4 obsolete files)
19.44 KB,
patch
|
miker
:
review+
miker
:
checkin+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 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
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 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.
Attachment #8508709 -
Flags: review?(vporof)
Assignee | ||
Comment 4•9 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.
Attachment #8508709 -
Flags: review?(vporof)
Updated•9 years ago
|
tracking-e10s:
--- → +
Assignee | ||
Comment 5•9 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.
Flags: needinfo?(jwalker)
Comment 6•9 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.
Flags: needinfo?(jwalker)
Assignee | ||
Comment 7•9 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
Attachment #8508709 -
Attachment is obsolete: true
Attachment #8511011 -
Flags: review?(vporof)
Comment 8•9 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.
Attachment #8511011 -
Flags: review?(vporof) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8511011 -
Flags: review?(jwalker)
Assignee | ||
Updated•9 years ago
|
Attachment #8511011 -
Flags: review?(jwalker)
Comment 9•9 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.
Assignee | ||
Comment 10•9 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
Attachment #8511011 -
Attachment is obsolete: true
Attachment #8513610 -
Flags: review?(jwalker)
Comment 11•9 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?
Assignee | ||
Comment 12•9 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
Attachment #8513610 -
Attachment is obsolete: true
Attachment #8513610 -
Flags: review?(jwalker)
Attachment #8514246 -
Flags: review?(jwalker)
Comment 13•9 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.
Attachment #8514246 -
Flags: review?(jwalker) → review+
Assignee | ||
Comment 14•9 years ago
|
||
Updated test as requested.
Attachment #8514246 -
Attachment is obsolete: true
Attachment #8514351 -
Flags: review+
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/cab7db08efb4
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Updated•9 years ago
|
Attachment #8514351 -
Flags: checkin+
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cab7db08efb4
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Depends on: 1094968
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•