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)

x86
Linux
defect
Not set
normal

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
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.

Attachment

General

Created:
Updated:
Size: