880511 - Web Console and Style Editor make hardcoded references to the browser window

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

If no window is passed, then navigator:browser is assumed. This doesn't work for other applications like Thunderbird. I haven't thought this through yet, but either the window will have to be passed in or another mechanism needs to be found to replace the window.

Right now I'm hacking around by replacing the constructor function, but I'd like a more future proof solution.

http://mxr.mozilla.org/mozilla-central/search?string=navigator%3Abrowser&find=toolkit%2Fdevtools&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 1

[Mihai Sucan [:msucan]]

See also bug 860672 comment 8

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Mihai, thanks for the link. Does this mean that with that bug fixed, the _window is not needed on the actor, making this bug obsolete? In that case I'd rather fix the other one of course.

From reading that comment it sounds to me like I would need to go through all callers that use the _window and find out if there is a way to change the code to work without a window?

Comment 3

[Mihai Sucan [:msucan]]

(In reply to Philipp Kewisch [:Fallen] from comment #2)
> Mihai, thanks for the link. Does this mean that with that bug fixed, the
> _window is not needed on the actor, making this bug obsolete? In that case
> I'd rather fix the other one of course.
> 
> From reading that comment it sounds to me like I would need to go through
> all callers that use the _window and find out if there is a way to change
> the code to work without a window?

Fixing bug 860672 will not fix this bug. Each actor is independent.

We still need _window. In bug 860672 I simply suggest a different window object: the browser console window object, instead of the browser chrome window.

I expect the situation is similar for the style editor actor. Instead of expecting a browser chrome window you may need to change the actor to allow for different kinds of windows. In web console's case the browser console window would suffice, but not in the case of the style editor. I haven't looked at that code yet, so I cannot tell you what needs to be changed.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Mihai Sucan [:msucan] from comment #3)

> Fixing bug 860672 will not fix this bug. Each actor is independent.
> 
> We still need _window. In bug 860672 I simply suggest a different window
> object: the browser console window object, instead of the browser chrome
> window.
Ah ok. The problem with using the browser console window is that for non-firefox apps that do not have a browser console, this window will not be existing. There still needs to be some fallback, or a way to override what window is used.

> I expect the situation is similar for the style editor actor. Instead of
> expecting a browser chrome window you may need to change the actor to allow
> for different kinds of windows. In web console's case the browser console
> window would suffice, but not in the case of the style editor. I haven't
> looked at that code yet, so I cannot tell you what needs to be changed.
I haven't looked into all bits yet, but some of the window references can be removed. I'll take a deeper look later.


So to fix this bug I will go with what I discussed with dcamp yesterday: Add a method DebuggerServer.setChromeWindowType() that takes a windowtype name and uses that everywhere that navigator:browser is currently used. I will also be changing the code that checks instanceof BrowserTabActor and instead use a well known attribute on the actor that holds the window.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

I went with a property instead of a method, since its necessary to get the chromeWindowType from the actors too.

We probably want to get this on try, as I don't have a compiled tree with Firefox, nor have I had a chance to test any of the b2g changes. There is also still a TODO in there specific to that big comment in the webconsole actor.

In theory the b2g actor should return the right window always, but in the webconsole actor there is a hack to use the global window for the b2g tab actor. This doesn't fit in well with the new structure and I don't think always using the global window is the right thing to do for b2g.

Maybe you or someone else can help me out on these last open issues, therefore requesting feedback first and not review.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Note to self: bug 887027 will land some changes that make use of contentWindow, which is renamed to window in this patch.

Comment 7

[Dave Camp (:dcamp)]

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

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

This approach looks good to me.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Philipp Kewisch [:Fallen] from comment #5)
> In theory the b2g actor should return the right window always, but in the
> webconsole actor there is a hack to use the global window for the b2g tab
> actor. This doesn't fit in well with the new structure and I don't think
> always using the global window is the right thing to do for b2g.
Dave, could you refer me to someone from the b2g team that could help me out on this point? I'd like to make sure this doesn't break on b2g, but I'm not sure how to continue.

Ideally this hack is obsolete by now and we can just return the window from the parent actor, i.e b2g's version of BrowserTabActor.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I was told ochameau knows a little bit about this, shifting needinfo request.

Comment 10

[Alexandre Poirot [:ochameau]]

See bug 817580 comment 32 with my comment followed by Mihai's answer.
Basically, you made what I suggested doing... so f+++ :)

The only thing I don't know is how important _isGlobalActor is on b2g.
I'll apply your patch and test it on b2g to see if the console still works as expected.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Alexandre Poirot (:ochameau) from comment #10)
> See bug 817580 comment 32 with my comment followed by Mihai's answer.
> Basically, you made what I suggested doing... so f+++ :)
Great, glad this is bringing things forward :) So I guess we could mark bug 889361 duplicate of this one? Or is there anything else needed to fix that bug?

 
> The only thing I don't know is how important _isGlobalActor is on b2g.
> I'll apply your patch and test it on b2g to see if the console still works
> as expected.

The _isGlobalActor check is replaced by this.parentActor.isRootActor. If the webconsole actor is added to the root actor (i.e when doing chrome debugging), then this is true. If needed we could just set isRootActor = true on the b2g variant of the tab actor. Let me know how it goes.

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to Philipp Kewisch [:Fallen] from comment #11)
> Great, glad this is bringing things forward :) So I guess we could mark bug
> 889361 duplicate of this one?

Yes.

> The _isGlobalActor check is replaced by this.parentActor.isRootActor. If the
> webconsole actor is added to the root actor (i.e when doing chrome
> debugging), then this is true. If needed we could just set isRootActor =
> true on the b2g variant of the tab actor. Let me know how it goes.

I can't see any difference with/without this patch regarding console behavior, so
 it looks good for b2g!

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Ok, pushed to try to make sure this works:
https://tbpl.mozilla.org/?tree=Try&rev=076c2e478038

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Note that there is another instance that you may want to look at in the patch for bug 895360.

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Thanks, I'll take a look. Try seems green aside from a few unrelated issues, so I will update the patch and attach it for review.

Comment 17

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v2

I guess the changes to bug 895360 depend on which lands first, I'll keep it in mind though. There was also one line from bug 817580 that needed to be changed (contentWindow vs window).

I think this is ready for review now. Dave, I see you have a few bugs in your queue, but you f+'d v1 so I have chosen you. Please delegate the review if needed.

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/07f2781b55ce