Implement gfx::Transform StructTraits unit test

Implement gfx::Transform StructTraits unit test

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[Fady Samuel, 2016-06-01 16:06:53]

Description was changed from

==========
Implement gfx::Transform StructTraits unit test

BUG=611802
==========

to

==========
Implement gfx::Transform StructTraits unit test

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-06-01 16:09:48]

fsamuel@chromium.org changed reviewers:
+ sadrul@chromium.org, tsepez@chromium.org

[Fady Samuel, 2016-06-01 16:09:49]

+sadrul@ for ui/
+tsepez for ipc

[sadrul, 2016-06-01 16:17:38]

https://codereview.chromium.org/2026333002/diff/20001/ui/gfx/mojo/traits_test...
File ui/gfx/mojo/traits_test_service.mojom (right):

https://codereview.chromium.org/2026333002/diff/20001/ui/gfx/mojo/traits_test...
ui/gfx/mojo/traits_test_service.mojom:9: interface TraitsTestService {
I think if you make this [Sync], then you don't need the lambda/capture?

[Fady Samuel, 2016-06-01 16:20:19]

PTAL

https://codereview.chromium.org/2026333002/diff/20001/ui/gfx/mojo/traits_test...
File ui/gfx/mojo/traits_test_service.mojom (right):

https://codereview.chromium.org/2026333002/diff/20001/ui/gfx/mojo/traits_test...
ui/gfx/mojo/traits_test_service.mojom:9: interface TraitsTestService {
On 2016/06/01 16:17:38, sadrul wrote:
> I think if you make this [Sync], then you don't need the lambda/capture?

Making it sync on one thread would deadlock I think.

[yzshen1, 2016-06-01 16:22:23]

yzshen@chromium.org changed reviewers:
+ yzshen@chromium.org

[yzshen1, 2016-06-01 16:22:24]

https://codereview.chromium.org/2026333002/diff/20001/ui/gfx/mojo/traits_test...
File ui/gfx/mojo/traits_test_service.mojom (right):

https://codereview.chromium.org/2026333002/diff/20001/ui/gfx/mojo/traits_test...
ui/gfx/mojo/traits_test_service.mojom:9: interface TraitsTestService {
On 2016/06/01 16:20:19, Fady Samuel wrote:
> On 2016/06/01 16:17:38, sadrul wrote:
> > I think if you make this [Sync], then you don't need the lambda/capture?
> 
> Making it sync on one thread would deadlock I think.

If you mean the sender and the receiver on the same thread, it won't deadlock.

Sync services will be able to reenter any ongoing sync calls on the same thread.

[Fady Samuel, 2016-06-01 16:32:32]

PTAL

https://codereview.chromium.org/2026333002/diff/20001/ui/gfx/mojo/traits_test...
File ui/gfx/mojo/traits_test_service.mojom (right):

https://codereview.chromium.org/2026333002/diff/20001/ui/gfx/mojo/traits_test...
ui/gfx/mojo/traits_test_service.mojom:9: interface TraitsTestService {
On 2016/06/01 16:22:24, yzshen1 wrote:
> On 2016/06/01 16:20:19, Fady Samuel wrote:
> > On 2016/06/01 16:17:38, sadrul wrote:
> > > I think if you make this [Sync], then you don't need the lambda/capture?
> > 
> > Making it sync on one thread would deadlock I think.
> 
> If you mean the sender and the receiver on the same thread, it won't deadlock.
> 
> Sync services will be able to reenter any ongoing sync calls on the same
thread.

Cool! I just tried it and it works and simplifies the unit test! w00t! I should
go back and update the other unit tests at some point.

[sadrul, 2016-06-01 16:39:14]

lgtm

https://codereview.chromium.org/2026333002/diff/60001/ui/gfx/mojo/struct_trai...
File ui/gfx/mojo/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2026333002/diff/60001/ui/gfx/mojo/struct_trai...
ui/gfx/mojo/struct_traits_unittest.cc:76: EXPECT_EQ(col4row4,
output.matrix().get(3, 3));
Maybe EXPECT_EQ(input.matrix(), output.matrix())?

[Fady Samuel, 2016-06-01 16:46:36]

https://codereview.chromium.org/2026333002/diff/60001/ui/gfx/mojo/struct_trai...
File ui/gfx/mojo/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2026333002/diff/60001/ui/gfx/mojo/struct_trai...
ui/gfx/mojo/struct_traits_unittest.cc:76: EXPECT_EQ(col4row4,
output.matrix().get(3, 3));
On 2016/06/01 16:39:14, sadrul wrote:
> Maybe EXPECT_EQ(input.matrix(), output.matrix())?

+tsepez@: How do you feel about this change? In theory a bad StructTraits could
modify input, so there is value here although it's very verbose.

[Fady Samuel, 2016-06-01 17:15:05]

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

[commit-bot: I haz the power, 2016-06-01 17:15:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2026333002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2026333002/60001

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

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

[commit-bot: I haz the power, 2016-06-01 17:23:07]

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

[Tom Sepez, 2016-06-01 17:46:19]

lgtm

https://codereview.chromium.org/2026333002/diff/60001/ui/gfx/mojo/struct_trai...
File ui/gfx/mojo/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2026333002/diff/60001/ui/gfx/mojo/struct_trai...
ui/gfx/mojo/struct_traits_unittest.cc:76: EXPECT_EQ(col4row4,
output.matrix().get(3, 3));
On 2016/06/01 16:46:36, Fady Samuel wrote:
> On 2016/06/01 16:39:14, sadrul wrote:
> > Maybe EXPECT_EQ(input.matrix(), output.matrix())?
> 
> +tsepez@: How do you feel about this change? In theory a bad StructTraits
could
> modify input, so there is value here although it's very verbose.

I'm happier the way it currently is.

[Fady Samuel, 2016-06-01 19:59:28]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-01 19:59:29]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2026333002/#ps80001
(title: "Rebased + Fixed GYP")

[commit-bot: I haz the power, 2016-06-01 20:00:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2026333002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2026333002/80001

[commit-bot: I haz the power, 2016-06-01 20:12:36]

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

[commit-bot: I haz the power, 2016-06-01 20:12:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[Fady Samuel, 2016-06-01 20:38:29]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-01 20:38:29]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2026333002/#ps100001
(title: "nogncheck")

[commit-bot: I haz the power, 2016-06-01 20:39:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2026333002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2026333002/100001

[commit-bot: I haz the power, 2016-06-01 20:46:50]

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

[commit-bot: I haz the power, 2016-06-01 20:46:52]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yzshen1, 2016-06-01 20:53:20]

On 2016/06/01 20:46:52, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

mojo DEPS LGTM

[Fady Samuel, 2016-06-01 21:08:16]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-06-01 21:08:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2026333002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2026333002/100001

[Fady Samuel, 2016-06-01 23:10:43]

The CQ bit was unchecked by fsamuel@chromium.org

[Fady Samuel, 2016-06-01 23:10:44]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-06-01 23:11:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2026333002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2026333002/100001

[commit-bot: I haz the power, 2016-06-02 00:48:32]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-02 00:50:00]

Description was changed from

==========
Implement gfx::Transform StructTraits unit test

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement gfx::Transform StructTraits unit test

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2016-06-02 00:50:02]

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