687472 - API to ease porting of current chrome<->content code to e10s world

Status: RESOLVED WONTFIX

(Toolkit :: General, defect, P1)

Description

[Asaf Romano (gone)]

Attachment: patch

After half a rewrite of the context menu and other browser code, I figured we could really benefit from a api that abstracts the e10s system a bit and make it a little bit more similar to addEventListener semantics.

My initital implementation is attached.

It is a quite similar to the fennec's solution - the key difference is the attempt to generalize the jsoned-object structure. In theory, it's not as good performance-wise, because I'm reading various properties that are not always necessary. However, given how optimized dom calls are and that this code is only invoked for ui-event-listener, and that there aren't so-many properties to read, I think the stable api makes it a good trade off.

Another decision I made here is not to use the window-level message manager. While it might seem useful, add ons (and the sidebar) will break if they make the assumption that there are only tabbrowser-browsers in a browser window.

The patch is missing documentation and tests. I'll add both as this api stabilizes a bit.  I prefer landing some version of this as soon as possible, in order to allow rapid fixes for e10s-builds.

Comment 1

[Benjamin Smedberg]

Comment on attachment 560920 [details] [diff] [review]
patch

How will this be used? The primary concern I have with this is that people will use it without thinking about the consequences of sending messages across in cases where we really should be batching or otherwise processing the data first.

Comment 2

[Asaf Romano (gone)]

Benjamin, Indeed the are cases where a "customized" content script would perform better.  This will be addressed in the documentation.  As a browser-reviewer I'm not going to auto-approve  usage of this api *for new code*.

The primary goals of this api are:
1) Simple cases (e.g. WillOpernModalDialog, LinkAdded).
2) Rapid transition for current front-end code.

The current data provided by this script isn't performance sensitive, and we should keep it that way.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Mano from comment #2)
> This will be addressed in the documentation.  As a
> browser-reviewer I'm not going to auto-approve  usage of this api *for new
> code*.

I'm not sure how much of a perf problem this is, but I'm afraid what you're saying doesn't scale.

Comment 4

[Asaf Romano (gone)]

In what sense? This script itself isn't extendible. If it doesn't work for your use case, you've to write your own script. If it does work, you can use it.

For the case of simple-event-forwarding (WillOpenModalDialog, TitleChanged, etc) I actually cannot think of a good alternative - this is pretty similar to fennec's solution, except that in fennec the returned data object is built different for each event - If perf isn't a concern (and I don't think it is), I'm not sure what's the benefit of this approach.

Comment 5

[Asaf Romano (gone)]

One more note - even if we go with fennec's solution, we cannot avoid making it an API. Our tabbrowser binding  lives in browser/, while the browser binding itself live in in toolkit/. This means that whatever solution is found for "simple forwarding", it ought to be part of browser's xml "public" methods.

Comment 6

[Asaf Romano (gone)]

One interesting problem is principals. Almost all content handlers which deal with targets do a security check at some point (d&d for example).  I'm not sure if there's a reliable way to re-create principals in chrome context.

Comment 7

[Asaf Romano (gone)]

Ah, principals implement nsISerializable (in a sync manner). Thanks smaug.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Mano from comment #4)
> In what sense?

In the sense that we can't rely on add-on developers adhering to MDN nor on all browser reviewers rejecting uses of a particular API.

> This script itself isn't extendible. If it doesn't work for
> your use case, you've to write your own script. If it does work, you can use
> it.

I'm not sure what this means, but good if it rules out misuse regardless of MDN and reviews.

Comment 9

[Asaf Romano (gone)]

It means that in order to misuse the limited functionality this API provides, you'd have to change the content script - going through toolkit reviewers.  Sometimes we (reviewers) we might actually approve such additions. Most of the times we won't. For example, I'm pretty sure we'll never include info about the event target's ancestors. For such functionality you better come up with your own listeners.
 
As far as I'm concerned, if the addon developer wants just the few things we provide here (and I'm not yet sure about the list), it's fine if he or she uses this api even if he could come up with something a little bit more efficient.

Comment 10

[Asaf Romano (gone)]

About security checks: I think I'm going to move them to the content script. Serializing the principal and then re-creating it in chrome seems an overkill.

Comment 11

[Asaf Romano (gone)]

Benjamin, Gavin, please do let me know what are you thoughts on this as soon as you can.  This is needed for fixing prompter and some basic browser interaction stuff - I consider this dogfood for e10s builds (btw, I decided not to use this for context menus).

Comment 12

[Benjamin Smedberg]

I have the same basic concern with this bug as with bug 666801 comment 11: you can't take any useful action from these events which ever reach back into content, because you won't know whether the content has already moved on. I feel like this is a footgun-type API. But I don't quite understand what you'd be using it for, can you link to the prompter/interaction stuff?

Comment 13

[Asaf Romano (gone)]

"you can't take any useful action from these events which ever reach back into content"

But this direction (chrome->content) isn't included here, on purpose. The main goal of this API is listeners which modify the UI based on content events - not the other way around. The best example is probably DOMTitleChanged, for updating the tab titles based on window.title changes in content. There's no action to do back on content. There's no way in this limited API to take any action.

Same goes for prompter - chrome needs to listen to DOMWillOpenModalDialog in order to focus the tab for which a prompt is invoked. Again - no action on content afterwards.

Comment 14

[Benjamin Smedberg]

Comment on attachment 560920 [details] [diff] [review]
patch

I'm going to clear the feedback flag because I've already provided the feedback I can: I think that we'd be doing a disservice to our code to use this mechanism instead of just writing messages specifically in those cases that require it. But I'll let gavin make the final call.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 560920 [details] [diff] [review]
patch

I think you mentioned on IRC that some of this was duplicating work done in another bug?

Do we have concrete users for this abstraction lined up (callers of addContentEventListener)? I would like to avoid adding this until we have many potential users, and it's not clear to me that we do at the moment. I'm open to revisiting this, of course.

Comment 16

[Asaf Romano (gone)]

Sadly.

Comment 17

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

I'm going to close this since we fixed the context menu bug using CPOWs instead of something like this.