1190687 - Implement webNavigation.onCreatedNavigationTarget

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://developer.chrome.com/extensions/webNavigation#event-onCreatedNavigationTarget

The documentation explains it poorly, but this seems to fire when an existing page uses window.open to create a new tab or window. It does not fire if the user hits Ctrl-T, for example.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

After some testing on Chromium-based browsers about to the behaviour that this webNavigation's event should have,
I found that it works as follows:

- onCreatedNavigationEvent IS sent
  - link clicked with mouse + modifier (e.g. Ctrl/Command to open a link in a new tab and Shift to open a link  in a new window)
  - open in new tab / open in new window from the context menu
 
- onCreatedNavigationEvent IS NOT sent
  - window.open
  - new tab opened using the shortcut or the new empty tab button

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

The onCreatedNavigationTarget event passes the following details to its listeners:

- sourceTabId (the tabId of the frame which has generated the navigation event)
- sourceFrameId (the frameID of the frame which has generated the navigation event)
- sourceProcessId (which we do not support, so it is going to always be undefined)
- tabId (which is the tabId of the newly created tab) 
- url (the url which is going to be loaded in the newly created tab)
- timestamp

It doesn't have a frameId because this event is raised after the tab has been created but before the tab has been fully initialized and ready to load the url.

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

After some digging into dxr and a number of experiments,  follows a brief description of the strategy that I'm going to use to build the upcoming proposed implementation:


1: "how to intercept links which are going to be opened using a mouse click or the context menu options"
  - we need to intercept this in the content process, near to where the event is caught 
    (browser/base/content/content.js) 
  - the context menu already propagate the |sourceWindowId| of the frame which originates the request [1]
  - the mouse click and the modifiers state is already propagated in a json message [2], and we can use it to
    propagate the |sourceWindowId| as well
   

2: "how to propagate the |sourceWindowId| to the |openLinkIn| method":
  - |openLinkIn| is responsible of opening the url in the new tab/window
  - we need to introduce some change so that the |sourceWindowId| from the event's json payload 
    described above reaches the |openLinkIn| method:
    - to cover the "context menu" scenario: |nsContextMenu.prototype._openLinkInParameters| [3] 
    - to cover the "mouse click" scenario: |ContentClick.contentAreaClick| [4]


3: "how to collect the source and destination tab and send all the collected info to ext-webNavigation.js"
  - the |openLinkIn| helper implemented in utilityOverlay.js [5] is where the new tab or 
    the new window is created, and we can use it to collect the remaining info (the source and destination tab):
    - the |sourceWindowId| reached |openLinkIn| through the extra params
    - in the "new tab" scenario [6]:
      - the |sourceTab| is the current selected tab (|w.gBrowser.selectedTab|)
      - the |tab| is the return value of |w.gBrowser.loadOneTab| [6]
    - in the "new window" scenario [7]:
      - the |sourceTab| is the current selected tab (|w.gBrowser.selectedTab|, 
        and |window.gBrowser.selectedTab| if |w| is null?)
      - the |tab| is the selectedTab of the window returned by |Services.ww.openWindow| [7], 
        once it is loaded

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#172
[2]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#436
[3]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#942
[4]: https://dxr.mozilla.org/mozilla-central/source/browser/modules/ContentClick.jsm#71
[5]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js
[6]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#358
[7]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#286

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

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

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8723118 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

I'm pushing this patch for an initial feedback, about both the general approach and a couple of points on which I'd like to get a second point of view, e.g.:

- I'm lazy loading "WebNavigation.jsm" module from "utilityOverlay.js" to be
  able to bring the collected info (|sourceTab|, |tab|, |sourceWindowId| and
  |url|) from where they are collected to where they will be routed to reach the
  "ext-webNavigation.js" listeners.  

- when a link is going to be opened in a new window, I have to wait the window
  to be fully loaded to be able to get the tab from it, to do that 
  I'm listening to the load event on the newly created window.

- it seems that when openLinkIn is going to open the url in a new window
  |w| can be undefined/null, in which conditions did it happens?

any idea on alternative strategies for the first two above?

(side notes: This patch currently lacks the new test case, and it will be rebased on the patch from Bug 1239349 which will introduce the onHistoryStateUpdated webNavigation event)

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

In Comment 3 (and in the patch Attachment 8723118 [details] submitted for initial feedback), I've not mentioned the need of "filter out" any onCreatedNavigationTarget which shouldn't be sent:

- the window.open doesn't take the same "path" described in Comment 1 and for this reason it doesn't produce any event that have to be filtered out explicitly

- creating a new empty tab using the shortcut or the button will curretly produce an invalid onCreatedNavigationTarget, which we want to filter out because it shouldn't be generated (e.g. we can probably filter out every onCreatedNavigationTarget event which doesn't have all the information which is supposed it should have)

Comment 7

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/36365/#review33475

For the most part, your code looks pretty good. I'm worried, though, that this doesn't feel like the right way to handle this. It seems like it leaves a lot of corner cases. And even if Chrome doesn't send this event for `window.open`, that seems more like a bug than a feature.

Have you thought about doing this from a content policy, or an onLocationChange handler, instead? Is there some reason they wouldn't work?

::: browser/base/content/utilityOverlay.js:292
(Diff revision 1)
> +      win.removeEventListener("load", waitNewWindowLoaded, true);

You need to make sure the event target is the browser window, and not a child document here. And, generally for browser windows, you're expected to wait for browser-delayed-startup-finished rather than load.

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Kris Maglione [:kmag] from comment #7)
> https://reviewboard.mozilla.org/r/36365/#review33475
> 
> For the most part, your code looks pretty good. I'm worried, though, that
> this doesn't feel like the right way to handle this. It seems like it leaves
> a lot of corner cases. And even if Chrome doesn't send this event for
> `window.open`, that seems more like a bug than a feature.

I've the same feeling about the 'window.open' (another one is: "maybe is because the window open is
already handler by the integrated popup blocker?", or "maybe both (a bug introduced by the integrated popup blocker)") 
 
> Have you thought about doing this from a content policy, or an
> onLocationChange handler, instead? Is there some reason they wouldn't work?

The main reason is that I had to collect both the source and destination parts, both needed to compose the final event details
(and frame ids have to be collected in the content process but tab ids have to be collected in the main process)

I didn't find a way to get all the data all in one place, so I thought that my only option was to follow the events flowing through the processes and find the best places to intercept and forward the needed pieces (and it was a way to determine which components currently create or have access to the needed pieces).

> 
> ::: browser/base/content/utilityOverlay.js:292
> (Diff revision 1)
> > +      win.removeEventListener("load", waitNewWindowLoaded, true);
> 
> You need to make sure the event target is the browser window, and not a
> child document here. And, generally for browser windows, you're expected to
> wait for browser-delayed-startup-finished rather than load.

Thanks! during the prototyping of this patch I felt that there was a better event to subscribe in this scenario.

I'm going to tweak the patch based on this initial feedback in the mean time.

Comment 9

[Kris Maglione [:kmag]]

It looks like I'm going to have to do something similar to this for bug 1238314, anyway. Maybe we should just handle them both at the same time.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8723118 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

I'm pretty sure Chrome does invoke onCreatedNavigationTarget for window.open as well as for link tags that open in a new window. At least it does for me.

I'm not sure how to make this more general, unfortunately. I can't think of a way to figure out the sourceTabId and sourceFrameId using a web progress listener or content policy. If you're lucky, window.opener will give you what you want. But that doesn't work if the user did "Open link in new tab/window" since the opener is null in that case.

I don't see any alternative but to hook some of these browser chrome functions. However, I think we could maybe pass this stuff through a little more cleanly. It would be nice if loadOneTab took optional parameters for the source tab/frame. We could also add a function for opening a link in a new window that wrapped Services.ww.openWindow.

As Kris mentioned, we'll also need some new code to cover the window.open case.

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Bill McCloskey (:billm) from comment #10)
> Comment on attachment 8723118 [details]
> MozReview Request: Bug 1190687 - [webext] Add
> webNavigation.onCreatedNavigationTarget implementation
> 
> I'm pretty sure Chrome does invoke onCreatedNavigationTarget for window.open
> as well as for link tags that open in a new window. At least it does for me.

Just checked again and Chrome does invoke onCreatedNavigationTarget if the popup blocking has not blocked the window.open
(and it doesn't if we allow Chrome to open the new tab after the popup blocker has already blocked it)  

> I'm not sure how to make this more general, unfortunately. I can't think of
> a way to figure out the sourceTabId and sourceFrameId using a web progress
> listener or content policy. If you're lucky, window.opener will give you
> what you want. But that doesn't work if the user did "Open link in new
> tab/window" since the opener is null in that case.
> 
> I don't see any alternative but to hook some of these browser chrome
> functions. However, I think we could maybe pass this stuff through a little
> more cleanly. It would be nice if loadOneTab took optional parameters for
> the source tab/frame. We could also add a function for opening a link in a
> new window that wrapped Services.ww.openWindow.

Sounds good, I'm going to look into them to get a picture on how to use them (and eventually change them) to pass the needed info more cleanly. 

> As Kris mentioned, we'll also need some new code to cover the window.open
> case.

Absolutely, coordinate the two efforts seems the right thing to do here.

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

As per discussion with Bob, it could be interesting to check how AdBlock Plus chrome extension uses this event.

Once we know if this event is critical for AdBlock Plus, we can evaluate what would be the effect of turning this event in a stub as for onTabReplaced in Bug 1280404, or if it is better to re-evaluate the attached proposed patch, and fix this issue separately from Bug 1238314, which is not a blocker but it is a related missing feature in the tabs API.

Comment 14

[Bob Silverberg [:bsilverberg]]

I took a look at the one place where AdBlock Plus uses onCreatedNavigationTarget, a snippet with context can be found at [1], with the listener for onCreatedNavigationTarget found at [2]. 

From what I can glean from the code, it looks like ABP uses it to initialize the listeners for the popupBlocker as well as to register the new tab in its cache of tabs and check it for a pop-up. It seems to make use of the following properties of the `details` object provided by onCreatedNavigationTarget:
- tabId
- url
- sourceTabId
- sourceProcessId
- sourceFrameId

Not knowing much about the ABP codebase, it does look like it's integral to the popup blocker, although that's not to say that what they are doing couldn't be achieved by another API event.

[1] https://gist.github.com/bobsilverberg/b77f8de3b6bc1c64b3898f1bd48d1372
[2] https://gist.github.com/bobsilverberg/b77f8de3b6bc1c64b3898f1bd48d1372#file-oncreatednavigationtarget-js-L114-L135

Comment 15

[Edoardo Putti [:edoput]]

Any news on this? I would like to use this API in Firefox with this addon https://github.com/EdoPut/back_in_time

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8723118 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

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

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8723118 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

https://reviewboard.mozilla.org/r/36365/#review33475

I updated this patch using the same approach of the previous version (but I changed it to observe the "browser-delayed-startup-finished" notification as suggested  and added the related tests).

The other suggested approaches (using the content policy or the onLocationChange) doesn't seem to have access to the various pieces that we need to correlate (source tab and frame id and the target tab) or to happen at the right time, but maybe I'm missing something, nevertheless currently we are missing this event and if we lack a better strategy, then it did seem reasonable to me to proceed to update this patch and add the related tests.

About the `window.open` scenario: I can agree that the reason why Chrome does not send this message when a new tab is opened using `window.open` can be a limitation or a bug in their implementation, but given that a different behavior can lead to some incompatibilities and that to track the `window.open` we need to follow the source frame and tab through a different path (which is completely do-able but pretty different and adds even more corner cases, e.g. the new tab can be blocked by the popup blocking feature and the window.open is actually executed twice, one internally once the user unblock the popup), I would like to propose to discuss and handle it in a follow up (that I can file in a different bugzilla issue if we decide that this can be a reasonable strategy).

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

In the meantime, I'm taking a deeper look at how we can cover the `window.open` scenario properly.

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Kris and Bill,

I've been exploring more deeply the various "code paths" that brings Firefox to open a new "navigation target" (a tab or a window), especially the `window.open` scenario that is not covered yet from the attached patch and unfortunately I don't currently see any way to make this event able to work in the `window.open` scenario without changes in some of the underlying C++ code
(while I'm still convinced that the attached patch is still valid for tracking the tabs/window opened from the context menu on Firefox Desktop in e10s mode).

As a preample, it is important to note that this webNavigation event contains the following informations:

- a sourceTabId (retrieved from the source tab browser element on Firefox Desktop)
- a tabId (the newly created tab browser element)
- a frameId (based on the outerWindowId of the iframe that originated the request)
- an url (the url that is going to be loaded, even if the url is not yet loaded in the newly created tab or window)

and for this reason the approach currently part of Bug 1238314 (Implement browser.tabs opener functionality) is not enough for this event (because it could probably give us the sourceTab, but not the iframe outerWindowId that we need to provide the frameId property).

And so, I looked into a different approach, based on one of the comment from Bill in that issue (https://bugzilla.mozilla.org/show_bug.cgi?id=1238314#c2),
and then evaluating it by prototyping the changes that can send this event on the window.open using the following strategy:

- retrieve the needed informations from the content process and forward them to the parent process through the ipc using the existent nsOpenURIInFrameParams 
(extended with the additional properties that we need to have in the parent process, where the tabs actually exists but the frame is not directly accessible)
- from the dom/ipc/ContentParent (which is the first component that has all the needed pieces) forwards them to WebNavigation.jsm using an ObserverService notification (using a nsIPropertyBag2 as the subject and storing the four pieces of information mentioned above in it).

Unfortunately this was still only part of the story, because it doesn't currently cover:

- tabs/windows opened in non e10s mode (I didn't yet completed to explore this scenario, but by reading the code and from some gdb session it seems that in the nsWindowWatcher::OpenWindowInternal all the needed info can already be retrieved without the need to forward them from the content process to the parent process through the ipc like in the e10s mode)

- tabs opened in Firefox for Android (I started to look a bit into the components involved and how different it is from the desktop browser, and I am going to look at the Bug 1260548 Basic tabs API support for Android WebExtensions, which should be helpful to get a better picture, but any additional information is obviously welcome)

I'm going to cleanup the experimental patch (even if it is still a rough draft, I'm not completely happy with it and it doesn't cover everything yet) to attach it to this issue for additional feedbacks and ideas, but I wanted to provide this initial picture so that we can discuss this issue in more detail, fill the gaps/missing pieces (e.g. the Android and non-e10s scenarios) and evaluate if we can extract a plan from these experimentations (or if I'm still off-track).

Comment 20

[Andy McKay]

Upping the priority to reflect its current need.

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think you're on the right track, Luca. First, I did some testing myself to make sure I understand when this event is invoked. Here's what I found:

* click on link with target="_blank":
  - Chrome sets openerTabId in onCreated
  - sourceTabId is set in onCreatedNavigationTarget
* choose "Open in new tab" from context menu
  - same as above
* choose "Open in new window" from context menu
  - Chrome does *not* set openerTabId in onCreated
  - sourceTabId is correct in onCreatedNavigationTarget
* window.open("url")
  - Chrome sets openerTabId in onCreated
  - sourceTabId is set in onCreatedNavigationTarget
* window.open("url", "_blank", "width=400,height=400")
  - Chrome does *not* set openerTabId in onCreated
  - sourceTabId is set in onCreatedNavigationTarget

So basically, onCreatedNavigationTarget always gives you the opener. openerTabId is undefined when the page opens in a new window (as opposed to a new tab). I also tested these cases with "rel=noopener", but it made no difference.

As I said, you're on the right track. In the e10s case, I think it makes sense to pass the outer window ID of the opener window up the the parent. From there it can go in the nsIOpenURIInFrameParams, as you described. That is then passed to OpenURIInFrame. Instead of using an observer, please just add your code here:
http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/browser/base/content/browser.js#5109

The non-e10s case is similar but easier. We'll end up here:
http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/browser/base/content/browser.js#5016
You can get the outer window ID of aOpener to get the source frame ID (and then use that mapping we have to get the tab ID). I *think* this should work even in the noopener case, but please test. If it doesn't we'll just need to do a little extra work.

Comment 22

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Bill,
Thanks for the initial feedback.

It is true that the openURI and openURIInFrame receive the "new tab" request in non-e10s mode and e10s mode, but unfortunately:

- they do not receive the final URL to load, they basically provide the window object where the final url will be loaded (but, openURIInFrame was my first pick during the experimentations, and I've been able to propagate the final URL to openURIInFrame through its params)

- they do not receive the "new window" requests (which seems to be completely handled in the underlying C++ code)

In the e10s mode, it seems to me that in 'ContentParent::CommonCreateWindow':

- if the url is going to be loaded in a new tab, 'openURIInFrame' is going to be called

- if the url is going to be loaded in a new window, 'nsPIWindowWatcher's OpenWindowWithTabParent/OpenWindowWithoutTabParent' are going to be called instead

In non e10s mode, it seems that in 'nsWindowWatcher::openWindowInternal:

- if the url is going to be loaded in a new tab, the window is provided by nsIContentTreeOwner and 'openURI' is going to be called (without the final URL to be loaded in the tab)

- if the url is going to be loaded in a new window, 'nsWindowWatcher::openWindowInternal' is going to create a new chrome window

So, to summarize what I saw:

- the final URL (if any, e.g. the window.open can be also called without the url parameter) is always loaded by the nsWindowWatcher::OpenWindowInternal

- a new tab is always provided with some help from a component implemented in "Privileged JavaScript" code

- a new window is always provided by components implemented in C++

am I reading it wrong or missing something about how we handle the "open url in a new window" scenario internally?

Thanks in advance for your help!

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Hi Luca,
Yes, you're right. I forgot about opening a new window. Perhaps the best thing to do is to add an observer notification that fires in nsWindowWatcher::OpenWindowInternal. You can pass it the URL as the data and the window as the subject. That should cover all the different window opening cases.

Comment 24

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from window.open.

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

Comment 25

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8723118 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

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

Comment 26

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8836101 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from window.open.

Hi Bill,
Thanks again for your feedback.

It seems (at least based on the pieces discussed so far) that the approach that I'm currently using to be able to send the onCreatedNavigationTarget events related to the window.open calls in my current draft patches is not that far what we discussed, and so we can move to the next step:
I'm now attaching the updated patches to this issue to discuss the implementation in more detail (and in the meantime I will increase the test cases, which are currently pretty basic).

As briefly described in the previous comments, this is the patch sends a notification from the underlying C++ code.

Follows a brief summary about the changes applied by this patch and the reasons behind them:

- from nsWindowWatcher::nsWindowWatcher::OpenWindowInternal, which sends the notification for the window.open calls in the main process

- from ContentParent::CommonCreateWindow, which sends the notification from the window.open calls that has been originated from the child processes

I'm still using a property bag to send the needed information in the notification event (but I'm open to suggestions, but I thought it was useful to look at the patch together as soon as possible, before starting to search for a better way to send the needed pieces through the observer service), mostly because I have to send in a single "observer service notification event":

- from ContentParent: url, frameOuterWindowId, sourceTabBrowser (the browser element related to the source tab) and tabBrowser (the browser element related to the new tab)

- from nsWindowWatcher: url (string), sourceTabDocShell (the docshell related to the source tab, while the frameOuterWindowId will be retrieved from the docShell directly) and tabDocShell (the docshell related to the new tab)

The observer of the "webNavigation-createdNavigationTarget" event is registered once in the main process from the WebNavigation.jsm module, and it normalizes the received data into a single format and send it to the listener subscribed from ext-webNavigation.js to turn it into the expected webNavigation event.

I'm trying this patches both manually and using the automated tests (which are pretty basic currently and I'm going to expand them to check all the event properties and more scenarios) in both e10s (with and without the extension process) and in non e10s (while I still need to try them on Android).

(I also pushed both these patches to try here to check if they break any of the other tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2c07b8c88b929e423498178f5c03f21de8a0eed , it is a previous version, but I didn't applied any big changes between this try build and the attached patches, just a small cleanup).

Comment 27

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8723118 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

This patch is an updated version of the patch that was already attached to this issue, the main changes are:

- it uses a observer service notification event to send the needed pieces from  "utilityOverlay.js" to the observer listener registered from "WebNavigation.jsm" (mostly because, if we are going to use this strategy to receive the data from the underlying C++ code, it is probably better to always use the same strategy to send the data from the "privileged JS code")

- it is rebased on top of the changes applied by Kris to port the tabs API on Android (and the changes on the related internal helpers, e.g. how a browser element is converted in its tabId and windowId)

- it also covers the "context menu" and the "ctrl/shift + click" scenarios from the main process (e.g. to cover new tab/window opened in non e10s mode, or on e10s mode when the links are opened from tabs running in the main process)

Comment 28

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8836101 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from window.open.

https://reviewboard.mozilla.org/r/111566/#review113920

I made a lot of comments, but I think the main issue is that I would like the notification to happen only in nsWindowWatcher.cpp, no matter if we're in the parent or the child process. Then WebNavigationContent.jsm can listen for it and forward it to the parent. Is there any reason you can't do it that way?

::: dom/ipc/ContentChild.cpp:715
(Diff revision 1)
>        rv = browserChrome3->ShouldLoadURIInThisProcess(aURI, &shouldLoad);
>        if (NS_SUCCEEDED(rv) && !shouldLoad) {
>          nsAutoCString baseURIString;
>          float fullZoom;
>          OriginAttributes originAttributes;
> +        uint64_t frameOuterWindowID;

This needs to be initialized to something since GetWindowParamsFromParent can return early.

::: dom/ipc/ContentChild.cpp:727
(Diff revision 1)
>          URIParams uriToLoad;
>          SerializeURI(aURI, uriToLoad);
> +
> +        // The url notified in the webNavigation.onCreatedNavigationTarget event.
> +        URIParams finalURIToLoad;
> +        SerializeURI(aURI, finalURIToLoad);

What's the purpose of this? You already have uriToLoad.

::: dom/ipc/ContentChild.cpp:816
(Diff revision 1)
>        // send nullptr's for primitives. We indicate that the nsString for the URI
>        // should be converted to a nullptr by voiding the string.
>        url.SetIsVoid(true);
>      }
>  
>      newChild->SendBrowserFrameOpenWindow(aTabOpener, renderFrame, NS_ConvertUTF8toUTF16(url),

We should make this work for mozbrowser elements too.

::: dom/ipc/ContentChild.cpp:824
(Diff revision 1)
>                                           &layersId);
>    } else {
>      nsAutoCString baseURIString;
>      float fullZoom;
>      OriginAttributes originAttributes;
> +    uint64_t frameOuterWindowID;

Same issue with initializing this.

::: dom/ipc/ContentChild.cpp:832
(Diff revision 1)
>      if (NS_WARN_IF(NS_FAILED(rv))) {
>        return rv;
>      }
>  
> +    // The url notified in the webNavigation.onCreatedNavigationTarget event.
> +    URIParams finalURIToLoad;

Please declare this as OptionalURIParams. Then you don't need to do the about:blank business. Also, please call it uriToLoad as above.

::: dom/ipc/ContentParent.cpp:4387
(Diff revision 1)
>        if (frameLoader) {
>          frameLoader->GetTabParent(getter_AddRefs(aNewTabParent));
> +        TabParent* newTab = TabParent::GetFrom(aNewTabParent);
> +        if (newTab) {
> +          props->SetPropertyAsInterface(NS_LITERAL_STRING("tabBrowser"),
> +                                        ((TabParent*)newTab)->GetOwnerElement());

No need for the cast here or elsewhere.

::: dom/ipc/ContentParent.cpp:4396
(Diff revision 1)
>        *aWindowIsNew = false;
>      }
>  
> +    // If this tab is opened by a window.open call in the content process, we need to
> +    // send all the data needed by the webNavigation.onCreatedNavigationTarget event.
> +    if (aCalledFromJS) {

Are you sure we want to do this check? What happens if you click on a link with target="_blank"? We want to run onCreatedNavigationTarget in that case.

::: dom/ipc/ContentParent.cpp:4400
(Diff revision 1)
> +    // send all the data needed by the webNavigation.onCreatedNavigationTarget event.
> +    if (aCalledFromJS) {
> +      nsCOMPtr<nsIObserverService> obsSvc =
> +        mozilla::services::GetObserverService();
> +      if (obsSvc) {
> +        obsSvc->NotifyObservers(static_cast<nsIPropertyBag2*>(props),

I don't think this case should be needed.

::: toolkit/components/windowwatcher/nsWindowWatcher.cpp:1221
(Diff revision 1)
>    // userContextId.
>    MOZ_ASSERT(CheckUserContextCompatibility(newDocShell));
>  
> +  // If this tab or window has been opened by a window.open call in the main process,
> +  // we have to provide all the data needed by the webNavigation.onCreatedNavigationTarget event.
> +  if (aCalledFromJS && !XRE_IsContentProcess()) {

I think it would be simpler if you only notify here and then add code to WebNavigationContent.jsm to listen for the observer notification and forward it to the parent process.

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think I messed up setting the flags somehow. But I think the other patch should be reviewed by Kris.

Comment 30

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8723118 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

Hope I'm not corrupting some crazy internal mozreview state by doing this...

Comment 31

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Bill McCloskey (:billm) from comment #28)
> Comment on attachment 8836101 [details]
> Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new
> windows/tabs from window.open.
> 
> https://reviewboard.mozilla.org/r/111566/#review113920
> 
> I made a lot of comments, but I think the main issue is that I would like
> the notification to happen only in nsWindowWatcher.cpp, no matter if we're
> in the parent or the child process. Then WebNavigationContent.jsm can listen
> for it and forward it to the parent. Is there any reason you can't do it
> that way?

Thanks Bill for taking a look at the patch, like I said in one of my previous comments I was still unhappy about this patch (that is more a rough draft that a final patch), but given the point reached by discussing about the general issues and strategies, I thought that looking at the exact "hook points" in the sources that I was looking into could be helpful to "complete the picture".

I totally agree that it is way better if we can catch every window.open from a single point in the code, what pushed me to handle the window.open coming from the child processes differently (and to ask your advices about it) is that, for this event, we need to be able to identify which are the two tab objects involved in the event: the source tab as well as the newly created tab.

Currently we are identifying the tab related to a webNavigation event based on the browser element that receives the message manager message from WebNavigationContent.js framescript, which work great for the webNavigation events that are related to a single tab, but given that in this case we need to identify two tabs this approach is not enough anymore (and so I started to search for the other approaches that can be used to handle this new scenario).

Are there other ways that I can use to turn the child processes' "docShell" objects related to the onCreatedNavigationTarget event into a representation that can be sent over the message manager messages and then used in the main process to retrieve the browser elements related to these docShell objects?

Comment 32

[Luca Greco [:rpl] [:luca] [:lgreco]]

Actually, I think that I found a reasonable way to do it: by pairing two message manager messages (one from the source tab and one from the new tab) using the outerWindowId of the newly created docShell.

I'm currently rewriting the patch to use this new approach (I'm basically removing all the changes to the C++ components besides the one in the nsWindowWatcher component, applying the part of the Bill's feedback comments that can still be applied on the new version, and polishing the changes that I applied to the WebNavigationContent.js and WebNavigation.jsm to prototype this new approach).

An updated patch will come soon for feedback, in the meantime I'm going to clear the pending needinfo flags.

Comment 33

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8723118 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36365/diff/3-4/

Comment 34

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8836101 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from window.open.

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

Comment 35

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8836101 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from window.open.

https://reviewboard.mozilla.org/r/111566/#review113920

> Are you sure we want to do this check? What happens if you click on a link with target="_blank"? We want to run onCreatedNavigationTarget in that case.

The tabs/windows opened by clicking on the link are handled by the other patch (that was the reason why I added an f? also on that patch, to provide the full picture about how the various scenarios are handled by this patches), because in this scenario the new tab (or window) is opened from privileged JS code in the main process.

Comment 36

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8836101 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from window.open.

Hi Bill,
Thanks again for the feedback, it has been very helpful and this new patch should be much more near to what you suggested in the Comment 28:

The notification event is now sent only from the nsWindowWatcher (and the event is observed from both the main process and the child processes).

When the message is received by WebNavigationContent.js (1 instances per tab) in the child process, we handle it only if the listener has been subscribed by the source or the new tab (and ignored otherwise), and both the source tab and the new tab are going to send a message manager event.

In the main process, WebNavigation.jsm receives both the messages (one for the source tab and one for the new tab), it pairs the two messages using the createdWindowId (the outerWindowId of the newly created docShell) and then fires the onCreatedNavigationTarget event.

I also expanded and refactored the test cases (e.g. added more assertions related to the properties of the webNavigation event, the "click/context menu/window.open" scenarios are tested from both the top level frame and a sub frame, and for both new tabs and new windows).

This patch is applied on top of the first patch attached to this issue and coverS the "window.open calls" scenario, while the "click and context menu" scenarios are both covered by the other patch.

Comment 37

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8836101 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from window.open.

https://reviewboard.mozilla.org/r/111566/#review115152

::: toolkit/modules/addons/WebNavigation.jsm:116
(Diff revision 2)
>     * @param {string | Object} data
>     */
>    observe: function(subject, topic, data) {
>      if (topic == "autocomplete-did-enter-text") {
>        this.onURLBarAutoCompletion(subject);
>      } else if (topic == "webNavigation-createdNavigationTarget") {

It's not a good idea to use the same observer name for different purposes. Please use different names for the nsWindowWatcher one and whatever the other thing is.

::: toolkit/modules/addons/WebNavigation.jsm:130
(Diff revision 2)
>          // or Ctrl/Shift + click on a link).
>          location = subject.wrappedJSObject.location;
>          sourceWindowId = subject.wrappedJSObject.sourceFrameOuterWindowID;
>          createdTabBrowser = subject.wrappedJSObject.createdTabBrowser;
>          sourceTabBrowser = subject.wrappedJSObject.sourceTabBrowser;
> +      } else if (subject instanceof Ci.nsIPropertyBag2) {

Can you just remove this code and rely on WebNavigationContent.js to work in the parent process as well? I don't see any reason it wouldn't.

::: toolkit/modules/addons/WebNavigation.jsm:333
(Diff revision 2)
>        }
>      }
>    },
>  
> +  onCreatedNavigationTarget(browser, data) {
> +    if (!Services.appinfo.browserTabsRemoteAutostart) {

This shouldn't be needed if everything goes through WebNavigationContent.js.

::: toolkit/modules/addons/WebNavigationContent.js:30
(Diff revision 2)
>  
> +var CreatedNavigationTargetListener = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsISupportsWeakReference]),
> +
> +  init() {
> +    Services.obs.addObserver(this, "webNavigation-createdNavigationTarget", true);

ownsWeak should not be true here.

::: toolkit/modules/addons/WebNavigationContent.js:60
(Diff revision 2)
> +      // and the source docShell is not a descendant of it)
> +      // there is nothing to do here and return early.
> +      return;
> +    }
> +
> +    const isSourceTab = docShell === sourceDocShell || isSourceTabDescendant;

I think you should be able to get sourceDocShell.sameTypeRootTreeItem, QI to nsIDocShell, and then compare it to docShell. That would be faster than walking up the chain of docshells.

Comment 38

[Kris Maglione [:kmag]]

Comment on attachment 8723118 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

https://reviewboard.mozilla.org/r/36365/#review115690

::: browser/base/content/nsContextMenu.js:961
(Diff revision 4)
>      for (let p in extra) {
>        params[p] = extra[p];
>      }
>  
> +    if (!this.isRemote) {
> +      params.frameOuterWindowID = this.target.ownerDocument.defaultView

s/ownerDocument.defaultView/ownerGlobal/

::: browser/base/content/utilityOverlay.js:319
(Diff revision 4)
> +        if (aSubject == win) {
> +          Services.obs.removeObserver(delayedStartupObserver, "browser-delayed-startup-finished");
> +          Services.obs.notifyObservers({
> +            wrappedJSObject: {
> +              location: url,
> +              createdTabBrowser: win.gBrowser.selectedTab.linkedBrowser,

`gBrowser.selectedBrowser`

::: browser/base/content/utilityOverlay.js:320
(Diff revision 4)
> +          Services.obs.removeObserver(delayedStartupObserver, "browser-delayed-startup-finished");
> +          Services.obs.notifyObservers({
> +            wrappedJSObject: {
> +              location: url,
> +              createdTabBrowser: win.gBrowser.selectedTab.linkedBrowser,
> +              sourceTabBrowser: (w || window).gBrowser.selectedTab.linkedBrowser,

`gBrowser.selectedBrowser`. Also, please store this before waiting for delayedStartupFinished, and store the result of `w || window` in a temporary variable.

::: browser/base/content/utilityOverlay.js:431
(Diff revision 4)
> +    if (params.frameOuterWindowID) {
> +      // Only notify it as a WebExtensions' webNavigation.onCreatedNavigationTarget
> +      // event if it contains the expected frameOuterWindowID params.
> +      // (e.g. we should not notify it as a onCreatedNavigationTarget if the user is
> +      // opening a new tab using the keyboard shortcut).
> +      Services.obs.notifyObservers({
> +        wrappedJSObject: {
> +          location: url,
> +          createdTabBrowser: tabUsedForLoad.linkedBrowser,
> +          sourceTabBrowser: w.gBrowser.selectedTab.linkedBrowser,
> +          sourceFrameOuterWindowID: params.frameOuterWindowID,
> +        },
> +      }, "webNavigation-createdNavigationTarget", null);

Can we please try to duplicate as little of this as possible?

::: browser/base/content/utilityOverlay.js:440
(Diff revision 4)
> +      // opening a new tab using the keyboard shortcut).
> +      Services.obs.notifyObservers({
> +        wrappedJSObject: {
> +          location: url,
> +          createdTabBrowser: tabUsedForLoad.linkedBrowser,
> +          sourceTabBrowser: w.gBrowser.selectedTab.linkedBrowser,

`gBrowser.selectedBrowser`

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:44
(Diff revision 4)
> +  is(webNavMsg.sourceFrameId, sourceFrameId, "Got the expected sourceFrameId property");
> +  is(webNavMsg.url, url, "Got the expected url property");
> +}
> +
> +add_task(function* test_on_created_navigation_target_from_mouse_click() {
> +  const tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, SOURCE_PAGE);

s/tab1/tab/

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:46
(Diff revision 4)
> +}
> +
> +add_task(function* test_on_created_navigation_target_from_mouse_click() {
> +  const tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, SOURCE_PAGE);
> +
> +  gBrowser.selectedTab = tab1;

Unnecessary.

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:59
(Diff revision 4)
> +
> +  yield extension.startup();
> +
> +  const expectedSourceTab = yield extension.awaitMessage("expectedSourceTab");
> +
> +  info("open link in a new tab using Ctrl-click");

Please capitalize.

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:75
(Diff revision 4)
> +      sourceFrameId: 0,
> +      url: `${OPENED_PAGE}#new-tab-from-mouse-click`,
> +    },
> +  });
> +
> +  info("open link in a new window using Shift-click");

Please capitalize.

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:97
(Diff revision 4)
> +
> +  yield extension.unload();
> +});
> +
> +add_task(function* test_on_created_navigation_target_from_context_menu() {
> +  const tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, SOURCE_PAGE);

s/tab1/tab/

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:99
(Diff revision 4)
> +});
> +
> +add_task(function* test_on_created_navigation_target_from_context_menu() {
> +  const tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, SOURCE_PAGE);
> +
> +  gBrowser.selectedTab = tab1;

Unnecessary.

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:112
(Diff revision 4)
> +
> +  yield extension.startup();
> +
> +  const expectedSourceTab = yield extension.awaitMessage("expectedSourceTab");
> +
> +  info("open link in a new tab from the context menu");

Please capitalize.

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:116
(Diff revision 4)
> +    openNavTarget() {
> +      return Task.spawn(function* () {

`async openNavTarget() { ... }`

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:118
(Diff revision 4)
> +        const contentAreaContextMenu = yield openContextMenu("#test-create-new-tab-from-context-menu");
> +        const item = contentAreaContextMenu.getElementsByAttribute("label", "Open Link in New Tab");
> +        is(item.length, 1, "found contextMenu item for opening the link in a new tab");
> +        item[0].click();
> +        yield closeContextMenu();

This pattern is repeated a lot. Please create a helper function.

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:132
(Diff revision 4)
> +      sourceFrameId: 0,
> +      url: `${OPENED_PAGE}#new-tab-from-context-menu`,
> +    },
> +  });
> +
> +  info("open link in a new window from the context menu");

Please capitalize.

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:136
(Diff revision 4)
> +    openNavTarget() {
> +      return Task.spawn(function* () {

`async openNavTarget() { ... }`

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:158
(Diff revision 4)
> +
> +  yield extension.unload();
> +});
> +
> +add_task(function* test_on_created_navigation_target_from_mouse_click_subframe() {
> +  const tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, SOURCE_PAGE);

s/tab1/tab/

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:160
(Diff revision 4)
> +});
> +
> +add_task(function* test_on_created_navigation_target_from_mouse_click_subframe() {
> +  const tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, SOURCE_PAGE);
> +
> +  gBrowser.selectedTab = tab1;

Not necessary.

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:173
(Diff revision 4)
> +
> +  yield extension.startup();
> +
> +  const expectedSourceTab = yield extension.awaitMessage("expectedSourceTab");
> +
> +  info("open a subframe link in a new tab using Ctrl-click");

Please capitalize.

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:192
(Diff revision 4)
> +      sourceFrameId: expectedSourceTab.sourceTabFrames[1].frameId,
> +      url: `${OPENED_PAGE}#new-tab-from-mouse-click-subframe`,
> +    },
> +  });
> +
> +  info("open a subframe link in a new window using Shift-click");

Please capitalize.

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:217
(Diff revision 4)
> +
> +  yield extension.unload();
> +});
> +
> +add_task(function* test_on_created_navigation_target_from_context_menu_subframe() {
> +  const tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, SOURCE_PAGE);

s/tab1/tab/

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:219
(Diff revision 4)
> +});
> +
> +add_task(function* test_on_created_navigation_target_from_context_menu_subframe() {
> +  const tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, SOURCE_PAGE);
> +
> +  gBrowser.selectedTab = tab1;

Not necessary.

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:232
(Diff revision 4)
> +
> +  yield extension.startup();
> +
> +  const expectedSourceTab = yield extension.awaitMessage("expectedSourceTab");
> +
> +  info("open a subframe link in a new tab from the context menu");

Please capitalize.

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget.js:257
(Diff revision 4)
> +      sourceFrameId: expectedSourceTab.sourceTabFrames[1].frameId,
> +      url: `${OPENED_PAGE}#new-tab-from-context-menu-subframe`,
> +    },
> +  });
> +
> +  info("open a subframe link in a new window from the context menu");

Please capitalize.

::: toolkit/components/extensions/ext-webNavigation.js:138
(Diff revision 4)
> +        let data3 = {};
> +        Object.assign(data3, tabTracker.getBrowserData(data.sourceTabBrowser));
> +        data2.sourceTabId = data3.tabId;

Please no `data3` ...

    data2.sourceTabId = tabTracker.getBrowserData(data.sourceTabBrowser).tabId;

::: toolkit/modules/addons/WebNavigation.jsm:100
(Diff revision 4)
>    /**
> -   * Observe autocomplete-did-enter-text topic to track the user interaction with
> -   * the awesome bar.
> +   * Observe autocomplete-did-enter-text (to track the user interaction with the awesomebar)
> +   * and webNavigation-createdNavigationTarget (to fire the onCreatedNavigationTarget
> +   * related to windows or tabs opened from the main process) topics.
>     *
> -   * @param {nsIAutoCompleteInput} subject
> +   * @param {nsIAutoCompleteInput | nsIPropertyBag2} subject

This is never a property bag.

::: toolkit/modules/addons/WebNavigation.jsm:102
(Diff revision 4)
> -   * the awesome bar.
> +   * and webNavigation-createdNavigationTarget (to fire the onCreatedNavigationTarget
> +   * related to windows or tabs opened from the main process) topics.
>     *
> -   * @param {nsIAutoCompleteInput} subject
> +   * @param {nsIAutoCompleteInput | nsIPropertyBag2} subject
>     * @param {string} topic
> -   * @param {string} data
> +   * @param {string | Object} data

This is never an object.

::: toolkit/modules/addons/WebNavigation.jsm:113
(Diff revision 4)
> +      let sourceWindowId;
> +      let sourceTabBrowser;
> +      let createdTabBrowser;
> +      let location;
> +
> +      if (subject.wrappedJSObject) {

This will always exist. No need for the `if` or predeclaration of variables.

::: toolkit/modules/addons/WebNavigation.jsm:117
(Diff revision 4)
> +        location = subject.wrappedJSObject.location;
> +        sourceWindowId = subject.wrappedJSObject.sourceFrameOuterWindowID;
> +        createdTabBrowser = subject.wrappedJSObject.createdTabBrowser;
> +        sourceTabBrowser = subject.wrappedJSObject.sourceTabBrowser;

Please assign `subject.wrappedJSObject` to a temporary variable.

::: toolkit/modules/addons/WebNavigation.jsm:117
(Diff revision 4)
> +
> +      if (subject.wrappedJSObject) {
> +        // The observed notification is coming from privileged JavaScript components running
> +        // in the main process (e.g. when a new tab or window is opened using the context menu
> +        // or Ctrl/Shift + click on a link).
> +        location = subject.wrappedJSObject.location;

s/\.location/&.href/

::: toolkit/modules/addons/WebNavigation.jsm:123
(Diff revision 4)
> +      if (!createdTabBrowser || !sourceTabBrowser) {
> +        throw new Error(`Unexpected invalid ${topic} event received: missing created tab reference`);
> +      }

This seems unnecessary.

::: toolkit/modules/addons/WebNavigation.jsm:127
(Diff revision 4)
> +
> +      if (!createdTabBrowser || !sourceTabBrowser) {
> +        throw new Error(`Unexpected invalid ${topic} event received: missing created tab reference`);
> +      }
> +
> +      this.fireCreatedNavigationTarget(sourceTabBrowser, createdTabBrowser, {sourceWindowId, location});

Why are the first two arguments separate from the object argument?

::: toolkit/modules/addons/WebNavigation.jsm:347
(Diff revision 4)
>  
>    onLoad(browser, data) {
>      this.fire("onDOMContentLoaded", browser, data, {url: data.url});
>    },
>  
> +  fireCreatedNavigationTarget(sourceTabBrowser, createdTabBrowser, data) {

I'm not sure why this intermediate functions is required, rather than just calling `fire(...)` directly...

::: toolkit/modules/addons/WebNavigation.jsm:348
(Diff revision 4)
>    onLoad(browser, data) {
>      this.fire("onDOMContentLoaded", browser, data, {url: data.url});
>    },
>  
> +  fireCreatedNavigationTarget(sourceTabBrowser, createdTabBrowser, data) {
> +    let {sourceWindowId, location: url} = data;

These names change around a lot. `location` starts out as `url`, winds up as `location` in this object, and then winds up as `url` again here.

And the frame window ID goes back and forth between frame ID and window ID several times, too, before it winds up as frameId.

Comment 39

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1190687 - [webext] Convert openContextMenu and closeContextMenu test helpers into async functions.

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

Comment 40

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8723118 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36365/diff/4-5/

Comment 41

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8836101 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from window.open.

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

Comment 42

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8836101 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from window.open.

https://reviewboard.mozilla.org/r/111566/#review115152

> It's not a good idea to use the same observer name for different purposes. Please use different names for the nsWindowWatcher one and whatever the other thing is.

yeah, I can absolutely understand why, I have renamed the topics in the last version as suggested.

> Can you just remove this code and rely on WebNavigationContent.js to work in the parent process as well? I don't see any reason it wouldn't.

That's true, I changed it as suggested (it works correctly and it is more clean).

> This shouldn't be needed if everything goes through WebNavigationContent.js.

Confirmed (removed in the last version).

> I think you should be able to get sourceDocShell.sameTypeRootTreeItem, QI to nsIDocShell, and then compare it to docShell. That would be faster than walking up the chain of docshells.

Sure, that's definitely better, changed as suggested in the last version.

Comment 43

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8723118 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

https://reviewboard.mozilla.org/r/36365/#review115690

> This pattern is repeated a lot. Please create a helper function.

yeah, I definitely agree, turned into an utility function re-used on the various test cases related to the context menu.

> `async openNavTarget() { ... }`

Sounds good to me, but to make it an async function I had to convert the `openContextMenu` and `closeContextMenu` from generators to async functions (created as a separate patch in this patch queue)

> I'm not sure why this intermediate functions is required, rather than just calling `fire(...)` directly...

intermediate function removed in favor of direct usage of the `fire` method.

> These names change around a lot. `location` starts out as `url`, winds up as `location` in this object, and then winds up as `url` again here.
> 
> And the frame window ID goes back and forth between frame ID and window ID several times, too, before it winds up as frameId.

I agree that this renaming can be a bit confusing, unfortunately it is how they are currently named elsewhere (windowId is how the frameId is called before it is translated, the main different is that the top level frameId is always 0).

Anyway, I tweaked the name of the properties a bit and it should be better now (e.g. at least location is gone and the property is now called url).

Comment 44

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8723118 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36365/diff/5-6/

Comment 45

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8836101 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from window.open.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111566/diff/3-4/

Comment 46

[Luca Greco [:rpl] [:luca] [:lgreco]]

Follows the last push to try related to the updated version of these patches:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f706b1bb7587365ab64565f310ef013221bb25b

Comment 47

[Kris Maglione [:kmag]]

Comment on attachment 8723118 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

https://reviewboard.mozilla.org/r/36365/#review118012

Comment 48

[Kris Maglione [:kmag]]

Comment on attachment 8840515 [details]
Bug 1190687 - [webext] Convert openContextMenu and closeContextMenu test helpers into async functions.

https://reviewboard.mozilla.org/r/114996/#review118014

Comment 49

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8840515 [details]
Bug 1190687 - [webext] Convert openContextMenu and closeContextMenu test helpers into async functions.

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

Comment 50

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8723118 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36365/diff/6-7/

Comment 51

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8836101 [details]
Bug 1190687 - [webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from window.open.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111566/diff/4-5/

Comment 52

[Luca Greco [:rpl] [:luca] [:lgreco]]

Patches rebased on a recent mozilla-central tip.

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53796fc215f8
[webext] Convert openContextMenu and closeContextMenu test helpers into async functions. r=kmag
https://hg.mozilla.org/integration/autoland/rev/3884829f39b0
[webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from context menu and user clicks on links. r=kmag
https://hg.mozilla.org/integration/autoland/rev/1733ced10f06
[webext] webNavigation.onCreatedNavigationTarget on new windows/tabs from window.open. r=billm

Comment 54

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

https://hg.mozilla.org/mozilla-central/rev/53796fc215f8
https://hg.mozilla.org/mozilla-central/rev/3884829f39b0
https://hg.mozilla.org/mozilla-central/rev/1733ced10f06

Comment 55

[Will Bamberg [:wbamberg]]

I've updated compat data and the docs page: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webNavigation/onCreatedNavigationTarget.

But please let me know if we need anything else!

Comment 56

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Will,
The new onCreatedNavigationTarget doc page looks great, the following are the only two additional note that I think it would be reasonable to add:

- related to the Firefox for Android support for the onCreatedNavigationTarget event,
  currently only the `window.open` scenarios are going to send the onCreatedNavigationTarget event
  on Firefox for Android (while the non-covered scenarios are the ones related to the context menu opened when the user
  does a "long touch" on the link).

- related to the following note 'but note that the event is not sent if the browser's popup blocker blocks the load',
  while on Chrome the event for a blocked popup is never sent, on Firefox the event is sent if/when the popup is unblocked.

Comment 57

[Will Bamberg [:wbamberg]]

Hey Luca

I finally got around to adding those notes: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webNavigation/onCreatedNavigationTarget#Browser_compatibility

Marking this as dev-doc-complete, but let me know if you see anything else.

Comment 58

[Cam]

Sorry to spam this closed bug but browser.webNavigation is undefined in FF 55.0b13 (Dev Edition) when I load an in-development webextension via about:debugging. Am I missing something? All the other attributes on the browser object (listed here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webNavigation) seem to be there.