Closed Bug 571371 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mfinkle, Assigned: mfinkle)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch wip 1 (obsolete) — Splinter Review
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
Attached patch patchSplinter Review
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.
Assignee: nobody → mark.finkle
Attachment #450463 - Attachment is obsolete: true
Attachment #450639 - Flags: review?(21)
oops, bug xxx, should be bug 571438
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.
Attachment #450639 - Flags: review?(21) → review+
(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
pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/62a2be294c46
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.