Closed Bug 1268134 Opened 3 years ago Closed 3 years ago

Merge WindowActor from Positron branch

Categories

(DevTools :: General, enhancement)

enhancement
Not set

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

A WindowActor was added for Positron to target a single browser window.  This could be useful in other places, for example as way to off a DOMi replacement for browser windows in process.

Let's use this to merge in that work.
Status: NEW → ASSIGNED
Review commit: https://reviewboard.mozilla.org/r/50105/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50105/
Attachment #8748022 - Flags: review?(poirot.alex)
Attachment #8748023 - Flags: review?(poirot.alex)
Attachment #8748024 - Flags: review?(poirot.alex)
Attachment #8748025 - Flags: review?(poirot.alex)
Attachment #8748026 - Flags: review?(poirot.alex)
:ochameau, these are the same patches you looked at over in https://github.com/mozilla/positron/pull/16.  This only includes the DevTools side, of course.  No code changes so far.
Comment on attachment 8748022 [details]
Bug 1268134 - Add actor to target windows by ID.

https://reviewboard.mozilla.org/r/50105/#review46949

Shouldn't need another round, but followups around isRootActor are more than welcomed ;)

::: devtools/server/actors/window.js:14
(Diff revision 1)
> +
> +/**
> + * Creates a WindowActor for debugging a single window, like a browser window in
> + * Firefox, but it can be used to reach any window in the process.  Most of the
> + * implementation is inherited from TabActor. WindowActor exposes all tab actors
> + * via its form() request, like TabActor.

s/process/parent process/
Just to be clear that given that WindowActor is only exposed via RootActor.getWindow, it can only target windows living in the parent process.

I would also drop a note saying that the targeted window may be a chrome one, but not necessarely has to be one. (It justify the use of chrome-webnavigation observers)

::: devtools/server/actors/window.js:38
(Diff revision 1)
> +
> +WindowActor.prototype = Object.create(TabActor.prototype);
> +
> +// Bug 1266561: This setting is mysteriously named, we should split up the
> +// functionality that is triggered by it.
> +WindowActor.prototype.isRootActor = true;

At first sight, most checks in webconsole are wrong. It looks like you do want to have isRootActor to false otherwise it will act like the browser console and log more than just the messages related to the targeted window.
See onStartListeners for ex.

Given positron priority, feel free to followup on this in bug 1266561. But I would like to know why you set this flag. Did you just copied it from chrome.js or you had to keep it? It looks like highlighters.js usage will be an issue as I imagime chromeEventHandler is null on positron windows. It should be using tabActor.chromeEventHandler which takes case of this edge case.

::: devtools/server/actors/window.js:61
(Diff revision 1)
> +  }
> +
> +  TabActor.prototype._attach.call(this);
> +
> +  // Listen for chrome docshells in addition to content docshells
> +  if (this.listenForNewDocShells) {

I don't know if it is any useful to listen for new docshells. It is used in the ChromeActor for the iframe switcher in the browser toolbox, to be able to list and switch to other top level windows.
It is also used in the TabActor, but only when loaded in child process, which shouldn't happen here.

::: devtools/server/actors/window.js:64
(Diff revision 1)
> +
> +  // Listen for chrome docshells in addition to content docshells
> +  if (this.listenForNewDocShells) {
> +    Services.obs.addObserver(this, "chrome-webnavigation-create", false);
> +  }
> +  Services.obs.addObserver(this, "chrome-webnavigation-destroy", false);

Here and on _detach, I would only watch/unwatch for this event if the targeted window is chrome.

Here is some ways to detect if it is chrome, but feel free to use simplier alternatives:
  Services.scriptSecurityManager.isSystemPrincipal(doc.nodePrincipal)
  docshell.itemType == Ci.nsIDocShellTreeItem.typeChrome
Attachment #8748022 - Flags: review?(poirot.alex) → review+
Comment on attachment 8748023 [details]
Bug 1268134 - Render ascii tree art literally.

https://reviewboard.mozilla.org/r/50107/#review46959
Attachment #8748023 - Flags: review?(poirot.alex) → review+
Comment on attachment 8748024 [details]
Bug 1268134 - Add client and toolbox access to specific windows.

https://reviewboard.mozilla.org/r/50109/#review46951

Feel free to r? again if you have significant changes. But I don't expect it to be a big deal.

::: devtools/client/framework/target-from-url.js:106
(Diff revision 1)
> +      }
> +      let response = yield client.mainRoot.getWindow({
> +        outerWindowID: id,
> +      });
> +      form = response.window;
> +      chrome = true;

Hum. Do you actually need to be chrome here?
If yes, may be we should set that only when the targetted document is a chrome one?

If my comment about supporting chrome *and* content windows are not relevant, just document getWindow as something that is only meant to target chrome windows and eventually throw if we try to attach a content one.
Attachment #8748024 - Flags: review?(poirot.alex) → review+
Comment on attachment 8748025 [details]
Bug 1268134 - Setup fake toolbox host for standalone windows.

https://reviewboard.mozilla.org/r/50111/#review46953

::: devtools/client/framework/toolbox-init.js:40
(Diff revision 1)
> +      // that is fine to skip and doesn't make sense when using the current
> +      // window.
> +      setAttribute() {},
> +      ownerDocument: document,
> +    };
> +  }

That may no longer be required if I make progress on bug 1266134 someday.
Attachment #8748025 - Flags: review?(poirot.alex) → review+
Comment on attachment 8748026 [details]
MozReview Request: Bug 1268134 - Destroy toolbox on host unload. r=ochameau

https://reviewboard.mozilla.org/r/50113/#review46955

::: devtools/client/framework/toolbox-init.js:102
(Diff revision 1)
> +  toolboxOpened.then(toolbox => {
> +    let hostUnload = () => {
> +      host.contentWindow.removeEventListener("unload", hostUnload);
> +      toolbox.destroy();
> +    };
> +    host.contentWindow.addEventListener("unload", hostUnload);

Isn't host.contentWindow == window here?

Also, if that helps, I think we could register this unload listener from toolbox.js:Toolbox.open.

::: devtools/client/framework/toolbox-init.js:103
(Diff revision 1)
> +    let hostUnload = () => {
> +      host.contentWindow.removeEventListener("unload", hostUnload);
> +      toolbox.destroy();
> +    };
> +    host.contentWindow.addEventListener("unload", hostUnload);
> +  });

I think myk suggested this catch(f)+then(s), but isn't it equivalent to: then(s, f)?
Attachment #8748026 - Flags: review?(poirot.alex) → review+
https://reviewboard.mozilla.org/r/50111/#review46963

::: devtools/client/framework/toolbox-init.js:29
(Diff revision 1)
>    // `host` is the frame element loading the toolbox.
>    let host = window.QueryInterface(Ci.nsIInterfaceRequestor)
>                     .getInterface(Ci.nsIDOMWindowUtils)
>                     .containerElement;
>  
> +  // If there's no containerElement, use the current window.

You may add that it happens when loading about:devtools-toolbox as a toplevel document.
jryans: any chance you can update your patches in this bug to current trunk and land them?

Alternately, here's a patch that takes the code you landed in Positron #16 and updates it to apply to current trunk, if you'd rather abandon your patches and start over.  (I haven't looked closely at them yet, so I'm unsure how different they are from the Positron changeset.)
Attachment #8837864 - Flags: feedback?(jryans)
Erm, actually that previous attachment was the original changeset.  Here's the equivalent updated to latest trunk.

(Also feedback -> needinfo per request in jryans's Bugzilla name.)
Attachment #8837864 - Attachment is obsolete: true
Attachment #8837864 - Flags: feedback?(jryans)
Flags: needinfo?(jryans)
(Any kind of bug flag is fine, so I can ensure requests aren't missed in the deluge of bug mail.)

Looks like I added additional changes here on top of the Positron change.  I'll try updating this branch and pressing forward.
Flags: needinfo?(jryans)
Destroy on unload patch is no longer needed here.  (Similar functionality was landed in bug 1298082.)
Attachment #8837867 - Attachment is obsolete: true
Comment on attachment 8748022 [details]
Bug 1268134 - Add actor to target windows by ID.

https://reviewboard.mozilla.org/r/50105/#review46949

> s/process/parent process/
> Just to be clear that given that WindowActor is only exposed via RootActor.getWindow, it can only target windows living in the parent process.
> 
> I would also drop a note saying that the targeted window may be a chrome one, but not necessarely has to be one. (It justify the use of chrome-webnavigation observers)

Okay, I've added these notes.

> At first sight, most checks in webconsole are wrong. It looks like you do want to have isRootActor to false otherwise it will act like the browser console and log more than just the messages related to the targeted window.
> See onStartListeners for ex.
> 
> Given positron priority, feel free to followup on this in bug 1266561. But I would like to know why you set this flag. Did you just copied it from chrome.js or you had to keep it? It looks like highlighters.js usage will be an issue as I imagime chromeEventHandler is null on positron windows. It should be using tabActor.chromeEventHandler which takes case of this edge case.

Well, I remember needing this for Positron, but I can't remember why anymore. x_x So, I think I'll leave it as-is for the moment, but we should clean it up as suggested so it's clear what each actor is assuming about the flag.

> I don't know if it is any useful to listen for new docshells. It is used in the ChromeActor for the iframe switcher in the browser toolbox, to be able to list and switch to other top level windows.
> It is also used in the TabActor, but only when loaded in child process, which shouldn't happen here.

I guess it's not needed, since it's only set to true in `ChromeActor`, so here it would always be false.

> Here and on _detach, I would only watch/unwatch for this event if the targeted window is chrome.
> 
> Here is some ways to detect if it is chrome, but feel free to use simplier alternatives:
>   Services.scriptSecurityManager.isSystemPrincipal(doc.nodePrincipal)
>   docshell.itemType == Ci.nsIDocShellTreeItem.typeChrome

Looks like the notification type is determined by docShell type[1], so I'll check that here.

[1]: http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/docshell/base/nsDocShell.cpp#5770
Comment on attachment 8748024 [details]
Bug 1268134 - Add client and toolbox access to specific windows.

https://reviewboard.mozilla.org/r/50109/#review46951

> Hum. Do you actually need to be chrome here?
> If yes, may be we should set that only when the targetted document is a chrome one?
> 
> If my comment about supporting chrome *and* content windows are not relevant, just document getWindow as something that is only meant to target chrome windows and eventually throw if we try to attach a content one.

Well, we do want full chrome access to the window, so it seems correct to set for that case.

I don't think we have a way to check the window type here, since this is the client and the window could be remote.
Comment on attachment 8748025 [details]
Bug 1268134 - Setup fake toolbox host for standalone windows.

https://reviewboard.mozilla.org/r/50111/#review46963

> You may add that it happens when loading about:devtools-toolbox as a toplevel document.

Okay, note added.
Comment on attachment 8748025 [details]
Bug 1268134 - Setup fake toolbox host for standalone windows.

https://reviewboard.mozilla.org/r/50111/#review46953

> That may no longer be required if I make progress on bug 1266134 someday.

It seems like this is still needed at the moment, as `toolbox-host-manager.js` makes the same assumptions.
Attachment #8748026 - Attachment is obsolete: true
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/981fefe750ae
Add actor to target windows by ID. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/be54236804e1
Render ascii tree art literally. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/fac67b07571e
Add client and toolbox access to specific windows. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/073f05a135cc
Setup fake toolbox host for standalone windows. r=ochameau
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.