Update notifications API after review.

Update notifications API after review.

type -> templateType
template names -> enum
base -> basic
multiple -> list
timestamp -> eventTime
unreadCount -> gone
buttonOne/Two -> buttons array

BUG=174680
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182413

[miket_OOO, 2013-02-06 18:42:34]

https://codereview.chromium.org/12212037/diff/1/chrome/test/data/extensions/a...
File chrome/test/data/extensions/api_test/notification/galore/app/controller.js
(right):

https://codereview.chromium.org/12212037/diff/1/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/notification/galore/app/controller.js:36:
type.templateType,
I seem to have broken the @ replacement scheme you use in the button icon URLs,
but I'm not sure how to fix it. Can you help?

[dharcourt, 2013-02-06 21:34:20]

LGTM, with a couple of questions and a possible fix for the test app breakage.

    - Charles

https://codereview.chromium.org/12212037/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/notification/notification_api.cc (right):

https://codereview.chromium.org/12212037/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/notification/notification_api.cc:133: return
ui::notifications::NOTIFICATION_TYPE_SIMPLE;
Wouldn't NOTIFICATION_TYPE_BASIC be more appropriate for this use case? If the
app code is newer, it's more likely to have notifications based on BASIC than on
SIMPLE (for example 80 pixel icons instead of 40 pixel ones).

https://codereview.chromium.org/12212037/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/experimental_notification.idl (right):

https://codereview.chromium.org/12212037/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental_notification.idl:106: // in the
message center without interrupting the user.
I'm confused... will onDisplayed() be called: 1) every time a particular
notification is show to a user; 2) the first time it's shown; or 3) when the
notification is received in the notification center, even if there is no alert
for it? I think this comment could be clarified to better communicate what the
expected behavior is.

FWIW, onDisplayed() currently get called for low-priority notification, even
when the notification center is not opened. I'm guessing from this comment that
this is a bug... right?

https://codereview.chromium.org/12212037/diff/1/chrome/test/data/extensions/a...
File chrome/test/data/extensions/api_test/notification/galore/app/controller.js
(right):

https://codereview.chromium.org/12212037/diff/1/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/notification/galore/app/controller.js:36:
type.templateType,
On 2013/02/06 18:42:34, miket wrote:
> I seem to have broken the @ replacement scheme you use in the button icon
URLs,
> but I'm not sure how to fix it. Can you help?

The icons shouldn't be broken, but the button images will be because their URLs
moved down into the property of an object in an array, and replace_() wasn't
written to handle that. The updated replace_() implementation in my comment
below _should_ fix that, but I haven't tested it. If you want, feel free to
leave it broken and I can test and fix that in a separate change list once your
API changes have landed.

https://codereview.chromium.org/12212037/diff/1/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/notification/galore/app/controller.js:86:
},
  replace_: function(option, size) {
    var replaced;
    if (typeof option === 'string') {
      replaced = option.replace(/\$#/g, this.counter);
      replaced = replaced.replace(/\$@/g, this.prefix);
      replaced = replaced.replace(/\$%/g, size);
    } else if (Array.isArray(option)) {
      replaced = [];
      option.forEach(function(element) {
        replaced.push(this.replace_(element, size));
      }, this);
    } else {
      replaced = {};
      Object.keys(option).forEach(function (key) {
        replaced[key] = this.replace_(option[key], size);
      }, this);
    }
    return replaced;
  },

[miket_OOO, 2013-02-12 23:20:39]

> I'm confused... will onDisplayed() be called: 1) every time a particular
> notification is show to a user; 2) the first time it's shown; or 3) when the
> notification is received in the notification center, even if there is no alert
> for it? I think this comment could be clarified to better communicate what the
> expected behavior is.

The documentation is most consistent with #1, and that's my intent. See further
comments.

> FWIW, onDisplayed() currently get called for low-priority notification, even
> when the notification center is not opened. I'm guessing from this comment
that
> this is a bug... right?

Yes, it is. I am not sure what value onDisplayed() would be in the case where...
nothing was displayed. Given that clarification, is the current documentation
OK?

> The updated replace_() implementation in my comment
> below _should_ fix that, but I haven't tested it. If you want, feel free to
> leave it broken and I can test and fix that in a separate change list once
your
> API changes have landed.

It seems not to work. I'll take you up on your offer to have a look at it.
Submitting as soon as the trybots report back...

[commit-bot: I haz the power, 2013-02-13 00:40:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/12212037/8003

[dharcourt, 2013-02-13 01:31:59]

On 2013/02/12 23:20:39, miket wrote:
> > I'm confused... will onDisplayed() be called: 1) every time a particular
> > notification is show to a user; 2) the first time it's shown; or 3) when the
> > notification is received in the notification center, even if there is no
alert
> > for it? I think this comment could be clarified to better communicate what
the
> > expected behavior is.
> 
> The documentation is most consistent with #1, and that's my intent. See
further
> comments.
> 
> > FWIW, onDisplayed() currently get called for low-priority notification, even
> > when the notification center is not opened. I'm guessing from this comment
> that
> > this is a bug... right?
> 
> Yes, it is. I am not sure what value onDisplayed() would be in the case
where...
> nothing was displayed. Given that clarification, is the current documentation
> OK?

Yes on the documentation, and I think this is the behavior developers will
expect. I'll set up a test case & report a bug for the above.

> > The updated replace_() implementation in my comment
> > below _should_ fix that, but I haven't tested it. If you want, feel free to
> > leave it broken and I can test and fix that in a separate change list once
> your
> > API changes have landed.
> 
> It seems not to work. I'll take you up on your offer to have a look at it.
> Submitting as soon as the trybots report back...

OK will do. I'll watch for the commit & fix as soon as possible after that.

[commit-bot: I haz the power, 2013-02-13 06:46:56]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-02-13 17:41:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/12212037/8003

[commit-bot: I haz the power, 2013-02-13 17:41:34]

Failed to apply patch for
chrome/browser/extensions/api/notification/notification_api.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/api/notification/notification_api.h
  Hunk #2 FAILED at 23.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/api/notification/notification_api.h.rej

Patch:       chrome/browser/extensions/api/notification/notification_api.h
Index: chrome/browser/extensions/api/notification/notification_api.h
diff --git a/chrome/browser/extensions/api/notification/notification_api.h
b/chrome/browser/extensions/api/notification/notification_api.h
index
37adb314829e9017b80bc4299b71ff10a82cd15e..c889b9f14465b31aac98ac58895e7659091d4126
100644
--- a/chrome/browser/extensions/api/notification/notification_api.h
+++ b/chrome/browser/extensions/api/notification/notification_api.h
@@ -5,12 +5,13 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_API_NOTIFICATION_NOTIFICATION_API_H_
 #define CHROME_BROWSER_EXTENSIONS_API_NOTIFICATION_NOTIFICATION_API_H_
 
+#include <string>
+
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/extensions/api/api_function.h"
 #include "chrome/browser/extensions/extension_function.h"
 #include "chrome/common/extensions/api/experimental_notification.h"
-
-#include <string>
+#include "ui/notifications/notification_types.h"
 
 namespace extensions {
 
@@ -22,6 +23,8 @@ class NotificationApiFunction : public ApiFunction {
   void CreateNotification(
       const std::string& id,
       api::experimental_notification::NotificationOptions* options);
+  ui::notifications::NotificationType MapApiTemplateTypeToType(
+      api::experimental_notification::TemplateType type);
 };
 
 class NotificationCreateFunction : public NotificationApiFunction {

[commit-bot: I haz the power, 2013-02-13 20:05:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/12212037/18001

[commit-bot: I haz the power, 2013-02-14 05:02:51]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-02-14 05:28:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/12212037/18001

[commit-bot: I haz the power, 2013-02-14 08:14:46]

Change committed as 182413