685564 - Make page-mod work on Firefox mobile

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

Page-mod module currently doesn't work on Fennec as tab lives in remote processes.
So we need to use browserMessager API to communicate with those.

I started digging the work needed and here is main issues to solve, and how I decided to solve it in this proposal, available as a branch on my repo:
  https://github.com/ochameau/addon-sdk/compare/8ce9140823df29d7490198fae4834cabc44c189e...mobile

 1/ We can't listen to `document-element-inserted` in chrome/addon process. 
These events are only dispatch on content processes.

  => I looked at native messages already dispatched by Fennec codebase and `Content:LocationChange` seems to be dispatched right after document-element-inserted. Not later, but just on the next evaluation loop. So I choosed to listen to this event in order to instanciate a new worker and start doing things with a specific tab.


 2/ `contentScriptWhen`: start, is way harder to handle with remote process.

  => Here, I evaluate content script right after having received `Content:LocationChange`, it may cause some delay before executing it. We need to verify the delay we face on real hardware and slow one. An important point of this current implementation is that I don't evaluate any additional code in remote processes before being sure that I want to inject a worker in it. We need to keep in mind that Firefox mobile is already considered as being too slow to load web pages so that we should avoid any operation that would slow down page load. "end" should be tuned for mobile to inject scripts without slowing documents load nor freezing touch-scrolling. I won't be surprised that we should only implement "end" on mobile!


 3/ We can't access to document references from chrome/addon process, only code in remote processes can do so. So that we can't identify workers by its `window` reference anymore.

  => I'm using Worker.`window` attribute to store `browser` reference instead. It was one quick hack to have something working, but I don't feel confident with this. Next point is discussing about how to handle differences between both platforms.


 4/ It definitively makes sense to share most of the code between firefox desktop and mobile. But there is still major differences between both that need to be addressed one way or another.

  => Here, I wanted to have something working quickly, so I used multiple "if(isFennec())" statements. I don't feel confortable with this. We may look into: 
   - dynamic dispatching: having duplicated method with same name but one for each platform,
   - use message manager for desktop too so that we run exactly the same codebase. We may have to execute additional code on Desktop to fake mobile events, but it doesn't change code execution path, it only add new parrallel code. I don't think we will loose much in performance, nor being substantially more heavyweight.


 5/ Message manager instances are shared. We share them with firefox codebase AND all addons. So that if we sent a new custom message, everyone (firefox, addons) may receive it. We can't use "emit" message to forward `port` events, otherwise it will be received by all enabled jetpack addons!
 
  => To solve this issue, I've implemented a new module currently called "remote-unique-pipe". It allows to create something very similar to Worker and `port`. Given a `browser`, it evaluates a script via an url into the remove process and it returns a "pipe" that expose EventEmitter methods (on/emit) allowing to communicate with this script. This module send/listen to only one message whose name is an unique id for the addon. (currently I use new Date().getTime(), but we can use addon id instead) Then it uses this message pipe to implement an EventEmitter interface on both sides.

   # file.js:
   Pipe.on("event", function (foo) {
     Pipe.emit("event", foo + "bar");
   });

   # 
   let pipe = require("remote-pipe").loadRemoteScript(browser, data.url("file.js"));
   pipe.on("event", function (foobar) {
     console.log(foobar);
   });
   pipe.emit("event", "foo");


 6/ Page-mod unit test rely on having direct access to `window` reference in order to verify correct page-mod execution. It won't be easy to keep simple tests being compatible with e10s.

  => I started converting page-mod unit test too. I've used an `evaluate` method that execute a piece of code into the remote process. It tends to introduce spaghetti code into our tests because of callbacks.

Comment 1

[Alexandre Poirot [:ochameau]]

matteo: we already discussed about most of these points, but feel free to add your though here or ping me for another 1v1 session.

myk: your global feedback would be appreciated to confirm that we are going in the right direction.

dave/myk: Then there is the question on how we should work together? 
I think, but it may depend on your feedback, that this work may just live as one or multiple pull requests. It may just be a matter of feedback then review before landing these things. If not, it will make sense for matteo and me to work on a common branch. (because it is painfull to work on someone else pull request/branch) Finally, if we decide to go with a branch, I'd like to have some idea on how to handle review.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

>   => Here, I evaluate content script right after having received
> `Content:LocationChange`, it may cause some delay before executing it. We
> need to verify the delay we face on real hardware and slow one.

Hmm, it sounds like this approach would force us to abandon "contentScriptWhen: start" entirely, although that's a key feature of the Page Mod API.


> An important
> point of this current implementation is that I don't evaluate any additional
> code in remote processes before being sure that I want to inject a worker in
> it. We need to keep in mind that Firefox mobile is already considered as
> being too slow to load web pages so that we should avoid any operation that
> would slow down page load. "end" should be tuned for mobile to inject
> scripts without slowing documents load nor freezing touch-scrolling. I won't
> be surprised that we should only implement "end" on mobile!

Mobile performance is important, but I'm wary of optimizing prematurely, especially if it means breaking important API promises.  How expensive is pre-injecting only the match pattern and a pattern-matching listener into the content process?  It seems likely to be pretty cheap, and if we lazily load everything else when a page matches, we should be able to maintain the existing API without significantly impacting performance.


>  3/ We can't access to document references from chrome/addon process, only
> code in remote processes can do so. So that we can't identify workers by its
> `window` reference anymore.

As I recall, Worker.window was intended for internal consumers, not addons, so we haven't made any promises about it, and it's ok to take it away (or morph it, as appropriate).


>    - use message manager for desktop too so that we run exactly the same
> codebase. We may have to execute additional code on Desktop to fake mobile
> events, but it doesn't change code execution path, it only add new parrallel
> code. I don't think we will loose much in performance, nor being
> substantially more heavyweight.

I'm game for using the message manager on desktop, too, especially if it simplifies the implementation without significantly impacting the experience.


>  5/ Message manager instances are shared. We share them with firefox
> codebase AND all addons. So that if we sent a new custom message, everyone
> (firefox, addons) may receive it. We can't use "emit" message to forward
> `port` events, otherwise it will be received by all enabled jetpack addons!

But main addon code doesn't have access to the message manager by default, right?  So even if our API implementations are able to see messages by other addons, as long as they are designed to ignore them and not expose them to their callers, it should be possible to use message manager without leaking information between addons.  And leaking information to chrome code is ok.

As long as access to message manager is gated behind a require("chrome") call or the equivalent, using the message manager seems fine.  However, if remote-unique-pipe is simpler, more robust, or otherwise preferable, then that's ok too.


> myk: your global feedback would be appreciated to confirm that we are going
> in the right direction.

It's sounds like you're mostly going in a great direction!  My only significant concern is breaking our Page Mod API promises.  If it turns out that it's essential to do so, then ok; but we need to pursue every possible avenue for maintaining those promises before coming to that conclusion.


> dave/myk: Then there is the question on how we should work together? 
> I think, but it may depend on your feedback, that this work may just live as
> one or multiple pull requests. It may just be a matter of feedback then
> review before landing these things. If not, it will make sense for matteo
> and me to work on a common branch. (because it is painfull to work on
> someone else pull request/branch) Finally, if we decide to go with a branch,
> I'd like to have some idea on how to handle review.

It sounds like there are several different questions here: pull requests vs. bug reports, branch vs. no branch, and how to do review.

First, yes, you can use pull requests instead of (or in addition to, at your discretion) bug reports.  You don't have to file a bug for every pull request if it doesn't seem warranted.

Second, in general, I think it's best for everyone to work on the main development branch, because it reduces integration problems.  See Paul Julius <http://pauljulius.com/blog/2009/09/03/feature-branches-are-poor-mans-modular-architecture/> and dig even deeper with Martin Fowler <http://martinfowler.com/bliki/FeatureBranch.html> for the reasoning.

So my ideal approach would be for you and Matteo to land mobile-related work on the main development branch, even if it's experimental or not yet complete.  However, in order to do that, you must isolate those changes from supported functionality via configuration options, command-line flags, "experimental" labels, and the like, so users who don't do anything special will only experience and interact with supported, complete features.

However, if that feels too hard, or it doesn't seem to apply in this particular case, then I'm ok with creating another feature/topic branch for you and Matteo to work on.  In that case, my recommendation would be to make sure you merge the main development branch into your branch as frequently as possible!

Third, regarding how to do review, there are two options: get review for each individual change, even when experimental and incomplete; or, provided the changes are isolated from supported use of the product, delay review until completion and integration.

The latter approach will help you go faster in the beginning, which I like a lot.  But I'm not entirely sure how to combine it with continuous integration on the development branch.  So if we choose that approach, then it probably makes sense for y'all to start on a separate branch.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 245

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/310

Pointer to Github pull-request

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 583828 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/310

Irakli has done a preliminary review, so let's have him do the full review.

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 583828 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/310

I noted few things and none of those is critical to me. Feel free to land it as is and fill a followup cleanup bug or do the cleanup with another review round.

Thanks!

Comment 7

[[github robot]]

Commit pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/6eccac8789ce538db267300091d353da6711b0c5
Merge pull request #310 from ZER0/fennec-birch

fix Bug 685564 - Make page-mod work on Firefox mobile; r=@Gozala, @mykmelez