Last Comment Bug 783565 - meta viewport should override handheldFriendly and doctype in nsContentUtils::GetViewportInfo
: meta viewport should override handheldFriendly and doctype in nsContentUtils:...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: mozilla17
Assigned To: Matt Brubeck (:mbrubeck)
:
Mentors:
Depends on: 744070 762043
Blocks: 716575
  Show dependency treegraph
 
Reported: 2012-08-17 08:14 PDT by Matt Brubeck (:mbrubeck)
Modified: 2012-08-23 03:47 PDT (History)
2 users (show)
mbrubeck: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (2.48 KB, patch)
2012-08-17 08:14 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
patch (6.47 KB, patch)
2012-08-17 08:59 PDT, Matt Brubeck (:mbrubeck)
jaywir3: review+
Details | Diff | Splinter Review

Description Matt Brubeck (:mbrubeck) 2012-08-17 08:14:04 PDT
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.
Comment 1 Matt Brubeck (:mbrubeck) 2012-08-17 08:59:30 PDT
Created attachment 652787 [details] [diff] [review]
patch

Now includes a test.
Comment 2 Matt Brubeck (:mbrubeck) 2012-08-22 09:54:21 PDT
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) 2012-08-22 12:53:41 PDT
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) 2012-08-22 13:11:46 PDT
(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) 2012-08-22 13:22:08 PDT
(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 7 Ed Morley [:emorley] 2012-08-23 03:47:46 PDT
https://hg.mozilla.org/mozilla-central/rev/2b5032429f80

Note You need to log in before you can comment on or make changes to this bug.