562773 - Viewport is not resized on window resize / orientation change

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

Attachment: patch

In the case where max-scale will not allow content to scale any further, we expand the content by the leftover pixels.  Currently the expansion only happens on load.  This patch ensures that is also done on window resize.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 442512 [details] [diff] [review]
patch

I don't like fetching the metadata all the time. It really shouldn't change. Wy don't we just add the meta data object to the browser?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

In fact we currently save the metadata in the browser view state. We should just save it to the browser object.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Another thought: How will this fix tabs that are not the selected tab, but become selected after the resize?

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Another thought: "determineSize" isn't the best name (you knew that was coming, right?) since it does more than determine the size, it sets it. How about "updateViewportSize"?

Comment 5

[Benjamin Stover (:stechz)]

All good points.  This patch needs to be updated.

Comment 6

[Benjamin Stover (:stechz)]

Attachment: v2

Adds loop, changes name, uses already calculated metadata, determineSize is now trace friendly

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 442860 [details] [diff] [review]
v2


>+
>+      for (let i = Browser._tabs.length - 1; i >= 0; i--)
>+        Browser._tabs[i].updateViewportSize();
>+

We used to use a loop like this to resize the browsers. Then we switched to use CSS meta-classes:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#402

If we move back to the loop approach, I think we should remove the browser-* meta classes (as long as only browsers use them).

Thoughts? Is there value to keeping the CSS meta classes?

Comment 8

[Benjamin Stover (:stechz)]

Actually I tend to agree.  The classes thing has become confusing since it's not the only path anymore.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 442860 [details] [diff] [review]
v2

This patch should be re-worked to remove the browser specific CSS meta classes.

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: patch v3

Updated patch:

- Get rid of browser-* classes/styles.

- Use the same code path for all document types (no more special cases for handheld, doctype, chrome).

- Changed getViewportMetadata to no longer depend on the window size, so it does not need to be called again on resize.

- Split updateViewportSize into two functions: One that is called only when the metadata changes (from endLoading) and one that is called when the window size changes.

- Fixed a bug exposed by these changes, where zoom levels like 1.00049 were saved in the browserViewportState but the actual zoom level was clamped to 1.0 exactly.

- Test code updated; all test cases are still passing.

Comment 11

[Benjamin Stover (:stechz)]

Comment on attachment 443472 [details] [diff] [review]
patch v3

/>diff -r ef8b3cd6003a chrome/content/BrowserView.js
>-    bvs.metaData = Util.getViewportMetadata(this._browser);
>-

>     if (md && md.defaultZoom)
>-      pageZoom = Math.max(pageZoom, md.defaultZoom);
>+      return Math.max(pageZoom, this.clampZoomLevel(md.defaultZoom));

Nit: I actually find the old way more readable.  If I was adding code below, I might not notice that for certain paths my code wouldn't get executed.

Otherwise, looks a lot cleaner!

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #11)
> (From update of attachment 443472 [details] [diff] [review])
> />diff -r ef8b3cd6003a chrome/content/BrowserView.js
> >-    bvs.metaData = Util.getViewportMetadata(this._browser);
> >-
> 
> >     if (md && md.defaultZoom)
> >-      pageZoom = Math.max(pageZoom, md.defaultZoom);
> >+      return Math.max(pageZoom, this.clampZoomLevel(md.defaultZoom));
> 
> Nit: I actually find the old way more readable.  If I was adding code below, I
> might not notice that for certain paths my code wouldn't get executed.

Why? Because it was in the same file?

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/98208ee91af6

baking on m-b before pushing to m-1.1

Comment 14

[Benjamin Stover (:stechz)]

> Why? Because it was in the same file?

Because what is in the same file?

Again this is a very small thing in this case, but to explain, imagine code like:
> if (...) {
>   ...
>   return blah;
> } else {
>   ...
>   ...
> }
>
> // My new code goes here

If returns aren't really obvious (like the ones that usually check for preconditions at the top of the function and return early), it's hard to tell if code after the conditional is going to get executed.  All other things equal, it's best that conditionals not return early.

In this case it's just a nitpick.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-1.1:
http://hg.mozilla.org/releases/mobile-1.1/rev/f84ced021ca1