730681 - [maple] Resize handling makes no sense

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Stop running resize handling uselessly

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Merge resize handlers

Comment 3

[Chris Lord [:cwiiis]]

Comment on attachment 600772 [details] [diff] [review]
Stop running resize handling uselessly

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

Looks good.

Comment 4

[Chris Lord [:cwiiis]]

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.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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

Comment 6

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/projects/maple/rev/d995a08515c2
https://hg.mozilla.org/projects/maple/rev/3e3ac6103147

Comment 7

[Chris Lord [:cwiiis]]

(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!

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Guard against zero outerWidth/outerHeight

Comment 10

[Chris Lord [:cwiiis]]

Comment on attachment 601069 [details] [diff] [review]
Guard against zero outerWidth/outerHeight

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

Looks good.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/projects/maple/rev/6cf5cc7b3dd9