1160345 - Inspect element / command buttons no longer shown for old servers

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Other command buttons missing include:

* Window / frame switching
* Split console

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I am not sure what the correct way to proceed here is.

How should we restore the command buttons for old servers?

Comment 3

[Joe Walker [:jwalker]]

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.

Comment 4

[Brian Grinstead [:bgrins]]

I'll fix this, it's something really simple

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: toolbox-buttons.patch

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?

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: toolbox-buttons.patch

Using target.hasActor instead of a trait

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 9

[Brian Grinstead [:bgrins]]

(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

Comment 10

[Brian Grinstead [:bgrins]]

(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

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: toolbox-buttons.patch

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

Comment 12

[Brian Grinstead [:bgrins]]

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.

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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?

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 15

[Brian Grinstead [:bgrins]]

(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

Comment 16

[Brian Grinstead [:bgrins]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39b30a4f3890

Comment 17

[Pulsebot]

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

Comment 18

[Phil Ringnalda (:philor)]

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

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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