716575 - Port fennec front-end support for <meta viewport> into native Gecko code

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P4)

Description

[Patrick Walton (:pcwalton)]

It would be nice to have <meta name="viewport"> support in the platform, so that the presentation shell is aware of it and won't draw until the browser size has been appropriately set. This would allow us to avoid suppressing painting in browser.js. It may be enough to add a flag to the <browser> element that autosizes the element based on the <meta name="viewport"> in the content document.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Scott, aren't you doing this in another bug?

Comment 2

[Scott Johnson (:jwir3)]

(In reply to Brad Lassey [:blassey] from comment #1)
> Scott, aren't you doing this in another bug?

Yes, I'm doing this as part of bug 706198, which, with any luck, should land this week.

Comment 4

[Patrick Walton (:pcwalton)]

Let's not dupe this, but instead use this to track switching over to this native Gecko support for <meta viewport> in Native Fennec.

Comment 5

[Scott Johnson (:jwir3)]

See bug 706198, comment 14. This is actually a bit different than what I thought was being asked. Patrick would like the platform code to change the browser size based on the contents of the meta viewport tag, which is something done in browser.js currently. 

The code in 706198 is only going to add an accessor function in platform to get the viewport information. I'll add the requested functionality to platform in this bug instead, just to keep the work separate.

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

This is a working patch that makes the Fennec use the existing GetViewportInfo code in Gecko rather than duplicating it in JS.  Remaining work (some of which might happen in other bugs):

* Clean up this patch (e.g. add documentation)
* Port some recent minor bugfixes from the JS version of this code to Gecko
* Implement more of the viewport-related code in Gecko

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 1: Add a scriptable interface to GetViewportInfo

These WIP patches aren't quite ready for review; I'm posting them early to help coordination with the B2G team.

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 2: Make Fennec use the platform GetViewportInfo code

Comment 9

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 3: Move more viewport calculations from Fennec code into nsContentUtils

Note: These patches depend on the patches in bug 783565, bug 784612, and bug 784704.  You can also find them at/near the front of my patch queue at http://hg.mozilla.org/users/mbrubeck_mozilla.com/native-fennec-patches/ which will apply to mozilla-inbound.

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: part 4: tests

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: part 1: Add a scriptable interface to GetViewportInfo

As we begin moving the calculation code from Fennec into Gecko, we need a way for the Fennec front-end to access it.  I hope we can remove this scriptable interface in the future when more of the functionality is moved from Fennec to platform code.

I'm not sure who else should review and/or superreview this patch.  Any suggestions?

Comment 12

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: Make Fennec use the platform GetViewportInfo code

Comment 13

[Matt Brubeck (:mbrubeck)]

Attachment: part 3: Move more viewport calculations from Fennec code into nsContentUtils

This moves "getScaleRatio" and assorted code from Fennec into Gecko.  It also cleans up the width/height calculations a bit to more closely match the original Fennec code, and fixes a minor float-vs-int bug.  (The calculation differences were caught by the tests in the next patch.)

On the Fennec side, it's no longer possible to call updateViewportSize alone when the window resizes; instead we need to go through updateMetadata every time, since some of the screen-size-handling code has moved into GetViewportInfo.  This means we will run slightly more code during a window resize, but this code should not be expensive at all (font inflation runs it during reflow).

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 654679 [details] [diff] [review]
part 4: tests

These are based on the old XUL Fennec browser-chrome tests.  XUL Fennec had a few additional test cases, but I want to get this first batch reviewed before porting any more in the same manner.

I'm not sure whether this is the best location for these tests, or who is the best person to review them.  Suggestions welcome.

At the moment, these test the scriptable GetViewportInfo interface so they don't depend on Fennec UI/graphics code.  In the future if we move the viewport-sizing code entirely out of Fennec and into the platform, they could be changed to test the viewport properties (like window.innerWidth) directly.

Comment 15

[Matt Brubeck (:mbrubeck)]

Attachment: part 3 (v3): Move more viewport calculations from Fennec code into nsContentUtils

Fix a crash in GetDevicePixelRatio when WidgetForDocument() returns null.

Comment 16

[Matt Brubeck (:mbrubeck)]

These are green on Try: https://tbpl.mozilla.org/?tree=Try&rev=0375dff65095

Comment 17

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

Comment on attachment 654686 [details] [diff] [review]
part 2: Make Fennec use the platform GetViewportInfo code

Looks sane, coupled with the other patches

Comment 18

[Matt Brubeck (:mbrubeck)]

Comment on attachment 654834 [details] [diff] [review]
part 3 (v3): Move more viewport calculations from Fennec code into nsContentUtils

I'd like David to take a look at this too; in particular I have some questions about nsContentUtils::GetDevicePixelRatio:

* Is there a better name for this function?  Maybe something like GetDevicePixelsPerMetaViewportCSSPixel?

* Where should this function live?  Here in nsContentUtils, or somewhere else like nsPresContext or nsDeviceContext?  In bug 779527 I want to add code to layout/style that depends on this function.

* Should anyone else review this change?

Comment 19

[Doug Sherk (:drs) (inactive)]

These patches fail try (Android opt R1):
https://tbpl.mozilla.org/?tree=Try&rev=0375dff65095

The reason this is failing is because the test harness previously had no idea about the meta viewport since it was all done in front-end code. For now, we have to add a pref that will disable all of this new code and then use that to pref it off it always when running the test harness.

We currently have no tests which actually check meta viewport (afaik) so we'll need to add those. Filed bug 786840.

Comment 20

[Matt Brubeck (:mbrubeck)]

(In reply to Doug Sherk (:drs) (:dRdR) from comment #19)
> These patches fail try (Android opt R1):
> https://tbpl.mozilla.org/?tree=Try&rev=0375dff65095

That push included the four patches here, plus one from bug 564815.  I've started some new Try pushes with different subsets of those patches to narrow down the cause:
https://tbpl.mozilla.org/?tree=Try&rev=0da13f43652f
https://tbpl.mozilla.org/?tree=Try&rev=c837c06fde45
https://tbpl.mozilla.org/?tree=Try&rev=e4021e069df1

> The reason this is failing is because the test harness previously had no
> idea about the meta viewport since it was all done in front-end code. For
> now, we have to add a pref that will disable all of this new code and then
> use that to pref it off it always when running the test harness.

I'm not too sure about that; these patches *shouldn't* cause any change in behavior.  Both with and without these patches, Fennec will calculate and set the viewport size for every page that loads, including pages loaded by the reftest harness.  The testcases that failed don't even have <meta name=viewport> elements, so they *really* shouldn't be affected.

The R1 orange is intermittent and affects different tests each time; it looks like the affected tests are drawn at the wrong scale and resolution.  Here's another run to compare:
https://tbpl.mozilla.org/?tree=Try&rev=66c98e4fe62f

The fact that it's intermittent, and that I don't see similar problems in manual testing, makes me think there is a timing issue.  The failing tests seem to be drawn at the wrong scale or resolution.  Sometimes it's the test image that's mis-scaled, and sometimes the reference image.

Comment 21

[Matt Brubeck (:mbrubeck)]

This push with all of the code changes from this bug does not show any of the new R1 orange:
https://tbpl.mozilla.org/?tree=Try&rev=c837c06fde45

and this one with the patch from bug 564815 does not either:
https://tbpl.mozilla.org/?tree=Try&rev=e4021e069df1

These are applied on top of the same m-c changeset as my earlier Try pushes.  So either the R1 orange is caused by some interaction between this bug and bug 564815 (which shouldn't interact at all), or my new pushes are freakishly lucky, or the earlier oranges were some infrastructure issue unrelated to my code changes, or some even odder explanation I haven't thought of...

Comment 22

[Matt Brubeck (:mbrubeck)]

R1 re-triggers on the previously-orange builds are now green (aside from known intermittent orange).  Philor things some badly-behaving tegras may have been added to the Try pool on 8/28, which is the only day that the new R1 orange appeared.

Comment 23

[Doug Sherk (:drs) (inactive)]

(In reply to Doug Sherk (:drs) (:dRdR) from comment #19)
> These patches fail try (Android opt R1):
> https://tbpl.mozilla.org/?tree=Try&rev=0375dff65095
> 
> The reason this is failing is because the test harness previously had no
> idea about the meta viewport since it was all done in front-end code. For
> now, we have to add a pref that will disable all of this new code and then
> use that to pref it off it always when running the test harness.
> 
> We currently have no tests which actually check meta viewport (afaik) so
> we'll need to add those. Filed bug 786840.

BTW, this is wrong, I forgot that browser.js still has to invoke the actual viewport settings. The only thing that changed is where the calculations are done.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 654685 [details] [diff] [review]
part 1: Add a scriptable interface to GetViewportInfo

r=dbaron (but ugh, out parameters -- it would be a nicer API to return an object with properties, but I guess that's substantially more work.  I wish it weren't.)

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 654834 [details] [diff] [review]
part 3 (v3): Move more viewport calculations from Fennec code into nsContentUtils

(In reply to Matt Brubeck (:mbrubeck) from comment #18)
> Comment on attachment 654834 [details] [diff] [review]
> part 3 (v3): Move more viewport calculations from Fennec code into
> nsContentUtils
> 
> I'd like David to take a look at this too; in particular I have some
> questions about nsContentUtils::GetDevicePixelRatio:
> 
> * Is there a better name for this function?  Maybe something like
> GetDevicePixelsPerMetaViewportCSSPixel?

Hmmm.  We do want to distinguish it from what nsDeviceContext::SetDPI does (it might also be good to mention nsDeviceContext::SetDPI in a comment).  I actually kinda like your suggested long name (GetDevicePixelsPerMetaViewportCSSPixel), though it could be shortened to "PerMetaViewportPixel", perhaps

> * Where should this function live?  Here in nsContentUtils, or somewhere
> else like nsPresContext or nsDeviceContext?  In bug 779527 I want to add
> code to layout/style that depends on this function.

I don't really have an opinion here.  I think this is fine, though nsLayoutUtils might be better..

> * Should anyone else review this change?

roc, I think

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, and a comment on all the patches (1, 2, and 4, at least):  I much prefer to have useful commit messages in the patches posted for review.  This is for two reasons:
 (1) I can review the commit messages (since I think commit messages often need review)
 (2) it helps me understand what the patch is trying to do.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 654679 [details] [diff] [review]
part 4: tests

r=dbaron.  More tests! :-)

(I did not look at the math.  I'm assuming they pass.)

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 654834 [details] [diff] [review]
part 3 (v3): Move more viewport calculations from Fennec code into nsContentUtils

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

::: content/base/public/nsContentUtils.h
@@ +1559,5 @@
>  
> +  /**
> +   * The device-pixel-to-CSS-px ratio used to adjust meta viewport values.
> +   */
> +  static double GetDevicePixelRatio(nsIWidget* aWidget);

I like your suggested longer name

::: content/base/src/nsContentUtils.cpp
@@ +5231,5 @@
> +  else if (dpi < 300.0) // Includes Nokia N900, and HDPI Android devices
> +    return 1.5;
> +
> +  // For very high-density displays like the iPhone 4, calculate an integer ratio.
> +  return floor(dpi / 150.0);

Why are we doing this thresholding?

Comment 29

[Matt Brubeck (:mbrubeck)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> > +  else if (dpi < 300.0) // Includes Nokia N900, and HDPI Android devices
> > +    return 1.5;
> > +
> > +  // For very high-density displays like the iPhone 4, calculate an integer ratio.
> > +  return floor(dpi / 150.0);
> 
> Why are we doing this thresholding?

In general, I think we want to use an integer ratio, to make it easy to avoid "blurry" interpolation of images on high-density displays.  The one exception is the 1.5 ratio for devices in the 200dpi range, which is the de-facto standard used by current mobile browsers on those devices (including Fennec).  [Have I understood and answered the question properly?]

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Brubeck (:mbrubeck) from comment #29)
> In general, I think we want to use an integer ratio, to make it easy to
> avoid "blurry" interpolation of images on high-density displays. The one
> exception is the 1.5 ratio for devices in the 200dpi range, which is the
> de-facto standard used by current mobile browsers on those devices
> (including Fennec).  [Have I understood and answered the question properly?]

Yes. However, I'm not sure I believe the answer :-). We scale images constantly in the mobile browser so why are we worried about "blurry" interpolation in this one case?

Comment 31

[Matt Brubeck (:mbrubeck)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> Yes. However, I'm not sure I believe the answer :-). We scale images
> constantly in the mobile browser so why are we worried about "blurry"
> interpolation in this one case?

This code is specifically for "mobile-optimized" sites and web apps.  A page that uses <meta name="viewport" content="width=device-width"> will have a pixel ratio of 1 on iPhone 3G or Android G1, a ratio of 1.5 on Droid and Nexus One, and a ratio of 2 on iPhone 4 or Galaxy Nexus.  These sites can use the resolution media query to provide pixel-perfect images at each of these densities.

And whether the blurriness is key or not, this is also a compatibility issue.  As I said above, a ratio of 1.5 is the de-facto standard for devices in the ~240dpi ("HDPI") class.  This ratio is used not just by the browser but also by the Android UI.  These devices are designed to have apps and content rendered with 1.5x the pixels of devices in the "MDPI" class.

Comment 32

[Matt Brubeck (:mbrubeck)]

On Android we could delegate this decision to the OS if we wanted, and just use DisplayMetrics.density as our dppx ratio:
http://developer.android.com/reference/android/util/DisplayMetrics.html#density

But we'd still need our own logic for other platforms. The logic here is what Fennec already uses on Android, Maemo, and desktop.  Bug 746502 makes B2G share the same logic.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK that makes sense.

Would it make sense to have nsIWidget::GetDefaultScale() return this computed value? I think it probably would. I mean, is there a reason to only do this computation for <meta viewport> and not for other kinds of rendering?

Comment 34

[Matt Brubeck (:mbrubeck)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
> Would it make sense to have nsIWidget::GetDefaultScale() return this
> computed value? I think it probably would. I mean, is there a reason to only
> do this computation for <meta viewport> and not for other kinds of rendering?

Yes, I think that would be a good idea.  If I understand right it should give proper default scaling on platforms that have high-density displays but that don't use "displayport-style" viewport panning and scaling.  And it seems like it might be the right way to fix bug 779527.

However, when I tried this, I saw that it changed AppUnitsPerDevPixel everywhere, which seemed to break assumptions somewhere in front-end and/or graphics code, at least in Fennec on Android.  For example, clicks were not targeted correctly, panning stopped before the end of the document, and some tiles had the wrong resolution and scale.

If possible, I'd like to limit *this* bug's scope to a direct port of Fennec's meta-viewport code into Gecko, without touching any other code.  I can assist with integrating it further into the layout/graphics in follow-up bugs (or in this bug if you prefer), but I may not be the right person to do that work myself.  (And I'm not sure how it should interact with bug 674373 which is also changing GetDefaultScale and related code.)

I'll start working on your suggestion now, but I'd also like to know if you think these patches are okay to land on their own in the meantime.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 654834 [details] [diff] [review]
part 3 (v3): Move more viewport calculations from Fennec code into nsContentUtils

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

We can go with this (with the longer name).

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Brubeck (:mbrubeck) from comment #34)
> Yes, I think that would be a good idea.  If I understand right it should
> give proper default scaling on platforms that have high-density displays but
> that don't use "displayport-style" viewport panning and scaling.

It should affect platforms that use displayport panning and scaling too, if we do things right.

> I'm not sure how it should interact with bug 674373 which is also changing
> GetDefaultScale and related code.

Bug 674373 changes GetDefaultScale on Mac, yes. It's basically using the same approach as I'm recommending here.

> I'll start working on your suggestion now, but I'd also like to know if you
> think these patches are okay to land on their own in the meantime.

Yeah, thanks.

But we do need to change GetDefaultScale for Android (and B2G) so that it takes care of this.

Comment 37

[Matt Brubeck (:mbrubeck)]

Landed with the longer function name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c1d26f8bc9
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b15c079564c
https://hg.mozilla.org/integration/mozilla-inbound/rev/910406b62ebc
https://hg.mozilla.org/integration/mozilla-inbound/rev/38052fd23aec

Comment 38

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/3be2deb368d3 - we were probably too optimistic about the reftest issue being badslaves, since your first R1 failed with a bunch of "hey, why are you drawing so huge and off to the right, the screen's over here!" failures, but in any case, turns out running talos on try would have been good, since when it didn't fail for infra reasons, it was permared from whatever it means by either "browser non-zero return code (1)" or "Could not find report in browser output".

Comment 39

[Matt Brubeck (:mbrubeck)]

I'm planning to re-land these patches today without the Fennec (/browser/android) parts, so the code will be in the tree but unused until we can fix the Fennec test failures.  The B2G team has basecamp blockers that depend on this code so they want the platform parts landed ASAP.

Comment 40

[Matt Brubeck (:mbrubeck)]

Re-landed all but the Fennec changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e80be1d7d18
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f144a06d0cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2e45d25a1a

Comment 41

[Matt Brubeck (:mbrubeck)]

Sorry, backed out again because it still broke Talos on Android.  Weird!  I don't see anything in the Talos log to indicate why it failing.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e5d4fe0ba07

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks like browser-output.txt was not even created?

Looks like nsIDOMWindowUtils.idl needs an IID rev but didn't get one. Could that possibly be the problem?

If all else fails, try relanding the patches one by one to see which one causes bustage?

Comment 43

[Matt Brubeck (:mbrubeck)]

It looks like the Talos red is caused by something in part 3, and is not fixed by restoring the missing IID change.
https://tbpl.mozilla.org/?tree=Try&rev=3ef6f50db739 (part 1 + IID change = green)
https://tbpl.mozilla.org/?tree=Try&rev=3ef6f50db739 (part 1 + IID change + part 3 = red)

Something in GetViewportInfo might be crashing on Android.  Since this is happening only on Talos, I'm guessing that mabye the Talos test harness sets one of the prefs used by this code to a value that causes a crash or OOM or something.

Comment 44

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 654834 [details] [diff] [review]
part 3 (v3): Move more viewport calculations from Fennec code into nsContentUtils

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

That's a lot of reviews... How about one from someone who owns this code?

::: content/base/src/nsContentUtils.cpp
@@ +11,5 @@
>  #include "jsapi.h"
>  #include "jsdbgapi.h"
>  #include "jsfriendapi.h"
>  
> +#include "math.h"

#include <math.h>

@@ +5222,5 @@
> +nsContentUtils::GetDevicePixelRatio(nsIWidget* aWidget)
> +{
> +  int prefValue = Preferences::GetInt("browser.viewport.scaleRatio", 0);
> +  if (prefValue > 0)
> +    return double(prefValue) / 100.0;

{}

@@ +5226,5 @@
> +    return double(prefValue) / 100.0;
> +
> +  float dpi = aWidget->GetDPI();
> +  if (dpi < 200.0) // Includes desktop displays, and LDPI and MDPI Android devices
> +    return 1.0;

{}

@@ +5227,5 @@
> +
> +  float dpi = aWidget->GetDPI();
> +  if (dpi < 200.0) // Includes desktop displays, and LDPI and MDPI Android devices
> +    return 1.0;
> +  else if (dpi < 300.0) // Includes Nokia N900, and HDPI Android devices

No else after return

@@ +5228,5 @@
> +  float dpi = aWidget->GetDPI();
> +  if (dpi < 200.0) // Includes desktop displays, and LDPI and MDPI Android devices
> +    return 1.0;
> +  else if (dpi < 300.0) // Includes Nokia N900, and HDPI Android devices
> +    return 1.5;

{}

Comment 45

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 654685 [details] [diff] [review]
part 1: Add a scriptable interface to GetViewportInfo

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

::: content/base/src/nsContentUtils.cpp
@@ +5048,5 @@
>                           (c == '\t') || (c == '\n') || (c == '\r'))
>  
>  /* static */
>  ViewportInfo
> +nsContentUtils::GetViewportInfo(nsIDocument *aDocument, uint32_t aDisplayWidth, uint32_t aDisplayHeight)

Wrap at 80 columns

::: dom/base/nsDOMWindowUtils.cpp
@@ +249,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +nsDOMWindowUtils::GetViewportInfo(uint32_t aDisplayWidth, uint32_t aDisplayHeight,

80 columns

@@ +258,5 @@
> +{
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
> +  NS_ENSURE_STATE(window);
> +
> +  nsCOMPtr<nsIDocument> doc(do_QueryInterface(window->GetExtantDocument()));

Use GetExtantDoc()

::: layout/base/nsLayoutUtils.cpp
@@ +5013,5 @@
> +
> +  // TODO:
> +  // Once bug 716575 has been resolved, this code should be changed so that it
> +  // does the right thing on all platforms.
> +  nsresult result;

rv

@@ +5014,5 @@
> +  // TODO:
> +  // Once bug 716575 has been resolved, this code should be changed so that it
> +  // does the right thing on all platforms.
> +  nsresult result;
> +  int32_t screenLeft, screenTop, screenWidth, screenHeight;

Declare those lower

@@ +5016,5 @@
> +  // does the right thing on all platforms.
> +  nsresult result;
> +  int32_t screenLeft, screenTop, screenWidth, screenHeight;
> +  nsCOMPtr<nsIScreenManager> screenMgr =
> +    do_GetService("@mozilla.org/gfx/screenmanager;1", &result);

Why are you passing &result if you're not going to check it?

@@ +5020,5 @@
> +    do_GetService("@mozilla.org/gfx/screenmanager;1", &result);
> +
> +  nsCOMPtr<nsIScreen> screen;
> +  screenMgr->GetPrimaryScreen(getter_AddRefs(screen));
> +  screen->GetRect(&screenLeft, &screenTop, &screenWidth, &screenHeight);

No need for a null-check?

Comment 46

[Matt Brubeck (:mbrubeck)]

Scott and I ran some experiments on Try, and discovered that the Talos crashes are caused because screen->GetRect is returning a 0-size rect (which ultimately causes a divide-by-zero in GetViewportInfo).

I believe this happens because the Android Java code does not send the screen size to Gecko until it receives a Gecko:Ready message.  This message is usually sent when browser.xul loads, but the Talos pageloader extension replaces browser.xul with its own pageloader.xul.

The crash can be avoided by adding a zero check before dividing by aDisplayWidth or aDisplayHeight.  The lack of screen information in Talos might be fixed just by sending a Gecko:Ready message from pageloader.js when running in native Android Fennec.

Comment 47

[Doug Sherk (:drs) (inactive)]

Awesome, did you notice intermittent failed R3 tests while you were experimenting?

Comment 48

[Matt Brubeck (:mbrubeck)]

Attachment: part 1 (v2): Add a scriptable interface to GetViewportInfo

Addresses Ms2ger's review comments (thanks!).

Comment 49

[Matt Brubeck (:mbrubeck)]

Attachment: part 3a (gecko changes only): Move more viewport calculations from Fennec code into nsContentUtils

Addresses Ms2ger's review comments, and adds a check to prevent divide-by-zero.  Pushed to Try: https://tbpl.mozilla.org/?tree=Try&rev=87f2a4cff6b9

(In reply to Doug Sherk (:drs) (:dRdR) from comment #47)
> Awesome, did you notice intermittent failed R3 tests while you were
> experimenting?

When I pushed without the Fennec front-end changes, I did not see any reftest failures.  I haven't yet investigated the failures that happened when I pushed *with* the Fennec changes.

Comment 50

[Matt Brubeck (:mbrubeck)]

Attachment: part 1 (v2): Add a scriptable interface to GetViewportInfo

Accidentally uploaded the wrong version of this patch.

Comment 51

[Matt Brubeck (:mbrubeck)]

Attachment: part 1 (v2): Add a scriptable interface to GetViewportInfo

Argh.

Comment 52

[Matt Brubeck (:mbrubeck)]

Attachment: part 3a (gecko changes only): Move more viewport calculations from Fennec code into nsContentUtils

For real.

Comment 53

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 663443 [details] [diff] [review]
part 3a (gecko changes only): Move more viewport calculations from Fennec code into nsContentUtils

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

::: content/base/src/nsContentUtils.cpp
@@ +11,5 @@
>  #include "jsapi.h"
>  #include "jsdbgapi.h"
>  #include "jsfriendapi.h"
>  
> +#include "math.h"

<>

@@ +5174,5 @@
>      autoSize = true;
>    }
>  
> +  // Now convert the scale into device pixels per CSS pixel.
> +  nsIWidget *widget = WidgetForDocument(aDocument);

* to the left

@@ +5197,5 @@
> +    bool validWidth = (!widthStr.IsEmpty() && NS_SUCCEEDED(widthErrorCode) && width > 0);
> +    bool validHeight = (!heightStr.IsEmpty() && NS_SUCCEEDED(heightErrorCode) && height > 0);
> +    if (!validWidth) {
> +      if (validHeight) {
> +        width = (uint32_t) ((height * aDisplayWidth) / aDisplayHeight);

uint32_t((height * aDisplayWidth) / aDisplayHeight)

@@ +5200,5 @@
> +      if (validHeight) {
> +        width = (uint32_t) ((height * aDisplayWidth) / aDisplayHeight);
> +      } else {
> +        width = Preferences::GetInt("browser.viewport.desktopWidth",
> +            kViewportDefaultScreenWidth);

Align the k of 'kViewportDefaultScreenWidth' under the first quote of '"browser.viewport.desktopWidth"'.

@@ +5204,5 @@
> +            kViewportDefaultScreenWidth);
> +      }
> +    }
> +    if (!validHeight) {
> +      height = (uint32_t) ((width * aDisplayHeight) / aDisplayWidth);

uint32_t((width * aDisplayHeight) / aDisplayWidth)

@@ +5214,5 @@
>  
>    // Also recalculate the default zoom, if it wasn't specified in the metadata,
>    // and the width is specified.
>    if (scaleStr.IsEmpty() && !widthStr.IsEmpty()) {
> +    scaleFloat = NS_MAX(scaleFloat, ((float)aDisplayWidth) / (float)width);

float(aDisplayWidth) / float(width)

@@ +5256,5 @@
> +nsContentUtils::GetDevicePixelsPerMetaViewportPixel(nsIWidget* aWidget)
> +{
> +  int prefValue = Preferences::GetInt("browser.viewport.scaleRatio", 0);
> +  if (prefValue > 0)
> +    return double(prefValue) / 100.0;

{}

@@ +5260,5 @@
> +    return double(prefValue) / 100.0;
> +
> +  float dpi = aWidget->GetDPI();
> +  if (dpi < 200.0) // Includes desktop displays, and LDPI and MDPI Android devices
> +    return 1.0;

{}

@@ +5261,5 @@
> +
> +  float dpi = aWidget->GetDPI();
> +  if (dpi < 200.0) // Includes desktop displays, and LDPI and MDPI Android devices
> +    return 1.0;
> +  else if (dpi < 300.0) // Includes Nokia N900, and HDPI Android devices

No else

@@ +5262,5 @@
> +  float dpi = aWidget->GetDPI();
> +  if (dpi < 200.0) // Includes desktop displays, and LDPI and MDPI Android devices
> +    return 1.0;
> +  else if (dpi < 300.0) // Includes Nokia N900, and HDPI Android devices
> +    return 1.5;

{}

Comment 54

[Matt Brubeck (:mbrubeck)]

Attachment: part 3a (v2): Move more viewport calculations from Fennec code into nsContentUtils (gecko changes only)

Sigh, got the zero check slightly wrong.  Dreadfully sorry for the bugspam.

Comment 55

[Matt Brubeck (:mbrubeck)]

Attachment: part 3a (v3): Move more viewport calculations from Fennec code into nsContentUtils (gecko changes only)

Addresses review comment 53.

Comment 56

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 663451 [details] [diff] [review]
part 3a (v3): Move more viewport calculations from Fennec code into nsContentUtils (gecko changes only)

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

Just one more comment:

::: content/base/src/nsContentUtils.cpp
@@ +5262,5 @@
>  /* static */
> +double
> +nsContentUtils::GetDevicePixelsPerMetaViewportPixel(nsIWidget* aWidget)
> +{
> +  int prefValue = Preferences::GetInt("browser.viewport.scaleRatio", 0);

int32_t, not int

Comment 57

[Matt Brubeck (:mbrubeck)]

Green on Try (including Talos): https://tbpl.mozilla.org/?tree=Try&rev=9ef5439d1bea

Pushed Gecko patches again, with final review comment fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab874d12dcde
https://hg.mozilla.org/integration/mozilla-inbound/rev/39803b4c2b9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9108fa673bfe

Comment 58

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

\o/ \o/

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ab874d12dcde
https://hg.mozilla.org/mozilla-central/rev/39803b4c2b9d
https://hg.mozilla.org/mozilla-central/rev/9108fa673bfe

Comment 60

[Scott Johnson (:jwir3)]

Nice work getting this finished, Matt!

Comment 61

[Geoff Brown [:gbrown]]

It appears this caused a significant Android Talos regression - see bug 793755.

Note these values from the Talos tests on try, from comment 57:

trobopan: 28437.75 (vs approx 19000 typical on Sept 20)
tdhtml_nochrome: 1021.15 (vs 900 typical)
tp4m_nochrome: 881.35 (vs 750 typical)

Comment 62

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should this bug have been marked fixed?

Comment 63

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] (recovering from illness; hopefully back Oct 8, but with backlog) from comment #62)
> Should this bug have been marked fixed?

I don't think so, because I don't think the fennec changes have landed yet.

Comment 64

[Matt Brubeck (:mbrubeck)]

To make it easier to track what landed when, I'll resolve this bug and split the Fennec parts into a new bug.

Comment 65

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #34)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
> > Would it make sense to have nsIWidget::GetDefaultScale() return this
> > computed value? I think it probably would. I mean, is there a reason to only
> > do this computation for <meta viewport> and not for other kinds of rendering?
> 
> Yes, I think that would be a good idea.

Filed follow-up bug 803207.