Uptake http://trac.webkit.org/changeset/169848

Uptake http://trac.webkit.org/changeset/169848

Set |length| based on |buffer|, not |content|. Switch to using a for loop.
Switch to unsigned types since we are working with positive offsets
into a String. Make sure to never cross the length boundary.

BUG=383703
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176713

[arv (Not doing code reviews), 2014-06-20 14:31:37]

https://codereview.chromium.org/342163003/diff/20001/Source/web/tests/data/vi...
File Source/web/tests/data/viewport/viewport-warnings-8.html (right):

https://codereview.chromium.org/342163003/diff/20001/Source/web/tests/data/vi...
Source/web/tests/data/viewport/viewport-warnings-8.html:3: <meta name="viewport"
content="width=123&#x0130;">
This test is suffering from the same issue as the invalid test in
http://trac.webkit.org/changeset/169849

Since the length got longer it just means we stopped checking the string before
we processed the whole thing.

To really test that the code works we need to put U+0130 at the beginning. I'm
not fully sure what the syntax is for meta viewport but something like this
might be a better test:

<meta name="viewport" content="&#x0130;&#x0130;=ignore,width=123">

and then assert that we treated width as "123" and not as "1", which it would
with the old code since the old code would end iterating 2 characters too early.

[kenneth.christiansen, 2014-06-20 14:49:25]

Right, I already pointed that out to Abhishek Arya

>
https://codereview.chromium.org/342163003/diff/20001/Source/web/tests/data/vi...
> Source/web/tests/data/viewport/viewport-warnings-8.html:3: <meta
name="viewport"
> content="width=123&#x0130;">
> This test is suffering from the same issue as the invalid test in
> http://trac.webkit.org/changeset/169849
> 
> Since the length got longer it just means we stopped checking the string
before
> we processed the whole thing.

Which also would make the added test fail as it would write i instead of an i
with an extra dot on top.

> To really test that the code works we need to put U+0130 at the beginning. I'm
> not fully sure what the syntax is for meta viewport but something like this
> might be a better test:
> 
> <meta name="viewport" content="&#x0130;&#x0130;=ignore,width=123">
> 
> and then assert that we treated width as "123" and not as "1", which it would
> with the old code since the old code would end iterating 2 characters too
early.

That is another test but the existing test actually already covers it

[kenneth.christiansen, 2014-06-20 14:51:51]

https://codereview.chromium.org/342163003/diff/20001/Source/web/tests/Viewpor...
File Source/web/tests/ViewportTest.cpp (right):

https://codereview.chromium.org/342163003/diff/20001/Source/web/tests/Viewpor...
Source/web/tests/ViewportTest.cpp:3221: EXPECT_STREQ("The value \"123i̇\" for
key \"width\" was truncated to its numeric prefix.",
Notice that this is not an i, but an i with a dot on top :-) 

If you copy it and compare it using JS with === "i" it will fail.

[arv (Not doing code reviews), 2014-06-20 15:07:42]

On 2014/06/20 14:49:25, kenneth.christiansen wrote:
> Right, I already pointed that out to Abhishek Arya
> 
> >
>
https://codereview.chromium.org/342163003/diff/20001/Source/web/tests/data/vi...
> > Source/web/tests/data/viewport/viewport-warnings-8.html:3: <meta
> name="viewport"
> > content="width=123&#x0130;">
> > This test is suffering from the same issue as the invalid test in
> > http://trac.webkit.org/changeset/169849
> > 
> > Since the length got longer it just means we stopped checking the string
> before
> > we processed the whole thing.
> 
> Which also would make the added test fail as it would write i instead of an i
> with an extra dot on top.
> 
> > To really test that the code works we need to put U+0130 at the beginning.
I'm
> > not fully sure what the syntax is for meta viewport but something like this
> > might be a better test:
> > 
> > <meta name="viewport" content="&#x0130;&#x0130;=ignore,width=123">
> > 
> > and then assert that we treated width as "123" and not as "1", which it
would
> > with the old code since the old code would end iterating 2 characters too
> early.
> 
> That is another test but the existing test actually already covers it

You are correct. I looked at this closer and it would include the "i".

[arv (Not doing code reviews), 2014-06-20 15:07:48]

LGTM

[arv (Not doing code reviews), 2014-06-20 15:15:41]

Still LGTM

[kenneth.r.christiansen, 2014-06-20 15:18:06]

The CQ bit was checked by kenneth.r.christiansen@intel.com

[commit-bot: I haz the power, 2014-06-20 15:18:59]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/kenneth.r.christiansen@intel.com/34216...

[kenneth.christiansen, 2014-06-20 15:55:19]

Can anyone check web/

[commit-bot: I haz the power, 2014-06-20 16:20:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/8137)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[commit-bot: I haz the power, 2014-06-20 16:23:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-20 16:23:37]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/8147)

[aarya, 2014-06-22 17:18:13]

+cc Adam for OWNERS review

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    Source/web/tests/ViewportTest.cpp
    Source/web/tests/data/viewport/viewport-138.html

[abarth-chromium, 2014-06-22 20:33:27]

LGTM

[commit-bot: I haz the power, 2014-06-22 20:36:51]

Change committed as 176713