783565 - meta viewport should override handheldFriendly and doctype in nsContentUtils::GetViewportInfo

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

In bug 762043 and bug 744070 we changed the viewport logic in the Fennec front-end code.  The code now checks for the handheldFriendly tag and mobile doctypes only if <meta name="viewport"> is missing or empty.  This patch makes the same change to the Gecko version of this code, to keep it in sync with Fennec so it can eventually replace the Fennec front-end code (bug 716575).

This patch is basically complete but needs some tests.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Now includes a test.

Comment 2

[Matt Brubeck (:mbrubeck)]

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

Switching review to Scott since he originally wrote this code.  Scott, feel free to switch it back or request additional reviews if you think someone else should review this.

Comment 3

[Scott Johnson (:jwir3)]

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

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

::: content/base/src/nsContentUtils.cpp
@@ +5191,5 @@
>    /* We never fail. */
>    nsresult rv = NS_OK;
>  
> +  aDocument->SetHeaderData(nsGkAtoms::viewport, viewportInfo);
> +

Why do you want this here? It seems like ProcessViewportInfo() is actually doing the parsing of some string of HTML, whereas GetViewportInfo() is actually returning data associated with the viewport as data. It is conceivable that someone could call ProcessViewportInfo() with a string that isn't actually a <META name="viewport"> string, which would result in us setting bogus data into the header data string.

I could be wrong, though... just looking for a reason why you thought this was necessary here. Was it failing some test?

Comment 4

[Matt Brubeck (:mbrubeck)]

(In reply to Scott Johnson (:jwir3) from comment #3)
> > +  aDocument->SetHeaderData(nsGkAtoms::viewport, viewportInfo);
> 
> Why do you want this here? It seems like ProcessViewportInfo() is actually
> doing the parsing of some string of HTML, whereas GetViewportInfo() is
> actually returning data associated with the viewport as data.

In the other part of this patch, I call "GetHeaderData(nsGKAtoms::viewport,...)" to ask the question "Does this page have a <meta name=viewport>?":
https://bugzilla.mozilla.org/attachment.cgi?id=652787&action=diff#a/content/base/src/nsContentUtils.cpp_sec2

This replaces some JavaScript code, which asked the same question in a more complicated way, by checking the existence of each individual viewport token:
http://hg.mozilla.org/mozilla-central/file/88e47f6905e9/mobile/android/chrome/content/browser.js#l4551

I could do the same in GetViewportInfo, but I found this alternate way both simpler and less fragile (if the set of allowed tokens changes in the future).  Or perhaps there's another way to ask the question that does not involve saving this string via SetHeaderData?  Would it be better to just store a boolean value?

Comment 5

[Scott Johnson (:jwir3)]

(In reply to Matt Brubeck (:mbrubeck) from comment #4)
> (In reply to Scott Johnson (:jwir3) from comment #3)
> > > +  aDocument->SetHeaderData(nsGkAtoms::viewport, viewportInfo);
> > 
> > Why do you want this here? It seems like ProcessViewportInfo() is actually
> > doing the parsing of some string of HTML, whereas GetViewportInfo() is
> > actually returning data associated with the viewport as data.
> 
> In the other part of this patch, I call
> "GetHeaderData(nsGKAtoms::viewport,...)" to ask the question "Does this page
> have a <meta name=viewport>?":
> https://bugzilla.mozilla.org/attachment.cgi?id=652787&action=diff#a/content/
> base/src/nsContentUtils.cpp_sec2
> 
> This replaces some JavaScript code, which asked the same question in a more
> complicated way, by checking the existence of each individual viewport token:
> http://hg.mozilla.org/mozilla-central/file/88e47f6905e9/mobile/android/
> chrome/content/browser.js#l4551
> 
> I could do the same in GetViewportInfo, but I found this alternate way both
> simpler and less fragile (if the set of allowed tokens changes in the
> future).  Or perhaps there's another way to ask the question that does not
> involve saving this string via SetHeaderData?  Would it be better to just
> store a boolean value?

You know, I just realized this was only called from nsHTMLMetaElement::BindToTree, so I think the approach you're taking is fine. Just didn't see exactly what was going on right away. I think it makes sense now.

Comment 6

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5032429f80

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b5032429f80