Closed Bug 560827 Opened 11 years ago Closed 11 years ago

e10s: Add remote support to browser XBL

Categories

(Toolkit :: XUL Widgets, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

Attachments

(1 file, 3 obsolete files)

When <browser remote="true">, IPC is used to create content in a separate process. This state bascially kills all the useful properties and methods of the browser XBL binding.

We need to create a browser binding specifically for remote content.
Attached patch wip 1 (obsolete) — Splinter Review
This patch creates an XBL binding for a remote-aware browser. It extends the normal browser binding, but overrides .webNavigation and .webProgress so we can make many of the other properties remote-aware.

I want to do the same for the "find" properties too.

The patch also starts making changes to browser-ui.js and browser.js, to use the normal properties instead of the current e10s workarounds. I need to do more of this too.

Some parts of the code just start working again, like bookmarking.
Is this patch supposed to contain browser.xml file?
Added missing files
Attachment #440517 - Attachment is obsolete: true
Blocks: 516521
Duplicate of this bug: 554271
I don't think I understand. Are you creating a *new* browser.xml binding? Why not just fix the one we already have so that it works OOP or IP?
(In reply to comment #5)
> I don't think I understand. Are you creating a *new* browser.xml binding? Why
> not just fix the one we already have so that it works OOP or IP?

I am creating a binding that extends the current non-IPC binding. It is not completely new. This binding will only be activated when <browser remote="true" ... >

The new binding will live in the current browser.xml file in toolkit widgets.
Attached patch Add progress listener support (obsolete) — Splinter Review
I cleaned up redundant Fennec stuff (making the overall e10s patch smaller) and added progress listener support.  There's several ways the progress listener dispatcher is different for remote=true:
* aWebProgress and aRequest don't really have anything in them
* isRootWindow has been added to aWebProgress request since DOMWindow isn't accessible
* browser element is added to aWebProgress
* addProgressListener takes an extra parameter so that we can filter out iframe content before it even comes to chrome.  This may be too pre-optimized, but I know onStateChanged gets completely spammed when multiple iframes are being loaded.
* security change has a JSON object returned with it

I don't know if I'm going in the right direction, what do you think?  Since this interface is frozen I don't really know if we can get away with changing web progress with a new argument (see https://developer.mozilla.org/en/nsIWebProgress).

Do you have any ideas?
Attachment #442197 - Flags: review?(mark.finkle)
Comment on attachment 442197 [details] [diff] [review]
Add progress listener support

overall I like this approach in this patch. I am merging it into my current patch.

I am leaving out some of the optimizations with the progress listener for now.
Attachment #442197 - Flags: review?(mark.finkle)
Attached patch patchSplinter Review
I think we should try to get this patch reviewed and landed so we can build on it. I don't expect this to be the final iteration by any means. The XBL is still in "mobile" while we clean up the fennec front-end code. We can easily move it to toolkit when we feel comfortable with it.

This patch includes:
* Removes more of the original browser XBL binding methods and properties from the new binding. We are inheriting more now, so we don't need the duplication.
* Adds support for basic .contentDocument property 
* Merges in Ben's web progress listener support and the Fennec code cleanup we got from that

With this patch Fennec is at least as functional as the existing e10s version, and probably more so, since bookmarking and other parts are working again.
Attachment #440522 - Attachment is obsolete: true
Attachment #442197 - Attachment is obsolete: true
Attachment #442303 - Flags: review?(webapps)
Blocks: 562945
Comment on attachment 442303 [details] [diff] [review]
patch

Looks good!  Everything seems to work properly (back/forward, favicon, reload, bookmark, progress...)
Attachment #442303 - Flags: review?(webapps) → review+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 566500
You need to log in before you can comment on or make changes to this bug.