IntersectionObserver constructor: fail on invalid rootMargin syntax.

IntersectionObserver constructor: fail on invalid rootMargin syntax.

BUG=618976
R=ojan@chromium.org,eae@chromium.org

Committed: https://crrev.com/1f7fb6ab2ddb97b65193d9a12008124096d85c6b
Cr-Commit-Position: refs/heads/master@{#401562}

[eae, 2016-06-22 10:51:04]

https://codereview.chromium.org/2088963002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/2088963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:45: if
(rootMargin.size() == 4) {
According the the comment above four tokens should be allowed. (1px 2px 3px 4px)

[szager1, 2016-06-22 15:11:45]

https://codereview.chromium.org/2088963002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/2088963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:45: if
(rootMargin.size() == 4) {
On 2016/06/22 10:51:04, eae wrote:
> According the the comment above four tokens should be allowed. (1px 2px 3px
4px)

At this point in the code, there are already four parsed tokens, and the loop is
about to process the fifth.  We know there's a fifth token, because the loop
conditional contains (tokenRange.peek().type() != EOFToken).

The existing tests verify that a rootMargin field with four tokens is
successfully parsed, and one with five tokens throws an exception.

[eae, 2016-06-22 16:54:40]

https://codereview.chromium.org/2088963002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/2088963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:45: if
(rootMargin.size() == 4) {
On 2016/06/22 15:11:45, szager1 wrote:
> On 2016/06/22 10:51:04, eae wrote:
> > According the the comment above four tokens should be allowed. (1px 2px 3px
> 4px)
> 
> At this point in the code, there are already four parsed tokens, and the loop
is
> about to process the fifth.  We know there's a fifth token, because the loop
> conditional contains (tokenRange.peek().type() != EOFToken).
> 
> The existing tests verify that a rootMargin field with four tokens is
> successfully parsed, and one with five tokens throws an exception.

The while conditions check that rootMargin.size() is less than 5 and this
condition checks if it's four.  How can that indicate the presence of a fifth
token? Does tokenRange.peek() mutate the rootMargins vector?

I might be missing something obvious here but I fail to wrap my head around this
logic.

[szager1, 2016-06-23 05:07:17]

On 2016/06/22 16:54:40, eae wrote:
>
https://codereview.chromium.org/2088963002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):
> 
>
https://codereview.chromium.org/2088963002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:45: if
> (rootMargin.size() == 4) {
> On 2016/06/22 15:11:45, szager1 wrote:
> > On 2016/06/22 10:51:04, eae wrote:
> > > According the the comment above four tokens should be allowed. (1px 2px
3px
> > 4px)
> > 
> > At this point in the code, there are already four parsed tokens, and the
loop
> is
> > about to process the fifth.  We know there's a fifth token, because the loop
> > conditional contains (tokenRange.peek().type() != EOFToken).
> > 
> > The existing tests verify that a rootMargin field with four tokens is
> > successfully parsed, and one with five tokens throws an exception.
> 
> The while conditions check that rootMargin.size() is less than 5 and this
> condition checks if it's four.  How can that indicate the presence of a fifth
> token? Does tokenRange.peek() mutate the rootMargins vector?
> 
> I might be missing something obvious here but I fail to wrap my head around
this
> logic.

PTAL -- I simplified the loop conditional; the logic is the same, but maybe it's
clearer now.

[eae, 2016-06-23 07:45:58]

Much cleaner, thank you!

LGTM

[eae, 2016-06-23 07:46:02]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2016-06-23 07:46:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2088963002/20001

[commit-bot: I haz the power, 2016-06-23 08:56:09]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-23 08:58:00]

Description was changed from

==========
IntersectionObserver constructor: fail on invalid rootMargin syntax.

BUG=618976
R=ojan@chromium.org,eae@chromium.org
==========

to

==========
IntersectionObserver constructor: fail on invalid rootMargin syntax.

BUG=618976
R=ojan@chromium.org,eae@chromium.org

Committed: https://crrev.com/1f7fb6ab2ddb97b65193d9a12008124096d85c6b
Cr-Commit-Position: refs/heads/master@{#401562}
==========

[commit-bot: I haz the power, 2016-06-23 08:58:01]

Patchset 2 (id:??) landed as
https://crrev.com/1f7fb6ab2ddb97b65193d9a12008124096d85c6b
Cr-Commit-Position: refs/heads/master@{#401562}