Bug 47607 - meta tag parser needs to support ; as separator due to Android having made that popular
Summary: meta tag parser needs to support ; as separator due to Android having made th...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P3 Normal
Assignee: Nobody
URL: https://android.git.kernel.org/?p=pla...
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-10-13 11:35 PDT by Kenneth Rohde Christiansen
Modified: 2010-10-19 14:10 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.72 KB, patch)
2010-10-13 11:57 PDT, Kenneth Rohde Christiansen
ap: review-
ap: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2010-10-13 11:35:18 PDT
The meta tag parser in Android supports ; in addition to , as separator [1] (see Document::processMetadataSettings(const String& content)) which means that due to the popularity of Android, pages such as Facebook now uses ';' to separate the viewport meta arguments instead of ',', which results in the viewport algorithm computing a wrong viewport (like for instance a width of 1800 pixels instead of 320).

[1] https://android.git.kernel.org/?p=platform/external/webkit.git;a=blob;f=WebCore/dom/Document.cpp;h=1f8edeb3505ce7b2e49ff2b88f2ef92c62d9a68d;hb=HEAD
Comment 1 Kenneth Rohde Christiansen 2010-10-13 11:57:00 PDT
Created attachment 70644 [details]
Patch
Comment 2 Alexey Proskuryakov 2010-10-14 10:25:02 PDT
Comment on attachment 70644 [details]
Patch

I'm not saying that I'm necessarily against this change, but it needs a better explanation. Does this work in IE? In Firefox? Opera Mobile? Are these pages (which ones?) broken on iPhone? What do relevant specs say?

What does Android version history say about reasons for such deviation from webkit.org?

I'm also unsure about what the scope of this change is. ChangeLog says "meta tag parser", but it seems that code changes only affect meta viewport. In the latter is the case, perhaps the question about IE and Firefox isn't valid, but other ones still stand.

R- because of all the questions, and the lack of test case or an explanation why one isn't possible.
Comment 3 Kenneth Rohde Christiansen 2010-10-14 10:38:02 PDT
(In reply to comment #2)
> (From update of attachment 70644 [details])
> I'm not saying that I'm necessarily against this change, but it needs a better explanation. Does this work in IE? In Firefox? Opera Mobile? Are these pages (which ones?) broken on iPhone? What do relevant specs say?

Facebook recently changed and I do not have an iPhone with me right now to test, but the site might be doing UA sniffing. On the other hand, I have ran into this problem at least twice before.

> What does Android version history say about reasons for such deviation from webkit.org?

The webkit.org code is upstreamed by RIM, presumable from the iPhone code dump, after Android started shipping this code. I do not know the original reason for this change.

> I'm also unsure about what the scope of this change is. ChangeLog says "meta tag parser", but it seems that code changes only affect meta viewport. In the latter is the case, perhaps the question about IE and Firefox isn't valid, but other ones still stand.

I'm not sure whether any shipped Opera mobile supports viewport meta tag or not, but at least Firefox support is pretty preliminary and IE does not support it.

> R- because of all the questions, and the lack of test case or an explanation why one isn't possible.

I just pushed the patch to get feedback. I believe that some of the viewport meta tests that we are not passing might be due to this issue. These tests come from Opera.
Comment 4 Antonio Gomes 2010-10-14 10:50:54 PDT
Grace, could help cc'ing the proper Android guys, herE?
Comment 5 Kenneth Rohde Christiansen 2010-10-14 10:57:02 PDT
Facebook, at least with Android UA, is using ; as separator.
Comment 6 Grace Kloba 2010-10-18 21:06:04 PDT
(In reply to comment #4)
> Grace, could help cc'ing the proper Android guys, herE?

Here is some history. In Android's original implementation, we didn't support semi-colon as separator. But we got bugs complaining some sites working on iPhone, but doesn't work on Android. So we added semi-colon support in Cupcake.

But I believe we have removed this as the version in WebKit doesn't support it. Can you double check with Froyo port?
Comment 7 Kenneth Rohde Christiansen 2010-10-19 07:34:50 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > Grace, could help cc'ing the proper Android guys, herE?
> 
> Here is some history. In Android's original implementation, we didn't support semi-colon as separator. But we got bugs complaining some sites working on iPhone, but doesn't work on Android. So we added semi-colon support in Cupcake.
> 
> But I believe we have removed this as the version in WebKit doesn't support it. Can you double check with Froyo port?

Following the links for Froyo I end up with the below link, still containing the ";" support.

http://android.git.kernel.org/?p=platform/external/webkit.git;a=blob_plain;f=WebCore/dom/Document.cpp;hb=HEAD

or 

http://android.git.kernel.org/?p=platform/external/webkit.git;a=blob;f=WebCore/dom/Document.cpp;h=1f8edeb3505ce7b2e49ff2b88f2ef92c62d9a68d;hb=0433734318551aac3b7b82b9d8ab5d485d7d0439
Comment 8 Grace Kloba 2010-10-19 14:04:47 PDT
Hmm, the internal change I referred to is made in May. Maybe it is after we cut the Froyo tree. So it should show up in the next public drop.
Comment 9 Kenneth Rohde Christiansen 2010-10-19 14:10:03 PDT
Closing as invalid then!