[SVG2] Allow leading and trailing whitespace in svg attributes

[SVG2] Allow leading and trailing whitespace in svg attributes using
<integer>, <angle>, <number>, <length> and <percentage>.

BUG=377503, 339899
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175574

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

[Erik Dahlström (inactive), 2014-05-28 17:01:45]

I'm not sure if we want to keep this behind a runtime flag, but I went with this
for a start.

In any case, aligning the attribute whitespace handling with HTML and the other
browsers is the main goal.

[pdr., 2014-05-28 19:55:28]

On 2014/05/28 17:01:45, Erik Dahlström wrote:
> I'm not sure if we want to keep this behind a runtime flag, but I went with
this
> for a start.

Instead of putting this behind a flag, what do you think of just sending an
intent-to-implement-and-ship email to blink-dev? I am not too worried about
breaking (or, unbreaking) content.

> 
> In any case, aligning the attribute whitespace handling with HTML and the
other
> browsers is the main goal.

Could you split this patch into the whitespace change and then a second patch on
top of that for angle parsing? These don't seem related.

[Erik Dahlström (inactive), 2014-06-02 08:15:14]

On 2014/05/28 19:55:28, pdr wrote:
> On 2014/05/28 17:01:45, Erik Dahlström wrote:
> > I'm not sure if we want to keep this behind a runtime flag, but I went with
> this
> > for a start.
> 
> Instead of putting this behind a flag, what do you think of just sending an
> intent-to-implement-and-ship email to blink-dev? I am not too worried about
> breaking (or, unbreaking) content.

Will do.

> > In any case, aligning the attribute whitespace handling with HTML and the
> other
> > browsers is the main goal.
> 
> Could you split this patch into the whitespace change and then a second patch
on
> top of that for angle parsing? These don't seem related.

Yes, no problem. Will file that one separately.

[pdr., 2014-06-02 14:35:19]

I like where this is heading. Mostly just nits below.

https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGAngle...
File Source/core/svg/SVGAngle.cpp (right):

https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGAngle...
Source/core/svg/SVGAngle.cpp:175: // If the unit contains three characters, but
is not deg or rad, then it's unknown.
Can you update these comments to say something like:
"If the unit contains all spaces, it's unspecified. If the unit contains three
characters but is not deg or rad, it's unknown.

This is sort of a "what" comment and not a "why" one but I think it would make
the logic here easier to reason about.

https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGLengt...
File Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGLengt...
Source/core/svg/SVGLength.cpp:79: return firstChar == '%' ? LengthTypePercentage
: (isSVGSpace(firstChar) ? LengthTypeNumber : LengthTypeUnknown);
This isn't needed: if the first character is a percent sign, it's not a space.

https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGLengt...
Source/core/svg/SVGLength.cpp:105: return LengthTypeNumber;
Two spaces are a number?

https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGParse...
File Source/core/svg/SVGParserUtilities.h (right):

https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGParse...
Source/core/svg/SVGParserUtilities.h:66: return c == ' ' || c == '\t' || c ==
'\n' || c == '\r' || c == '\f';
This now matches isHTMLSpace(). Do we need both?

[fs, 2014-06-03 14:35:36]

https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGLengt...
File Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGLengt...
Source/core/svg/SVGLength.cpp:79: return firstChar == '%' ? LengthTypePercentage
: (isSVGSpace(firstChar) ? LengthTypeNumber : LengthTypeUnknown);
On 2014/06/02 14:35:19, pdr wrote:
> This isn't needed: if the first character is a percent sign, it's not a space.

Nested ternary expressions FTW? =)

if (firstChar == '%')
  return percentage;
else if (isSVGSpace(firstChar))
  return number;
else
  return confuzzled;

https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGLengt...
Source/core/svg/SVGLength.cpp:105: return LengthTypeNumber;
On 2014/06/02 14:35:19, pdr wrote:
> Two spaces are a number?

More like "two spaces are not a unit".

"42  "

And with this and the above I just want to say that I find this parsing
structure kind of hard to follow w/ the WS-bits. I suppose a structure like:

UChar firstChar = *++ptr;
if (firstChar == '%') {
  type = percentage;
} else if (firstChar == 'e') {
  UChar secondChar = ptr < end ? *++ptr : 0;
  if (secondChar == 'm')
    type = ems;
  ...
  else
    return unknown;
} else ... {
  ...
}

if (!skipOptionalSVGSpaces(ptr, end))
  return type;
return unknown;

might read more easily. (Not necessarily suggesting any change be done though.)

(Another option would be sinking the WS-handling out into the caller, i.e.:

  ...
  type = stringToLengthType(ptr, end);
  ASSERT(ptr <= end);
  if (skipOptionalSVGSpaces(ptr, end))
      return false;
  ASSERT(ptr == end);
  return true;

[still requires a minor change to rewind ptr after if second char is
unrecognized])

[Erik Dahlström (inactive), 2014-06-04 16:25:43]

https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGAngle...
File Source/core/svg/SVGAngle.cpp (right):

https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGAngle...
Source/core/svg/SVGAngle.cpp:175: // If the unit contains three characters, but
is not deg or rad, then it's unknown.
On 2014/06/02 14:35:19, pdr wrote:
> Can you update these comments to say something like:
> "If the unit contains all spaces, it's unspecified. If the unit contains three
> characters but is not deg or rad, it's unknown.
> 
> This is sort of a "what" comment and not a "why" one but I think it would make
> the logic here easier to reason about.

I went with a rewrite to hopefully make it more clear.

https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGParse...
File Source/core/svg/SVGParserUtilities.h (right):

https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGParse...
Source/core/svg/SVGParserUtilities.h:66: return c == ' ' || c == '\t' || c ==
'\n' || c == '\r' || c == '\f';
On 2014/06/02 14:35:19, pdr wrote:
> This now matches isHTMLSpace(). Do we need both?

Replaced all instances of isSVGSpace with isHTMLSpace.

[pdr., 2014-06-05 04:46:51]

On 2014/06/04 16:25:43, Erik Dahlström wrote:
>
https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGAngle...
> File Source/core/svg/SVGAngle.cpp (right):
> 
>
https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGAngle...
> Source/core/svg/SVGAngle.cpp:175: // If the unit contains three characters,
but
> is not deg or rad, then it's unknown.
> On 2014/06/02 14:35:19, pdr wrote:
> > Can you update these comments to say something like:
> > "If the unit contains all spaces, it's unspecified. If the unit contains
three
> > characters but is not deg or rad, it's unknown.
> > 
> > This is sort of a "what" comment and not a "why" one but I think it would
make
> > the logic here easier to reason about.
> 
> I went with a rewrite to hopefully make it more clear.
> 
>
https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGParse...
> File Source/core/svg/SVGParserUtilities.h (right):
> 
>
https://codereview.chromium.org/302643004/diff/40001/Source/core/svg/SVGParse...
> Source/core/svg/SVGParserUtilities.h:66: return c == ' ' || c == '\t' || c ==
> '\n' || c == '\r' || c == '\f';
> On 2014/06/02 14:35:19, pdr wrote:
> > This now matches isHTMLSpace(). Do we need both?
> 
> Replaced all instances of isSVGSpace with isHTMLSpace.

LGTM

I'm a little worried the tests will be slow on the debug bots. Lets just keep an
eye out for them being flaky.

[Erik Dahlström (inactive), 2014-06-05 07:53:01]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-06-05 07:53:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ed@opera.com/302643004/70001

[Erik Dahlström (inactive), 2014-06-05 09:17:13]

The CQ bit was unchecked by ed@opera.com

[Erik Dahlström (inactive), 2014-06-05 12:39:48]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-06-05 12:40:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ed@opera.com/302643004/80001

[commit-bot: I haz the power, 2014-06-05 13:49:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10425)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10790)

[commit-bot: I haz the power, 2014-06-05 14:36:43]

Change committed as 175574

[Erik Dahlström (inactive), 2014-06-09 08:35:42]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-06-09 08:36:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ed@opera.com/302643004/90001

[Erik Dahlström (inactive), 2014-06-09 08:37:01]

Split test, attempt to reland after revert -
https://codereview.chromium.org/319883002

[commit-bot: I haz the power, 2014-06-09 09:39:06]

Change committed as 175785