Closed
Bug 571371
Opened 15 years ago
Closed 15 years ago
[e10s] Move BrowserView event handling to message-based API
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mfinkle, Assigned: mfinkle)
Details
Attachments
(1 file, 1 obsolete file)
15.29 KB,
patch
|
vingtetun
:
review+
|
Details | Diff | 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
Assignee | ||
Comment 1•15 years ago
|
||
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)
Assignee | ||
Comment 2•15 years ago
|
||
oops, bug xxx, should be bug 571438
Comment 3•15 years ago
|
||
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+
Assignee | ||
Comment 4•15 years ago
|
||
(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
Assignee | ||
Comment 5•15 years ago
|
||
pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/62a2be294c46
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•