Bug 170548

Summary: Tweak window.open features argument tokenizer to match HTML standard and Edge
Product: WebKit Reporter: Simon Pieters (:zcorpan) <simon>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, aestes, ap, buildbot, cdumez, commit-queue, darin, dbates, esprehn+autocc, ggaren, kangil.han, rniwa, sam, simon.fraser, youennf
Priority: P2    
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 171313    
Bug Blocks:    
Attachments:
Description Flags
WIP Patch
none
WIP Patch
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews117 for mac-elcapitan
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
WIP Patch
none
Patch
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Patch none

Description Simon Pieters (:zcorpan) 2017-04-06 08:06:51 PDT
See the following spec changes and tests in web-platform-tests:
https://github.com/whatwg/html/pull/2476
https://github.com/w3c/csswg-drafts/pull/1138
https://github.com/w3c/web-platform-tests/pull/5306
https://github.com/w3c/web-platform-tests/pull/5390

The PR for whatwg/html has a commit that effectively shows what the diff is between WebKit and Edge (ignoring Edge's behavior for U+0000):
https://github.com/whatwg/html/pull/2476/commits/e74dc9ad3872a89cc8f2fa62e160c076741170e6

The test that fails because of not tokenizing like Edge is
https://github.com/w3c/web-platform-tests/pull/5306/files#diff-f218f27c4d582eebff6fcf48d4f4e219R65

Apart from implementing that diff, WebKit also needs to add U+000C as a "feature separator" character.
https://github.com/w3c/web-platform-tests/pull/5306/files#diff-f218f27c4d582eebff6fcf48d4f4e219R24
Comment 1 Chris Dumez 2017-04-25 21:20:04 PDT
Created attachment 308209 [details]
WIP Patch
Comment 2 Chris Dumez 2017-04-25 21:45:19 PDT
I am still trying to figure out from https://html.spec.whatwg.org/#concept-window-open-features-tokenize why '\u000Cnoopener' would turn on no-opener feature given that \u000 is not a feature separator.
Comment 3 Chris Dumez 2017-04-25 21:50:32 PDT
(In reply to Chris Dumez from comment #2)
> I am still trying to figure out from
> https://html.spec.whatwg.org/#concept-window-open-features-tokenize why
> '\u000Cnoopener' would turn on no-opener feature given that \u000 is not a
> feature separator.

Ah, it is \u000C, never mind. We are indeed missing this separator.
Comment 4 Chris Dumez 2017-04-25 21:51:12 PDT
Created attachment 308211 [details]
WIP Patch
Comment 5 Build Bot 2017-04-25 22:31:04 PDT
Comment on attachment 308211 [details]
WIP Patch

Attachment 308211 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3607528

New failing tests:
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-001.html
Comment 6 Build Bot 2017-04-25 22:31:05 PDT
Created attachment 308216 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 7 Build Bot 2017-04-25 23:02:44 PDT
Comment on attachment 308211 [details]
WIP Patch

Attachment 308211 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3607680

New failing tests:
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-001.html
Comment 8 Build Bot 2017-04-25 23:02:45 PDT
Created attachment 308220 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-04-25 23:32:32 PDT
Comment on attachment 308211 [details]
WIP Patch

Attachment 308211 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3607725

New failing tests:
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-001.html
Comment 10 Build Bot 2017-04-25 23:32:33 PDT
Created attachment 308222 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 11 Build Bot 2017-04-26 00:01:53 PDT
Comment on attachment 308211 [details]
WIP Patch

Attachment 308211 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3607718

New failing tests:
fast/viewport/viewport-122.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-001.html
fast/viewport/viewport-121.html
Comment 12 Build Bot 2017-04-26 00:01:54 PDT
Created attachment 308225 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 13 Build Bot 2017-04-26 01:14:29 PDT
Comment on attachment 308211 [details]
WIP Patch

Attachment 308211 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3608140

New failing tests:
fast/viewport/viewport-122.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-001.html
fast/viewport/viewport-121.html
Comment 14 Build Bot 2017-04-26 01:14:31 PDT
Created attachment 308233 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 15 Chris Dumez 2017-04-26 12:33:12 PDT
Created attachment 308274 [details]
WIP Patch
Comment 16 Chris Dumez 2017-04-26 13:49:44 PDT
Created attachment 308281 [details]
Patch
Comment 17 Build Bot 2017-04-26 15:25:55 PDT
Comment on attachment 308281 [details]
Patch

Attachment 308281 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3612288

New failing tests:
fast/viewport/viewport-117.html
fast/viewport/viewport-126.html
fast/viewport/viewport-74.html
fast/viewport/viewport-113.html
fast/viewport/viewport-87.html
fast/viewport/viewport-111.html
fast/viewport/viewport-108.html
fast/viewport/viewport-121.html
fast/viewport/viewport-116.html
fast/viewport/viewport-114.html
fast/viewport/viewport-115.html
fast/viewport/viewport-110.html
fast/viewport/viewport-109.html
fast/viewport/viewport-75.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-noopener.html
fast/viewport/viewport-70.html
fast/viewport/viewport-122.html
fast/viewport/viewport-73.html
Comment 18 Build Bot 2017-04-26 15:25:57 PDT
Created attachment 308292 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 19 Chris Dumez 2017-04-26 15:32:07 PDT
Created attachment 308293 [details]
Patch
Comment 20 Geoffrey Garen 2017-04-26 15:38:11 PDT
Comment on attachment 308293 [details]
Patch

r=me
Comment 21 Chris Dumez 2017-04-26 16:39:32 PDT
(In reply to Build Bot from comment #17)
> Comment on attachment 308281 [details]
> Patch
> 
> Attachment 308281 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: http://webkit-queues.webkit.org/results/3612288
> 
> New failing tests:
> fast/viewport/viewport-117.html
> fast/viewport/viewport-126.html
> fast/viewport/viewport-74.html
> fast/viewport/viewport-113.html
> fast/viewport/viewport-87.html
> fast/viewport/viewport-111.html
> fast/viewport/viewport-108.html
> fast/viewport/viewport-121.html
> fast/viewport/viewport-116.html
> fast/viewport/viewport-114.html
> fast/viewport/viewport-115.html
> fast/viewport/viewport-110.html
> fast/viewport/viewport-109.html
> fast/viewport/viewport-75.html
> imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-
> creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-
> noopener.html
> fast/viewport/viewport-70.html
> fast/viewport/viewport-122.html
> fast/viewport/viewport-73.html

I currently have no idea about those fast/viewport/ on iOS yet. Those tests are not using window.open() at all so they look unrelated to my change.

The diffs look like:
--- /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/layout-test-results/retries/fast/viewport/viewport-70-expected.txt
+++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/layout-test-results/retries/fast/viewport/viewport-70-actual.txt
@@ -1,3 +1,3 @@
 CONSOLE MESSAGE: line 3: Viewport argument key "x" not recognized and ignored.
-ALERT: viewport size 100x110 scale 3.2 with limits [3.2, 5] and userScalable true
+ALERT: viewport size 320x352 scale 1 with limits [1, 5] and userScalable true
Comment 22 Chris Dumez 2017-04-26 16:49:33 PDT
(In reply to Chris Dumez from comment #21)
> (In reply to Build Bot from comment #17)
> > Comment on attachment 308281 [details]
> > Patch
> > 
> > Attachment 308281 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> > Output: http://webkit-queues.webkit.org/results/3612288
> > 
> > New failing tests:
> > fast/viewport/viewport-117.html
> > fast/viewport/viewport-126.html
> > fast/viewport/viewport-74.html
> > fast/viewport/viewport-113.html
> > fast/viewport/viewport-87.html
> > fast/viewport/viewport-111.html
> > fast/viewport/viewport-108.html
> > fast/viewport/viewport-121.html
> > fast/viewport/viewport-116.html
> > fast/viewport/viewport-114.html
> > fast/viewport/viewport-115.html
> > fast/viewport/viewport-110.html
> > fast/viewport/viewport-109.html
> > fast/viewport/viewport-75.html
> > imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-
> > creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-
> > noopener.html
> > fast/viewport/viewport-70.html
> > fast/viewport/viewport-122.html
> > fast/viewport/viewport-73.html
> 
> I currently have no idea about those fast/viewport/ on iOS yet. Those tests
> are not using window.open() at all so they look unrelated to my change.
> 
> The diffs look like:
> ---
> /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/layout-test-
> results/retries/fast/viewport/viewport-70-expected.txt
> +++
> /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/layout-test-
> results/retries/fast/viewport/viewport-70-actual.txt
> @@ -1,3 +1,3 @@
>  CONSOLE MESSAGE: line 3: Viewport argument key "x" not recognized and
> ignored.
> -ALERT: viewport size 100x110 scale 3.2 with limits [3.2, 5] and
> userScalable true
> +ALERT: viewport size 320x352 scale 1 with limits [1, 5] and userScalable
> true

Ok, so this is legit. It looks like the processFeaturesString() which I aligned with the HTML spec is also used to parse viewport meta tag values such as:
<meta name="viewport" content="width=100 x initial-scale=1">

I am not sure yet if we want different parsing behavior for viewport and window features.
Comment 23 Chris Dumez 2017-04-26 17:01:41 PDT
(In reply to Chris Dumez from comment #22)
> (In reply to Chris Dumez from comment #21)
> > (In reply to Build Bot from comment #17)
> > > Comment on attachment 308281 [details]
> > > Patch
> > > 
> > > Attachment 308281 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> > > Output: http://webkit-queues.webkit.org/results/3612288
> > > 
> > > New failing tests:
> > > fast/viewport/viewport-117.html
> > > fast/viewport/viewport-126.html
> > > fast/viewport/viewport-74.html
> > > fast/viewport/viewport-113.html
> > > fast/viewport/viewport-87.html
> > > fast/viewport/viewport-111.html
> > > fast/viewport/viewport-108.html
> > > fast/viewport/viewport-121.html
> > > fast/viewport/viewport-116.html
> > > fast/viewport/viewport-114.html
> > > fast/viewport/viewport-115.html
> > > fast/viewport/viewport-110.html
> > > fast/viewport/viewport-109.html
> > > fast/viewport/viewport-75.html
> > > imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-
> > > creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-
> > > noopener.html
> > > fast/viewport/viewport-70.html
> > > fast/viewport/viewport-122.html
> > > fast/viewport/viewport-73.html
> > 
> > I currently have no idea about those fast/viewport/ on iOS yet. Those tests
> > are not using window.open() at all so they look unrelated to my change.
> > 
> > The diffs look like:
> > ---
> > /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/layout-test-
> > results/retries/fast/viewport/viewport-70-expected.txt
> > +++
> > /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/layout-test-
> > results/retries/fast/viewport/viewport-70-actual.txt
> > @@ -1,3 +1,3 @@
> >  CONSOLE MESSAGE: line 3: Viewport argument key "x" not recognized and
> > ignored.
> > -ALERT: viewport size 100x110 scale 3.2 with limits [3.2, 5] and
> > userScalable true
> > +ALERT: viewport size 320x352 scale 1 with limits [1, 5] and userScalable
> > true
> 
> Ok, so this is legit. It looks like the processFeaturesString() which I
> aligned with the HTML spec is also used to parse viewport meta tag values
> such as:
> <meta name="viewport" content="width=100 x initial-scale=1">
> 
> I am not sure yet if we want different parsing behavior for viewport and
> window features.

The spec for meta viewport seems to be at https://drafts.csswg.org/css-device-adapt/#parsing-algorithm. It seems to have its own algorithm so I may need to use different algorithms for parsing meta viewport and window features.
Comment 24 Build Bot 2017-04-26 17:06:12 PDT
Comment on attachment 308293 [details]
Patch

Attachment 308293 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3614308

New failing tests:
fast/viewport/viewport-117.html
fast/viewport/viewport-126.html
fast/viewport/viewport-74.html
fast/viewport/viewport-113.html
fast/viewport/viewport-87.html
fast/viewport/viewport-111.html
fast/viewport/viewport-108.html
fast/viewport/viewport-121.html
fast/viewport/viewport-116.html
fast/viewport/viewport-114.html
fast/viewport/viewport-115.html
fast/viewport/viewport-110.html
fast/viewport/viewport-109.html
fast/viewport/viewport-75.html
fast/viewport/viewport-73.html
fast/viewport/viewport-70.html
fast/viewport/viewport-122.html
Comment 25 Build Bot 2017-04-26 17:06:14 PDT
Created attachment 308309 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 26 Chris Dumez 2017-04-26 18:34:44 PDT
Created attachment 308318 [details]
Patch
Comment 27 Chris Dumez 2017-04-26 19:25:07 PDT
Comment on attachment 308318 [details]
Patch

Requesting review again because I had to update my change to maintain background compatibility for the meta tag viewport parsing.
Comment 28 Build Bot 2017-04-26 19:41:12 PDT
Comment on attachment 308318 [details]
Patch

Attachment 308318 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3615446

New failing tests:
workers/wasm-hashset-many-2.html
Comment 29 Build Bot 2017-04-26 19:41:14 PDT
Created attachment 308324 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 30 Chris Dumez 2017-04-26 19:42:19 PDT
(In reply to Build Bot from comment #28)
> Comment on attachment 308318 [details]
> Patch
> 
> Attachment 308318 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/3615446
> 
> New failing tests:
> workers/wasm-hashset-many-2.html

Please disregard this failure, it looks unrelated.
Comment 31 Daniel Bates 2017-04-26 19:50:13 PDT
Comment on attachment 308318 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308318&action=review

> Source/WebCore/page/WindowFeatures.cpp:84
> +// Viewport: https://drafts.csswg.org/css-device-adapt/#parsing-algorithm

From my understanding this is not the spec. that we follow for viewport. We implement the behavior described in <https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariWebContent/UsingtheViewport/UsingtheViewport.html>
Comment 32 Chris Dumez 2017-04-26 19:53:09 PDT
(In reply to Daniel Bates from comment #31)
> Comment on attachment 308318 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308318&action=review
> 
> > Source/WebCore/page/WindowFeatures.cpp:84
> > +// Viewport: https://drafts.csswg.org/css-device-adapt/#parsing-algorithm
> 
> From my understanding this is not the spec. that we follow for viewport. We
> implement the behavior described in
> <https://developer.apple.com/library/content/documentation/AppleApplications/
> Reference/SafariWebContent/UsingtheViewport/UsingtheViewport.html>

Well, this is *a* spec which document's Apple's behavior. I am not convinced pointing to the Apple documentation is better given that it does not document the actual algorithm.
Comment 33 Daniel Bates 2017-04-26 20:16:08 PDT
(In reply to Chris Dumez from comment #32)
> (In reply to Daniel Bates from comment #31)
> > Comment on attachment 308318 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=308318&action=review
> > 
> > > Source/WebCore/page/WindowFeatures.cpp:84
> > > +// Viewport: https://drafts.csswg.org/css-device-adapt/#parsing-algorithm
> > 
> > From my understanding this is not the spec. that we follow for viewport. We
> > implement the behavior described in
> > <https://developer.apple.com/library/content/documentation/AppleApplications/
> > Reference/SafariWebContent/UsingtheViewport/UsingtheViewport.html>
> 
> Well, this is *a* spec which document's Apple's behavior. 

No, it documents an attempt at reverse engineering Apple's behavior:

"Below is an algorithm for parsing the content attribute of the <META> tag produced from testing Safari on the iPhone."

Notice the key phrase "produced from testing Safari on the iPhone". Apple has never contributed to this spec. and the closet thing to a spec that Apple has is the description in <https://developer.apple.com/library/content/documentation/AppleApplications/

> I am not convinced pointing to the Apple documentation is better given that it does not document the actual algorithm.

I have not verified your code change. If your code change does not change the behavior of the meta viewport tag AND the algorithm used is identical to the algorithm described in <https://drafts.csswg.org/css-device-adapt/#parsing-algorithm> then it seems OK to link to <https://drafts.csswg.org/css-device-adapt/#parsing-algorithm>. Otherwise, the definitive reference for the meta viewport syntax that we want to implement is at <https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/MetaTags.html#//apple_ref/doc/uid/TP40008193-SW6>.
Comment 34 Daniel Bates 2017-04-26 20:18:53 PDT
(In reply to Daniel Bates from comment #33)
[...]
> Apple
> has never contributed to this spec. and the closet thing to a spec that
> Apple has is the description in
> <https://developer.apple.com/library/content/documentation/AppleApplications/
> 

This should read:

Apple has never contributed to this spec. and the closet thing to a spec that Apple has is the description in <https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/MetaTags.html#//apple_ref/doc/uid/TP40008193-SW6> and <https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariWebContent/UsingtheViewport/UsingtheViewport.html>.
Comment 35 Chris Dumez 2017-04-26 20:34:04 PDT
(In reply to Daniel Bates from comment #33)
> (In reply to Chris Dumez from comment #32)
> > (In reply to Daniel Bates from comment #31)
> > > Comment on attachment 308318 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=308318&action=review
> > > 
> > > > Source/WebCore/page/WindowFeatures.cpp:84
> > > > +// Viewport: https://drafts.csswg.org/css-device-adapt/#parsing-algorithm
> > > 
> > > From my understanding this is not the spec. that we follow for viewport. We
> > > implement the behavior described in
> > > <https://developer.apple.com/library/content/documentation/AppleApplications/
> > > Reference/SafariWebContent/UsingtheViewport/UsingtheViewport.html>
> > 
> > Well, this is *a* spec which document's Apple's behavior. 
> 
> No, it documents an attempt at reverse engineering Apple's behavior:
> 
> "Below is an algorithm for parsing the content attribute of the <META> tag
> produced from testing Safari on the iPhone."
> 
> Notice the key phrase "produced from testing Safari on the iPhone". Apple
> has never contributed to this spec. and the closet thing to a spec that
> Apple has is the description in
> <https://developer.apple.com/library/content/documentation/AppleApplications/
> 
> > I am not convinced pointing to the Apple documentation is better given that it does not document the actual algorithm.
> 
> I have not verified your code change. If your code change does not change
> the behavior of the meta viewport tag AND the algorithm used is identical to
> the algorithm described in
> <https://drafts.csswg.org/css-device-adapt/#parsing-algorithm> then it seems
> OK to link to
> <https://drafts.csswg.org/css-device-adapt/#parsing-algorithm>. Otherwise,
> the definitive reference for the meta viewport syntax that we want to
> implement is at
> <https://developer.apple.com/library/content/documentation/AppleApplications/
> Reference/SafariHTMLRef/Articles/MetaTags.html#//apple_ref/doc/uid/
> TP40008193-SW6>.

It seemed to match our behavior but i will double check tomorrow. I’ll definitely not point to it if it does not match exactly.
Comment 36 Simon Pieters (:zcorpan) 2017-04-27 02:45:20 PDT
> The spec for meta viewport seems to be at https://drafts.csswg.org/css-device-adapt/#parsing-algorithm. It seems to have its own algorithm so I may need to use different algorithms for parsing meta viewport and window features.

Ugh. My knee-jerk reaction is that we should continue to use the same parser for both, unless web compat requires anything else. The CSS spec can be updated to hook into HTML's feature tokenizer. I can file an issue for css-device-adapt.
Comment 37 Simon Pieters (:zcorpan) 2017-04-27 02:55:44 PDT
Filed https://github.com/w3c/csswg-drafts/issues/1302
Comment 38 Chris Dumez 2017-04-27 10:02:54 PDT
Created attachment 308404 [details]
Patch
Comment 39 Chris Dumez 2017-04-27 15:27:59 PDT
Patch is ready for review.
Comment 40 Geoffrey Garen 2017-04-28 13:56:44 PDT
Comment on attachment 308404 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308404&action=review

r=me

> Source/WebCore/page/WindowFeatures.cpp:51
> +    switch (mode) {
> +    case FeatureMode::Viewport:
> +        return character == ' ' || character == '\t' || character == '\n' || character == '\r' || character == '=' || character == ',';
> +    case FeatureMode::Window:
> +        break;
> +    }

I think this would be clearer as an if.
Comment 41 Chris Dumez 2017-04-28 14:02:22 PDT
Committed r215945: <http://trac.webkit.org/changeset/215945>