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

RESOLVED FIXED in Firefox 27

Status

RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

Trunk
Firefox 27
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 803346 [details] [diff] [review]
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?
Attachment #803346 - Flags: review?(rcampbell)
Blocks: 914732, 914743
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.
Attachment #803346 - Flags: review?(rcampbell)
Attachment #803346 - Flags: review?(fayearthur)
Attachment #803346 - Flags: review?(dcamp)
Thanks Philipp. How are you testing this?
Flags: needinfo?(philipp)

Comment 3

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

Looks like harth's taking care of this one.
Attachment #803346 - Flags: review?(dcamp)
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.
Flags: needinfo?(philipp)

Comment 5

5 years ago
Why do you need to add `get webProgress`?
(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.
Blocks: 912057
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?
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.
(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.
(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.
Let's do that, get the webProgress in.
Created attachment 818586 [details] [diff] [review]
Fix - v2

Too bad. Here it is.
Attachment #803346 - Attachment is obsolete: true
Attachment #803346 - Flags: review?(fayearthur)
Attachment #818586 - Flags: review?(fayearthur)
(Assignee)

Updated

5 years ago
Summary: Make inspector and style editor work for remote chrome windows → Add webProgress to root actor for the chrome style editor and inspector
(Assignee)

Updated

5 years ago
Blocks: 928018
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.
Attachment #818586 - Flags: review?(fayearthur) → review+
Created attachment 818613 [details] [diff] [review]
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 :)
Attachment #818586 - Attachment is obsolete: true
Attachment #818613 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/a2ad798f4b90
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a2ad798f4b90
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27

Comment 17

5 years ago
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Relanded in https://hg.mozilla.org/integration/fx-team/rev/292a6f5e1b0a once we finally found something better to blame.
https://hg.mozilla.org/mozilla-central/rev/292a6f5e1b0a
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.