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

RESOLVED FIXED in mozilla17

Status

()

Core
Layout: Text
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Tracking

Trunk
mozilla17
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 652773 [details] [diff] [review]
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.
(Assignee)

Comment 1

5 years ago
Created attachment 652787 [details] [diff] [review]
patch

Now includes a test.
Attachment #652773 - Attachment is obsolete: true
Attachment #652787 - Flags: review?(dbaron)
(Assignee)

Comment 2

5 years ago
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.
Attachment #652787 - Flags: review?(dbaron) → review?(sjohnson)
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?
(Assignee)

Comment 4

5 years ago
(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?
(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.

Updated

5 years ago
Attachment #652787 - Flags: review?(sjohnson) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5032429f80
Flags: in-testsuite+
Version: 17 Branch → Trunk

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/2b5032429f80
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.