Plumbing in notification replies: PlatformNotificationService -> SW

Plumbing in notification replies: PlatformNotificationService -> SW

- Created an interactive_ui_test, which simulates a
notification click with a reply, and tests the reply is
received in the notificationclick event.
- Did enough plumbing so this new test passes.
- Note inline replies are not fully hooked up yet, still need
to pass the reply back from Android to native.
- Layout tests will be added in subsequent patch (see
https://codereview.chromium.org/2403893002/)

BUG=599859
Committed: https://crrev.com/f42998eab3fc522255647f6bf5fed75dcb444436
Cr-Commit-Position: refs/heads/master@{#425065}

[awdf, 2016-10-05 11:59:20]

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

[commit-bot: I haz the power, 2016-10-05 11:59:35]

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, 2016-10-05 12:34:49]

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

[commit-bot: I haz the power, 2016-10-05 12:34:50]

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

[awdf, 2016-10-07 15:43:07]

Patchset #1 (id:1) has been deleted

[awdf, 2016-10-07 15:48:14]

Description was changed from

==========
Added a failing browser test for notification reply
- Did some plumbing so it compiles (NullableString16 reply parameter)

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

to

==========
- Created an interactive_ui_test, which simulates a notification click with a
reply, and tests the reply is received in the notificationclick event.
- Did enough plumbing so this new test passes.
- Note inline replies are not fully hooked up yet, still need to pass the reply
back from Android to native.

BUG=599859
==========

[awdf, 2016-10-07 15:49:01]

Description was changed from

==========
- Created an interactive_ui_test, which simulates a notification click with a
reply, and tests the reply is received in the notificationclick event.
- Did enough plumbing so this new test passes.
- Note inline replies are not fully hooked up yet, still need to pass the reply
back from Android to native.

BUG=599859
==========

to

==========
- Created an interactive_ui_test, which simulates a 
notification click with a reply, and tests the reply is 
received in the notificationclick event.
- Did enough plumbing so this new test passes.
- Note inline replies are not fully hooked up yet, still need 
to pass the reply back from Android to native.

BUG=599859
==========

[awdf, 2016-10-07 15:59:28]

Patchset #1 (id:20001) has been deleted

[awdf, 2016-10-07 15:59:39]

Patchset #1 (id:40001) has been deleted

[awdf, 2016-10-07 16:08:36]

Patchset #2 (id:80001) has been deleted

[awdf, 2016-10-07 16:12:35]

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

[commit-bot: I haz the power, 2016-10-07 16:12:57]

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

[awdf, 2016-10-07 16:32:26]

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

[commit-bot: I haz the power, 2016-10-07 16:32:32]

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

[awdf, 2016-10-07 17:15:39]

awdf@chromium.org changed reviewers:
+ peter@chromium.org

[awdf, 2016-10-07 17:15:41]

As mentioned in the description, this doesn't actually fully hook up Android
inline replies, because I'm not sure how to test the Java -> native part of the
pipeline, so here I've just implemented the part I can test, from PNSI onwards.
Any ideas for how to test it properly end-to-end? (Is it possible?)

https://codereview.chromium.org/2392343002/diff/100001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_notification_manager.cc
(right):

https://codereview.chromium.org/2392343002/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_notification_manager.cc:117:
action_index, base::NullableString16(),
Just passing null here so it compiles but it would be cool to have this
SimulateClick method also take a reply (or have a new SimulateClickWithReply
mehthod) and pass one through from a new
serviceworker-notificationclick-event-reply.html test, that checks the reply is
then received in the notificationclick javascript event. Think I'll do that in a
separate CL though. Sound good?

[Peter Beverloo, 2016-10-11 14:22:56]

Thanks! I have a few comments and questions:

  - Please make sure that the first line of the CLs description is the
    subject, otherwise it gets lost. (Another one for your Rietveld doc?)
  - You chose to add an additional parameter to NotificationHandler's
    OnClick method, but add a new method to NotificationDelegate. Why
    the discrepancy?
  - It's a bit unfortunate that we have to amend the NotificationDelegate
    without supporting the feature in the Message Center. Perhaps add a
    TODO comment to the method declaration pointing to a bug for adding
    said support?
  - Do we call ButtonClick() instead of ButtonClickWithReply() for actions
    having type="text" when no input was given? That seems a bit
    unfortunate since it may lead to duplication. Have you considered
    using a NullableString16 there too and always calling *WithReply()?

In regards to end-to-end testing, I guess we should just add a new
instrumentation test to NotificationPlatformBridgeTest?

And yes, SimulateClick() should take a reply argument. We probably want to
shy away from adding SimulateClickWithReply() because the amount of
plumbing required is a bit silly, and the need for this will go away
anyway when we'll move to Mojo. Separate CL SGTM.

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
File chrome/browser/notifications/native_notification_display_service.cc
(right):

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
chrome/browser/notifications/native_notification_display_service.cc:89:
base::NullableString16());
nit: /* reply */

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
File chrome/browser/notifications/non_persistent_notification_handler.h (right):

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
chrome/browser/notifications/non_persistent_notification_handler.h:12: #include
"base/strings/nullable_string16.h"
nit: drop. Pretty much everywhere else too. Here's why:

non_persistent_notification_handler.h
persistent_notification_delegate.h
persistent_notification_handler.h
notification_event_dispatcher_impl.h

  We usually don't include headers for types required by an interface
  that's being implemented, since the interface's declaration
  (NotificationHandler, in this case) takes care of that.

non_persistent_notification_handler.cc
persistent_notification_handler.cc
platform_notification_service_impl.cc
notification_delegate.cc (for base::string16)

  The type is not used, so we don't need the full definition. For example,
  passing a const reference to another function is the same as passing
  a pointer - we don't need type information, only the address.

notification_handler.h
platform_notification_service_impl.h
notification_event_dispatcher.h
notification_delegate.h (for base::string16)

  We should forward declare the type instead. That means that files
  including one of these headers don't need to parse nullable_string16.h.

  namespace base {
  class NullableString16;
  }

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
File chrome/browser/notifications/persistent_notification_delegate.cc (right):

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
chrome/browser/notifications/persistent_notification_delegate.cc:51: if
(button_index == notification_settings_index_) {
DCHECK_NE? Why would the site settings button take a reply?

https://codereview.chromium.org/2392343002/diff/130001/content/browser/notifi...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/2392343002/diff/130001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:219:
reply.is_null() ? base::ASCIIToUTF16("") : reply.string()));
Why don't we just pass the NullableString16 over IPC (it has IPC traits defined
in ipc_message_utils.h)? I thought that we decided that the difference between a
NULL string and an empty string was significant, how would you achieve that with
just base::string16?

[awdf, 2016-10-11 15:11:35]

Description was changed from

==========
- Created an interactive_ui_test, which simulates a 
notification click with a reply, and tests the reply is 
received in the notificationclick event.
- Did enough plumbing so this new test passes.
- Note inline replies are not fully hooked up yet, still need 
to pass the reply back from Android to native.

BUG=599859
==========

to

==========
Plumbing in notification replies: PlatformNotificationService -> SW

- Created an interactive_ui_test, which simulates a 
notification click with a reply, and tests the reply is 
received in the notificationclick event.
- Did enough plumbing so this new test passes.
- Note inline replies are not fully hooked up yet, still need 
to pass the reply back from Android to native.

BUG=599859
==========

[awdf, 2016-10-11 15:51:27]

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
File chrome/browser/notifications/persistent_notification_delegate.cc (right):

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
chrome/browser/notifications/persistent_notification_delegate.cc:51: if
(button_index == notification_settings_index_) {
On 2016/10/11 14:22:56, Peter Beverloo wrote:
> DCHECK_NE? Why would the site settings button take a reply?

oops - this came from copy pasting the method above without really thinking.
You're right it doesn't make sense to do this check if we know this came from a
notification reply.

But if we unify ButtonClick and ButtonClickWithReply, it's irrelevant.

(I did start trying to merge ButtonClick and ButtonClickWithReply last week, but
it got a little bit gnarly so I abandoned it. I still have the local branch
though so I'll give it another go..)

https://codereview.chromium.org/2392343002/diff/130001/content/browser/notifi...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/2392343002/diff/130001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:219:
reply.is_null() ? base::ASCIIToUTF16("") : reply.string()));
On 2016/10/11 14:22:56, Peter Beverloo wrote:
> Why don't we just pass the NullableString16 over IPC (it has IPC traits
defined
> in ipc_message_utils.h)? I thought that we decided that the difference between
a
> NULL string and an empty string was significant, how would you achieve that
with
> just base::string16?

Thinking about it, I'm not 100% sure the difference between NULL & empty string
*is* significant, although maybe it's elegant to send null if the action wasn't
a text action, and empty string if it was but no text entered. Let's discuss on
this design doc thread
https://docs.google.com/document/d/1cWGf1HpuOsRklsC4lZ9uUVxq0yedRT1JhpUn8sIJZ...


(You might be recalling the discussion we had re: whether the distinction
between empty & no placeholder was significant which we decided it was)

[awdf, 2016-10-12 10:44:58]

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
File chrome/browser/notifications/native_notification_display_service.cc
(right):

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
chrome/browser/notifications/native_notification_display_service.cc:89:
base::NullableString16());
On 2016/10/11 14:22:56, Peter Beverloo wrote:
> nit: /* reply */

Done.

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
File chrome/browser/notifications/non_persistent_notification_handler.h (right):

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
chrome/browser/notifications/non_persistent_notification_handler.h:12: #include
"base/strings/nullable_string16.h"
On 2016/10/11 14:22:56, Peter Beverloo wrote:
> nit: drop. Pretty much everywhere else too. Here's why:
> 
> non_persistent_notification_handler.h
> persistent_notification_delegate.h
> persistent_notification_handler.h
> notification_event_dispatcher_impl.h
> 
>   We usually don't include headers for types required by an interface
>   that's being implemented, since the interface's declaration
>   (NotificationHandler, in this case) takes care of that.
> 
> non_persistent_notification_handler.cc
> persistent_notification_handler.cc
> platform_notification_service_impl.cc
> notification_delegate.cc (for base::string16)
> 
>   The type is not used, so we don't need the full definition. For example,
>   passing a const reference to another function is the same as passing
>   a pointer - we don't need type information, only the address.
> 
> notification_handler.h
> platform_notification_service_impl.h
> notification_event_dispatcher.h
> notification_delegate.h (for base::string16)
> 
>   We should forward declare the type instead. That means that files
>   including one of these headers don't need to parse nullable_string16.h.
> 
>   namespace base {
>   class NullableString16;
>   }
>  

Done.

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
File chrome/browser/notifications/persistent_notification_delegate.cc (right):

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
chrome/browser/notifications/persistent_notification_delegate.cc:51: if
(button_index == notification_settings_index_) {
On 2016/10/11 15:51:27, awdf wrote:
> On 2016/10/11 14:22:56, Peter Beverloo wrote:
> > DCHECK_NE? Why would the site settings button take a reply?
> 
> oops - this came from copy pasting the method above without really thinking.
> You're right it doesn't make sense to do this check if we know this came from
a
> notification reply.
> 
> But if we unify ButtonClick and ButtonClickWithReply, it's irrelevant.
> 
> (I did start trying to merge ButtonClick and ButtonClickWithReply last week,
but
> it got a little bit gnarly so I abandoned it. I still have the local branch
> though so I'll give it another go..)

I remember why it's gnarly now - it's because there's 39 overrides of
NotificationDelegate.ButtonClick, most of which won't ever care about the reply
parameter (a lot don't even care about the button index)
https://cs.chromium.org/chromium/src/ui/message_center/notification_delegate....
. But perhaps someday some of them might I suppose, and it's probably less
confusing to have a single method for button clicks.. I'll press on (shout now
if you change your mind though!)

[awdf, 2016-10-12 13:26:42]

I think that's all your comments addressed now.

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
File chrome/browser/notifications/persistent_notification_delegate.cc (right):

https://codereview.chromium.org/2392343002/diff/130001/chrome/browser/notific...
chrome/browser/notifications/persistent_notification_delegate.cc:51: if
(button_index == notification_settings_index_) {
On 2016/10/12 10:44:58, awdf wrote:
> On 2016/10/11 15:51:27, awdf wrote:
> > On 2016/10/11 14:22:56, Peter Beverloo wrote:
> > > DCHECK_NE? Why would the site settings button take a reply?
> > 
> > oops - this came from copy pasting the method above without really thinking.
> > You're right it doesn't make sense to do this check if we know this came
from
> a
> > notification reply.
> > 
> > But if we unify ButtonClick and ButtonClickWithReply, it's irrelevant.
> > 
> > (I did start trying to merge ButtonClick and ButtonClickWithReply last week,
> but
> > it got a little bit gnarly so I abandoned it. I still have the local branch
> > though so I'll give it another go..)
> 
> I remember why it's gnarly now - it's because there's 39 overrides of
> NotificationDelegate.ButtonClick, most of which won't ever care about the
reply
> parameter (a lot don't even care about the button index)
>
https://cs.chromium.org/chromium/src/ui/message_center/notification_delegate....
> . But perhaps someday some of them might I suppose, and it's probably less
> confusing to have a single method for button clicks.. I'll press on (shout now
> if you change your mind though!)

Done (removed the unnecessary settings check, but didn't unify the methods, as
discussed).

https://codereview.chromium.org/2392343002/diff/130001/content/browser/notifi...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/2392343002/diff/130001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:219:
reply.is_null() ? base::ASCIIToUTF16("") : reply.string()));
On 2016/10/11 15:51:27, awdf wrote:
> On 2016/10/11 14:22:56, Peter Beverloo wrote:
> > Why don't we just pass the NullableString16 over IPC (it has IPC traits
> defined
> > in ipc_message_utils.h)? I thought that we decided that the difference
between
> a
> > NULL string and an empty string was significant, how would you achieve that
> with
> > just base::string16?
> 
> Thinking about it, I'm not 100% sure the difference between NULL & empty
string
> *is* significant, although maybe it's elegant to send null if the action
wasn't
> a text action, and empty string if it was but no text entered. Let's discuss
on
> this design doc thread
>
https://docs.google.com/document/d/1cWGf1HpuOsRklsC4lZ9uUVxq0yedRT1JhpUn8sIJZ...
> 
> 
> (You might be recalling the discussion we had re: whether the distinction
> between empty & no placeholder was significant which we decided it was)

Done (As discussed on the doc, the difference can be significant).

[Peter Beverloo, 2016-10-12 13:58:31]

lgtm \o/

Please note in your CL description that layout tests will be added in the
subsequent patch. (You could even link to it.)

https://codereview.chromium.org/2392343002/diff/190001/chrome/browser/notific...
File chrome/browser/notifications/non_persistent_notification_handler.cc
(right):

https://codereview.chromium.org/2392343002/diff/190001/chrome/browser/notific...
chrome/browser/notifications/non_persistent_notification_handler.cc:29: const
base::NullableString16& reply) {
Could we DCHECK(reply.is_null()) since we don't support inline replies for
non-persistent notifications?

https://codereview.chromium.org/2392343002/diff/190001/chrome/browser/notific...
File chrome/browser/notifications/persistent_notification_delegate.cc (right):

https://codereview.chromium.org/2392343002/diff/190001/chrome/browser/notific...
chrome/browser/notifications/persistent_notification_delegate.cc:49:
DCHECK_GE(button_index, 0);
DCHECK_NE(button_index, notification_settings_index_);

https://codereview.chromium.org/2392343002/diff/190001/content/browser/notifi...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/2392343002/diff/190001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:9: #include
"base/strings/utf_string_conversions.h"
delete both (unused)

https://codereview.chromium.org/2392343002/diff/190001/content/common/service...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2392343002/diff/190001/content/common/service...
content/common/service_worker/service_worker_messages.h:493:
content::ServiceWorkerFetchRequest)
bad merge?

https://codereview.chromium.org/2392343002/diff/190001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2392343002/diff/190001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:13: #include
"base/strings/string16.h"
delete?

https://codereview.chromium.org/2392343002/diff/190001/ui/message_center/noti...
File ui/message_center/notification_delegate.h (right):

https://codereview.chromium.org/2392343002/diff/190001/ui/message_center/noti...
ui/message_center/notification_delegate.h:48: // TODO(crbug.com/599859) Support
this feature in the message center.
Sorry, one more addendum: it would be good to note that this currently is only
available on Android.

[awdf, 2016-10-12 16:23:54]

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

[commit-bot: I haz the power, 2016-10-12 16:24:16]

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

[awdf, 2016-10-12 16:35:20]

Description was changed from

==========
Plumbing in notification replies: PlatformNotificationService -> SW

- Created an interactive_ui_test, which simulates a 
notification click with a reply, and tests the reply is 
received in the notificationclick event.
- Did enough plumbing so this new test passes.
- Note inline replies are not fully hooked up yet, still need 
to pass the reply back from Android to native.

BUG=599859
==========

to

==========
Plumbing in notification replies: PlatformNotificationService -> SW

- Created an interactive_ui_test, which simulates a 
notification click with a reply, and tests the reply is 
received in the notificationclick event.
- Did enough plumbing so this new test passes.
- Note inline replies are not fully hooked up yet, still need 
to pass the reply back from Android to native.
- Layout test will be added in subsequent patch (see 
https://codereview.chromium.org/2403893002/)

BUG=599859
==========

[awdf, 2016-10-12 16:35:44]

Description was changed from

==========
Plumbing in notification replies: PlatformNotificationService -> SW

- Created an interactive_ui_test, which simulates a 
notification click with a reply, and tests the reply is 
received in the notificationclick event.
- Did enough plumbing so this new test passes.
- Note inline replies are not fully hooked up yet, still need 
to pass the reply back from Android to native.
- Layout test will be added in subsequent patch (see 
https://codereview.chromium.org/2403893002/)

BUG=599859
==========

to

==========
Plumbing in notification replies: PlatformNotificationService -> SW

- Created an interactive_ui_test, which simulates a 
notification click with a reply, and tests the reply is 
received in the notificationclick event.
- Did enough plumbing so this new test passes.
- Note inline replies are not fully hooked up yet, still need 
to pass the reply back from Android to native.
- Layout tests will be added in subsequent patch (see 
https://codereview.chromium.org/2403893002/)

BUG=599859
==========

[commit-bot: I haz the power, 2016-10-12 16:36:17]

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

[commit-bot: I haz the power, 2016-10-12 16:36:18]

Dry run: Exceeded global retry quota

[awdf, 2016-10-12 17:04:01]

https://codereview.chromium.org/2392343002/diff/190001/chrome/browser/notific...
File chrome/browser/notifications/non_persistent_notification_handler.cc
(right):

https://codereview.chromium.org/2392343002/diff/190001/chrome/browser/notific...
chrome/browser/notifications/non_persistent_notification_handler.cc:29: const
base::NullableString16& reply) {
On 2016/10/12 13:58:31, Peter Beverloo wrote:
> Could we DCHECK(reply.is_null()) since we don't support inline replies for
> non-persistent notifications?

Done.

https://codereview.chromium.org/2392343002/diff/190001/content/browser/notifi...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/2392343002/diff/190001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:9: #include
"base/strings/utf_string_conversions.h"
On 2016/10/12 13:58:31, Peter Beverloo wrote:
> delete both (unused)

Done.

https://codereview.chromium.org/2392343002/diff/190001/content/common/service...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2392343002/diff/190001/content/common/service...
content/common/service_worker/service_worker_messages.h:493:
content::ServiceWorkerFetchRequest)
On 2016/10/12 13:58:31, Peter Beverloo wrote:
> bad merge?

Oops, yes, I thought that it had been added in the rebase but actually it had
been removed.

Removed now.

https://codereview.chromium.org/2392343002/diff/190001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2392343002/diff/190001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:13: #include
"base/strings/string16.h"
On 2016/10/12 13:58:31, Peter Beverloo wrote:
> delete?

Done.

https://codereview.chromium.org/2392343002/diff/190001/ui/message_center/noti...
File ui/message_center/notification_delegate.h (right):

https://codereview.chromium.org/2392343002/diff/190001/ui/message_center/noti...
ui/message_center/notification_delegate.h:48: // TODO(crbug.com/599859) Support
this feature in the message center.
On 2016/10/12 13:58:31, Peter Beverloo wrote:
> Sorry, one more addendum: it would be good to note that this currently is only
> available on Android.

Done.

[awdf, 2016-10-12 17:04:49]

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

[commit-bot: I haz the power, 2016-10-12 17:05:18]

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

[awdf, 2016-10-12 17:15:16]

awdf@chromium.org changed reviewers:
+ avi@chromium.org, dewittj@chromium.org, tkent@chromium.org,
tsepez@chromium.org

[awdf, 2016-10-12 17:15:17]

tkent@chromium.org: Please review changes in 
Source/web

dewittj@chromium.org: Please review changes in 
ui/message_center/

avi@chromium.org: Please review changes in 
content/

tsepez@chromium.org: Please review changes in 
IPC

[commit-bot: I haz the power, 2016-10-12 17:15:49]

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

[commit-bot: I haz the power, 2016-10-12 17:15:50]

Dry run: Exceeded global retry quota

[Avi (use Gerrit), 2016-10-12 17:16:55]

content lgtm

[Tom Sepez, 2016-10-12 19:17:23]

messages LGTM

[tkent, 2016-10-12 23:17:42]

On 2016/10/12 at 17:15:17, awdf wrote:
> tkent@chromium.org: Please review changes in 
> Source/web

lgtm

[dewittj, 2016-10-12 23:21:11]

lgtm

https://codereview.chromium.org/2392343002/diff/210001/ui/message_center/noti...
File ui/message_center/notification_delegate.cc (right):

https://codereview.chromium.org/2392343002/diff/210001/ui/message_center/noti...
ui/message_center/notification_delegate.cc:22: const base::string16& reply) {}
Perhaps this should be NOTIMPLEMENTED() since we don't expect this delegate to
ever get called in the normal case?

[awdf, 2016-10-13 14:31:07]

https://codereview.chromium.org/2392343002/diff/210001/ui/message_center/noti...
File ui/message_center/notification_delegate.cc (right):

https://codereview.chromium.org/2392343002/diff/210001/ui/message_center/noti...
ui/message_center/notification_delegate.cc:22: const base::string16& reply) {}
On 2016/10/12 23:21:11, dewittj wrote:
> Perhaps this should be NOTIMPLEMENTED() since we don't expect this delegate to
> ever get called in the normal case?

Done. And I'll do the same for the rest of the methods in a separate cleanup CL
- ticketed at https://bugs.chromium.org/p/chromium/issues/detail?id=655640

[awdf, 2016-10-13 14:31:19]

The CQ bit was checked by awdf@chromium.org

[awdf, 2016-10-13 14:31:19]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
tkent@chromium.org, avi@chromium.org, tsepez@chromium.org, dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/2392343002/#ps250001
(title: "Put back a linebreak accidentally removed")

[commit-bot: I haz the power, 2016-10-13 14:31:35]

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, 2016-10-13 14:48:23]

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

[commit-bot: I haz the power, 2016-10-13 14:48:24]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[awdf, 2016-10-13 15:23:53]

The CQ bit was checked by awdf@chromium.org

[awdf, 2016-10-13 15:23:54]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
tkent@chromium.org, avi@chromium.org, tsepez@chromium.org, dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/2392343002/#ps270001
(title: "include base/logging.h for NOTIMPLEMENTED")

[commit-bot: I haz the power, 2016-10-13 15:24:23]

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

[awdf, 2016-10-13 16:04:09]

> 
> In regards to end-to-end testing, I guess we should just add a new
> instrumentation test to NotificationPlatformBridgeTest?
> 

NotificationPlatformBridgeTest.java ? How - is there any way to listen for
javascript events from there? The other tests in there only go as far as
checking that the NotificationManager has the correct entries after waiting for
it to mutate.. Ideally i'd want to create a notification from javascript in
there (fine, already doing it), then fake a click with a reply (which I could
probably do by mutating the RemoteAction and calling the intent on the
notification), and then check the reply exists in the subsequent javascript
event - no idea how to do this last step.

[Peter Beverloo, 2016-10-13 16:10:57]

On 2016/10/13 16:04:09, awdf wrote:
> > 
> > In regards to end-to-end testing, I guess we should just add a new
> > instrumentation test to NotificationPlatformBridgeTest?
> > 
> 
> NotificationPlatformBridgeTest.java ? How - is there any way to listen for
> javascript events from there? The other tests in there only go as far as
> checking that the NotificationManager has the correct entries after waiting
for
> it to mutate.. Ideally i'd want to create a notification from javascript in
> there (fine, already doing it), then fake a click with a reply (which I could
> probably do by mutating the RemoteAction and calling the intent on the
> notification), and then check the reply exists in the subsequent javascript
> event - no idea how to do this last step.

testShowAndCloseMultipleNotifications does most of this - it shows a
notification, sends the contentIntent (fakes a click), which then ends up in the
`notificationclick` event in android_test_worker.js. From there you could send a
message to the page (messagePort.postMessage()) and use that to update the
page's title.

You can then use waitForTitle() in the test to wait until the expected title
gets set.

[commit-bot: I haz the power, 2016-10-13 16:59:18]

Description was changed from

==========
Plumbing in notification replies: PlatformNotificationService -> SW

- Created an interactive_ui_test, which simulates a 
notification click with a reply, and tests the reply is 
received in the notificationclick event.
- Did enough plumbing so this new test passes.
- Note inline replies are not fully hooked up yet, still need 
to pass the reply back from Android to native.
- Layout tests will be added in subsequent patch (see 
https://codereview.chromium.org/2403893002/)

BUG=599859
==========

to

==========
Plumbing in notification replies: PlatformNotificationService -> SW

- Created an interactive_ui_test, which simulates a 
notification click with a reply, and tests the reply is 
received in the notificationclick event.
- Did enough plumbing so this new test passes.
- Note inline replies are not fully hooked up yet, still need 
to pass the reply back from Android to native.
- Layout tests will be added in subsequent patch (see 
https://codereview.chromium.org/2403893002/)

BUG=599859
==========

[commit-bot: I haz the power, 2016-10-13 16:59:20]

Committed patchset #11 (id:270001)

[commit-bot: I haz the power, 2016-10-13 17:02:41]

Description was changed from

==========
Plumbing in notification replies: PlatformNotificationService -> SW

- Created an interactive_ui_test, which simulates a 
notification click with a reply, and tests the reply is 
received in the notificationclick event.
- Did enough plumbing so this new test passes.
- Note inline replies are not fully hooked up yet, still need 
to pass the reply back from Android to native.
- Layout tests will be added in subsequent patch (see 
https://codereview.chromium.org/2403893002/)

BUG=599859
==========

to

==========
Plumbing in notification replies: PlatformNotificationService -> SW

- Created an interactive_ui_test, which simulates a
notification click with a reply, and tests the reply is
received in the notificationclick event.
- Did enough plumbing so this new test passes.
- Note inline replies are not fully hooked up yet, still need
to pass the reply back from Android to native.
- Layout tests will be added in subsequent patch (see
https://codereview.chromium.org/2403893002/)

BUG=599859

Committed: https://crrev.com/f42998eab3fc522255647f6bf5fed75dcb444436
Cr-Commit-Position: refs/heads/master@{#425065}
==========

[commit-bot: I haz the power, 2016-10-13 17:02:43]

Patchset 11 (id:??) landed as
https://crrev.com/f42998eab3fc522255647f6bf5fed75dcb444436
Cr-Commit-Position: refs/heads/master@{#425065}