571371 - [e10s] Move BrowserView event handling to message-based API

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: wip 1

We need to move several BrowserView events to messages. The events are: MozAfterPaint, MozScrolledAreaChanged and scroll.

The patch moves the events to be messages and cleans up some problems we have with returning nothing from snyc message handlers.

The big problem I am having is the MozAfterPaint event does not seem to be fired from inside content.js

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

I filed a separate bug, bug xxx, about the MozAfterPaint failures. I confirmed that with the hack described in that bug, the MozAfterPaints were firing correctly. I removed the hack from this patch though.

The event handlers were converted to message handlers using the code from mobile-e10s. I also updated the message name of MozApplicationManifest -> Browser:MozApplicationManifest, to match the evloving convention.

Tests are all green, even without the MozAfterPaint hack, which only affects the rendered state of the canvas tiles.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

oops, bug xxx, should be bug 571438

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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


>+    let self = this;
>+    messageManager.addMessageListener("Browser:MozScrolledAreaChanged", this);
>+    messageManager.addMessageListener("Browser:MozAfterPaint", this);
>+    messageManager.addMessageListener("Browser:PageScroll", this);

I wonder if "PageScroll" should not be "ContentScroll" since the tab can contains a webapp or just an image or ...

>       if (vis.right > viewport.right || vis.bottom > viewport.bottom) {
>         // Content has shrunk outside of the visible rectangle.
>         // XXX for some reason scroller doesn't know it is outside its bounds
>         Browser.contentScrollboxScroller.scrollBy(0, 0);
>-        this.onAfterVisibleMove();
>       }
>+      this.onAfterVisibleMove();
>     }

Why is this moving outside of the if?


>+         return {};

I would like to not forgot to clean those once bug 568818 land.

r+ with question answered.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #3)
> (From update of attachment 450639 [details] [diff] [review])
> 
> >+    let self = this;
> >+    messageManager.addMessageListener("Browser:MozScrolledAreaChanged", this);
> >+    messageManager.addMessageListener("Browser:MozAfterPaint", this);
> >+    messageManager.addMessageListener("Browser:PageScroll", this);
> 
> I wonder if "PageScroll" should not be "ContentScroll" since the tab can
> contains a webapp or just an image or ...

I left it as PageScroll for now. SOme of our other code uses "PageScroll", like _ignorePageScroll.

> >       if (vis.right > viewport.right || vis.bottom > viewport.bottom) {
> >         // Content has shrunk outside of the visible rectangle.
> >         // XXX for some reason scroller doesn't know it is outside its bounds
> >         Browser.contentScrollboxScroller.scrollBy(0, 0);
> >-        this.onAfterVisibleMove();
> >       }
> >+      this.onAfterVisibleMove();
> >     }
> 
> Why is this moving outside of the if?

This was part of Ben's changes to mobile-e10s. After looking for the changeset, I see that it was part of the "remove bact ops" changes. We don't need to do that, so I removed this change.

> >+         return {};
> 
> I would like to not forgot to clean those once bug 568818 land.

Right

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/62a2be294c46