Rework the "rules for parsing dimension values" implementation

Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.{cpp,h}.
In general, behavior deviating from the specced version is kept with the
following exceptions:

 * Allow all of the "space characters" [2], rather than just U+0020.

 * Cases with multiple full stops (ex: "1.2.3") now parse the same as
   "1.2" rather than failing.

Comments are added where the implementation is known to deviate from the
spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there. This requires a mechanism for disallowing percentage values
though, to properly handle 'cellspacing' on <table>.

[1] https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-dimension-values
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character

BUG=668478
Committed: https://crrev.com/337cac12ea5b43b0d8dd2d1a9fe64a8e0dbb523b
Cr-Commit-Position: refs/heads/master@{#434678}

[fs, 2016-11-23 15:26:25]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 15:26:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-11-23 15:59:52]

Description was changed from

==========
Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.
In general, behavior deviating from the specced version is kept. The
only exception being that this allows all of the "space characters" [2],
rather than just U+0020. Add comments where the implementation deviates
from the spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there.

[1]
https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-...
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character
==========

to

==========
Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.{cpp,h}.
In general, behavior deviating from the specced version is kept. The
only exception being that this allows all of the "space characters" [2],
rather than just U+0020. Add comments where the implementation deviates
from the spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there.

[1]
https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-...
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character
==========

[commit-bot: I haz the power, 2016-11-23 16:29:50]

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

[commit-bot: I haz the power, 2016-11-23 16:29:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fs, 2016-11-23 17:39:27]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 17:40:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-11-23 17:40:42]

Description was changed from

==========
Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.{cpp,h}.
In general, behavior deviating from the specced version is kept. The
only exception being that this allows all of the "space characters" [2],
rather than just U+0020. Add comments where the implementation deviates
from the spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there.

[1]
https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-...
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character
==========

to

==========
Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.{cpp,h}.
In general, behavior deviating from the specced version is kept. The
only exception being that this allows all of the "space characters" [2],
rather than just U+0020. Add comments where the implementation deviates
from the spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there. This requires a mechanism for disallowing percentage values
though, to properly handle 'cellspacing' on <table>.


[1]
https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-...
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character
==========

[commit-bot: I haz the power, 2016-11-23 19:40:54]

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

[commit-bot: I haz the power, 2016-11-23 19:40:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fs, 2016-11-24 09:25:57]

Description was changed from

==========
Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.{cpp,h}.
In general, behavior deviating from the specced version is kept. The
only exception being that this allows all of the "space characters" [2],
rather than just U+0020. Add comments where the implementation deviates
from the spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there. This requires a mechanism for disallowing percentage values
though, to properly handle 'cellspacing' on <table>.


[1]
https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-...
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character
==========

to

==========
Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.{cpp,h}.
In general, behavior deviating from the specced version is kept. The
only exception being that this allows all of the "space characters" [2],
rather than just U+0020. Add comments where the implementation deviates
from the spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there. This requires a mechanism for disallowing percentage values
though, to properly handle 'cellspacing' on <table>.

[1]
https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-...
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character
==========

[fs, 2016-11-24 09:47:31]

Description was changed from

==========
Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.{cpp,h}.
In general, behavior deviating from the specced version is kept. The
only exception being that this allows all of the "space characters" [2],
rather than just U+0020. Add comments where the implementation deviates
from the spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there. This requires a mechanism for disallowing percentage values
though, to properly handle 'cellspacing' on <table>.

[1]
https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-...
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character
==========

to

==========
Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.{cpp,h}.
In general, behavior deviating from the specced version is kept with the
following exceptions:

 * Allow all of the "space characters" [2], rather than just U+0020.

 * Cases with multiple full stops (ex: "1.2.3") now parse as "1.2"
   rather than failing.

Comments are added where the implementation is known to deviate from the
spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there. This requires a mechanism for disallowing percentage values
though, to properly handle 'cellspacing' on <table>.

[1]
https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-...
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character
==========

[fs, 2016-11-24 09:48:28]

Description was changed from

==========
Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.{cpp,h}.
In general, behavior deviating from the specced version is kept with the
following exceptions:

 * Allow all of the "space characters" [2], rather than just U+0020.

 * Cases with multiple full stops (ex: "1.2.3") now parse as "1.2"
   rather than failing.

Comments are added where the implementation is known to deviate from the
spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there. This requires a mechanism for disallowing percentage values
though, to properly handle 'cellspacing' on <table>.

[1]
https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-...
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character
==========

to

==========
Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.{cpp,h}.
In general, behavior deviating from the specced version is kept with the
following exceptions:

 * Allow all of the "space characters" [2], rather than just U+0020.

 * Cases with multiple full stops (ex: "1.2.3") now parse the same as
   "1.2" rather than failing.

Comments are added where the implementation is known to deviate from the
spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there. This requires a mechanism for disallowing percentage values
though, to properly handle 'cellspacing' on <table>.

[1]
https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-...
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character
==========

[fs, 2016-11-24 12:15:48]

Description was changed from

==========
Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.{cpp,h}.
In general, behavior deviating from the specced version is kept with the
following exceptions:

 * Allow all of the "space characters" [2], rather than just U+0020.

 * Cases with multiple full stops (ex: "1.2.3") now parse the same as
   "1.2" rather than failing.

Comments are added where the implementation is known to deviate from the
spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there. This requires a mechanism for disallowing percentage values
though, to properly handle 'cellspacing' on <table>.

[1]
https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-...
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character
==========

to

==========
Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.{cpp,h}.
In general, behavior deviating from the specced version is kept with the
following exceptions:

 * Allow all of the "space characters" [2], rather than just U+0020.

 * Cases with multiple full stops (ex: "1.2.3") now parse the same as
   "1.2" rather than failing.

Comments are added where the implementation is known to deviate from the
spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there. This requires a mechanism for disallowing percentage values
though, to properly handle 'cellspacing' on <table>.

[1]
https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-...
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character

BUG=668478
==========

[fs, 2016-11-24 12:16:18]

fs@opera.com changed reviewers:
+ foolip@chromium.org, timloh@chromium.org

[Timothy Loh, 2016-11-25 00:07:47]

lgtm

https://codereview.chromium.org/2528673003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLDimension.cpp (right):

https://codereview.chromium.org/2528673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLDimension.cpp:162: // Deviation: HTML
requires a digit after the full stop. Gecko and Edge
I think you misread the spec here? From my reading it's fine to not have a digit
after, we just return the value immediately (so 10.... -> 10, 10.% -> 10). It
looks like we currently interpret 10.% as a percentage and so does FF.

[fs, 2016-11-25 10:18:43]

https://codereview.chromium.org/2528673003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLDimension.cpp (right):

https://codereview.chromium.org/2528673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLDimension.cpp:162: // Deviation: HTML
requires a digit after the full stop. Gecko and Edge
On 2016/11/25 at 00:07:47, Timothy Loh wrote:
> I think you misread the spec here? From my reading it's fine to not have a
digit after, we just return the value immediately (so 10.... -> 10, 10.% -> 10).
It looks like we currently interpret 10.% as a percentage and so does FF.

My comment could be made more clear I guess. I was thinking of the case you
mention (10.% -> 10), which both we, Gecko and Edge (and WebKit) interpret as a
percentage (while we should not per spec.) I'll make sure to update the comment
to mention this.

[foolip, 2016-11-28 13:33:00]

Explicitly leaving this to timloh@

[fs, 2016-11-28 14:28:44]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 14:28:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 17:32:22]

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

[commit-bot: I haz the power, 2016-11-28 17:32:23]

Dry run: This issue passed the CQ dry run.

[fs, 2016-11-28 17:33:43]

The CQ bit was checked by fs@opera.com

[fs, 2016-11-28 17:33:43]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/2528673003/#ps40001
(title: "Rebase; clarify comment.")

[commit-bot: I haz the power, 2016-11-28 17:34:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 17:38:02]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480354423262360,
"parent_rev": "dd76add0ae573d67d25dc03bd8f2475208c5f741", "commit_rev":
"a9726fe40cf1b3b226fd0d3460e9d9c07cac8e5b"}

[commit-bot: I haz the power, 2016-11-28 17:38:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-28 17:40:31]

Description was changed from

==========
Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.{cpp,h}.
In general, behavior deviating from the specced version is kept with the
following exceptions:

 * Allow all of the "space characters" [2], rather than just U+0020.

 * Cases with multiple full stops (ex: "1.2.3") now parse the same as
   "1.2" rather than failing.

Comments are added where the implementation is known to deviate from the
spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there. This requires a mechanism for disallowing percentage values
though, to properly handle 'cellspacing' on <table>.

[1]
https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-...
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character

BUG=668478
==========

to

==========
Rework the "rules for parsing dimension values" implementation

This CL reworks the current implementation of the "rules for parsing
dimension values" [1] (HTMLElement::addHTMLLengthToStyle) into a
separate function and moves it to HTMLDimension.{cpp,h}.
In general, behavior deviating from the specced version is kept with the
following exceptions:

 * Allow all of the "space characters" [2], rather than just U+0020.

 * Cases with multiple full stops (ex: "1.2.3") now parse the same as
   "1.2" rather than failing.

Comments are added where the implementation is known to deviate from the
spec.

This also makes it possible to avoid calling into the CSS parser for
actual parsing, which should reduce the amount of special-cases needed
there. This requires a mechanism for disallowing percentage values
though, to properly handle 'cellspacing' on <table>.

[1]
https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-...
[2] https://html.spec.whatwg.org/multipage/infrastructure.html#space-character

BUG=668478

Committed: https://crrev.com/337cac12ea5b43b0d8dd2d1a9fe64a8e0dbb523b
Cr-Commit-Position: refs/heads/master@{#434678}
==========

[commit-bot: I haz the power, 2016-11-28 17:40:32]

Patchset 3 (id:??) landed as
https://crrev.com/337cac12ea5b43b0d8dd2d1a9fe64a8e0dbb523b
Cr-Commit-Position: refs/heads/master@{#434678}

[fs, 2016-11-30 18:27:18]

https://codereview.chromium.org/2528673003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLDimension.cpp (right):

https://codereview.chromium.org/2528673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLDimension.cpp:162: // Deviation: HTML
requires a digit after the full stop. Gecko and Edge
On 2016/11/25 at 10:18:43, fs wrote:
> On 2016/11/25 at 00:07:47, Timothy Loh wrote:
> > I think you misread the spec here? From my reading it's fine to not have a
digit after, we just return the value immediately (so 10.... -> 10, 10.% -> 10).
It looks like we currently interpret 10.% as a percentage and so does FF.
> 
> My comment could be made more clear I guess. I was thinking of the case you
mention (10.% -> 10), which both we, Gecko and Edge (and WebKit) interpret as a
percentage (while we should not per spec.) I'll make sure to update the comment
to mention this.

Clarified.