Madhava Enros [:madhava] Reporter
	Description • 13 years ago

(this may be an unwarranted generalization)

At least on the following mobile sites:

m.nytimes.com - http://www.flickr.com/photos/madhava_work/6426482027/
and
www.thestar.com/iphone - http://www.flickr.com/photos/madhava_work/6426524147/
and
m.flickr.com - http://www.flickr.com/photos/madhava_work/6426526275/

There are a lot of parts of the page that are smaller or bigger than they should be. Again - I might be conflating a lot of different problems into this one bug, but I wonder -- should we be skipping font inflation on sites designed specifically for mobile screens?

	Matt Brubeck (:mbrubeck)
	Comment 1 • 13 years ago

In my "Bigger Text" add-on, I had a preference (enabled by default) to disable text zoom on "mobile" sites, which meant any site with a <meta name="viewport"> tag that included "width=device-width" or otherwise resulted in an initial zoom level of 1.0 or higher:

https://addons.mozilla.org/en-US/mobile/addon/bigger-text/

http://hg.mozilla.org/users/mbrubeck_mozilla.com/biggertext/file/edeed0470bad/content/overlay.js#l74

Perhaps Gecko should have a similar heuristic for text inflation.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 2 • 13 years ago

Feel free to dupe this to another font inflation bug

	David Baron :dbaron:
	Comment 3 • 13 years ago

Seems reasonable to want to opt out of inflation for mobile sites.  Do you know if any other mobile browsers also opt out of font inflation or similar for mobile sites, and if so what the criterion for being a mobile site is?

	Scott Johnson (:jwir3) Assignee
	Comment 4 • 13 years ago

(In reply to David Baron [:dbaron] from comment #3)
> Seems reasonable to want to opt out of inflation for mobile sites.  Do you
> know if any other mobile browsers also opt out of font inflation or similar
> for mobile sites, and if so what the criterion for being a mobile site is?

I'm not entirely sure this is the answer. Two of the screenshots that Madhava showed had text that was 'too small'. I don't think suppressing the font inflation is going to take care of these, unless I'm incorrect?

	Matt Brubeck (:mbrubeck)
	Comment 5 • 13 years ago

(In reply to Scott Johnson (:jwir3) from comment #4)
> I'm not entirely sure this is the answer. Two of the screenshots that
> Madhava showed had text that was 'too small'.

I think the "two small" parts are already fixed by bug 694901, which landed one day after Madhava's screenshots were posted.

	Madhava Enros [:madhava] Reporter
	Comment 6 • 13 years ago

Can we try the mechanism suggested in comment 1 "any site with a <meta name="viewport"> tag that included "width=device-width" or otherwise resulted in an initial zoom level of 1.0 or higher" or some other preferred heuristic to see what we end up with? Inflation is doing a pretty good job now on most desktop sites for me, but I still encounter large text on mobile sites.

	David Baron :dbaron:
	Comment 7 • 13 years ago

So one problem here is that there are a bunch of things that trigger this sort of behavior, and the logic for it all lives in frontend code -- in particular, in the getViewportMetadata function in mobile/android/chrome/content/browser.js (or, for XUL fennec, mobile/xul/chrome/content/content.js ).

I tend to think that a first step towards making "mobile site" determinations in Gecko is moving that logic from the front-end into Gecko.

For example, mobile.nytimes.com has no viewport meta, but hits this part of getViewportMetadata:
    let doctype = content.document.doctype;
    if (doctype && /(WAP|WML|Mobile)/.test(doctype.publicId))
      return { defaultZoom: 1, autoSize: true, allowZoom: true, autoScale: true };

	Scott Johnson (:jwir3) Assignee
	Comment 8 • 13 years ago

(In reply to David Baron [:dbaron] from comment #7)
> So one problem here is that there are a bunch of things that trigger this
> sort of behavior, and the logic for it all lives in frontend code -- in
> particular, in the getViewportMetadata function in
> mobile/android/chrome/content/browser.js (or, for XUL fennec,
> mobile/xul/chrome/content/content.js ).

Is this different than nsContentUtils::ProcessViewportInfo()? It seems like this function has the same effect, but sets the viewport information in the header data of the nsIDocument object, rather than returning it.

	Scott Johnson (:jwir3) Assignee
	Comment 9 • 13 years ago

David, if you could give me an idea of whether I'm on the right track here, I would appreciate it. I think I have the logic from the front end code you were talking about moved into nsContentUtils (I'm not sure whether this is the correct place for it). 

Also, this patch hasn't been tested on my phone yet. This is due to bug 714289, which is showing only blank pages for me right now with the latest nightly. I'm going to try to move back to an earlier revision and see if I can apply the patch to a working changeset, to test it. In the meantime, I wanted to post it for initial review.

	Patrick Walton (:pcwalton)
	Comment 12 • 13 years ago

Just thought I'd leave a quick reminder that if the platform is gaining support for <meta name="viewport">, then mPaintingSuppressed on the presentation shell needs to be set to true until that tag is added to the document and the browser is resized. Otherwise we will regress some of the work in bug 708746 (users will see jumpiness when navigating from page to page because the new content will be displayed with the old viewport).

	Scott Johnson (:jwir3) Assignee
	Comment 13 • 13 years ago

(In reply to Patrick Walton (:pcwalton) from comment #12)
> Just thought I'd leave a quick reminder that if the platform is gaining
> support for <meta name="viewport">, then mPaintingSuppressed on the
> presentation shell needs to be set to true until that tag is added to the
> document and the browser is resized. Otherwise we will regress some of the
> work in bug 708746 (users will see jumpiness when navigating from page to
> page because the new content will be displayed with the old viewport).

Well, pretty much all I did was give the availability to retrieve the viewport information from within the platform code. I'm not entirely sure this is what you are talking about. In other words, all I did was add a 'GetViewportMetadata()'-like function to nsContentUtils.

	Patrick Walton (:pcwalton)
	Comment 14 • 13 years ago

(In reply to Scott Johnson (:jwir3) from comment #13)
> Well, pretty much all I did was give the availability to retrieve the
> viewport information from within the platform code. I'm not entirely sure
> this is what you are talking about. In other words, all I did was add a
> 'GetViewportMetadata()'-like function to nsContentUtils.

Ah, ok. What I would also like is for the ability to a XUL <browser> to resize itself automatically based on the <meta name="viewport"> in the content. This would allow us to remove the code that does this in browser.js.

	Scott Johnson (:jwir3) Assignee
	Comment 15 • 13 years ago

Comment on attachment 586489 [details] [diff] [review]
b706198 - Disable font inflation on sites optimized for mobile.

Making a few changes and adding some unit tests.

	Scott Johnson (:jwir3) Assignee
	Comment 16 • 13 years ago

Version that's been tested on mobile, along with several reftests.

	Scott Johnson (:jwir3) Assignee
	Comment 17 • 13 years ago

Comment on attachment 588443 [details] [diff] [review]
b706198 (v2) - Disable font inflation on sites optimized for mobile.

Hi Mark - 

Just added you for reviewing the viewport accessor function I was talking about at the meeting. Basically, I added GetViewportInfo(), which should be fairly symmetric to what's in browser.js currently.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 18 • 13 years ago

Comment on attachment 588443 [details] [diff] [review]
b706198 (v2) - Disable font inflation on sites optimized for mobile.

The GetViewportInfo method looks pretty faithful to the JS one, but Matt has more experience with that code. Passing review to him.

	Matt Brubeck (:mbrubeck)
	Comment 19 • 13 years ago

Comment on attachment 588443 [details] [diff] [review]
b706198 (v2) - Disable font inflation on sites optimized for mobile.

>+struct ViewportInfo
>+{
>+    double defaultZoom;
>+    double minZoom;
>+    double maxZoom;
>+    PRUint32 width;
>+    PRUint32 height;
>+    bool autoSize;
>+    bool allowZoom;
>+    bool autoScale;
>+};

Just thinking out loud... I'm not sure we need to preserve "autoScale" here.  It was added in e10s Fennec as a hack to work around bugs that prevented us from zooming documents in the parent process.  We still use it in native Fennec for XUL documents only, but probably we shouldn't.

I guess we can keep it for now (to keep this code in sync with the front-end), but we should plan to get rid of it in the future, in a separate bug.

>+  scaleFloat = NS_MIN(scaleFloat, kViewportMaxScale);
>+  scaleFloat = NS_MAX(scaleFloat, kViewportMinScale);

We should compute scaleMinFloat and scaleMaxFloat first, and use them here instead of the default values.  This will ensure, for example, that content="minimum-scale=2.0,maximum-scale=2.0" will also result in an initial-scale of 2.0.

>+  PRUint32 width = widthStr.ToInteger(&errorCode);
>+  if (errorCode) {
>+    width = kViewportMinWidth;
>+  }

If the width is not provided, it should instead be calculated based on the screen width and the initial-scale (if provided) or height (if provided), or set to a default value (Fennec uses 980px).

If this isn't the appropriate place to do these calculations, maybe we should return 0 instead of kViewportMinWidth, to indicate that no width was specified.

>+  PRUint32 height = heightStr.ToInteger(&errorCode);
>+  if (errorCode) {
>+    height = kViewportMinHeight;
>+  }

Similarly, if the height is not provided, it should be calculated based on the width, scale, and screen size parameters.

>+  if ((userScalable.Find("0") != -1) || (userScalable.Find("No") != -1)
>+    || (userScalable.Find("false") != -1)) {
>+    allowZoom = false;
>+  }

This should be testing for "no" (lowercase).  I'm not sure whether other browsers use a case-sensitive comparison.

According to http://dev.w3.org/csswg/css-device-adapt/#the-user-scalable-property, only "yes" or a number outside the range (-1, 1) should result in allowZoom = true; other values should result in allowZoom = false.  However, this spec is still an Editor's Draft and I don't think it quite matches current implementations.  The current Fennec code sets allowZoom to false only for "0" or "no" or "false", which I believe was based on WebKit's behavior.

>+  // If we are on a mobile-only website, then we want to disable the font
>+  // inflation.
>+  nsPresContext* presContext = aReflowState.frame->PresContext();
>+  NS_ASSERTION(presContext, "PresContext should not be null");
>+
>+  ViewportInfo vInf =
>+    nsContentUtils::GetViewportInfo(presContext->PresShell()->GetDocument());
>+
>+  if (vInf.defaultZoom >= 1.0) {
>+    return 0;
>+  }

I would change this to "if (vInf.defaultZoom >= 1.0 || vInf.autoSize)".  A page with <meta name="viewport" content="width=device-width"> should be treated as mobile-optimized, especially since this is defined to be equivalent to content="initial-zoom=1".

There's also the common case of <meta name="viewport" content="width=320"> (or similar), where the initial zoom level is not specified but instead will depend on the display size.  For displays wider than 320px, the zoom level will necessarily be greater than 1.0  We want to disable inflation in this case too.

>+++ b/layout/base/tests/font-inflation/disable-fontinfl-on-mobile-ref.html
>@@ -0,0 +1,19 @@
>+<!--
>+Without the patch for bug 706198, this website should not show up as inflated. That means
>+that with a 450px container, the minimum font size with 15em per line should be 30px.
>+So, we map 0px-45px into 30px-45px, and thus 12px gets mapped to 34px.
>+
>+With the patch, the text should be uninflated, which means that 12px should still be
>+12px.
>+-->
>+<!DOCTYPE html>
>+<html>
>+  <head>
>+    <style>
>+      p { font-size: 12px; line-height: 1.0;}
>+    </style>
>+    <body>
>+      <p>Some uninflated text.</p>
>+    </body>
>+  </head>
>+</html>

I don't understand this test.  Why is inflation disabled for this reference file?

	Matt Brubeck (:mbrubeck)
	Comment 20 • 13 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #19)
> If the width is not provided, it should instead be calculated based on the
> screen width and the initial-scale (if provided) or height (if provided), or
> set to a default value (Fennec uses 980px).
> 
> If this isn't the appropriate place to do these calculations, maybe we
> should return 0 instead of kViewportMinWidth, to indicate that no width was
> specified.

For a bit more context:  In Fennec, the viewport calculations are split into two parts.  That organization dates from Electrolysis: in e10s Fennec, the first part of the code retrieves data available only in the content process, like the meta tag contents and the doctype.  The second part of the code receives that data and combines it with data available only in the parent process, like the size of the <browser> element.

In mobile/android/chrome/content/browser.js the two main functions involved are ViewportHandler.getViewportMetadata and Tab.prototype.updateViewportSize.  (See also Tab.prototype.updateViewportMetadata, which is where "autoScale" is applied.)

This patch replicates the logic from the first "part" of the code (getViewportMetadata), but does not include the logic from the second "part" (updateViewportMetadata) which is needed to fully compute the viewport state.

For this bug we don't *need* the full calculation, but we should implement enough that "minimum-scale=1,maximum-scale=1" will imply "initial-scale=1", and that we can distinguish "width=200" from an unspecified width.  (Not losing data about unspecified values is also important if this code will later be used to implement the full calculation.)

	Scott Johnson (:jwir3) Assignee
	Comment 21 • 13 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #19)

Hi Matt. I just have a couple of questions about some of what you brought up here. I'm posting a rough work-in-progress patch, so you can see what I'm thinking about doing in response to your review comments. 

I'll address the issues inline here:

> Just thinking out loud... I'm not sure we need to preserve "autoScale" here.
> It was added in e10s Fennec as a hack to work around bugs that prevented us
> from zooming documents in the parent process.  We still use it in native
> Fennec for XUL documents only, but probably we shouldn't.
> 
> I guess we can keep it for now (to keep this code in sync with the
> front-end), but we should plan to get rid of it in the future, in a separate
> bug.

I decided to leave this in for now, because even though it's a hack, this will at least mirror the logic in the browser.js code for now.

> We should compute scaleMinFloat and scaleMaxFloat first, and use them here
> instead of the default values.  This will ensure, for example, that
> content="minimum-scale=2.0,maximum-scale=2.0" will also result in an
> initial-scale of 2.0.

This is reasonable. I've moved this code higher up in the execution.

> If the width is not provided, it should instead be calculated based on the
> screen width and the initial-scale (if provided) or height (if provided), or
> set to a default value (Fennec uses 980px).
> 
> If this isn't the appropriate place to do these calculations, maybe we
> should return 0 instead of kViewportMinWidth, to indicate that no width was
> specified.

Ok. I think I have this handled, except that when I have a page that has something like <meta name="viewport" content="width=320">, which should be failing this test, it's actually not getting the width read in correctly.

> Similarly, if the height is not provided, it should be calculated based on
> the width, scale, and screen size parameters.

Agreed. There doesn't seem to be a good default value for the height as there is with the width. I know that the property we discussed on IRC, "browser.viewport.desktopWidth" isn't defined on desktop (ironically), and thus we need to check for this, which is in the current code. In this case, though, it seems that screenWidth/screenHeight should always be defined.

I guess what I'm asking is that I can't seem to set the height in the event that the call to do_GetService("@mozilla.org/gfx/screenmanager;1", &result) fails. As a side note, I'm not sure this can fail anyway, but I am checking the result, as a precaution.

> This should be testing for "no" (lowercase).  I'm not sure whether other
> browsers use a case-sensitive comparison.
> 
> According to
> http://dev.w3.org/csswg/css-device-adapt/#the-user-scalable-property, only
> "yes" or a number outside the range (-1, 1) should result in allowZoom =
> true; other values should result in allowZoom = false.  However, this spec
> is still an Editor's Draft and I don't think it quite matches current
> implementations.  The current Fennec code sets allowZoom to false only for
> "0" or "no" or "false", which I believe was based on WebKit's behavior.
> 

Changed to account for this.

> I would change this to "if (vInf.defaultZoom >= 1.0 || vInf.autoSize)".  

I changed this.

> A
> page with <meta name="viewport" content="width=device-width"> should be
> treated as mobile-optimized, especially since this is defined to be
> equivalent to content="initial-zoom=1".

Yes, actually one of the tests I have checks for this.

> 
> There's also the common case of <meta name="viewport" content="width=320">
> (or similar), where the initial zoom level is not specified but instead will
> depend on the display size.  For displays wider than 320px, the zoom level
> will necessarily be greater than 1.0  We want to disable inflation in this
> case too.

So, this is where I seem to be having some difficulty. I don't seem to be pulling in this string for 'width' when content="width=320". I'm still looking into this. So, once the device width is read in as '320' (in the example), then initial-zoom should be set to MAX(initial-zoom, (screenWidth / width)), where width is the width that's read in from the meta tag. Is this correct?


> I don't understand this test.  Why is inflation disabled for this reference
> file?

See layout/base/tests/test_font_inflation_reftests.html. It's a mochitest that actually takes two different snapshots - one of them is taken with minEmPerLine = 15, and the other is taken with minEmPerLine = 0. It then compares the two to see if the font inflation is having an effect.

Finally, should the default for initial-scale be 1.0? For some reason, I have 0 in as a default, which doesn't seem correct, especially since I do a calculation like;

  PRUint32 convertedWidth = screenWidth / scaleFloat;
  width = NS_MAX(width, convertedWidth);

Thanks for the feedback. I appreciate you letting me know whether I'm on the right track here.

	Scott Johnson (:jwir3) Assignee
	Comment 22 • 13 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #20)
> For a bit more context:  In Fennec, the viewport calculations are split into
> two parts.  That organization dates from Electrolysis: in e10s Fennec, the
> first part of the code retrieves data available only in the content process,
> like the meta tag contents and the doctype.  The second part of the code
> receives that data and combines it with data available only in the parent
> process, like the size of the <browser> element.
> 

Right. This is what I understood to be the case. The requirements of this function are a partial solution to the overall problem - that is, it's simply moving the code that allows access to the viewport metadata into the platform so that we can use it for font-inflation.

Bug 716575 addresses the other aspect of this.

> In mobile/android/chrome/content/browser.js the two main functions involved
> are ViewportHandler.getViewportMetadata and
> Tab.prototype.updateViewportSize.  (See also
> Tab.prototype.updateViewportMetadata, which is where "autoScale" is applied.)
> 
> This patch replicates the logic from the first "part" of the code
> (getViewportMetadata), but does not include the logic from the second "part"
> (updateViewportMetadata) which is needed to fully compute the viewport state.
> 
> For this bug we don't *need* the full calculation, but we should implement
> enough that "minimum-scale=1,maximum-scale=1" will imply "initial-scale=1",
> and that we can distinguish "width=200" from an unspecified width.  (Not
> losing data about unspecified values is also important if this code will
> later be used to implement the full calculation.)

Yes, I agree. I'm trying to implement viewport functionality in the platform in two stages, and the last part about distinguishing 'width=200' from an unspecified width is where I'm having a bit of difficulty right now.

	Matt Brubeck (:mbrubeck)
	Comment 23 • 13 years ago

Comment on attachment 589687 [details] [diff] [review]
b706198 (WIP)

This is looking good.  See comments below; I'm mostly focusing on any differences I see between this patch and the current JavaScript implementation.

>+  if (aDocument->IsXUL()) {
>+    return ret;
>+  }

I think you want to set "ret.autoScale = false" here, to mirror browser.js.

>+  // We retrieve the screen size in the event that we don't have any metadata
>+  // about the width and/or height.
>+  nsresult result;
>+  PRInt32 screenLeft, screenTop, screenWidth, screenHeight;
>+
>+  nsCOMPtr<nsIScreenManager> screenMgr =
>+    do_GetService("@mozilla.org/gfx/screenmanager;1", &result);

Note:  Technically it's the size of the "content area" on the screen that we want here, rather than the screen itself.  (On mobile these are typically close or equal, because apps are nearly full-screen, so mobile developers like myself may be sloppy and say "screen width" when we sometimes mean something else.)

In XUL Fennec, the "content area" size is the size of the <browser> widget.  In native Android Fennec, it's the size of the Gecko LayerView.  I think the native Android widget code actually lies and sets the "screen" size equal to the content area size, so this will actually produce the correct values on Android, but it can be wrong on other platforms.  I don't know the right thing to do here.

>+  if (NS_FAILED(result)) {
>+    // Check for a default screen width preference, or use 0 as a default.
>+    screenWidth = Preferences::GetInt("browser.viewport.desktopWidth", 0);
>+    screenHeight = 0;
>+  } else {
>+    nsCOMPtr<nsIScreen> screen;
>+    screenMgr->GetPrimaryScreen(getter_AddRefs(screen));
>+    screen->GetRect(&screenLeft, &screenTop, &screenWidth, &screenHeight);
>+  }
>+
>+  PRUint32 width = widthStr.ToInteger(&errorCode);
>+  if (errorCode) {
>+    width = screenWidth;
>+  }

This isn't quite right.  The logic should be something like this (in kind of pseudo-C++):

  screen->GetRect(&screenLeft, &screenTop, &screenWidth, &screenHeight);

  width = widthStr.ToInteger(&errorCode)
  if (errorCode) {
    if (autoScale)
      width = screenWidth;
    else
      width = Preferences::GetInt("browser.viewport.desktopWidth", kViewportDefaultScreenWidth);
  }

In English: The "desktopWidth" pref should be used whenever the metadata does not specify a width or scale.  The screen width should be used whenever the metedata sets "width=device-width" or equivalent.

>+  PRUint32 convertedWidth = screenWidth / scaleFloat;
>+  width = NS_MAX(width, convertedWidth);

This adjustment should be done only if "initial-scale" or "maximum-scale" were set explicitly.  To match the code from XUL Fennec, you'd want something like this (except you actually care whether ToFloat succeeded, not just whether the string is empty):

  if (!scaleStr.IsEmpty())
    width = NS_MAX(width, screenWidth / scaleFloat)
  else if (!maxScaleStr.IsEmpty())
    width = NS_MAX(width, screenWidth / scaleMaxFloat)

Also note: XUL Fennec includes this adjustment logic, but native Fennec doesn't.  (It doesn't yet set any zooming constraints based on scale metadata.)  I should file a bug to fix that...

>+  PRUint32 height = heightStr.ToInteger(&errorCode);
>+
>+  if (errorCode) {
>+    height = width * (screenHeight / screenWidth);
>+  }

The Fennec JS code also handles the case where "height" is provided but "width" isn't, and sets width = (height * screenWidth / screenHeight).

>+++ b/layout/base/nsLayoutUtils.cpp
>@@ -4679,16 +4679,28 @@ nsLayoutUtils::InflationMinFontSizeFor(c

>+  // If we are on a mobile-only website, then we want to disable the font
>+  // inflation.
>+  nsPresContext* presContext = aReflowState.frame->PresContext();
>+  NS_ASSERTION(presContext, "PresContext should not be null");
>+
>+  ViewportInfo vInf =
>+    nsContentUtils::GetViewportInfo(presContext->PresShell()->GetDocument());
>+
>+  if (vInf.defaultZoom >= 1.0 || vInf.autoSize) {
>+    return 0;
>+  }

It looks like you updated this to check "vInf.autoSize" (good)...

>@@ -4734,16 +4746,29 @@ nsLayoutUtils::InflationMinFontSizeFor(c

>+  // If we are on a mobile-only website, then we want to disable the font
>+  // inflation.
>+  nsPresContext* presContext = aFrame->PresContext();
>+  NS_ASSERTION(presContext, "PresContext should not be null");
>+
>+  ViewportInfo vInf =
>+    nsContentUtils::GetViewportInfo(presContext->PresShell()->GetDocument());
>+
>+  if (vInf.defaultZoom >= 1.0) {
>+
>+    return 0;
>+  }

>@@ -4758,16 +4783,28 @@ nsLayoutUtils::InflationMinFontSizeFor(c

>+  // If we are on a mobile-only website, then we want to disable the font
>+  // inflation.
>+  nsPresContext* presContext = aFrame->PresContext();
>+  NS_ASSERTION(presContext, "PresContext should not be null");
>+
>+  ViewportInfo vInf =
>+    nsContentUtils::GetViewportInfo(presContext->PresShell()->GetDocument());
>+
>+  if (vInf.defaultZoom >= 1.0) {
>+    return 0;
>+  }

... but not these ones?

	Matt Brubeck (:mbrubeck)
	Comment 24 • 13 years ago

(In reply to Scott Johnson (:jwir3) from comment #21)
> Ok. I think I have this handled, except that when I have a page that has
> something like <meta name="viewport" content="width=320">, which should be
> failing this test, it's actually not getting the width read in correctly.

Hmm, I have no idea why "width=320" would not be read correctly.

> I guess what I'm asking is that I can't seem to set the height in the event
> that the call to do_GetService("@mozilla.org/gfx/screenmanager;1", &result)
> fails. As a side note, I'm not sure this can fail anyway, but I am checking
> the result, as a precaution.

If we can't get the "screen" (content area) width/height, then there are lots of cases where we can't produce the correct values.  I don't think it matters what you return in this case; the important thing is to avoid it.  Maybe you can just choose something arbitrary like height = width, as a last resort.

(Just thinking aloud... perhaps the content area width/height should be passed into this function by the caller?)

> > There's also the common case of <meta name="viewport" content="width=320">
> > (or similar), where the initial zoom level is not specified but instead will
> > depend on the display size.  For displays wider than 320px, the zoom level
> > will necessarily be greater than 1.0  We want to disable inflation in this
> > case too.
> 
> So, this is where I seem to be having some difficulty. I don't seem to be
> pulling in this string for 'width' when content="width=320". I'm still
> looking into this. So, once the device width is read in as '320' (in the
> example), then initial-zoom should be set to MAX(initial-zoom, (screenWidth
> / width)), where width is the width that's read in from the meta tag. Is
> this correct?

Yes, basically, though with some complications (of course)...

In Fennec, in the case where initial-zoom is not specified by the metadata, Fennec uses a default zoom value of screenWidth / viewportWidth (subject to any minimum/maximum zoom constraints):
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser.js#3110

Those minimum/maximum zoom constraints include the minimum-scale/maximum-scale metadata values if present, and the default min/max values otherwise, and finally a hard minimum of the "page zoom" level (screenWidth / document.width).  Note that this hard minimum is based on the width of the document's scrollable area, *not* the width of the viewport.  Fennec lets you zoom out to show the entire width of the page, even if it is wider than the viewport.  I imagine this information won't necessarily be available at the time this function is called, so that last calculation might need to happen elsewhere.

> Finally, should the default for initial-scale be 1.0? For some reason, I
> have 0 in as a default [...]

If a page does not specify an initial scale, Fennec loads it at the smallest scale possible (subject to the constraints mentioned above).  So I think using 0 as the default is correct, as long as you apply the minimum scale constraint before doing any division.

	Scott Johnson (:jwir3) Assignee
	Comment 25 • 13 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #23)
> I think you want to set "ret.autoScale = false" here, to mirror browser.js.

Yes. This has been fixed in the latest version.

> Note:  Technically it's the size of the "content area" on the screen that we
> want here, rather than the screen itself.  (On mobile these are typically
> close or equal, because apps are nearly full-screen, so mobile developers
> like myself may be sloppy and say "screen width" when we sometimes mean
> something else.)
> 
> In XUL Fennec, the "content area" size is the size of the <browser> widget. 
> In native Android Fennec, it's the size of the Gecko LayerView.  I think the
> native Android widget code actually lies and sets the "screen" size equal to
> the content area size, so this will actually produce the correct values on
> Android, but it can be wrong on other platforms.  I don't know the right
> thing to do here.

Well, this will work on native fennec, which is what I'm mostly focused on. As we discussed on IRC. I'm going to leave the more complex calculation of the actual content area size to bug 716575. I did add a substantial comment, though. discussing this issue and referring to both this bug and bug 716575, in the event that someone stumbles across it before it's been fixed.

> This isn't quite right.  The logic should be something like this (in kind of
> pseudo-C++):
> 

I've changed this to code that you laid out in your algorithm. 


> This adjustment should be done only if "initial-scale" or "maximum-scale"
> were set explicitly.  To match the code from XUL Fennec, you'd want
> something like this (except you actually care whether ToFloat succeeded, not
> just whether the string is empty):

Fixed in the revised version.

> The Fennec JS code also handles the case where "height" is provided but
> "width" isn't, and sets width = (height * screenWidth / screenHeight).

I fixed this.

> It looks like you updated this to check "vInf.autoSize" (good)...
> ... but not these ones?

Yep, and I knew I was going to make that mistake when I originally put the same code in multiple locations like that. I've moved this code into FontSizeInflationEnabled(), which is called by each of these functions. That way, if there is something wrong in the future, we only have to fix it in once place, rather than 3.

	Matt Brubeck (:mbrubeck)
	Comment 26 • 13 years ago

Comment on attachment 590522 [details] [diff] [review]
b706198 (v4) - Disable font inflation on sites optimized for mobile.

A content peer should review this too, since my C++ license is a bit expired...

	Mounir Lamouri (:mounir)
	Comment 27 • 13 years ago

Comment on attachment 590522 [details] [diff] [review]
b706198 (v4) - Disable font inflation on sites optimized for mobile.

(In reply to Matt Brubeck (:mbrubeck) from comment #26)
> Comment on attachment 590522 [details] [diff] [review]
> b706198 (v4) - Disable font inflation on sites optimized for mobile.
> 
> A content peer should review this too, since my C++ license is a bit
> expired...

That's a good idea but I'm not a content peer. In addition, I guess dbaron would be more appropraite to review that patch.

	Matt Brubeck (:mbrubeck)
	Comment 29 • 13 years ago

Comment on attachment 590522 [details] [diff] [review]
b706198 (v4) - Disable font inflation on sites optimized for mobile.

>+  if (errorCode) {
>+    height = width * ((float)screenHeight / screenWidth);
>+  }
>+
>+  // If height was provided by the user, but width wasn't, then we should
>+  // calculate the width.
>+  if (widthStr.IsEmpty() && !heightStr.IsEmpty()) {
>+    width = (PRUint32) ((height * screenHeight) / screenWidth);
>+  }

That last line should be 'width = (PRUint32) ((height * screenWidth) / screenHeight);"

>+  // We need to perform a conversion, but only if the initial or maximum
>+  // scale were set explicitly by the user.
>+  if (!scaleStr.IsEmpty() && !scaleErrorCode) {
>+    width = NS_MAX(width, (PRUint32)(screenWidth / scaleFloat));
>+    height = NS_MAX(width, (PRUint32)(screenHeight / scaleFloat));
>+  } else if (!maxScaleStr.IsEmpty() && !scaleMaxErrorCode) {
>+    width = NS_MAX(width, (PRUint32)(screenWidth / scaleMaxFloat));
>+    height = NS_MAX(width, (PRUint32)(screenHeight / scaleMaxFloat));
>+  }

Both of the "height =" lines should have s/width/height/ in the first argument.

>+  if ((userScalable.Find("0") != -1) || (userScalable.Find("no") != -1)
>+    || (userScalable.Find("false") != -1)) {
>+    allowZoom = false;
>+  }

I just realized these should use Equals instead of Find.

>@@ -4828,16 +4833,21 @@ nsLayoutUtils::InflationMinFontSizeFor(c
>   // reflowed, since they will be eventually and then we'll get the
>   // right size.
> #endif
> 
>   if (!FontSizeInflationEnabled(aFrame->PresContext())) {
>     return 0;
>   }
> 
>+  // If we are on a mobile-only website, then we want to disable the font
>+  // inflation.
>+  nsPresContext* presContext = aFrame->PresContext();
>+  NS_ASSERTION(presContext, "PresContext should not be null");
>+
>   for (const nsIFrame *f = aFrame; f; f = f->GetParent()) {
>     if (IsContainerForFontSizeInflation(f)) {
>       if (!ShouldInflateFontsForContainer(f)) {
>         return 0;
>       }
> 
>       return MinimumFontSizeFor(aFrame->PresContext(),
>                                 f->GetContentRect().width);

It looks like presContext is unused here (except for the assertion), and the comment seems out of place.  Is this code still needed?  (The same applies to the other two changes in this file.)

Aside from the above nits, this looks correct as far as I can tell.  r=mbrubeck with these comments addressed.  (Again, I'm just checking the logic... I am not qualified to comment on C++ correctness, style, or performance...)

	Matt Brubeck (:mbrubeck)
	Comment 31 • 13 years ago

Comment on attachment 590913 [details] [diff] [review]
b706198 (v5) - Disable font inflation on sites optimized for mobile. [r=mbrubeck]

Sorry, just noticed a few more minor things:

>+  nsAutoString zeroString, noString, falseString;
>+  nsGkAtoms::zeroDigit->ToString(zeroString);

I don't think zeroDigit is what you want here.  It seems to be the string "zero-digit", an XSLT attribute.

>+  ret.allowZoom = allowZoom;
>+  ret.width = width;
>+  ret.height = height;
>+  ret.defaultZoom = scaleFloat;
>+  ret.minZoom = scaleMinFloat;
>+  ret.maxZoom = scaleMaxFloat;
>+  ret.autoSize = autoSize;
>+  ret.autoScale = true;

The last line should be "ret.autoScale = autoScale;"

	David Baron :dbaron:
	Comment 32 • 13 years ago

Comment on attachment 590913 [details] [diff] [review]
b706198 (v5) - Disable font inflation on sites optimized for mobile. [r=mbrubeck]

>+  nsGkAtoms::handheldFriendly->ToString(handheldFriendlyAtom);
>+
>+  rv = windowUtils->GetDocumentMetadata(handheldFriendlyAtom,
>+                                        handheldFriendly);

If you were going to use windowUtils then this should be either (just
so you learn better string usage):
  nsAutoString handheldFriendly;
  rv = windowUtils->GetDocumentMetadata(
         nsDependentAtomString(nsGkAtoms::handheldFrienly), handheldFriendly);
or:
  nsAutoString handheldFriendly;
  rv = windowUtils->GetDocumentMetadata(
         NS_LITERAL_STRING("HandheldFriendly"), handheldFriendly);
However, given that you're using nsIDocument, it should actually be:
  nsAutoString handheldFriendly;
  doc->GetHeaderData(nsGkAtoms::handheldFrienly, handheldFriendly);

Same for the rest below.

>+  nsAutoString trueAtomString;
>+  nsGkAtoms::_true->ToString(trueAtomString);
>+  if (handheldFriendly.Equals(trueAtomString)) {

This should be:
  if (handheldFriendly.EqualsLiteral("true")) {

Same for the rest of the things like this below.


I'm going to trust that mbrubeck reviewed the logic here.  (He did,
right?)  Perhaps he should also check that it matches the comments
I requested above describing the member variables?

r=dbaron with those things fixed

	David Baron :dbaron:
	Comment 33 • 13 years ago

Er, I managed to cut off the beginning of my review.  Since I already deleted it, I need to rewrite it.

	David Baron :dbaron:
	Comment 34 • 13 years ago

Comment on attachment 590913 [details] [diff] [review]
b706198 (v5) - Disable font inflation on sites optimized for mobile. [r=mbrubeck]

>+/**
>+ * Information retrieved from the <meta name="viewport"> tag. See
>+ * GetViewportInfo for more information on this functionality.
>+ */
>+struct ViewportInfo
>+{
>+    double defaultZoom;
>+    double minZoom;
>+    double maxZoom;
>+    PRUint32 width;
>+    PRUint32 height;
>+    bool autoSize;
>+    bool allowZoom;
>+    bool autoScale;
>+};

This needs comments.  At the very least:
 * what units width/height are in
 * what the booleans mean
 * under what conditions the variables that are only sometimes initialized are initialized.

Maybe a little more.

>+  nsIDOMDocumentType* docType;
>+  nsresult rv = domDoc->GetDoctype(&docType);

This leaks.  Use nsCOMPtr<nsIDOMDocumentType> and getter_AddRefs().

>+      if ((docId.Find("WAP") != -1)
>+           || (docId.Find("Mobile") != -1)
>+           || (docId.Find("WML") != -1))

|| at end of line

>+  nsIDOMWindow* window = aDocument->GetWindow();
>+  nsCOMPtr<nsIDOMWindowUtils> windowUtils(do_GetInterface(window));
>+
>+  if (!windowUtils) {
>+    return ret;
>+  }

Don't do this; all the window utils does is go back to the document.  (see continuation for what to do instead)

>+  nsAutoString handheldFriendly;
>+  nsAutoString handheldFriendlyAtom;

continued above...

(Sorry, less verbose the second time around.)

	Scott Johnson (:jwir3) Assignee
	Comment 35 • 13 years ago

Ok, I've addressed the items in comments 31, 32, and 34, by mbrubeck and dbaron (and dbaron), respectively.

> I'm going to trust that mbrubeck reviewed the logic here.  (He did,
> right?)  Perhaps he should also check that it matches the comments
> I requested above describing the member variables?

Yes, we went through the logic together both on IRC and here, as part of the review comments. He was just concerned that he couldn't go over the C++-syntax like stuff, as it's been a while since he worked in C/C++.

As for the comments requested, I am re-requesting review from mbrubeck so that he can double check the comments I made for the ViewportInfo struct. I'm fairly confident on most of these, as I got them from mbrubeck himself (via IRC talks or review comments), or the Safari website. The only ones I'm not 100% sure about is the width/height units. I believe, from http://www.quirksmode.org/blog/archives/2010/04/a_pixel_is_not.html that these are in CSS pixels, but I'm not 100% sure. The Safari site only calls them 'pixels'.

	Scott Johnson (:jwir3) Assignee
	Comment 36 • 13 years ago

Sorry, accidentally forgot to hg qref one last time before posting the patch.

	Scott Johnson (:jwir3) Assignee
	Comment 37 • 13 years ago

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d180635060d

	Ed Morley [:emorley]
	Comment 38 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/6d180635060d

	Scott Johnson (:jwir3) Assignee
	Comment 39 • 13 years ago

Comment on attachment 592296 [details] [diff] [review]
b706198 (v7) - Disable font inflation on sites optimized for mobile. [r=mbrubeck,dbaron]

[Approval Request Comment]
User impact if declined: font-inflation on mobile websites might cause pages to have readability issues.
Testing completed (on m-c, etc.): testing not yet completed, but in progress.
Risk to taking this patch (and alternatives if risky): fairly low. only affects websites that are optimized for mobile, and whether font-inflation is enabled for those

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 40 • 13 years ago

Comment on attachment 592296 [details] [diff] [review]
b706198 (v7) - Disable font inflation on sites optimized for mobile. [r=mbrubeck,dbaron]

>--- a/content/base/src/nsContentUtils.cpp
>+++ b/content/base/src/nsContentUtils.cpp
>+  scaleMinFloat = NS_MIN(scaleMinFloat, kViewportMaxScale);
>+  scaleMinFloat = NS_MAX(scaleMinFloat, kViewportMinScale);

This is what mozilla::clamped is for, btw.

	Alex Keybl [:akeybl]
	Comment 41 • 13 years ago

Comment on attachment 592296 [details] [diff] [review]
b706198 (v7) - Disable font inflation on sites optimized for mobile. [r=mbrubeck,dbaron]

[Triage Comment]
Mobile only - approved for Beta 11.

	Scott Johnson (:jwir3) Assignee
	Comment 42 • 13 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/160cc61ae20e

	Scott Johnson (:jwir3) Assignee
	Comment 43 • 13 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/6d180635060d

	Camelia Urian
	Comment 44 • 13 years ago

Nightly 13.0a1 (2012-02-07)
Aurora 12.0a2 (2012-02-07)
Beta 11.0 (2012-02-07)(20120207195459 - http://hg.mozilla.org/releases/mozilla-beta/rev/c0feb8e404a0)
Device: Samsung Google Nexus S - Android 2.3.6

Font inflation disabled on mobile sites. Verified on sites from bug description and more.