Closed Bug 730681 Opened 10 years ago Closed 10 years ago

[maple] Resize handling makes no sense

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kats, Assigned: kats)

Details

Attachments

(3 files)

Currently there are two different resize event listeners registered in browser.js. One of them gets run directly every single time we scroll by a pixel, and this triggers the other one. There's a lot of unnecessary resize handling happening.
Attachment #600773 - Flags: review?(chrislord.net)
Comment on attachment 600772 [details] [diff] [review]
Stop running resize handling uselessly

Review of attachment 600772 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
Attachment #600772 - Flags: review?(chrislord.net) → review+
Comment on attachment 600773 [details] [diff] [review]
Merge resize handlers

Review of attachment 600773 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me - the only thing about not using window.onresize, is that there may be some frames where we render with the incorrect screen-size (the buffer will be resized on the SIZE_CHANGED event, but the viewport event doesn't come until later) - I don't think this is a problem, but it's worth noting.

::: mobile/android/chrome/content/browser.js
@@ +2001,5 @@
>  
>    /** Update viewport when the metadata or the window size changes. */
>    updateViewportSize: function updateViewportSize() {
> +    gScreenWidth = window.outerWidth;
> +    gScreenHeight = window.outerHeight;

It's probably worth commenting above here that it's expected for updateViewport to be called with 'reset' equal to true, so that refreshDisplayPort gets called. I could see this getting lost, and it's essential for that to be called when the screen size/zoom change.
Attachment #600773 - Flags: review?(chrislord.net) → review+
(In reply to Chris Lord [:cwiiis] from comment #4)
> Comment on attachment 600773 [details] [diff] [review]
> Merge resize handlers
> 
> Review of attachment 600773 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me - the only thing about not using window.onresize, is that
> there may be some frames where we render with the incorrect screen-size (the
> buffer will be resized on the SIZE_CHANGED event, but the viewport event
> doesn't come until later) - I don't think this is a problem, but it's worth
> noting.

I'm not sure if I understand this correctly. We are still using window.onresize, because the updateViewportSize() function gets called when ViewportHandler.handleEvent gets a resize event - that resize event is the same as the window.onresize we were listening for.

> It's probably worth commenting above here that it's expected for
> updateViewport to be called with 'reset' equal to true, so that
> refreshDisplayPort gets called. I could see this getting lost, and it's
> essential for that to be called when the screen size/zoom change.

Agreed. I'll add the comment.
(In reply to Kartikaya Gupta (:kats) from comment #5)
> (In reply to Chris Lord [:cwiiis] from comment #4)
> > Comment on attachment 600773 [details] [diff] [review]
> > Merge resize handlers
> > 
> > Review of attachment 600773 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good to me - the only thing about not using window.onresize, is that
> > there may be some frames where we render with the incorrect screen-size (the
> > buffer will be resized on the SIZE_CHANGED event, but the viewport event
> > doesn't come until later) - I don't think this is a problem, but it's worth
> > noting.
> 
> I'm not sure if I understand this correctly. We are still using
> window.onresize, because the updateViewportSize() function gets called when
> ViewportHandler.handleEvent gets a resize event - that resize event is the
> same as the window.onresize we were listening for.

Ah, I didn't realise this. Thanks!
I ran into a problem where window.outerWidth/outerHeight are sometimes zero when starting fennec, and this messes up everything, eventually giving us a zero page size and NaN viewport. Patch to guard against this coming up.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 601069 [details] [diff] [review]
Guard against zero outerWidth/outerHeight

Review of attachment 601069 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
Attachment #601069 - Flags: review?(chrislord.net) → review+
https://hg.mozilla.org/projects/maple/rev/6cf5cc7b3dd9
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.