arc: Support more types of notifications.

arc: Support more types of notifications.

These patches add a support of InboxStyle and BigPictureStyle
notifications.

Related CLs:
- http://crrev.com/1883473002 (Chromium side change, this patch)
- ag/923744 (mojom change)
- ag/923745 (Android side change)

Bug=b/28027339

Committed: https://crrev.com/4e040e320c7a55f4d72e406e2f78bbcb5cb5ccc1
Cr-Commit-Position: refs/heads/master@{#391275}

[yoshiki, 2016-04-12 06:22:38]

Patchset #2 (id:20001) has been deleted

[yoshiki, 2016-04-12 06:22:49]

Patchset #1 (id:1) has been deleted

[yoshiki, 2016-04-12 06:29:39]

Description was changed from

==========
wip

BUG=
==========

to

==========
arc: Support more types of notifications.

These patches add a support of InboxStyle and BigPictureStyle
notifications.

Related CLs:
- http://crrev.com/1883473002 (Chromium side change, this patch)
- ag/923744 (mojom change)
- ag/923745 (Android side change)

Bug=b/28027339
==========

[yoshiki, 2016-04-12 06:29:39]

yoshiki@chromium.org changed reviewers:
+ hidehiko@chromium.org

[yoshiki, 2016-04-12 06:33:24]

yoshiki@chromium.org changed reviewers:
+ dcheng@chromium.org

[yoshiki, 2016-04-12 06:34:00]

dcheng@, PTAL at mojom change?
hidehiko@, PTAL at arc notification change?

Thanks.

[hidehiko, 2016-04-13 21:12:34]

On 2016/04/12 06:34:00, yoshiki wrote:
> dcheng@, PTAL at mojom change?
> hidehiko@, PTAL at arc notification change?
> 
> Thanks.

[hidehiko, 2016-04-13 21:13:16]

On 2016/04/13 21:12:34, hidehiko wrote:
> On 2016/04/12 06:34:00, yoshiki wrote:
> > dcheng@, PTAL at mojom change?
> > hidehiko@, PTAL at arc notification change?
> > 
> > Thanks.

Sorry, mistake...

The bot failure looks real one.
Could you fix it first?

[dcheng, 2016-04-13 23:55:20]

https://codereview.chromium.org/1883473002/diff/40001/components/arc/common/n...
File components/arc/common/notifications.mojom (right):

https://codereview.chromium.org/1883473002/diff/40001/components/arc/common/n...
components/arc/common/notifications.mojom:28: MAX = LIST
I don't think MAX serves a purpose in mojo

https://codereview.chromium.org/1883473002/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_item.cc (right):

https://codereview.chromium.org/1883473002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_item.cc:46: const
mojo::StructPtr<ArcNotificationBitmap>& arcBitmap) {
1) This seems like it should be a type converter or struct traits.
2) Is it possible to use SkBitmap on both ends? I'm guessing not, but figured
I'd ask.
3) Transporting SkBitmaps through Mojo seems like a thing we'll want generally,
so I'm not sure this is the best layer to add it.

Also, this should be called arc_bitmap.

https://codereview.chromium.org/1883473002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_item.cc:55: &arcBitmap->pixel_data[0],
(arcBitmap->width * 4))) {
This doesn't work if pixel_data has no data.

mojo::Array should probably just expose mojo::Array::data(), to mirror
std::vector::data, which works even if the vector is empty.

[yoshiki, 2016-04-14 14:09:23]

Patchset #2 (id:60001) has been deleted

[yoshiki, 2016-04-15 00:32:34]

hidehiko@, dcheng@, PTAL.

https://codereview.chromium.org/1883473002/diff/40001/components/arc/common/n...
File components/arc/common/notifications.mojom (right):

https://codereview.chromium.org/1883473002/diff/40001/components/arc/common/n...
components/arc/common/notifications.mojom:28: MAX = LIST
On 2016/04/13 23:55:20, dcheng wrote:
> I don't think MAX serves a purpose in mojo

Done.

https://codereview.chromium.org/1883473002/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_item.cc (right):

https://codereview.chromium.org/1883473002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_item.cc:46: const
mojo::StructPtr<ArcNotificationBitmap>& arcBitmap) {
On 2016/04/13 23:55:20, dcheng wrote:
> 1) This seems like it should be a type converter or struct traits.

Thanks. I made it a type converter.

> 2) Is it possible to use SkBitmap on both ends? I'm guessing not, but figured
> I'd ask.

We can't. Android side is Java. We use android.graphics.Bitmap in Java side.

> 3) Transporting SkBitmaps through Mojo seems like a thing we'll want
generally,
> so I'm not sure this is the best layer to add it.
>
> Also, this should be called arc_bitmap.

I extracted this to component/arc.

https://codereview.chromium.org/1883473002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_item.cc:55: &arcBitmap->pixel_data[0],
(arcBitmap->width * 4))) {
On 2016/04/13 23:55:20, dcheng wrote:
> This doesn't work if pixel_data has no data.
> 
> mojo::Array should probably just expose mojo::Array::data(), to mirror
> std::vector::data, which works even if the vector is empty.

Done.

[hidehiko, 2016-04-15 07:09:10]

https://codereview.chromium.org/1883473002/diff/100001/components/arc/BUILD.gn
File components/arc/BUILD.gn (right):

https://codereview.chromium.org/1883473002/diff/100001/components/arc/BUILD.g...
components/arc/BUILD.gn:67: "//skia",
Is this needs to be in .gyp, too?

https://codereview.chromium.org/1883473002/diff/100001/components/arc/bitmap/...
File components/arc/bitmap/bitmap_type_converters.cc (right):

https://codereview.chromium.org/1883473002/diff/100001/components/arc/bitmap/...
components/arc/bitmap/bitmap_type_converters.cc:14: // Creates the SkBitmap
object which wraps the arc bitmap pixels.
nit: s/Creates/Create/

https://codereview.chromium.org/1883473002/diff/100001/components/arc/bitmap/...
components/arc/bitmap/bitmap_type_converters.cc:20: (arcBitmap->width * 4))) {
nit: no paren is needed?

https://codereview.chromium.org/1883473002/diff/100001/components/arc/bitmap/...
components/arc/bitmap/bitmap_type_converters.cc:29: return nativeColorBitmap;
How about adding std::move explicitly?
I guess NRVO will probably work, though.

https://codereview.chromium.org/1883473002/diff/100001/components/arc/common/...
File components/arc/common/bitmap.mojom (right):

https://codereview.chromium.org/1883473002/diff/100001/components/arc/common/...
components/arc/common/bitmap.mojom:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
2016

https://codereview.chromium.org/1883473002/diff/100001/ui/arc/notification/ar...
File ui/arc/notification/arc_notification_item.cc (right):

https://codereview.chromium.org/1883473002/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_notification_item.cc:46: SkBitmap CropImage(const
SkBitmap& original_bitmap) {
Any function comments? How to be cropped?

[yoshiki, 2016-04-15 14:36:39]

PTAL. Thanks.

https://codereview.chromium.org/1883473002/diff/100001/components/arc/BUILD.gn
File components/arc/BUILD.gn (right):

https://codereview.chromium.org/1883473002/diff/100001/components/arc/BUILD.g...
components/arc/BUILD.gn:67: "//skia",
On 2016/04/15 07:09:09, hidehiko wrote:
> Is this needs to be in .gyp, too?

In .gyp, there is the reference to third_party/skia.

https://codereview.chromium.org/1883473002/diff/100001/components/arc/bitmap/...
File components/arc/bitmap/bitmap_type_converters.cc (right):

https://codereview.chromium.org/1883473002/diff/100001/components/arc/bitmap/...
components/arc/bitmap/bitmap_type_converters.cc:14: // Creates the SkBitmap
object which wraps the arc bitmap pixels.
On 2016/04/15 07:09:10, hidehiko wrote:
> nit: s/Creates/Create/

Done.

https://codereview.chromium.org/1883473002/diff/100001/components/arc/bitmap/...
components/arc/bitmap/bitmap_type_converters.cc:20: (arcBitmap->width * 4))) {
On 2016/04/15 07:09:10, hidehiko wrote:
> nit: no paren is needed?

Done.

https://codereview.chromium.org/1883473002/diff/100001/components/arc/bitmap/...
components/arc/bitmap/bitmap_type_converters.cc:29: return nativeColorBitmap;
On 2016/04/15 07:09:10, hidehiko wrote:
> How about adding std::move explicitly?
> I guess NRVO will probably work, though.

I think NRVO should be working here and we don't need to use move semantics
explicitly.

https://codereview.chromium.org/1883473002/diff/100001/components/arc/common/...
File components/arc/common/bitmap.mojom (right):

https://codereview.chromium.org/1883473002/diff/100001/components/arc/common/...
components/arc/common/bitmap.mojom:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
On 2016/04/15 07:09:10, hidehiko wrote:
> 2016

Done.

https://codereview.chromium.org/1883473002/diff/100001/ui/arc/notification/ar...
File ui/arc/notification/arc_notification_item.cc (right):

https://codereview.chromium.org/1883473002/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_notification_item.cc:46: SkBitmap CropImage(const
SkBitmap& original_bitmap) {
On 2016/04/15 07:09:10, hidehiko wrote:
> Any function comments? How to be cropped?

Added comment.

[yoshiki, 2016-04-18 17:41:54]

Hidehiko-san, ping.

[hidehiko, 2016-04-18 17:54:41]

Oops, sorry for delay.

https://codereview.chromium.org/1883473002/diff/100001/components/arc/BUILD.gn
File components/arc/BUILD.gn (right):

https://codereview.chromium.org/1883473002/diff/100001/components/arc/BUILD.g...
components/arc/BUILD.gn:67: "//skia",
On 2016/04/15 14:36:38, yoshiki wrote:
> On 2016/04/15 07:09:09, hidehiko wrote:
> > Is this needs to be in .gyp, too?
> 
> In .gyp, there is the reference to third_party/skia. 

? Looks no entry in arc.gyp?

https://codereview.chromium.org/1883473002/diff/120001/ui/arc/notification/ar...
File ui/arc/notification/arc_notification_item.cc (right):

https://codereview.chromium.org/1883473002/diff/120001/ui/arc/notification/ar...
ui/arc/notification/arc_notification_item.cc:46: // Crop the image to proper
size for Chrome Notification. It accepts only
s/Crop/Crops/

[yoshiki, 2016-04-18 19:21:23]

Hidehiko-san, PTAL

https://codereview.chromium.org/1883473002/diff/100001/components/arc/BUILD.gn
File components/arc/BUILD.gn (right):

https://codereview.chromium.org/1883473002/diff/100001/components/arc/BUILD.g...
components/arc/BUILD.gn:67: "//skia",
On 2016/04/18 17:54:41, hidehiko wrote:
> On 2016/04/15 14:36:38, yoshiki wrote:
> > On 2016/04/15 07:09:09, hidehiko wrote:
> > > Is this needs to be in .gyp, too?
> > 
> > In .gyp, there is the reference to third_party/skia. 
> 
> ? Looks no entry in arc.gyp?

Sorry, I was confused with the other working patch. Added.

https://codereview.chromium.org/1883473002/diff/120001/ui/arc/notification/ar...
File ui/arc/notification/arc_notification_item.cc (right):

https://codereview.chromium.org/1883473002/diff/120001/ui/arc/notification/ar...
ui/arc/notification/arc_notification_item.cc:46: // Crop the image to proper
size for Chrome Notification. It accepts only
On 2016/04/18 17:54:41, hidehiko wrote:
> s/Crop/Crops/

Done.

[hidehiko, 2016-04-18 19:24:44]

https://codereview.chromium.org/1883473002/diff/160001/ui/arc/notification/ar...
File ui/arc/notification/arc_notification_item.cc (right):

https://codereview.chromium.org/1883473002/diff/160001/ui/arc/notification/ar...
ui/arc/notification/arc_notification_item.cc:162: case
ArcNotificationType::LIST:
Please add "mojom::"

https://codereview.chromium.org/1883473002/diff/160001/ui/arc/notification/ar...
ui/arc/notification/arc_notification_item.cc:187: case
ArcNotificationType::IMAGE:
Ditto.

[yoshiki, 2016-04-18 19:45:35]

Patchset #7 (id:180001) has been deleted

[yoshiki, 2016-04-18 19:46:35]

Hidehiko-san, PTAL. Thanks.

https://codereview.chromium.org/1883473002/diff/160001/ui/arc/notification/ar...
File ui/arc/notification/arc_notification_item.cc (right):

https://codereview.chromium.org/1883473002/diff/160001/ui/arc/notification/ar...
ui/arc/notification/arc_notification_item.cc:162: case
ArcNotificationType::LIST:
On 2016/04/18 19:24:44, hidehiko wrote:
> Please add "mojom::"

Done.

https://codereview.chromium.org/1883473002/diff/160001/ui/arc/notification/ar...
ui/arc/notification/arc_notification_item.cc:187: case
ArcNotificationType::IMAGE:
On 2016/04/18 19:24:44, hidehiko wrote:
> Ditto.

Done.

[hidehiko, 2016-04-18 21:30:49]

components/arc LGTM.

[yoshiki, 2016-04-19 02:33:04]

yoshiki@chromium.org changed reviewers:
+ dewittj@chromium.org, reed@chromium.org

[yoshiki, 2016-04-19 02:34:30]

PTAL

@dcheng, IPC (mojom) stuff.
@reed, dependency to third_party/skia.
@dewittj, change in ui/message_center/*.

[dewittj, 2016-04-19 17:11:19]

message_center lgtm

[yoshiki, 2016-04-20 03:49:41]

reed@, dcheng@, ping?

dcheng: IPC (mojom) stuff.
reed: dependency to third_party/skia.

[reed1, 2016-04-20 13:19:56]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2016-04-20 13:19:57]

skia lgtm

[yoshiki, 2016-04-26 02:53:24]

dcheng@, PTAL.

[dcheng, 2016-04-27 01:04:10]

https://codereview.chromium.org/1883473002/diff/200001/components/arc/bitmap/...
File components/arc/bitmap/bitmap_type_converters.cc (right):

https://codereview.chromium.org/1883473002/diff/200001/components/arc/bitmap/...
components/arc/bitmap/bitmap_type_converters.cc:17:
SkImageInfo::Make(arcBitmap->width, arcBitmap->height,
I'd suggest something more like this (since we should really be checking size as
well):

SkImageInfo info = SkImageInfo::Make(width, height, color_type, premultiplied);
if (info.getSafeSize() > arcBitmap->pixel_data.size())
  return SkBitmap();

SkBitmap bitmap;
if (!bitmap.installPixels(info, data, info.minRowBytes()))
  return SkBitmap();

https://codereview.chromium.org/1883473002/diff/200001/ui/message_center/mess...
File ui/message_center/message_center_style.h (right):

https://codereview.chromium.org/1883473002/diff/200001/ui/message_center/mess...
ui/message_center/message_center_style.h:82: kImageBackgroundColor;        //
Background of the image.
I'd suggest moving this comment to before this line to avoid this funny
indenting.

(Also, not necessary for this CL, but why don't we just inline the colors in the
header file? SkColor is just a uint32. It'll reduce the clunkiness of having to
use the EXPORT macros)

[yoshiki, 2016-04-28 18:43:21]

dcheng@, PTAL. Thanks.

https://codereview.chromium.org/1883473002/diff/200001/components/arc/bitmap/...
File components/arc/bitmap/bitmap_type_converters.cc (right):

https://codereview.chromium.org/1883473002/diff/200001/components/arc/bitmap/...
components/arc/bitmap/bitmap_type_converters.cc:17:
SkImageInfo::Make(arcBitmap->width, arcBitmap->height,
On 2016/04/27 01:04:09, dcheng wrote:
> I'd suggest something more like this (since we should really be checking size
as
> well):
> 
> SkImageInfo info = SkImageInfo::Make(width, height, color_type,
premultiplied);
> if (info.getSafeSize() > arcBitmap->pixel_data.size())
>   return SkBitmap();
> 
> SkBitmap bitmap;
> if (!bitmap.installPixels(info, data, info.minRowBytes()))
>   return SkBitmap();

Done.

https://codereview.chromium.org/1883473002/diff/200001/ui/message_center/mess...
File ui/message_center/message_center_style.h (right):

https://codereview.chromium.org/1883473002/diff/200001/ui/message_center/mess...
ui/message_center/message_center_style.h:82: kImageBackgroundColor;        //
Background of the image.
On 2016/04/27 01:04:09, dcheng wrote:
> I'd suggest moving this comment to before this line to avoid this funny
> indenting.

Done.

> (Also, not necessary for this CL, but why don't we just inline the colors in
the
> header file? SkColor is just a uint32. It'll reduce the clunkiness of having
to
> use the EXPORT macros)

Let me fix these extern'd SkColors in a separated patch.

[dcheng, 2016-04-29 23:39:32]

https://codereview.chromium.org/1883473002/diff/240001/components/arc/bitmap/...
File components/arc/bitmap/bitmap_type_converters.cc (right):

https://codereview.chromium.org/1883473002/diff/240001/components/arc/bitmap/...
components/arc/bitmap/bitmap_type_converters.cc:9: SkBitmap
TypeConverter<SkBitmap, arc::mojom::ArcBitmapPtr>::Convert(
I worked on Mojo documentation this week with the mojo team... and apparently
type converters are deprecated. Sorry =(

Would you be willing to rewrite this to use typemapping [1]? If you'd prefer to
do it in a followup CL, that'd work for me.

[1] https://www.chromium.org/developers/design-documents/mojo/type-mapping

[yoshiki, 2016-05-02 08:17:20]

Patchset #10 (id:260001) has been deleted

[yoshiki, 2016-05-02 09:14:57]

dcheng@, PTAL.

https://codereview.chromium.org/1883473002/diff/240001/components/arc/bitmap/...
File components/arc/bitmap/bitmap_type_converters.cc (right):

https://codereview.chromium.org/1883473002/diff/240001/components/arc/bitmap/...
components/arc/bitmap/bitmap_type_converters.cc:9: SkBitmap
TypeConverter<SkBitmap, arc::mojom::ArcBitmapPtr>::Convert(
On 2016/04/29 23:39:32, dcheng wrote:
> I worked on Mojo documentation this week with the mojo team... and apparently
> type converters are deprecated. Sorry =(
> 
> Would you be willing to rewrite this to use typemapping [1]? If you'd prefer
to
> do it in a followup CL, that'd work for me.
> 
> [1] https://www.chromium.org/developers/design-documents/mojo/type-mapping

Thanks. Let me do it soon in a separated patch.

[dcheng, 2016-05-03 06:15:56]

lgtm

[yoshiki, 2016-05-03 16:16:51]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2016-05-03 16:16:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org, dewittj@chromium.org, reed@google.com
Link to the patchset: https://codereview.chromium.org/1883473002/#ps280001
(title: "Rebased")

[commit-bot: I haz the power, 2016-05-03 16:17:03]

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

[commit-bot: I haz the power, 2016-05-03 17:16:45]

Description was changed from

==========
arc: Support more types of notifications.

These patches add a support of InboxStyle and BigPictureStyle
notifications.

Related CLs:
- http://crrev.com/1883473002 (Chromium side change, this patch)
- ag/923744 (mojom change)
- ag/923745 (Android side change)

Bug=b/28027339
==========

to

==========
arc: Support more types of notifications.

These patches add a support of InboxStyle and BigPictureStyle
notifications.

Related CLs:
- http://crrev.com/1883473002 (Chromium side change, this patch)
- ag/923744 (mojom change)
- ag/923745 (Android side change)

Bug=b/28027339
==========

[commit-bot: I haz the power, 2016-05-03 17:16:50]

Committed patchset #10 (id:280001)

[commit-bot: I haz the power, 2016-05-03 17:18:07]

Description was changed from

==========
arc: Support more types of notifications.

These patches add a support of InboxStyle and BigPictureStyle
notifications.

Related CLs:
- http://crrev.com/1883473002 (Chromium side change, this patch)
- ag/923744 (mojom change)
- ag/923745 (Android side change)

Bug=b/28027339
==========

to

==========
arc: Support more types of notifications.

These patches add a support of InboxStyle and BigPictureStyle
notifications.

Related CLs:
- http://crrev.com/1883473002 (Chromium side change, this patch)
- ag/923744 (mojom change)
- ag/923745 (Android side change)

Bug=b/28027339

Committed: https://crrev.com/4e040e320c7a55f4d72e406e2f78bbcb5cb5ccc1
Cr-Commit-Position: refs/heads/master@{#391275}
==========

[commit-bot: I haz the power, 2016-05-03 17:18:09]

Patchset 10 (id:??) landed as
https://crrev.com/4e040e320c7a55f4d72e406e2f78bbcb5cb5ccc1
Cr-Commit-Position: refs/heads/master@{#391275}