47607 2010-10-13 11:35:18 -0700 meta tag parser needs to support ; as separator due to Android having made that popular 2010-10-19 14:10:03 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC All RESOLVED INVALID https://android.git.kernel.org/?p=platform/external/webkit.git;a=blob;f=WebCore/dom/Document.cpp;h=1f8edeb3505ce7b2e49ff2b88f2ef92c62d9a68d;hb=HEAD Qt P3 Normal --- 1 kenneth webkit-unassigned ap christian.webkit dbates ddkilzer gyuyoung.kim joepeck joone kling klobag tonikitoo oldest_to_newest 293560 0 kenneth 2010-10-13 11:35:18 -0700 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 293577 1 70644 kenneth 2010-10-13 11:57:00 -0700 Created attachment 70644 Patch 294178 2 70644 ap 2010-10-14 10:25:02 -0700 Comment on attachment 70644 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. 294186 3 kenneth 2010-10-14 10:38:02 -0700 (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. 294194 4 tonikitoo 2010-10-14 10:50:54 -0700 Grace, could help cc'ing the proper Android guys, herE? 294199 5 kenneth 2010-10-14 10:57:02 -0700 Facebook, at least with Android UA, is using ; as separator. 295961 6 klobag 2010-10-18 21:06:04 -0700 (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? 296183 7 kenneth 2010-10-19 07:34:50 -0700 (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 296392 8 klobag 2010-10-19 14:04:47 -0700 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. 296396 9 kenneth 2010-10-19 14:10:03 -0700 Closing as invalid then! 70644 2010-10-13 11:57:00 -0700 2010-10-14 10:25:02 -0700 Patch 0001-Meta-tag-parser-needs-to-support-as-separator-due-to.patch text/plain 3809 kenneth RnJvbSAwZjQ4NjdiYjlhMDcxMDgyMjdiMTUyYWQ2ZmU4NjQ3MTQ0MWM3ODZiIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBLZW5uZXRoIFJvaGRlIENocmlzdGlhbnNlbiA8a2VubmV0aC5j aHJpc3RpYW5zZW5Ab3BlbmJvc3NhLm9yZz4KRGF0ZTogV2VkLCAxMyBPY3QgMjAxMCAxNTo1NToz OSAtMDMwMApTdWJqZWN0OiBbUEFUQ0hdIE1ldGEgdGFnIHBhcnNlciBuZWVkcyB0byBzdXBwb3J0 IDsgYXMgc2VwYXJhdG9yIGR1ZSB0byBBbmRyb2lkIGhhdmluZwogbWFkZSB0aGF0IHBvcHVsYXIK IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00NzYwNwoKUGF0Y2ggYnkg S2VubmV0aCBSb2hkZSBDaHJpc3RpYW5zZW4gPGtlbm5ldGhAd2Via2l0Lm9yZz4gb24gMjAxMC0x MC0xMwpSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KClRoZSBtZXRhIHRhZyBwYXJzZXIgaW4g QW5kcm9pZCBzdXBwb3J0cyA7IGluIGFkZGl0aW9uIHRvICwgYXMKc2VwYXJhdG9yIChzZWUgRG9j dW1lbnQ6OnByb2Nlc3NNZXRhZGF0YVNldHRpbmdzKGNvbnN0IFN0cmluZyYKY29udGVudCkgaW4g QW5kcm9pZCBzb3VyY2UpIHdoaWNoIG1lYW5zIHRoYXQgZHVlIHRvIHRoZSBwb3B1bGFyaXR5IG9m CkFuZHJvaWQsIHBhZ2VzIHN1Y2ggYXMgRmFjZWJvb2sgbm93IHVzZXMgJzsnIHRvIHNlcGFyYXRl IHRoZSB2aWV3cG9ydAptZXRhIGFyZ3VtZW50cyBpbnN0ZWFkIG9mICcsJywgd2hpY2ggcmVzdWx0 cyBpbiB0aGUgdmlld3BvcnQgYWxnb3JpdGhtCmNvbXB1dGluZyBhIHdyb25nIHZpZXdwb3J0IChs aWtlIGZvciBpbnN0YW5jZSBhIHdpZHRoIG9mIDE4MDAgcGl4ZWxzCmluc3RlYWQgb2YgMzIwKS4K CiogZG9tL0RvY3VtZW50LmNwcDoKKFdlYkNvcmU6OmlzU2VwYXJhdG9yKToKKFdlYkNvcmU6OkRv Y3VtZW50Ojpwcm9jZXNzQXJndW1lbnRzKToKLS0tCiBXZWJDb3JlL0NoYW5nZUxvZyAgICAgICAg fCAgIDIwICsrKysrKysrKysrKysrKysrKysrCiBXZWJDb3JlL2RvbS9Eb2N1bWVudC5jcHAgfCAg IDEwICsrKysrLS0tLS0KIDIgZmlsZXMgY2hhbmdlZCwgMjUgaW5zZXJ0aW9ucygrKSwgNSBkZWxl dGlvbnMoLSkKCmRpZmYgLS1naXQgYS9XZWJDb3JlL0NoYW5nZUxvZyBiL1dlYkNvcmUvQ2hhbmdl TG9nCmluZGV4IDAyZTc1OTUuLmE1ZDQ1MjYgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvQ2hhbmdlTG9n CisrKyBiL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjMgQEAKKzIwMTAtMTAtMTMgIEtl bm5ldGggUm9oZGUgQ2hyaXN0aWFuc2VuICA8a2VubmV0aEB3ZWJraXQub3JnPgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE1ldGEgdGFnIHBhcnNlciBu ZWVkcyB0byBzdXBwb3J0IDsgYXMgc2VwYXJhdG9yIGR1ZSB0byBBbmRyb2lkIGhhdmluZworICAg ICAgICBtYWRlIHRoYXQgcG9wdWxhcgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9NDc2MDcKKworICAgICAgICBUaGUgbWV0YSB0YWcgcGFyc2VyIGluIEFu ZHJvaWQgc3VwcG9ydHMgOyBpbiBhZGRpdGlvbiB0byAsIGFzCisgICAgICAgIHNlcGFyYXRvciAo c2VlIERvY3VtZW50Ojpwcm9jZXNzTWV0YWRhdGFTZXR0aW5ncyhjb25zdCBTdHJpbmcmCisgICAg ICAgIGNvbnRlbnQpIGluIEFuZHJvaWQgc291cmNlKSB3aGljaCBtZWFucyB0aGF0IGR1ZSB0byB0 aGUgcG9wdWxhcml0eSBvZgorICAgICAgICBBbmRyb2lkLCBwYWdlcyBzdWNoIGFzIEZhY2Vib29r IG5vdyB1c2VzICc7JyB0byBzZXBhcmF0ZSB0aGUgdmlld3BvcnQKKyAgICAgICAgbWV0YSBhcmd1 bWVudHMgaW5zdGVhZCBvZiAnLCcsIHdoaWNoIHJlc3VsdHMgaW4gdGhlIHZpZXdwb3J0IGFsZ29y aXRobQorICAgICAgICBjb21wdXRpbmcgYSB3cm9uZyB2aWV3cG9ydCAobGlrZSBmb3IgaW5zdGFu Y2UgYSB3aWR0aCBvZiAxODAwIHBpeGVscworICAgICAgICBpbnN0ZWFkIG9mIDMyMCkuCisKKyAg ICAgICAgKiBkb20vRG9jdW1lbnQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6aXNTZXBhcmF0b3Ip OgorICAgICAgICAoV2ViQ29yZTo6RG9jdW1lbnQ6OnByb2Nlc3NBcmd1bWVudHMpOgorCiAyMDEw LTEwLTExICBNYXJ0aW4gUm9iaW5zb24gIDxtcm9iaW5zb25AaWdhbGlhLmNvbT4KIAogICAgICAg ICBSZXZpZXdlZCBieSBYYW4gTG9wZXouCmRpZmYgLS1naXQgYS9XZWJDb3JlL2RvbS9Eb2N1bWVu dC5jcHAgYi9XZWJDb3JlL2RvbS9Eb2N1bWVudC5jcHAKaW5kZXggZjIzZWE3Yi4uYzM3NWJmMiAx MDA2NDQKLS0tIGEvV2ViQ29yZS9kb20vRG9jdW1lbnQuY3BwCisrKyBiL1dlYkNvcmUvZG9tL0Rv Y3VtZW50LmNwcApAQCAtMjUzNyw3ICsyNTM3LDcgQEAgdm9pZCBEb2N1bWVudDo6cHJvY2Vzc0h0 dHBFcXVpdihjb25zdCBTdHJpbmcmIGVxdWl2LCBjb25zdCBTdHJpbmcmIGNvbnRlbnQpCiAvLyBU aG91Z2ggaXNzcGFjZSgpIGNvbnNpZGVycyBcdCBhbmQgXHYgdG8gYmUgd2hpdGVzcGFjZSwgV2lu IElFIGRvZXNuJ3QuCiBzdGF0aWMgYm9vbCBpc1NlcGFyYXRvcihVQ2hhciBjKQogewotICAgIHJl dHVybiBjID09ICcgJyB8fCBjID09ICdcdCcgfHwgYyA9PSAnXG4nIHx8IGMgPT0gJ1xyJyB8fCBj ID09ICc9JyB8fCBjID09ICcsJyB8fCBjID09ICdcMCc7CisgICAgcmV0dXJuIGMgPT0gJyAnIHx8 IGMgPT0gJ1x0JyB8fCBjID09ICdcbicgfHwgYyA9PSAnXHInIHx8IGMgPT0gJz0nIHx8IGMgPT0g JywnIHx8IGMgPT0gJzsnIHx8IGMgPT0gJ1wwJzsKIH0KIAogdm9pZCBEb2N1bWVudDo6cHJvY2Vz c0FyZ3VtZW50cyhjb25zdCBTdHJpbmcmIGZlYXR1cmVzLCB2b2lkKiBkYXRhLCBBcmd1bWVudHND YWxsYmFjayBjYWxsYmFjaykKQEAgLTI1NjMsMTYgKzI1NjMsMTYgQEAgdm9pZCBEb2N1bWVudDo6 cHJvY2Vzc0FyZ3VtZW50cyhjb25zdCBTdHJpbmcmIGZlYXR1cmVzLCB2b2lkKiBkYXRhLCBBcmd1 bWVudHNDYWwKICAgICAgICAgICAgIGkrKzsKICAgICAgICAga2V5RW5kID0gaTsKIAotICAgICAg ICAvLyBza2lwIHRvIGZpcnN0ICc9JywgYnV0IGRvbid0IHNraXAgcGFzdCBhICcsJyBvciB0aGUg ZW5kIG9mIHRoZSBzdHJpbmcKKyAgICAgICAgLy8gc2tpcCB0byBmaXJzdCAnPScsIGJ1dCBkb24n dCBza2lwIHBhc3QgYSAnLCcsICc7JyBvciB0aGUgZW5kIG9mIHRoZSBzdHJpbmcKICAgICAgICAg d2hpbGUgKGJ1ZmZlcltpXSAhPSAnPScpIHsKLSAgICAgICAgICAgIGlmIChidWZmZXJbaV0gPT0g JywnIHx8IGkgPj0gbGVuZ3RoKQorICAgICAgICAgICAgaWYgKGJ1ZmZlcltpXSA9PSAnLCcgfHwg YnVmZmVyW2ldID09ICc7JyB8fCBpID49IGxlbmd0aCkKICAgICAgICAgICAgICAgICBicmVhazsK ICAgICAgICAgICAgIGkrKzsKICAgICAgICAgfQogCi0gICAgICAgIC8vIHNraXAgdG8gZmlyc3Qg bm9uLXNlcGFyYXRvciwgYnV0IGRvbid0IHNraXAgcGFzdCBhICcsJyBvciB0aGUgZW5kIG9mIHRo ZSBzdHJpbmcKKyAgICAgICAgLy8gc2tpcCB0byBmaXJzdCBub24tc2VwYXJhdG9yLCBidXQgZG9u J3Qgc2tpcCBwYXN0IGEgJywnLCAnOycgb3IgdGhlIGVuZCBvZiB0aGUgc3RyaW5nCiAgICAgICAg IHdoaWxlIChpc1NlcGFyYXRvcihidWZmZXJbaV0pKSB7Ci0gICAgICAgICAgICBpZiAoYnVmZmVy W2ldID09ICcsJyB8fCBpID49IGxlbmd0aCkKKyAgICAgICAgICAgIGlmIChidWZmZXJbaV0gPT0g JywnIHx8IGJ1ZmZlcltpXSA9PSAnOycgfHwgaSA+PSBsZW5ndGgpCiAgICAgICAgICAgICAgICAg YnJlYWs7CiAgICAgICAgICAgICBpKys7CiAgICAgICAgIH0KLS0gCjEuNy4xCgo=