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.

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

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

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

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.

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

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

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.

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?

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

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

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

Comment on attachment 654686 [details] [diff] [review] part 2: Make Fennec use the platform GetViewportInfo code Looks sane, coupled with the other patches

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?

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.

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

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

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.

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

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

(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?]

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

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

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.

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?

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

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

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

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

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.

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

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

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?

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

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.

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

Addresses Ms2ger's review comments (thanks!).

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.

Accidentally uploaded the wrong version of this patch.

Argh.

For real.

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; {}

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

Addresses review comment 53.

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

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

\o/ \o/

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

Nice work getting this finished, Matt!

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)

Should this bug have been marked fixed?

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

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

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