Update the stringifier behavior for DOMMatrixReadOnly

Update the stringifier behavior for DOMMatrixReadOnly

They are appending a much lower precision than a double. It should append the 
value of the double atrributes to the string.

Link: https://github.com/w3c/fxtf-drafts/issues/120

BUG=none

Review-Url: https://codereview.chromium.org/2846523002
Cr-Commit-Position: refs/heads/master@{#477328}
Committed: https://chromium.googlesource.com/chromium/src/+/38211a9a22a7d3ad0ed32e8250f8ae2b4b3ff94a

[Byoungkwon Ko, 2017-04-26 13:19:13]

Description was changed from

==========
We fixed stringifier behavior in geometry. They are appending a much lower
precision than a double. It should append the value of the double atrributes to
the string.

BUG=https://github.com/w3c/fxtf-drafts/issues/120
==========

to

==========
We fixed stringifier behavior in geometry. 
They are appending a much lower precision than a double. It should append the 
value of the double atrributes to the string.

BUG=https://github.com/w3c/fxtf-drafts/issues/120
==========

[Byoungkwon Ko, 2017-04-26 13:21:21]

Description was changed from

==========
We fixed stringifier behavior in geometry. 
They are appending a much lower precision than a double. It should append the 
value of the double atrributes to the string.

BUG=https://github.com/w3c/fxtf-drafts/issues/120
==========

to

==========
We fixed stringifier behavior in geometry. 
They are appending a much lower precision than a double. It should append the 
value of the double atrributes to the string.

Link: https://github.com/w3c/fxtf-drafts/issues/120

BUG=none
==========

[Byoungkwon Ko, 2017-04-26 13:23:30]

gogag2@gmail.com changed reviewers:
+ cabanier@adobe.com, foolip@chromium.org, jinho.bang@samsung.com

[Byoungkwon Ko, 2017-04-26 13:23:31]

I split it from "Make WebkitCSSMatrix an alias of DOMMatrix".
Could you review it?
Thanks.

[foolip, 2017-04-26 13:47:37]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 13:48:02]

Dry run: CQ is trying da patch.

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

[foolip, 2017-04-26 13:50:00]

I started a dry run. If no tests fail, then please add tests that exercise the
change. You can add them in LayoutTests/external/wpt/css/geometry-1/, which
should soon be imported due to https://codereview.chromium.org/2841823002

[foolip, 2017-04-26 13:53:15]

Description was changed from

==========
We fixed stringifier behavior in geometry. 
They are appending a much lower precision than a double. It should append the 
value of the double atrributes to the string.

Link: https://github.com/w3c/fxtf-drafts/issues/120

BUG=none
==========

to

==========
They are appending a much lower precision than a double. It should append the 
value of the double atrributes to the string.

Link: https://github.com/w3c/fxtf-drafts/issues/120

BUG=none
==========

[foolip, 2017-04-26 13:53:50]

Description was changed from

==========
They are appending a much lower precision than a double. It should append the 
value of the double atrributes to the string.

Link: https://github.com/w3c/fxtf-drafts/issues/120

BUG=none
==========

to

==========
Update the stringifier behavior for DOMMatrixReadOnly

They are appending a much lower precision than a double. It should append the 
value of the double atrributes to the string.

Link: https://github.com/w3c/fxtf-drafts/issues/120

BUG=none
==========

[foolip, 2017-04-26 13:54:18]

(I updated the description)

[foolip, 2017-04-26 13:55:17]

If you are on GitHub, can you please comment on
https://github.com/w3c/fxtf-drafts/issues/120 to say that you'd like to change
Blink and thus match the spec?

[commit-bot: I haz the power, 2017-04-26 13:55:52]

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

[commit-bot: I haz the power, 2017-04-26 13:55:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Byoungkwon Ko, 2017-04-26 14:18:29]

On 2017/04/26 13:55:17, foolip OOO until May 2 wrote:
> If you are on GitHub, can you please comment on
> https://github.com/w3c/fxtf-drafts/issues/120 to say that you'd like to change
> Blink and thus match the spec?

I updated comment to the link :)
thanks.

[zino, 2017-04-26 14:46:15]

On 2017/04/26 14:18:29, Byoungkwon Ko wrote:
> On 2017/04/26 13:55:17, foolip OOO until May 2 wrote:
> > If you are on GitHub, can you please comment on
> > https://github.com/w3c/fxtf-drafts/issues/120 to say that you'd like to
change
> > Blink and thus match the spec?
> 
> I updated comment to the link :)
> thanks.

You should use String::Format() instead of format().

Reformatting happened recently. :)

[Byoungkwon Ko, 2017-04-26 14:49:05]

On 2017/04/26 14:46:15, zino wrote:
> On 2017/04/26 14:18:29, Byoungkwon Ko wrote:
> > On 2017/04/26 13:55:17, foolip OOO until May 2 wrote:
> > > If you are on GitHub, can you please comment on
> > > https://github.com/w3c/fxtf-drafts/issues/120 to say that you'd like to
> change
> > > Blink and thus match the spec?
> > 
> > I updated comment to the link :)
> > thanks.
> 
> You should use String::Format() instead of format().
> 
> Reformatting happened recently. :)

yeah!! Right :)
i will fix it asap!!
thanks

[Byoungkwon Ko, 2017-04-26 16:15:56]

The CQ bit was checked by gogag2@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 16:16:38]

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, 2017-04-26 18:06:27]

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

[commit-bot: I haz the power, 2017-04-26 18:06:28]

Dry run: This issue passed the CQ dry run.

[Byoungkwon Ko, 2017-05-08 01:29:24]

The CQ bit was checked by gogag2@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 01:29:38]

Dry run: CQ is trying da patch.

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

[Byoungkwon Ko, 2017-05-08 01:32:15]

The CQ bit was checked by gogag2@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 01:32:28]

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, 2017-05-08 02:48:29]

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

[commit-bot: I haz the power, 2017-05-08 02:48:30]

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

[Byoungkwon Ko, 2017-05-08 03:55:55]

The CQ bit was checked by gogag2@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 03:56:14]

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, 2017-05-08 04:54:58]

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

[commit-bot: I haz the power, 2017-05-08 04:54:59]

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

[Byoungkwon Ko, 2017-05-09 01:39:51]

The CQ bit was checked by gogag2@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 01:40:59]

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, 2017-05-09 02:47:54]

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

[commit-bot: I haz the power, 2017-05-09 02:47:54]

Dry run: This issue passed the CQ dry run.

[simonp, 2017-05-09 09:05:52]

On 2017/04/26 14:49:05, Byoungkwon Ko wrote:
> > You should use String::Format() instead of format().
> > 
> > Reformatting happened recently. :)
> 
> yeah!! Right :)
> i will fix it asap!!
> thanks

Per discussion in https://github.com/w3c/fxtf-drafts/issues/120 using
String::NumberToStringECMAScript is probably better. (I will update the spec to
be clear about expected behavior after any input from the CSS WG.)

[zino, 2017-05-09 09:32:04]

https://codereview.chromium.org/2846523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp (left):

https://codereview.chromium.org/2846523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:309: stream <<
"matrix(" << a() << ", " << b() << ", " << c() << ", " << d()
Looks good to me.

BTW, don't we use stringstream like previous patch?

stream << "matrix(" <<String::NumberToStringECMAScript(a()) << ", "
...

[Byoungkwon Ko, 2017-05-10 07:23:43]

On 2017/05/09 09:32:04, zino wrote:
>
https://codereview.chromium.org/2846523002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp (left):
> 
>
https://codereview.chromium.org/2846523002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:309: stream <<
> "matrix(" << a() << ", " << b() << ", " << c() << ", " << d()
> Looks good to me.
> 
> BTW, don't we use stringstream like previous patch?
> 
> stream << "matrix(" <<String::NumberToStringECMAScript(a()) << ", "
> ...

if we use stream, they show us like below.
matrix(/"1/", /"2/", /"3/", /"4/", /"5/", /"6/")
So, I changed using append() instead of stream.

[simonp, 2017-05-11 08:14:21]

On 2017/05/09 09:05:52, simonp wrote:
> Per discussion in https://github.com/w3c/fxtf-drafts/issues/120 using
> String::NumberToStringECMAScript is probably better. (I will update the spec
to
> be clear about expected behavior after any input from the CSS WG.)

We discussed NaN/Infinity yesterday and the WG opinion was that it should throw
if there are any non-finite values.

https://github.com/w3c/fxtf-drafts/pull/148 is the proposed spec change.

[zino, 2017-05-15 19:11:14]

jinho.bang@samsung.com changed reviewers:
+ fserb@chromium.org
- cabanier@adobe.com

[zino, 2017-05-15 19:11:55]

jinho.bang@samsung.com changed reviewers:
+ cabanier@adobe.com

[zino, 2017-05-15 19:12:16]

+ fserb@ for this review.

[zino, 2017-05-15 19:17:04]

Byungkown Ko@ ping.

I think you should resolve simonp@'s comment.
Please see the spec bug. You probably should change the stringifier to throw
exception if it has NaN or Infinity.

[Byoungkwon Ko, 2017-05-16 19:03:53]

PTAL my latest patch.

[zino, 2017-05-16 22:25:37]

lgtm with nits

https://codereview.chromium.org/2846523002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp (right):

https://codereview.chromium.org/2846523002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:329:
exception_state.ThrowDOMException(kInvalidStateError, "Wrong values.");
nit: Please update this exception message in more detail.

https://codereview.chromium.org/2846523002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.h (right):

https://codereview.chromium.org/2846523002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.h:67: bool
isNaNorInf(double x) const;
nit: Please remove this?

[Byoungkwon Ko, 2017-05-18 13:52:15]

PTAL

https://codereview.chromium.org/2846523002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp (right):

https://codereview.chromium.org/2846523002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:329:
exception_state.ThrowDOMException(kInvalidStateError, "Wrong values.");
On 2017/05/16 22:25:37, zino wrote:
> nit: Please update this exception message in more detail.

Done.

https://codereview.chromium.org/2846523002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.h (right):

https://codereview.chromium.org/2846523002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.h:67: bool
isNaNorInf(double x) const;
On 2017/05/16 22:25:37, zino wrote:
> nit: Please remove this?

Done.

[Byoungkwon Ko, 2017-05-18 13:52:22]

The CQ bit was checked by gogag2@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 13:52:52]

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, 2017-05-18 15:52:40]

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

[commit-bot: I haz the power, 2017-05-18 15:52:41]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[zino, 2017-05-18 16:15:38]

https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp (right):

https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:327: if
(std::isfinite(a()) || std::isfinite(b()) || std::isfinite(c()) ||
Shouldn't be && instead of || as follows:
std::isfinite(a()) && std::isfinite(b()) ...

[fserb, 2017-05-18 17:10:55]

https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-readonly.html
(right):

https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-readonly.html:69:
var matrix2d = new DOMMatrixReadOnly([NaN, NaN, NaN, NaN, NaN, NaN]);
tests like this are very misleading. Because you are creating multiple codepaths
to trigger. If I remove one of those tests by accident, the test will still
pass.

https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp (right):

https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:327: if
(std::isfinite(a()) || std::isfinite(b()) || std::isfinite(c()) ||
On 2017/05/18 at 16:15:38, zino wrote:
> Shouldn't be && instead of || as follows:
> std::isfinite(a()) && std::isfinite(b()) ...

actually, shouldn't it be !std::isfinite(a()) || !std::isfinite(b()) .. ?

https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:331: "DOMMatrix
does not support NaN and Infinity values.");
This is false.
"DOMMatrix cannot be serialized with NaN or Infinity values."

https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:351: if
(std::isfinite(m11()) || std::isfinite(m12()) || std::isfinite(m13()) ||
same as above.

https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:359: "DOMMatrix
does not support NaN and infinity values.");
same as above.

[zino, 2017-05-18 18:19:02]

https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp (right):

https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:327: if
(std::isfinite(a()) || std::isfinite(b()) || std::isfinite(c()) ||
On 2017/05/18 17:10:55, fserb wrote:
> On 2017/05/18 at 16:15:38, zino wrote:
> > Shouldn't be && instead of || as follows:
> > std::isfinite(a()) && std::isfinite(b()) ...
> 
> actually, shouldn't it be !std::isfinite(a()) || !std::isfinite(b()) .. ?

Yeah it's the same. :) (de morgan's law)

[blink-reviews, 2017-05-18 18:39:09]

Not unless you have a big ! outside, right?

On Thu, May 18, 2017, 14:19 <jinho.bang@samsung.com> wrote:

>
>
>
https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp
> (right):
>
>
>
https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:327: if
> (std::isfinite(a()) || std::isfinite(b()) || std::isfinite(c()) ||
> On 2017/05/18 17:10:55, fserb wrote:
> > On 2017/05/18 at 16:15:38, zino wrote:
> > > Shouldn't be && instead of || as follows:
> > > std::isfinite(a()) && std::isfinite(b()) ...
> >
> > actually, shouldn't it be !std::isfinite(a()) || !std::isfinite(b())
> .. ?
>
> Yeah it's the same. :) (de morgan's law)
>
> https://codereview.chromium.org/2846523002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-05-18 18:39:09]

Not unless you have a big ! outside, right?

On Thu, May 18, 2017, 14:19 <jinho.bang@samsung.com> wrote:

>
>
>
https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp
> (right):
>
>
>
https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:327: if
> (std::isfinite(a()) || std::isfinite(b()) || std::isfinite(c()) ||
> On 2017/05/18 17:10:55, fserb wrote:
> > On 2017/05/18 at 16:15:38, zino wrote:
> > > Shouldn't be && instead of || as follows:
> > > std::isfinite(a()) && std::isfinite(b()) ...
> >
> > actually, shouldn't it be !std::isfinite(a()) || !std::isfinite(b())
> .. ?
>
> Yeah it's the same. :) (de morgan's law)
>
> https://codereview.chromium.org/2846523002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[zino, 2017-05-18 18:53:02]

On 2017/05/18 18:39:09, chromium-reviews wrote:
> Not unless you have a big ! outside, right?
> 
> On Thu, May 18, 2017, 14:19 <mailto:jinho.bang@samsung.com> wrote:
> 
> >
> >
> >
>
https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp
> > (right):
> >
> >
> >
>
https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:327: if
> > (std::isfinite(a()) || std::isfinite(b()) || std::isfinite(c()) ||
> > On 2017/05/18 17:10:55, fserb wrote:
> > > On 2017/05/18 at 16:15:38, zino wrote:
> > > > Shouldn't be && instead of || as follows:
> > > > std::isfinite(a()) && std::isfinite(b()) ...
> > >
> > > actually, shouldn't it be !std::isfinite(a()) || !std::isfinite(b())
> > .. ?
> >
> > Yeah it's the same. :) (de morgan's law)
> >
> > https://codereview.chromium.org/2846523002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Yeap.. right. Sorry for confusing comment :P

Byungkwon Ko@, you can fix it as follows:
!std::isfinite() || !std::isfinite() || ... || !std::isfinite())
!(std::isfinite() && std:: isfinite() && ... && std::isfinite())

IMHO, first option is better for readability.

[Byoungkwon Ko, 2017-05-20 00:51:34]

The CQ bit was checked by gogag2@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-20 00:52:22]

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, 2017-05-20 02:34:54]

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

[commit-bot: I haz the power, 2017-05-20 02:34:55]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Byoungkwon Ko, 2017-05-20 08:41:09]

The CQ bit was checked by gogag2@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-20 08:41:16]

Dry run: CQ is trying da patch.

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

[Byoungkwon Ko, 2017-05-20 08:41:29]

https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-readonly.html
(right):

https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-readonly.html:69:
var matrix2d = new DOMMatrixReadOnly([NaN, NaN, NaN, NaN, NaN, NaN]);
On 2017/05/18 17:10:54, fserb wrote:
> tests like this are very misleading. Because you are creating multiple
codepaths
> to trigger. If I remove one of those tests by accident, the test will still
> pass.

As you know, I am newbie for chromium.
I tried to find what you mean about tests.
But I don't know what you want to fix exactly.
Could you tell me more ?
Or if you have any example, could you share it to me?
Really  Thanks.

[commit-bot: I haz the power, 2017-05-20 10:24:13]

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

[commit-bot: I haz the power, 2017-05-20 10:24:14]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fserb, 2017-05-20 15:50:57]

On 2017/05/20 at 08:41:29, gogag2 wrote:
>
https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-readonly.html
(right):
> 
>
https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-readonly.html:69:
var matrix2d = new DOMMatrixReadOnly([NaN, NaN, NaN, NaN, NaN, NaN]);
> On 2017/05/18 17:10:54, fserb wrote:
> > tests like this are very misleading. Because you are creating multiple
codepaths
> > to trigger. If I remove one of those tests by accident, the test will still
> > pass.
> 
> As you know, I am newbie for chromium.
> I tried to find what you mean about tests.
> But I don't know what you want to fix exactly.
> Could you tell me more ?
> Or if you have any example, could you share it to me?
> Really  Thanks.

Don't worry, it's not a big deal. And I'm sorry for being cryptic about it, I
was in a hurry for some reason, should have slowed down. :)

Ideally you'd test if the exception is thrown if any of the members is NaN. The
way you are doing here, if you had, for example, missed one of the checks on the
main function (on the big !isFinite thingy) this test would still pass, because
you are testing multiple conditions at the same time. Do you see what I mean?
Also, are you sure this test passed on the previous version of the code?
Maybe we should do something like:

for (int i = 0; i < 6; ++i) {
  Array<Float> seq = [1,1,1,1,1,1];  // or int[] maybe?
  seq[i] = NaN;
  var matrix2d = new DOMMatrixReadOnly(seq, 6);
  ... do your test here
}

[Byoungkwon Ko, 2017-05-21 07:05:48]

The CQ bit was checked by gogag2@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-21 07:05:59]

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, 2017-05-21 08:34:51]

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

[commit-bot: I haz the power, 2017-05-21 08:34:52]

Dry run: This issue passed the CQ dry run.

[Byoungkwon Ko, 2017-05-21 14:19:29]

On 2017/05/20 15:50:57, fserb wrote:
> On 2017/05/20 at 08:41:29, gogag2 wrote:
> >
>
https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Lay...
> > File
>
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-readonly.html
> (right):
> > 
> >
>
https://codereview.chromium.org/2846523002/diff/120001/third_party/WebKit/Lay...
> >
>
third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-matrix-readonly.html:69:
> var matrix2d = new DOMMatrixReadOnly([NaN, NaN, NaN, NaN, NaN, NaN]);
> > On 2017/05/18 17:10:54, fserb wrote:
> > > tests like this are very misleading. Because you are creating multiple
> codepaths
> > > to trigger. If I remove one of those tests by accident, the test will
still
> > > pass.
> > 
> > As you know, I am newbie for chromium.
> > I tried to find what you mean about tests.
> > But I don't know what you want to fix exactly.
> > Could you tell me more ?
> > Or if you have any example, could you share it to me?
> > Really  Thanks.
> 
> Don't worry, it's not a big deal. And I'm sorry for being cryptic about it, I
> was in a hurry for some reason, should have slowed down. :)
> 
> Ideally you'd test if the exception is thrown if any of the members is NaN.
The
> way you are doing here, if you had, for example, missed one of the checks on
the
> main function (on the big !isFinite thingy) this test would still pass,
because
> you are testing multiple conditions at the same time. Do you see what I mean?
> Also, are you sure this test passed on the previous version of the code?
> Maybe we should do something like:
> 
> for (int i = 0; i < 6; ++i) {
>   Array<Float> seq = [1,1,1,1,1,1];  // or int[] maybe?
>   seq[i] = NaN;
>   var matrix2d = new DOMMatrixReadOnly(seq, 6);
>   ... do your test here
> }

Oh~ I see what you mean.
Thanks for your explanation detailed.
I will update test soon.
Thanks you.

[Byoungkwon Ko, 2017-05-21 16:50:56]

The CQ bit was checked by gogag2@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-21 16:51:09]

Dry run: CQ is trying da patch.

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

[Byoungkwon Ko, 2017-05-21 16:53:12]

@fserb

Could you review test code??
is it right?

[commit-bot: I haz the power, 2017-05-21 20:59:32]

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

[commit-bot: I haz the power, 2017-05-21 20:59:33]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Byoungkwon Ko, 2017-05-21 22:05:52]

The CQ bit was checked by gogag2@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-21 22:06:02]

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, 2017-05-21 23:38:53]

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

[commit-bot: I haz the power, 2017-05-21 23:38:54]

Dry run: This issue passed the CQ dry run.

[Byoungkwon Ko, 2017-05-29 07:08:56]

@fserb.

If you have time to review my code, plz check it : )
Thanks.

[fserb, 2017-05-29 17:22:51]

https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-stringifier-expected.txt
(right):

https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-stringifier-expected.txt:55:
FAIL WebKitCSSMatrix stringifier: identity (2d) assert_equals: expected
"matrix(1, 0, 0, 1, 0, 0)" but got "matrix(1.000000, 0.000000, 0.000000,
1.000000, 0.000000, 0.000000)"
this should be passing. Could you please check and either update the WebPlatform
tests or the code?
And if all tests are passing, remove the expected file.

https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp (right):

https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:351: if
(!std::isfinite(m11()) || !std::isfinite(m12()) || !std::isfinite(m13()) ||
git cl format?

[Byoungkwon Ko, 2017-05-31 15:00:41]

@fserb

Could you review my comment?
Thanks.

https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-stringifier-expected.txt
(right):

https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-stringifier-expected.txt:55:
FAIL WebKitCSSMatrix stringifier: identity (2d) assert_equals: expected
"matrix(1, 0, 0, 1, 0, 0)" but got "matrix(1.000000, 0.000000, 0.000000,
1.000000, 0.000000, 0.000000)"
On 2017/05/29 17:22:51, fserb wrote:
> this should be passing. Could you please check and either update the
WebPlatform
> tests or the code?
> And if all tests are passing, remove the expected file.

Yes, right. It should be passing.
But I think when making WebKitCSSMatrix an alias of DOMMatrix, all of them
should be passing.
Could I fix below CL?
https://codereview.chromium.org/2709763004/

If it should be passing now, I will fix it.
Plz tell me : )
Thanks.

https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp (right):

https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:351: if
(!std::isfinite(m11()) || !std::isfinite(m12()) || !std::isfinite(m13()) ||
On 2017/05/29 17:22:51, fserb wrote:
> git cl format?

Yes, I did.
Is it wrong?

[fserb, 2017-05-31 20:21:35]

On 2017/05/31 at 15:00:41, gogag2 wrote:
> @fserb
> 
> Could you review my comment?
> Thanks.
> 
>
https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-stringifier-expected.txt
(right):
> 
>
https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-stringifier-expected.txt:55:
FAIL WebKitCSSMatrix stringifier: identity (2d) assert_equals: expected
"matrix(1, 0, 0, 1, 0, 0)" but got "matrix(1.000000, 0.000000, 0.000000,
1.000000, 0.000000, 0.000000)"
> On 2017/05/29 17:22:51, fserb wrote:
> > this should be passing. Could you please check and either update the
WebPlatform
> > tests or the code?
> > And if all tests are passing, remove the expected file.
> 
> Yes, right. It should be passing.
> But I think when making WebKitCSSMatrix an alias of DOMMatrix, all of them
should be passing.
> Could I fix below CL?
> https://codereview.chromium.org/2709763004/
> 
> If it should be passing now, I will fix it.
> Plz tell me : )
> Thanks.

Are you sure those are related? It seems that is expecting "matrix(1, 0, 0...)"
and getting "matrix(1.00000, 0.0000, 0.0000...)". It doesn't seem to be related
to me.

> 
>
https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp (right):
> 
>
https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:351: if
(!std::isfinite(m11()) || !std::isfinite(m12()) || !std::isfinite(m13()) ||
> On 2017/05/29 17:22:51, fserb wrote:
> > git cl format?
> 
> Yes, I did.
> Is it wrong?

Ahn, nvm, on my monitor it was breaking between lines. It's all good :)

[fserb, 2017-05-31 20:21:58]

On 2017/05/31 at 20:21:35, fserb wrote:
> On 2017/05/31 at 15:00:41, gogag2 wrote:
> > @fserb
> > 
> > Could you review my comment?
> > Thanks.
> > 
> >
https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Lay...
> > File
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-stringifier-expected.txt
(right):
> > 
> >
https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Lay...
> >
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-stringifier-expected.txt:55:
FAIL WebKitCSSMatrix stringifier: identity (2d) assert_equals: expected
"matrix(1, 0, 0, 1, 0, 0)" but got "matrix(1.000000, 0.000000, 0.000000,
1.000000, 0.000000, 0.000000)"
> > On 2017/05/29 17:22:51, fserb wrote:
> > > this should be passing. Could you please check and either update the
WebPlatform
> > > tests or the code?
> > > And if all tests are passing, remove the expected file.
> > 
> > Yes, right. It should be passing.
> > But I think when making WebKitCSSMatrix an alias of DOMMatrix, all of them
should be passing.
> > Could I fix below CL?
> > https://codereview.chromium.org/2709763004/
> > 
> > If it should be passing now, I will fix it.
> > Plz tell me : )
> > Thanks.
> 
> Are you sure those are related? It seems that is expecting "matrix(1, 0,
0...)" and getting "matrix(1.00000, 0.0000, 0.0000...)". It doesn't seem to be
related to me.
> 
> > 
> >
https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp (right):
> > 
> >
https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:351: if
(!std::isfinite(m11()) || !std::isfinite(m12()) || !std::isfinite(m13()) ||
> > On 2017/05/29 17:22:51, fserb wrote:
> > > git cl format?
> > 
> > Yes, I did.
> > Is it wrong?
> 
> Ahn, nvm, on my monitor it was breaking between lines. It's all good :)

Please look at the test again and see what's wrong with that, otherwise it's
good. :)

[Byoungkwon Ko, 2017-06-05 20:51:21]

The CQ bit was checked by gogag2@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 20:52:05]

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, 2017-06-05 22:47:33]

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

[commit-bot: I haz the power, 2017-06-05 22:47:34]

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_...)

[Byoungkwon Ko, 2017-06-06 03:01:09]

The CQ bit was checked by gogag2@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 03:01:38]

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, 2017-06-06 05:15:13]

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

[commit-bot: I haz the power, 2017-06-06 05:15:15]

Dry run: This issue passed the CQ dry run.

[Byoungkwon Ko, 2017-06-06 06:12:10]

On 2017/05/31 20:21:58, fserb wrote:
> On 2017/05/31 at 20:21:35, fserb wrote:
> > On 2017/05/31 at 15:00:41, gogag2 wrote:
> > > @fserb
> > > 
> > > Could you review my comment?
> > > Thanks.
> > > 
> > >
>
https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Lay...
> > > File
>
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-stringifier-expected.txt
> (right):
> > > 
> > >
>
https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Lay...
> > >
>
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-stringifier-expected.txt:55:
> FAIL WebKitCSSMatrix stringifier: identity (2d) assert_equals: expected
> "matrix(1, 0, 0, 1, 0, 0)" but got "matrix(1.000000, 0.000000, 0.000000,
> 1.000000, 0.000000, 0.000000)"
> > > On 2017/05/29 17:22:51, fserb wrote:
> > > > this should be passing. Could you please check and either update the
> WebPlatform
> > > > tests or the code?
> > > > And if all tests are passing, remove the expected file.
> > > 
> > > Yes, right. It should be passing.
> > > But I think when making WebKitCSSMatrix an alias of DOMMatrix, all of them
> should be passing.
> > > Could I fix below CL?
> > > https://codereview.chromium.org/2709763004/
> > > 
> > > If it should be passing now, I will fix it.
> > > Plz tell me : )
> > > Thanks.
> > 
> > Are you sure those are related? It seems that is expecting "matrix(1, 0,
> 0...)" and getting "matrix(1.00000, 0.0000, 0.0000...)". It doesn't seem to be
> related to me.
> > 
> > > 
> > >
>
https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp
(right):
> > > 
> > >
>
https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/geometry/DOMMatrixReadOnly.cpp:351: if
> (!std::isfinite(m11()) || !std::isfinite(m12()) || !std::isfinite(m13()) ||
> > > On 2017/05/29 17:22:51, fserb wrote:
> > > > git cl format?
> > > 
> > > Yes, I did.
> > > Is it wrong?
> > 
> > Ahn, nvm, on my monitor it was breaking between lines. It's all good :)
> 
> Please look at the test again and see what's wrong with that, otherwise it's
> good. :)

@fserb 
Hi.
I am fixing it as following your comments.
Could you review it?? :)
Thanks.

[fserb, 2017-06-06 06:52:08]

lgtm

[Byoungkwon Ko, 2017-06-06 09:06:35]

gogag2@gmail.com changed reviewers:
+ junov@chromium.org

[Byoungkwon Ko, 2017-06-06 09:06:36]

@junov

PTAL!

[zino, 2017-06-06 09:10:08]

On 2017/05/31 20:21:35, fserb wrote:
> On 2017/05/31 at 15:00:41, gogag2 wrote:
> > @fserb
> > 
> > Could you review my comment?
> > Thanks.
> > 
> >
>
https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Lay...
> > File
>
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-stringifier-expected.txt
> (right):
> > 
> >
>
https://codereview.chromium.org/2846523002/diff/180001/third_party/WebKit/Lay...
> >
>
third_party/WebKit/LayoutTests/external/wpt/css/geometry-1/DOMMatrix-stringifier-expected.txt:55:
> FAIL WebKitCSSMatrix stringifier: identity (2d) assert_equals: expected
> "matrix(1, 0, 0, 1, 0, 0)" but got "matrix(1.000000, 0.000000, 0.000000,
> 1.000000, 0.000000, 0.000000)"
> > On 2017/05/29 17:22:51, fserb wrote:
> > > this should be passing. Could you please check and either update the
> WebPlatform
> > > tests or the code?
> > > And if all tests are passing, remove the expected file.
> > 
> > Yes, right. It should be passing.
> > But I think when making WebKitCSSMatrix an alias of DOMMatrix, all of them
> should be passing.
> > Could I fix below CL?
> > https://codereview.chromium.org/2709763004/
> > 
> > If it should be passing now, I will fix it.
> > Plz tell me : )
> > Thanks.
> 
> Are you sure those are related? It seems that is expecting "matrix(1, 0,
0...)"
> and getting "matrix(1.00000, 0.0000, 0.0000...)". It doesn't seem to be
related
> to me.
> 

fserb@, Byoungkown Ko@,

I agree that the test should eventually be fixed before shipping, but do we need
to fix it in this CL?
Because this is fixing stringifier behavior for DOMMatrix not WebKitCSSMatrix.
The problem is the known issue of WebKitCSSMatrix but if we make WebKitCSSMatrix
an alias for DOMMatrix, it will be resolved.
So, I think we can just update the expected files in another CL for alias[1].
Otherwise, even if it is the same result, we might have to send another intent
to fix behavior of WebKitCSSMatrix.
We already sent a intent to ship DOMMatrix (including making an alias) but it's
not for existing WebKitCSSMatrix.
WDYT?

[1] https://codereview.chromium.org/2709763004/

[zino, 2017-06-06 09:20:08]

But if you guys don't care of it much, I'm okay because it's the same result.
foolip@ what do you think as an API owner?

https://codereview.chromium.org/2846523002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSMatrix.cpp (right):

https://codereview.chromium.org/2846523002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSMatrix.cpp:196: return
String::Format("matrix(%g, %g, %g, %g, %g, %g)", matrix_->A(),
I mean that this change should not be contained in this CL.

[Justin Novosad, 2017-06-06 15:13:29]

lgtm

[Byoungkwon Ko, 2017-06-06 15:39:49]

@All

Could I commit this CL? : )

[fserb, 2017-06-06 17:16:48]

The CQ bit was checked by fserb@chromium.org

[fserb, 2017-06-06 17:16:49]

lgtm

[fserb, 2017-06-06 17:16:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
jinho.bang@samsung.com
Link to the patchset: https://codereview.chromium.org/2846523002/#ps220001
(title: "Update the stringifier behavior for DOMMatrixReadOnly")

[commit-bot: I haz the power, 2017-06-06 17:17:14]

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, 2017-06-06 17:30:40]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496769408277090,
"parent_rev": "9cc516bf79d5adb8c2425d42b4d4baaf729fd21e", "commit_rev":
"38211a9a22a7d3ad0ed32e8250f8ae2b4b3ff94a"}

[commit-bot: I haz the power, 2017-06-06 17:30:54]

Description was changed from

==========
Update the stringifier behavior for DOMMatrixReadOnly

They are appending a much lower precision than a double. It should append the 
value of the double atrributes to the string.

Link: https://github.com/w3c/fxtf-drafts/issues/120

BUG=none
==========

to

==========
Update the stringifier behavior for DOMMatrixReadOnly

They are appending a much lower precision than a double. It should append the 
value of the double atrributes to the string.

Link: https://github.com/w3c/fxtf-drafts/issues/120

BUG=none

Review-Url: https://codereview.chromium.org/2846523002
Cr-Commit-Position: refs/heads/master@{#477328}
Committed:
https://chromium.googlesource.com/chromium/src/+/38211a9a22a7d3ad0ed32e8250f8...
==========

[commit-bot: I haz the power, 2017-06-06 17:30:56]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/38211a9a22a7d3ad0ed32e8250f8...