Last Comment Bug 721777 - Capture window.open calls to redirect them depending on their target
: Capture window.open calls to redirect them depending on their target
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
:
Mentors:
Depends on:
Blocks: 716664
  Show dependency treegraph
 
Reported: 2012-01-27 08:58 PST by Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
Modified: 2012-02-02 06:57 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Capture window.open calls (6.70 KB, patch)
2012-01-27 08:58 PST, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Review
Patch (3.39 KB, patch)
2012-01-27 16:18 PST, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Review
Patch v0.3 (3.38 KB, patch)
2012-01-27 16:58 PST, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
bzbarsky: review-
Details | Diff | Review
Patch v0.4 (3.45 KB, patch)
2012-01-27 17:25 PST, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
bzbarsky: review+
fabrice: review+
Details | Diff | Review

Description Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-27 08:58:53 PST
Created attachment 592152 [details] [diff] [review]
Capture window.open calls

This is a pre-intents patch. The code in shell.js can be replace by intents.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-27 10:13:14 PST
Why do you need to do this?  That interfaces is noscript for pretty good reasons (it's _really_ easy to end up with a footgun).  Does nsIWindowProvider not let you do what you want to do here?

Because as far as I can tell, the attached patch would break window.open() in the web browser (not to mention confusing the heck out of security code: openWindowJS is clearly documented as relying on the JS stack, which you're now inserting chrome code into).
Comment 2 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-27 16:18:51 PST
Created attachment 592318 [details] [diff] [review]
Patch
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-27 16:37:04 PST
Comment on attachment 592318 [details] [diff] [review]
Patch

Do b2g.js and shell.js correspond to different browser processes?  Why does b2g.js want to divert everything to the window the load is started from?  Trying to understand how the pieces here work together.

For the nsBrowserAccess bits, it looks like it's assuming that the caller will always pass a null URI.  This is a warranted assumption as far as nsContentTreeOwner is concerned... for now.  You don't expect any other consumers of nsIBrowserDOMWindow to call you?  Should you at least warn if the URI is not null?
Comment 4 [:fabrice] Fabrice Desré 2012-01-27 16:39:35 PST
Vivien, we don't differentiate links in new tabs, new windows from "normal" links here. Is it just because the current ApplicationManager() doesn't deal with these cases?
Comment 5 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-27 16:54:09 PST
(In reply to Boris Zbarsky (:bz) from comment #3)
> Comment on attachment 592318 [details] [diff] [review]
> Patch
> 
> Do b2g.js and shell.js correspond to different browser processes?  Why does
> b2g.js want to divert everything to the window the load is started from? 
> Trying to understand how the pieces here work together.

I don't have a strong comprehension of what the preferences means here. I have just tried to set the correct values to reach the point I was looking for. I have removed the browser.link.open_newwindow.restrictio preference.

> 
> For the nsBrowserAccess bits, it looks like it's assuming that the caller
> will always pass a null URI.  This is a warranted assumption as far as
> nsContentTreeOwner is concerned... for now.  You don't expect any other
> consumers of nsIBrowserDOMWindow to call you?  Should you at least warn if
> the URI is not null?

I have |let url = uri ? uri.spec : 'about:blank'| and use that.
Comment 6 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-27 16:57:19 PST
(In reply to Fabrice Desré [:fabrice] from comment #4)
> Vivien, we don't differentiate links in new tabs, new windows from "normal"
> links here. Is it just because the current ApplicationManager() doesn't deal
> with these cases?

I have added the 'where' arguments to launch() so you will be able to distinguish between those.

But this ApplicationManager code with be replace by a webintent (bug 715814) right?
Comment 7 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-27 16:58:40 PST
Created attachment 592327 [details] [diff] [review]
Patch v0.3
Comment 8 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-27 17:06:29 PST
(In reply to Boris Zbarsky (:bz) from comment #3)
 > Do b2g.js and shell.js correspond to different browser processes?  Why does
> b2g.js want to divert everything to the window the load is started from? 
> Trying to understand how the pieces here work together.

For the moment there is only one process in b2g, so I'm unsure of your question.

The structure of B2G/Gaia is:

<xul:window>
  ...
 <xul:browser> <--homescreen -->
    ...
   <html:iframe name="app1"/>
   <html:iframe name="app2"/>
   <html:iframe name="browserapp">
     ...
     <html:iframe nane="web-content" mozbrowser/>
   </html/iframe>
 </xul:browser>
</xulwindow>

What I'm trying to achieve here is to have |window.open| calls from the web-content beeing handle by the main <xul:window>.
The main window will fire an intent, let's say 'open-in-browser', that will be handle by the browser app.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-27 17:07:17 PST
> I don't have a strong comprehension of what the preferences means here.

They're documented in firefox.js, no?
Comment 10 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-27 17:12:06 PST
(In reply to Boris Zbarsky (:bz) from comment #9)
> > I don't have a strong comprehension of what the preferences means here.
> 
> They're documented in firefox.js, no?

Sure. But I'm unsure of what 'divert' means in this context.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-27 17:12:52 PST
Comment on attachment 592327 [details] [diff] [review]
Patch v0.3

> For the moment there is only one process in b2g

Ok, in that case the patch here looks fundamentally broken.  Setting open_newwindow to 1 will force the call into your nsIBRowserDOMWindow impl, but will also set *aWindowIsNew to false.  That will crew up things like .opener on the new window to some extent, security permissions on the new window, and the load flags for the actual load.  Why _did_ you set it to 1 instead of 3?  Seems like 3 would have made more sense....
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-27 17:14:47 PST
> But I'm unsure of what 'divert' means in this context.

The usual meaning of "send somewhere different than where it was heading".  "where it was heading" being "a new toplevel window".

I recommend just changing that pref in Firefox and using window.open() with a URI in a web page and seeing what happens if you want to get a feel for how it ends up working there.

I assume you did read nsContentTreeOwner::ProvideWindow, which is what made you change that pref?
Comment 13 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-27 17:25:14 PST
Created attachment 592329 [details] [diff] [review]
Patch v0.4
Comment 14 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-27 17:26:22 PST
(In reply to Boris Zbarsky (:bz) from comment #12)
> > But I'm unsure of what 'divert' means in this context.
> 
> The usual meaning of "send somewhere different than where it was heading". 
> "where it was heading" being "a new toplevel window".
> 
> I recommend just changing that pref in Firefox and using window.open() with
> a URI in a web page and seeing what happens if you want to get a feel for
> how it ends up working there.

Done.

> 
> I assume you did read nsContentTreeOwner::ProvideWindow, which is what made
> you change that pref?

I did.
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-27 17:32:07 PST
Comment on attachment 592329 [details] [diff] [review]
Patch v0.4

OK, this I think I buy.  You do want to set newwindow.restriction to 0, though.  Otherwise opening a sized window will bypass your code, right?

r=me with that, I think.  Assuming I understood what you're trying to do here correctly....
Comment 16 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-02-01 17:54:37 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/4342b1492c38
Comment 17 Ed Morley [:emorley] 2012-02-02 06:57:22 PST
https://hg.mozilla.org/mozilla-central/rev/4342b1492c38

Note You need to log in before you can comment on or make changes to this bug.