915444 - Add webProgress to root actor for the chrome style editor and inspector

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

Right now when you connect to a chrome window remotely, style editor and inspector both don't work. This is because they are not added as global actors.

The attached patch should take care. I'm not sure if its by design that the actors are not added globally, maybe you have other plans with that?

Comment 1

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 803346 [details] [diff] [review]
Fix - v1

Review of attachment 803346 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to pass the review onto some people more familiar with the design of the inspector and style editor actors.

Comment 2

[Heather Arthur [:harth]]

Thanks Philipp. How are you testing this?

Comment 3

[Dave Camp (:dcamp)]

Comment on attachment 803346 [details] [diff] [review]
Fix - v1

Looks like harth's taking care of this one.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Heather, just by manual testing with Thunderbird. This might work for two Firefox instances:


1. Start the debugger server on Profile A by entering this in your error console:
  
    Components.utils.import("resource://gre/modules/devtools/dbg-server.jsm");
    DebuggerServer.init();
    DebuggerServer.addBrowserActors();
    DebuggerServer.openListener(6003);

2. In Profile B, use the remote connection page to connect to localhost 6003
3. Open the style editor pane


Without the patch: On the style editor page, only "new" and "import" buttons visible, the rest is gray. With the patch (hopefully), the style editor actually shows styles. Similar for the inspector.

Comment 5

[Paul Rouget [:paul]]

Why do you need to add `get webProgress`?

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Paul Rouget [:paul] from comment #5)
> Why do you need to add `get webProgress`?

For global actors, the parent actor is the root actor. The inspector uses "tabActor.webProgress", referencing the parent actor.

Comment 7

[Heather Arthur [:harth]]

I talked about this with dcamp. It's not really correct to have the inspector and style editor actors as global actors, as they're per-window, and Firefox can have multiple windows.

Could you add actors from Thunderbird with `DebuggerServer.addGlobalActor` instead?

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Yes, I can add these from Thunderbird, aside from the webProgress changes which should land globally. I don't think it solves the problem correctly though. The same problem applies to using the browser debugger, there should be some way to use these tools, right?

Not making the tool compatible because it is per window seems wrong to me, I think rather a solution for per-window tools on a chrome level should be solved. This could mean extending the connection page to advertise multiple windows, or a way inside the tool to switch between chrome windows.

I'd be happy to look into this, but I think it would be better to add this code into toolkit until we get there. Even though sub-windows won't be taken into account, this patch would at least allow inspector/style editor to work for the main browser/mail window.

Comment 9

[Heather Arthur [:harth]]

(In reply to Philipp Kewisch [:Fallen] from comment #8)
> Not making the tool compatible because it is per window seems wrong to me, I
> think rather a solution for per-window tools on a chrome level should be
> solved. This could mean extending the connection page to advertise multiple
> windows, or a way inside the tool to switch between chrome windows.

Yeah, we need a UI for switching window debugging targets, and we need a solution on the backend side
as well. We need WindowActors.

> 
> I'd be happy to look into this, but I think it would be better to add this
> code into toolkit until we get there. Even though sub-windows won't be taken
> into account, this patch would at least allow inspector/style editor to work
> for the main browser/mail window.

The problem is that there's no main browser window. It's a small case where people have more than one window open, but it's just not correct to expose these actors as per-browser globals when they're not.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Heather Arthur [:harth] from comment #9)
> (In reply to Philipp Kewisch [:Fallen] from comment #8)
> > Not making the tool compatible because it is per window seems wrong to me, I
> > think rather a solution for per-window tools on a chrome level should be
> > solved. This could mean extending the connection page to advertise multiple
> > windows, or a way inside the tool to switch between chrome windows.
> 
> Yeah, we need a UI for switching window debugging targets, and we need a
> solution on the backend side
> as well. We need WindowActors.
Yes, indeed. Have bugs been filed to take care of this? I don't recall seeing any lately.


> 
> > 
> > I'd be happy to look into this, but I think it would be better to add this
> > code into toolkit until we get there. Even though sub-windows won't be taken
> > into account, this patch would at least allow inspector/style editor to work
> > for the main browser/mail window.
> 
> The problem is that there's no main browser window. It's a small case where
> people have more than one window open, but it's just not correct to expose
> these actors as per-browser globals when they're not.
I agree its not the correct solution, but I'm thinking about the 80/20 case here. In most cases developers will be debugging one window and if not, its reasonable for them to see that if the wrong window is being targeted it might be a good idea to close the other one. The alternative is that it just won't work at all.


At the end its your decision though. If you still think this patch is not worth pushing, I'll modify the patch to only add the webProgress code and do the rest from Thunderbird.

Comment 11

[Heather Arthur [:harth]]

Let's do that, get the webProgress in.

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v2

Too bad. Here it is.

Comment 13

[Heather Arthur [:harth]]

Comment on attachment 818586 [details] [diff] [review]
Fix - v2

Review of attachment 818586 [details] [diff] [review]:
-----------------------------------------------------------------

I understand your frustration. Hopefully we can get windows working soon, as browser debugging is a priority.

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v2 (for checkin)

No worries, the workaround is easy enough. I've filed bug 928018 to figure out the next steps on accessing windows. As long as you don't WONTFIX that one I am happy :)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[(no longer active)]

Sorry, but I had to back this out because we suspect that one of the 4 patches in this push has caused frequent failures in the mochitest-bc suite (see <https://tbpl.mozilla.org/?tree=Fx-Team&fromchange=8904c520ed40&tochange=39500fdd5007&jobname=browser-chrome>).  Please verify that your patch is not the culprit by running it through the try server and verifying that multiple retriggers of the mochitest-bc suite are all green, and then request checkin-needed again.

Thanks!

(Backout: https://hg.mozilla.org/mozilla-central/rev/0d53f285a7f1)

Comment 18

[Phil Ringnalda (:philor)]

Relanded in https://hg.mozilla.org/integration/fx-team/rev/292a6f5e1b0a once we finally found something better to blame.

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/292a6f5e1b0a