Closed
Bug 1160345
Opened 10 years ago
Closed 10 years ago
Inspect element / command buttons no longer shown for old servers
Categories
(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)
DevTools Graveyard
Graphic Commandline and Toolbar
Tracking
(firefox40 fixed)
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: jryans, Assigned: bgrins)
References
Details
(Keywords: regression, Whiteboard: [polish-backlog])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
It appears that bug 1128988 (GCLI for e10s) does not properly support backwards compatibility for old servers.
For example, if I start Simulator 2.2 from WebIDE and connect to an app, Inspect Element is missing and the server gives the error:
"Protocol error (unrecognizedPacketType): Actor conn0.child21/gcliActor11 does not recognize the packet type specs" protocol.js:20
onPacket threw an exception: Error: Server did not specify an actor, dropping packet: {"error":"unknownError","message":"Error occurred while creating actor 'undefined: TypeError: browser is undefined\nStack: exports.GcliActor<.initialize@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/gcli.js:1:952\nconstructor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/heritage.js:16:80\nObservedActorFactory.prototype.createActor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/common.js:13:99\nDebuggerServerConnection.prototype._getOrCreateActor@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:81:53\nDSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:91:11\nChildDebuggerTransport.prototype.receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:38:288\nLine: 1, column: 951"}
Stack: DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:944:1
DebuggerTransport.prototype._onJSONObjectReady/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:471:9
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
Line: 944, column: 0
We need to restore previous functionality in some way here.
Reporter | ||
Updated•10 years ago
|
Blocks: gaia-devtools
Reporter | ||
Comment 1•10 years ago
|
||
Other command buttons missing include:
* Window / frame switching
* Split console
Summary: Inspect element no longer shown for old servers → Inspect element / command buttons no longer shown for old servers
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 2•10 years ago
|
||
I am not sure what the correct way to proceed here is.
How should we restore the command buttons for old servers?
Flags: needinfo?(jwalker)
Flags: needinfo?(bgrinstead)
Comment 3•10 years ago
|
||
Prior to the addition of runAt support, we did this:
https://hg.mozilla.org/mozilla-central/diff/6ecd33215340/browser/devtools/shared/DeveloperToolbar.jsm#l1.84
I think that logic really belongs in toolbox.js, probably somewhere like this:
https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#716
> How should we restore the command buttons for old servers?
Are we sure they were there before? If they were, then it must have been via a mechanism other than GCLI.
Flags: needinfo?(jwalker)
Assignee | ||
Comment 4•10 years ago
|
||
I'll fix this, it's something really simple
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 5•10 years ago
|
||
The problem is just that the call to gcli is blocking the call to setToolboxButtonsVisibility (which sets the non-gcli buttons like inspect
element).
Here I just make that call and then bail out of the function if it's going to fail (these buttons would have never worked previously anyway since gcli wasn't remote safe).
Ryan, is there a better way to detect the presence of the actor than using a trait as I have here?
Attachment #8600351 -
Flags: feedback?(jryans)
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 6•10 years ago
|
||
Using target.hasActor instead of a trait
Attachment #8600351 -
Attachment is obsolete: true
Attachment #8600351 -
Flags: feedback?(jryans)
Attachment #8600372 -
Flags: review?(jryans)
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8600372 [details] [diff] [review]
toolbox-buttons.patch
Review of attachment 8600372 [details] [diff] [review]:
-----------------------------------------------------------------
This doesn't seem quite right. The trouble here is that there *was* in fact a GCLI actor[1] before bug 1128988. After 1128988, the actor was heavily changed, and so far it's unclear to me if we still know how to connect to the old version of it at all.
Or maybe you mean that we *should* be able to connect to the old actor, but there are just more things to fix before it actually works? With this patch applied and starting simulator 2.2 and opening an app, I see the same "browser is undefined" error from comment 0. Are we even meant to use the old GCLI actor (pre bug 1128988) remotely like this? If not, then the trait version seems more appropriate.
This patch does restore:
* Inspect element
* Frame switching
However:
* Split console
is still missing.
If you try Dev. Edition (39) with Simulator 2.2, you can see the split console button was there. When I try Nightly w/ this patch applied, it's not there anymore.
[1]: https://hg.mozilla.org/mozilla-central/filelog/7723b15ea695/toolkit/devtools/server/actors/gcli.js
Attachment #8600372 -
Flags: review?(jryans)
Reporter | ||
Comment 8•10 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Or maybe you mean that we *should* be able to connect to the old actor, but
> there are just more things to fix before it actually works? With this patch
> applied and starting simulator 2.2 and opening an app, I see the same
> "browser is undefined" error from comment 0. Are we even meant to use the
> old GCLI actor (pre bug 1128988) remotely like this? If not, then the trait
> version seems more appropriate.
Or, if the new GCLI actor has a new method that did not exist before, you could possibly use target.actorHasMethod to check for it. Though, this might be less clear than a trait if you are just picking a random method you don't actually need at the moment as a stand-in for "is new".
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Comment on attachment 8600372 [details] [diff] [review]
> toolbox-buttons.patch
>
> Review of attachment 8600372 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> This doesn't seem quite right. The trouble here is that there *was* in fact
> a GCLI actor[1] before bug 1128988. After 1128988, the actor was heavily
> changed, and so far it's unclear to me if we still know how to connect to
> the old version of it at all.
Hm, good catch. The intention was to have the same behavior as the trait, but that isn't working as you point out. I'm not sure exactly what the solution is for this yet, but maybe we should first land a patch that just toggles the visibility correctly and then do the split console (aka old gcli actor support) separately
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #9)
> but maybe we should first land a patch that just
> toggles the visibility correctly and then do the split console (aka old gcli
> actor support) separately
The reasoning being that the 'inspect element' button missing is a major problem for anyone using WebIDE
Assignee | ||
Comment 11•10 years ago
|
||
My idea from comment 9 so we can get these buttons back in the next nightly. I will look further into getting the split console button to show up in a separate patch. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb7e552ead12
Attachment #8600372 -
Attachment is obsolete: true
Attachment #8600394 -
Flags: review?(jryans)
Assignee | ||
Comment 12•10 years ago
|
||
If the only remaining button that doesn't show up with an old server is the split console, one option would be to proceed by decoupling that button from gcli altogether.
Reporter | ||
Comment 13•10 years ago
|
||
Comment on attachment 8600394 [details] [diff] [review]
toolbox-buttons.patch
Review of attachment 8600394 [details] [diff] [review]:
-----------------------------------------------------------------
Great, this makes sense to me.
It also brings the buttons back for new b2g servers (bug 1160361).
Should I file a separate bug for split console / other command buttons?
Attachment #8600394 -
Flags: review?(jryans) → review+
Reporter | ||
Comment 14•10 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)
> It also brings the buttons back for new b2g servers (bug 1160361).
I updated the title of bug 1160361, since the patch here will also restore inspect element for new b2g servers. Bug 1160361 is now just about getting the GCLI actor to work right on b2g.
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)
> > It also brings the buttons back for new b2g servers (bug 1160361).
>
> I updated the title of bug 1160361, since the patch here will also restore
> inspect element for new b2g servers. Bug 1160361 is now just about getting
> the GCLI actor to work right on b2g.
OK, sounds great
Assignee | ||
Comment 16•10 years ago
|
||
Comment 18•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 19•10 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)
> Should I file a separate bug for split console / other command buttons?
Since this bug is now marked fixed, I filed bug 1161131 for the remaining "command buttons / split console" issue.
Updated•9 years ago
|
Whiteboard: [devedition-40] → [polish-backlog]
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•