mash: Fix ShelfItem mojo struct; add enums and traits.

mash: Fix ShelfItem mojo struct; add enums and traits.

Make ash::mojom::ShelfItem match ash::ShelfItem.
Add required type and status enums; traits for all three.
Add a unit test for the ShelfItem struct traits.

Move ash/common/shelf/shelf_item_types.h to ash/public/cpp/shelf_item.h
Define ShelfID as int64_t for cross-platform and mojo type clarity.
Header and type usage cleanup.

BUG=557406
TEST=NONE; no functional behavior changes.
R=jamescook@chromium.org,tsepez@chromium.org
TBR=sky@chromium.org,reed@google.com

Review-Url: https://codereview.chromium.org/2750463009
Cr-Commit-Position: refs/heads/master@{#457704}
Committed: https://chromium.googlesource.com/chromium/src/+/127e697b3d265b427a83a9e82cb5fcc2ef12d395

[msw, 2017-03-16 00:21:39]

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

[commit-bot: I haz the power, 2017-03-16 00:22:33]

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

[msw, 2017-03-16 00:24:37]

Description was changed from

==========
mash: Fix ShelfItem mojo struct; add enums and traits.

BUG=557406
==========

to

==========
mash: Fix ShelfItem mojo struct; add enums and traits.

Make ash::mojom::ShelfItem match ash::ShelfItem
Add required type and status enums; traits for all three.

Move ash/common/shelf/shelf_item_types.h to ash/public/cpp/shelf_item.h
Define ShelfID as int64_t for cross-platform and mojo type clarity.
Header and type usage cleanup.

BUG=557406
TEST=NONE; no functional behavior changes.
R=jamescook@chromium.org
==========

[msw, 2017-03-16 00:24:38]

msw@chromium.org changed reviewers:
+ jamescook@chromium.org

[commit-bot: I haz the power, 2017-03-16 01:33:14]

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

[commit-bot: I haz the power, 2017-03-16 01:33:15]

Dry run: This issue passed the CQ dry run.

[msw, 2017-03-16 01:43:14]

Description was changed from

==========
mash: Fix ShelfItem mojo struct; add enums and traits.

Make ash::mojom::ShelfItem match ash::ShelfItem
Add required type and status enums; traits for all three.

Move ash/common/shelf/shelf_item_types.h to ash/public/cpp/shelf_item.h
Define ShelfID as int64_t for cross-platform and mojo type clarity.
Header and type usage cleanup.

BUG=557406
TEST=NONE; no functional behavior changes.
R=jamescook@chromium.org
==========

to

==========
mash: Fix ShelfItem mojo struct; add enums and traits.

Make ash::mojom::ShelfItem match ash::ShelfItem
Add required type and status enums; traits for all three.

Move ash/common/shelf/shelf_item_types.h to ash/public/cpp/shelf_item.h
Define ShelfID as int64_t for cross-platform and mojo type clarity.
Header and type usage cleanup.

BUG=557406
TEST=NONE; no functional behavior changes.
R=jamescook@chromium.org,tsepez@chromium.org
==========

[msw, 2017-03-16 01:43:15]

msw@chromium.org changed reviewers:
+ tsepez@chromium.org

[msw, 2017-03-16 01:44:16]

Hey James, please take a look; thanks!
Hey Tom, please review ash/public/interfaces/ for OWNERS; thanks!

[James Cook, 2017-03-16 16:48:51]

I like it.

https://codereview.chromium.org/2750463009/diff/20001/ash/public/cpp/BUILD.gn
File ash/public/cpp/BUILD.gn (right):

https://codereview.chromium.org/2750463009/diff/20001/ash/public/cpp/BUILD.gn...
ash/public/cpp/BUILD.gn:29: "//ui/gfx",
Should this be public_deps, since if a client wants to use ShelfItem they must
include gfx for skia?

https://codereview.chromium.org/2750463009/diff/20001/ash/public/cpp/shelf_it...
File ash/public/cpp/shelf_item.h (left):

https://codereview.chromium.org/2750463009/diff/20001/ash/public/cpp/shelf_it...
ash/public/cpp/shelf_item.h:8: // TODO(msw): Rename these files to shelf_item.*;
audit users.
TODONE! :-)

https://codereview.chromium.org/2750463009/diff/20001/ash/public/cpp/shelf_ty...
File ash/public/cpp/shelf_types.h (right):

https://codereview.chromium.org/2750463009/diff/20001/ash/public/cpp/shelf_ty...
ash/public/cpp/shelf_types.h:92: typedef int64_t ShelfID;
nit: using ShelfID = int64_t

Also, are there any useful docs you can add for shelf id, like are they
sequential, are they persistent across restarts of chrome, can they be safely
written to disk, etc.?

Also also, is there a particular motivation to make these int64? Just so they
don't get confused with regular int? If you want to prevent int/shelfid
confusion I wonder if making the ID type unsigned would help generate compiler
warnings. I can't remember if we disable those everywhere anymore.

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
File ash/public/interfaces/shelf.mojom (right):

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/shelf.mojom:129: // the user cannot un-pin these items.
Thanks for documenting all the stuff in this file.

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
File ash/public/interfaces/shelf_struct_traits.cc (right):

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/shelf_struct_traits.cc:14: ash::ShelfItemType
StructTraits<ash::mojom::ShelfItemDataView,
optional: Is it possible to do "using ash::mojom::ShelfItemDataView" to improve
line wrapping in this file?

also optional: That might give you enough space to change "i" to "item"

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/shelf_struct_traits.cc:77: out->image =
gfx::ImageSkia::CreateFrom1xBitmap(image);
This needs a TODO or bug -- I think we support 2x bitmaps for high DPI icons. If
we do, and we convert to mojo for classic ash, we might break that.

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
File ash/public/interfaces/shelf_struct_traits.h (right):

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/shelf_struct_traits.h:262: static bool
Read(ash::mojom::ShelfItemDataView data, ash::ShelfItem* out);
optional super nit: blank line above?

[Tom Sepez, 2017-03-16 17:59:26]

Otherwise LGTM

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
File ash/public/interfaces/shelf_struct_traits.cc (right):

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/shelf_struct_traits.cc:77: out->image =
gfx::ImageSkia::CreateFrom1xBitmap(image);
Can this fail?

[msw, 2017-03-16 21:22:26]

Description was changed from

==========
mash: Fix ShelfItem mojo struct; add enums and traits.

Make ash::mojom::ShelfItem match ash::ShelfItem
Add required type and status enums; traits for all three.

Move ash/common/shelf/shelf_item_types.h to ash/public/cpp/shelf_item.h
Define ShelfID as int64_t for cross-platform and mojo type clarity.
Header and type usage cleanup.

BUG=557406
TEST=NONE; no functional behavior changes.
R=jamescook@chromium.org,tsepez@chromium.org
==========

to

==========
mash: Fix ShelfItem mojo struct; add enums and traits.

Make ash::mojom::ShelfItem match ash::ShelfItem.
Add required type and status enums; traits for all three.
Add a unit test for the ShelfItem struct traits.

Move ash/common/shelf/shelf_item_types.h to ash/public/cpp/shelf_item.h
Define ShelfID as int64_t for cross-platform and mojo type clarity.
Header and type usage cleanup.

BUG=557406
TEST=NONE; no functional behavior changes.
R=jamescook@chromium.org,tsepez@chromium.org
==========

[msw, 2017-03-16 22:43:58]

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

[commit-bot: I haz the power, 2017-03-16 22:44:35]

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

[msw, 2017-03-16 23:15:50]

Please take another look; thanks! I addressed comments, added a unit test, moved
struct_traits files to ash/public/cpp (Scott's suggestion), and have a
workaround for crbug.com/702397 (a struct traits export/deps issue).

https://codereview.chromium.org/2750463009/diff/20001/ash/public/cpp/BUILD.gn
File ash/public/cpp/BUILD.gn (right):

https://codereview.chromium.org/2750463009/diff/20001/ash/public/cpp/BUILD.gn...
ash/public/cpp/BUILD.gn:29: "//ui/gfx",
On 2017/03/16 16:48:50, James Cook wrote:
> Should this be public_deps, since if a client wants to use ShelfItem they must
> include gfx for skia?

Hmm, I guess so, and ditto for base for string16?. I wonder if it ever makes
sense to instead require that clients include the same dependency when it's only
needed by a small part of a component target. (that would alleviate users of
window_properties from needing to link against //ui/gfx, right?) I guess I could
make a separate shelf target here if that really became a burden.

https://codereview.chromium.org/2750463009/diff/20001/ash/public/cpp/shelf_it...
File ash/public/cpp/shelf_item.h (left):

https://codereview.chromium.org/2750463009/diff/20001/ash/public/cpp/shelf_it...
ash/public/cpp/shelf_item.h:8: // TODO(msw): Rename these files to shelf_item.*;
audit users.
On 2017/03/16 16:48:50, James Cook wrote:
> TODONE! :-)

:)

https://codereview.chromium.org/2750463009/diff/20001/ash/public/cpp/shelf_ty...
File ash/public/cpp/shelf_types.h (right):

https://codereview.chromium.org/2750463009/diff/20001/ash/public/cpp/shelf_ty...
ash/public/cpp/shelf_types.h:92: typedef int64_t ShelfID;
On 2017/03/16 16:48:50, James Cook wrote:
> nit: using ShelfID = int64_t

Done.
 
> Also, are there any useful docs you can add for shelf id, like are they
> sequential, are they persistent across restarts of chrome, can they be safely
> written to disk, etc.?

Added a comment with some answers.

> Also also, is there a particular motivation to make these int64? Just so they
> don't get confused with regular int? If you want to prevent int/shelfid
> confusion I wonder if making the ID type unsigned would help generate compiler
> warnings. I can't remember if we disable those everywhere anymore.

My motivation was to pick a concrete type that would match mojo, which doesn't
handle 32-/64-bit int type size inference from the platform. They are only
non-negative and generally small (# of shelf items), so I changed this to
uint32_t ... I might need to pair them with a client id (similar to mus windows)
later on.

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
File ash/public/interfaces/shelf.mojom (right):

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/shelf.mojom:129: // the user cannot un-pin these items.
On 2017/03/16 16:48:50, James Cook wrote:
> Thanks for documenting all the stuff in this file.

Acknowledged.

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
File ash/public/interfaces/shelf_struct_traits.cc (right):

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/shelf_struct_traits.cc:14: ash::ShelfItemType
StructTraits<ash::mojom::ShelfItemDataView,
On 2017/03/16 16:48:50, James Cook wrote:
> optional: Is it possible to do "using ash::mojom::ShelfItemDataView" to
improve
> line wrapping in this file?
> 
> also optional: That might give you enough space to change "i" to "item"

I defined these inline for efficiency gains, at Yuzhu's suggestion.

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/shelf_struct_traits.cc:77: out->image =
gfx::ImageSkia::CreateFrom1xBitmap(image);
On 2017/03/16 16:48:50, James Cook wrote:
> This needs a TODO or bug -- I think we support 2x bitmaps for high DPI icons.
If
> we do, and we convert to mojo for classic ash, we might break that.

Done.

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/shelf_struct_traits.cc:77: out->image =
gfx::ImageSkia::CreateFrom1xBitmap(image);
On 2017/03/16 17:59:25, Tom Sepez wrote:
> Can this fail?

It seems the returned ImageSkia instance is always valid but may be isNull(). It
still seems okay to return true in that case; it's not technically a requirement
that a ShelfItem have a non-empty image icon.

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
File ash/public/interfaces/shelf_struct_traits.h (right):

https://codereview.chromium.org/2750463009/diff/20001/ash/public/interfaces/s...
ash/public/interfaces/shelf_struct_traits.h:262: static bool
Read(ash::mojom::ShelfItemDataView data, ash::ShelfItem* out);
On 2017/03/16 16:48:51, James Cook wrote:
> optional super nit: blank line above?

Done.

[commit-bot: I haz the power, 2017-03-17 00:57:21]

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

[commit-bot: I haz the power, 2017-03-17 00:57:22]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-03-17 02:32:29]

LGTM with one question/nit

https://codereview.chromium.org/2750463009/diff/60001/ash/public/cpp/shelf_st...
File ash/public/cpp/shelf_struct_traits.h (right):

https://codereview.chromium.org/2750463009/diff/60001/ash/public/cpp/shelf_st...
ash/public/cpp/shelf_struct_traits.h:259: static int64_t shelf_id(const
ash::ShelfItem& i) { return i.id; }
Wait, should this be uint32_t ? Or ShelfID?

https://codereview.chromium.org/2750463009/diff/60001/ash/public/cpp/shelf_st...
File ash/public/cpp/shelf_struct_traits_unittest.cc (right):

https://codereview.chromium.org/2750463009/diff/60001/ash/public/cpp/shelf_st...
ash/public/cpp/shelf_struct_traits_unittest.cc:44: }  // namespace
Thanks for adding a test, and bonus points for anonymous namespace.

[msw, 2017-03-17 05:35:20]

Comments addressed; landing.

https://codereview.chromium.org/2750463009/diff/60001/ash/public/cpp/shelf_st...
File ash/public/cpp/shelf_struct_traits.h (right):

https://codereview.chromium.org/2750463009/diff/60001/ash/public/cpp/shelf_st...
ash/public/cpp/shelf_struct_traits.h:259: static int64_t shelf_id(const
ash::ShelfItem& i) { return i.id; }
On 2017/03/17 02:32:29, James Cook wrote:
> Wait, should this be uint32_t ? Or ShelfID?

Good catch! Changed to ShelfID.

https://codereview.chromium.org/2750463009/diff/60001/ash/public/cpp/shelf_st...
File ash/public/cpp/shelf_struct_traits_unittest.cc (right):

https://codereview.chromium.org/2750463009/diff/60001/ash/public/cpp/shelf_st...
ash/public/cpp/shelf_struct_traits_unittest.cc:44: }  // namespace
On 2017/03/17 02:32:29, James Cook wrote:
> Thanks for adding a test, and bonus points for anonymous namespace.

Acknowledged.

[msw, 2017-03-17 05:35:26]

The CQ bit was checked by msw@chromium.org

[msw, 2017-03-17 05:35:26]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2750463009/#ps80001
(title: "Address comment.")

[commit-bot: I haz the power, 2017-03-17 05:35:47]

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-03-17 05:49:42]

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

[commit-bot: I haz the power, 2017-03-17 05:49:43]

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

[msw, 2017-03-17 06:42:07]

Description was changed from

==========
mash: Fix ShelfItem mojo struct; add enums and traits.

Make ash::mojom::ShelfItem match ash::ShelfItem.
Add required type and status enums; traits for all three.
Add a unit test for the ShelfItem struct traits.

Move ash/common/shelf/shelf_item_types.h to ash/public/cpp/shelf_item.h
Define ShelfID as int64_t for cross-platform and mojo type clarity.
Header and type usage cleanup.

BUG=557406
TEST=NONE; no functional behavior changes.
R=jamescook@chromium.org,tsepez@chromium.org
==========

to

==========
mash: Fix ShelfItem mojo struct; add enums and traits.

Make ash::mojom::ShelfItem match ash::ShelfItem.
Add required type and status enums; traits for all three.
Add a unit test for the ShelfItem struct traits.

Move ash/common/shelf/shelf_item_types.h to ash/public/cpp/shelf_item.h
Define ShelfID as int64_t for cross-platform and mojo type clarity.
Header and type usage cleanup.

BUG=557406
TEST=NONE; no functional behavior changes.
R=jamescook@chromium.org,tsepez@chromium.org,jam@chromium.org
==========

[msw, 2017-03-17 06:42:07]

msw@chromium.org changed reviewers:
+ jam@chromium.org

[msw, 2017-03-17 06:46:45]

Description was changed from

==========
mash: Fix ShelfItem mojo struct; add enums and traits.

Make ash::mojom::ShelfItem match ash::ShelfItem.
Add required type and status enums; traits for all three.
Add a unit test for the ShelfItem struct traits.

Move ash/common/shelf/shelf_item_types.h to ash/public/cpp/shelf_item.h
Define ShelfID as int64_t for cross-platform and mojo type clarity.
Header and type usage cleanup.

BUG=557406
TEST=NONE; no functional behavior changes.
R=jamescook@chromium.org,tsepez@chromium.org,jam@chromium.org
==========

to

==========
mash: Fix ShelfItem mojo struct; add enums and traits.

Make ash::mojom::ShelfItem match ash::ShelfItem.
Add required type and status enums; traits for all three.
Add a unit test for the ShelfItem struct traits.

Move ash/common/shelf/shelf_item_types.h to ash/public/cpp/shelf_item.h
Define ShelfID as int64_t for cross-platform and mojo type clarity.
Header and type usage cleanup.

BUG=557406
TEST=NONE; no functional behavior changes.
R=jamescook@chromium.org,tsepez@chromium.org
TBR=sky@chromium.org,reed@google.com
==========

[msw, 2017-03-17 06:46:45]

msw@chromium.org changed reviewers:
+ reed@google.com, sky@chromium.org
- jam@chromium.org

[msw, 2017-03-17 06:47:49]

+TBR owners for 'new' ash/public/cpp/DEPS:
Scott: +mojo/common
Mike: +skia/public/interfaces

[msw, 2017-03-17 06:47:57]

The CQ bit was checked by msw@chromium.org

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

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-03-17 06:53:43]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489733277088780,
"parent_rev": "204011460e76cb45440a4301e72f589df200257e", "commit_rev":
"127e697b3d265b427a83a9e82cb5fcc2ef12d395"}

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

Description was changed from

==========
mash: Fix ShelfItem mojo struct; add enums and traits.

Make ash::mojom::ShelfItem match ash::ShelfItem.
Add required type and status enums; traits for all three.
Add a unit test for the ShelfItem struct traits.

Move ash/common/shelf/shelf_item_types.h to ash/public/cpp/shelf_item.h
Define ShelfID as int64_t for cross-platform and mojo type clarity.
Header and type usage cleanup.

BUG=557406
TEST=NONE; no functional behavior changes.
R=jamescook@chromium.org,tsepez@chromium.org
TBR=sky@chromium.org,reed@google.com
==========

to

==========
mash: Fix ShelfItem mojo struct; add enums and traits.

Make ash::mojom::ShelfItem match ash::ShelfItem.
Add required type and status enums; traits for all three.
Add a unit test for the ShelfItem struct traits.

Move ash/common/shelf/shelf_item_types.h to ash/public/cpp/shelf_item.h
Define ShelfID as int64_t for cross-platform and mojo type clarity.
Header and type usage cleanup.

BUG=557406
TEST=NONE; no functional behavior changes.
R=jamescook@chromium.org,tsepez@chromium.org
TBR=sky@chromium.org,reed@google.com

Review-Url: https://codereview.chromium.org/2750463009
Cr-Commit-Position: refs/heads/master@{#457704}
Committed:
https://chromium.googlesource.com/chromium/src/+/127e697b3d265b427a83a9e82cb5...
==========

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

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/127e697b3d265b427a83a9e82cb5...

[sky, 2017-03-17 15:16:07]

deps LGTM