1268134 - Merge WindowActor from Positron branch

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

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

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.

Comment 1

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

Attachment: Bug 1268134 - Add actor to target windows by ID.

Review commit: https://reviewboard.mozilla.org/r/50105/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50105/

Comment 2

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

Attachment: Bug 1268134 - Render ascii tree art literally.

Review commit: https://reviewboard.mozilla.org/r/50107/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50107/

Comment 3

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

Attachment: Bug 1268134 - Add client and toolbox access to specific windows.

Review commit: https://reviewboard.mozilla.org/r/50109/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50109/

Comment 4

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

Attachment: Bug 1268134 - Setup fake toolbox host for standalone windows.

Review commit: https://reviewboard.mozilla.org/r/50111/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50111/

Comment 5

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

Attachment: MozReview Request: Bug 1268134 - Destroy toolbox on host unload. r=ochameau

Review commit: https://reviewboard.mozilla.org/r/50113/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50113/

Comment 6

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

: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 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cac03841cfc4

Comment 8

[Alexandre Poirot [:ochameau]]

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

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8748023 [details]
Bug 1268134 - Render ascii tree art literally.

https://reviewboard.mozilla.org/r/50107/#review46959

Comment 10

[Alexandre Poirot [:ochameau]]

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.

Comment 11

[Alexandre Poirot [:ochameau]]

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.

Comment 12

[Alexandre Poirot [:ochameau]]

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)?

Comment 13

[Alexandre Poirot [:ochameau]]

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.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: code in Positron #16 updated to current trunk

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

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Attachment: code in Positron #16 updated to current trunk

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

Comment 16

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

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

Comment 17

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

Destroy on unload patch is no longer needed here.  (Similar functionality was landed in bug 1298082.)

Comment 18

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

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 19

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

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 20

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

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 21

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

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.

Comment 22

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e74b8d68fec8843167f36d19c03576a3ee295daf

Comment 23

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

Comment on attachment 8748022 [details]
Bug 1268134 - Add actor to target windows by ID.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50105/diff/1-2/

Comment 24

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

Comment on attachment 8748023 [details]
Bug 1268134 - Render ascii tree art literally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50107/diff/1-2/

Comment 25

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

Comment on attachment 8748024 [details]
Bug 1268134 - Add client and toolbox access to specific windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50109/diff/1-2/

Comment 26

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

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50111/diff/1-2/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/981fefe750ae
https://hg.mozilla.org/mozilla-central/rev/be54236804e1
https://hg.mozilla.org/mozilla-central/rev/fac67b07571e
https://hg.mozilla.org/mozilla-central/rev/073f05a135cc