[e10s] Clicking on a link in an app tab should open it on the left side of the tabs bar

VERIFIED FIXED in Firefox 35

Status

()

defect
P3
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: billm, Assigned: mossop)

Tracking

unspecified
Firefox 35
x86_64
Linux
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(e10s+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

For this to work, we'll need to change how isAppTab is set in tabbrowser.xml (currently it's disabled) and also ensure that nsIWebBrowserChrome3::OnBeforeLinkTraversal is called correctly somewhere. I'm not sure what the best way is to implement this, or whether it's best for isAppTab to live in the parent, the child, or both.
Blocks: old-e10s-m2
Summary: [e10s] Implement the logic for app tabs → [e10s] Clicking on a link in an app tab should open it on the left side of the tabs bar
Duplicate of this bug: 989481
Assignee: nobody → wmccloskey
Priority: -- → P3
Ended up tinkering around in code related to this anyway so might as well take this.
Assignee: wmccloskey → dtownsend+bugmail
Posted patch WIP (obsolete) — Splinter Review
This patch works but I'm not sure the architecture is right and I'd appreciate some feedback.

We need something like nsContentTreeOwner's XULBrowserWindow but for content processes. I added a webBrowserChrome property to TabChild but with some tabs being remote and some not that leads to onBeforeLinkTraversal sometimes being called through TabChild and sometimes through nsContentTreeOwner. To avoid code duplication I added a way for the in-process frame script to register its WebBrowserChrome with the main XULBrowserWindow so it can pass off calls to the right object. It works but it seems a little hacky.

Another option might be to just have nsDocShell call onBeforeLinkTraversal on something else before going to the treeOwner. Something at the level of sameTypeRootTreeItem would make sense, I could add the webBrowserChrome property to the docshell there but I'm guessing you folks might have a better idea for me.
Attachment #8478477 - Flags: feedback?(wmccloskey)
Attachment #8478477 - Flags: feedback?(bzbarsky)
I would really rather not add more possible codepaths for this stuff on the docshell level.  Things are already complicated enough with the whole treeowner/windowprovider mess.
I guess the easier option to remove code duplication would be to move the onBeforeLinkTraversal code into a module that both browser.js and content.js import
Comment on attachment 8478477 [details] [diff] [review]
WIP

A cleaner patch is on the way here.
Attachment #8478477 - Attachment is obsolete: true
Attachment #8478477 - Flags: feedback?(wmccloskey)
Attachment #8478477 - Flags: feedback?(bzbarsky)
This patch allows something in a frame script to register a webBrowserChrome object with TabChild that it can pass calls to onBeforeLinkTraversal to. This is similar to how we register in the chrome side with XULBrowserWindow.
Attachment #8480780 - Flags: review?(bugs)
This passes the isAppTab setting through to the frame script and responds to onBeforeLinkTraversal as necessary. We can enable a bunch of tests in e10s mode with this :)
Attachment #8480782 - Flags: review?(felipc)
Status: NEW → ASSIGNED
Duplicate of this bug: 1059962
Attachment #8480782 - Flags: review?(felipc) → review+
Comment on attachment 8480780 [details] [diff] [review]
TabChild patch

> [scriptable, uuid(2eb3bc54-78bf-40f2-b301-a5b5b70f7da0)]
> interface nsITabChild : nsISupports
> {
>   readonly attribute nsIContentFrameMessageManager messageManager;
> 
>+  attribute nsIInterfaceRequestor webBrowserChrome;
>+
Why nsIInterfaceRequestor? Why don't you just use nsIWebBrowserChrome3?

> TabChild::GetInterface(const nsIID & aIID, void **aSink)
> {
>+    if (aIID.Equals(NS_GET_IID(nsIWebBrowserChrome3))) {
>+        if (mWBC)
>+            return mWBC->GetInterface(aIID, aSink);
>+        return NS_ERROR_NO_INTERFACE;
>+    }

2 spaces for indentation (I know the file isn't quite consistent), and
use {} always with if.

>+    nsCOMPtr<nsIInterfaceRequestor> mWBC;
I'm rather surprised if this doesn't cause leaks.
Rename the member variable to mWebBrowserChrome3 or some such and move it to
TabChildBase and add it to cycle collection.

(And please test on a debug build with XPCOM_MEM_LEAK_LOG=1 env variable to see you don't leak anything.)
Attachment #8480780 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8480780 [details] [diff] [review]
> TabChild patch
> 
> > [scriptable, uuid(2eb3bc54-78bf-40f2-b301-a5b5b70f7da0)]
> > interface nsITabChild : nsISupports
> > {
> >   readonly attribute nsIContentFrameMessageManager messageManager;
> > 
> >+  attribute nsIInterfaceRequestor webBrowserChrome;
> >+
> Why nsIInterfaceRequestor? Why don't you just use nsIWebBrowserChrome3?

Sorry I failed to explain my reasoning there. So over in bug 999239 I need to catch attempts to load URLs in remote tabs that shouldn't be remote. My proposal for doing so is to add a hook into docshell that let JS cancel attempts to load pages and then redirect them to the right process. I'm doing this through the same mechanism as here, XULBrowserWindow in the main process and mWebBrowserChrome in child processes.

> >+    nsCOMPtr<nsIInterfaceRequestor> mWBC;
> I'm rather surprised if this doesn't cause leaks.
> Rename the member variable to mWebBrowserChrome3 or some such and move it to
> TabChildBase and add it to cycle collection.
> 
> (And please test on a debug build with XPCOM_MEM_LEAK_LOG=1 env variable to
> see you don't leak anything.)

Attempting to figure this out, but running in debug with this doesn't show any leaks. Not sure why.
(In reply to Dave Townsend [:mossop] from comment #11)

> > Why nsIInterfaceRequestor? Why don't you just use nsIWebBrowserChrome3?
> 
> Sorry I failed to explain my reasoning there. So over in bug 999239 I need
> to catch attempts to load URLs in remote tabs that shouldn't be remote. My
> proposal for doing so is to add a hook into docshell that let JS cancel
> attempts to load pages and then redirect them to the right process. I'm
> doing this through the same mechanism as here, XULBrowserWindow in the main
> process and mWebBrowserChrome in child processes.
That doesn't still answer to my question ;) Why you use type nsIInterfaceRequestor and not 
nsIWebBrowserChrome3?

> Attempting to figure this out, but running in debug with this doesn't show
> any leaks. Not sure why.
Well, add the member variables to CC anyway.
(In reply to Olli Pettay [:smaug] from comment #12)
> (In reply to Dave Townsend [:mossop] from comment #11)
> 
> > > Why nsIInterfaceRequestor? Why don't you just use nsIWebBrowserChrome3?
> > 
> > Sorry I failed to explain my reasoning there. So over in bug 999239 I need
> > to catch attempts to load URLs in remote tabs that shouldn't be remote. My
> > proposal for doing so is to add a hook into docshell that let JS cancel
> > attempts to load pages and then redirect them to the right process. I'm
> > doing this through the same mechanism as here, XULBrowserWindow in the main
> > process and mWebBrowserChrome in child processes.
> That doesn't still answer to my question ;) Why you use type
> nsIInterfaceRequestor and not 
> nsIWebBrowserChrome3?

Because I added the new function to a nsIWebBrowserChrome4 since that seems to have been the way to do things in the past so the object needed to have the option to implement both though. But maybe I should just add the function to nsIWebBrowserChrome3?
Even nsIWebBrowserChrome3 is rather bad.
We should just merge all nsIWebBrowserChromeX, and then let scripts to provide some functionality.
But ok for now, nsIWebBrowserChrome3 is fine, and if you want to add more functionality there, 
just add it.
It is just seeing nsIInterfaceRequestor as a property type which is really odd.
https://hg.mozilla.org/mozilla-central/rev/52b4f9600a14
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Backed out for bug 1062522

https://hg.mozilla.org/integration/fx-team/rev/1917953b1f89
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Points: --- → 8
Flags: firefox-backlog+
Flags: qe-verify?
Status: REOPENED → ASSIGNED
I.. um.. put it back in:

https://hg.mozilla.org/integration/fx-team/rev/8703c1895505

due to permanent bc-1 failures:

https://tbpl.mozilla.org/php/getParsedLog.php?id=47418182&tree=Fx-Team

495 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Waited too long for click-to-play notification -
496 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Should have a click-to-play notification in the tab in the new window -
498 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Waited too long for click-to-play notification -
499 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Should have a click-to-play notification in the initial tab again -
505 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Waited too long for click-to-play notification -
...
Depends on: 1062522
Iteration: --- → 35.1
Flags: qe-verify? → qe-verify+
This was re-landed.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
QA Contact: jbecerra
This seems to have regressed recently.
(In reply to Chris AtLee [:catlee] from comment #21)
> This seems to have regressed recently.

Please file a new bug.
Depends on: 1080055
Reproduced the initial issue on a Nightly build before and after it regressed, verified on Windows 7 64bit, Ubuntu 14.04 64bit and Mac OS X 10.9.5 using latest Nightly.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.