Mark create/update/clear callbacks of notification API as optional

Mark create/update/clear callbacks of notification API and create ID as optional

Also improved error reporting: if an error occurs, and
chrome.runtime.lastError is not checked, then an error is printed to the
console (to be consistent with error reporting in the other async APIs).

The documentation has been updated to mention that these parameters
were required before Chrome 42, these lines should be removed once
Chrome 42 hits stable (https://crbug.com/452990).

BUG=163750
TEST=./out/Debug/interactive_ui_tests --gtest_filter=NotificationsApiTest.TestBasicUsage
TEST=Manually, called chrome.tabs.create/update/clear without callback
     and observed that the notification is created as expected, and that
     an error is printed to the console when an error occurs (e.g.
     invalid iconUrl).
R=kalman@chromium.org

Committed: https://crrev.com/ff49cf31bc0e2c4c56e330a978a9f9599d75c6ab
Cr-Commit-Position: refs/heads/master@{#314383}

[not at google - send to devlin, 2015-01-20 17:41:34]

kalman@chromium.org changed reviewers:
+ dewittj@chromium.org

[not at google - send to devlin, 2015-01-20 17:41:35]

Adding +dewittj here as the author of the notifications API.

I think that changing the callbacks to be optional is the right thing to do in
hindsight, though a little iffy given it's not forwards compatible. I would err
on the side of developer annoyance over developer surprise that their extension
stops working.

https://codereview.chromium.org/855813002/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/notifications_custom_bindings.js
(right):

https://codereview.chromium.org/855813002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/notifications_custom_bindings.js:113: //
TODO(dewittj): Remove this hack. This is used as a way to deep
The best way to deep copy would be to expose a native function which uses
WebSerializedScriptValue.serialize(obj).deserialize(). I think the TODO should
be changed to reflect that. The main advantage over JSON is that recursive
objects wouldn't crash; it may or may not be any quicker.

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/last_error.js (right):

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/last_error.js:116: if (!hasAccessed(targetChrome))
This doesn't look right, it'll always print an error no matter whether the
function succeeded or failed.

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/last_error.js:130: function
reportUncheckedLastError(name, message, stack) {
Given that this method is under the "lastError" namespace, "reportUnchecked"
would be more accurate. Also, I think it should assert that there actually is a
last error at this point.

[robwu, 2015-01-20 22:34:01]

https://codereview.chromium.org/855813002/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/notifications_custom_bindings.js
(right):

https://codereview.chromium.org/855813002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/notifications_custom_bindings.js:113: //
TODO(dewittj): Remove this hack. This is used as a way to deep
On 2015/01/20 17:41:35, kalman wrote:
> The best way to deep copy would be to expose a native function which uses
> WebSerializedScriptValue.serialize(obj).deserialize(). I think the TODO should
> be changed to reflect that. The main advantage over JSON is that recursive
> objects wouldn't crash; it may or may not be any quicker.

Recursive objects are invalid for this API, so that is not something that we
have to worry about.

I *think* that the purpose of deep cloning the object is to make sure that
changing the object that is being passed in to the function after the function
call does not change the behavior of the first function call.

var buttons = [{ ... }, { ... }];
chrome.notifications.create(..., { buttons: buttons, ... });
buttons.length = 0; // Should not cause buttons to not appear in the
notification.

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/last_error.js (right):

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/last_error.js:116: if (!hasAccessed(targetChrome))
On 2015/01/20 17:41:35, kalman wrote:
> This doesn't look right, it'll always print an error no matter whether the
> function succeeded or failed.

This is correct. Function run sets chrome.runtime.lastError, which should be
printed to the console if the user did not check its value.

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/last_error.js:130: function
reportUncheckedLastError(name, message, stack) {
On 2015/01/20 17:41:35, kalman wrote:
> Given that this method is under the "lastError" namespace, "reportUnchecked"
> would be more accurate. Also, I think it should assert that there actually is
a
> last error at this point.

If we couple this method with the chrome object, then we'd better use the
following instead, to make sure that the reported error is consistent to what's
assigned to lastError:

function reportIfUnchecked(name, targetChrome, stack) {
  var error = hasAccessed(targetChrome) && chrome.runtime.lastError;
  if (!error)
    return;
  console.error("Unchecked runtime.lastError while running " +
      (name || "unknown") + ": " + error.message + (stack ? "\n" + stack : ""));
}

[robwu, 2015-01-20 22:39:13]

On 2015/01/20 17:41:35, kalman wrote:
> Adding +dewittj here as the author of the notifications API.
> 
> I think that changing the callbacks to be optional is the right thing to do in
> hindsight, though a little iffy given it's not forwards compatible. I would
err
> on the side of developer annoyance over developer surprise that their
extension
> stops working.

Declaring callbacks as optional does not break compatibility of extensions.
However, extensions that are only designed with optional callbacks may not work
in versions where the callback was optional. Adding a note to the documentation
should be sufficient to address this compat issue.

[dewittj, 2015-01-20 22:46:51]

Do we aim for 100% forward compatibility in extension APIs?

https://codereview.chromium.org/855813002/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/notifications_custom_bindings.js
(right):

https://codereview.chromium.org/855813002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/notifications_custom_bindings.js:113: //
TODO(dewittj): Remove this hack. This is used as a way to deep
On 2015/01/20 22:34:01, robwu wrote:
> On 2015/01/20 17:41:35, kalman wrote:
> > The best way to deep copy would be to expose a native function which uses
> > WebSerializedScriptValue.serialize(obj).deserialize(). I think the TODO
should
> > be changed to reflect that. The main advantage over JSON is that recursive
> > objects wouldn't crash; it may or may not be any quicker.
> 
> Recursive objects are invalid for this API, so that is not something that we
> have to worry about.
> 
> I *think* that the purpose of deep cloning the object is to make sure that
> changing the object that is being passed in to the function after the function
> call does not change the behavior of the first function call.
> 
> var buttons = [{ ... }, { ... }];
> chrome.notifications.create(..., { buttons: buttons, ... });
> buttons.length = 0; // Should not cause buttons to not appear in the
> notification.

The JSON copy has 2 important properties:
1) Custom bindings can change the copy without affecting client data structures
2) Client program can run while the API is asynchronously downloading images,
and change the input object without affecting the data passed to Chrome in the
custom bindings.

At the time, extension bindings did not work with prototype properties; i.e.
given var b = Object.create(a), passing b to the bindings would cause a crash
since the schema validator would work but the c++ code would not grab the
properties from a.  Even if it did work, it would be susceptible to #2 above.

I believe Blink contains a function that does deep cloning but it is not part of
the JS runtime.  I'm open to fixing this if there are better answers nowadays.

[robwu, 2015-01-20 22:58:44]

https://codereview.chromium.org/855813002/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/notifications_custom_bindings.js
(right):

https://codereview.chromium.org/855813002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/notifications_custom_bindings.js:113: //
TODO(dewittj): Remove this hack. This is used as a way to deep
On 2015/01/20 22:46:51, dewittj wrote:
> On 2015/01/20 22:34:01, robwu wrote:
> > On 2015/01/20 17:41:35, kalman wrote:
> > > The best way to deep copy would be to expose a native function which uses
> > > WebSerializedScriptValue.serialize(obj).deserialize(). I think the TODO
> should
> > > be changed to reflect that. The main advantage over JSON is that recursive
> > > objects wouldn't crash; it may or may not be any quicker.
> > 
> > Recursive objects are invalid for this API, so that is not something that we
> > have to worry about.
> > 
> > I *think* that the purpose of deep cloning the object is to make sure that
> > changing the object that is being passed in to the function after the
function
> > call does not change the behavior of the first function call.
> > 
> > var buttons = [{ ... }, { ... }];
> > chrome.notifications.create(..., { buttons: buttons, ... });
> > buttons.length = 0; // Should not cause buttons to not appear in the
> > notification.
> 
> The JSON copy has 2 important properties:
> 1) Custom bindings can change the copy without affecting client data
structures
> 2) Client program can run while the API is asynchronously downloading images,
> and change the input object without affecting the data passed to Chrome in the
> custom bindings.
> 
> At the time, extension bindings did not work with prototype properties; i.e.
> given var b = Object.create(a), passing b to the bindings would cause a crash
> since the schema validator would work but the c++ code would not grab the
> properties from a.  Even if it did work, it would be susceptible to #2 above.

There are many other extension APIs that suffer from 1 or 2, see
https://crbug.com/436588. I believe that we should not develop custom
serialization solutions for each potentially affected extension API, but make
deep cloning a part of the extension parameter validator. If you want to discuss
this further, just follow up on the related bug instead of here to avoid
fragmentation of the discussion.

[dewittj, 2015-01-21 18:46:28]

what is the motivation for the optional/required change?  Consistency among
apis?

Conceptually lgtm, but I leave it to kalman to decide about the relative
importance of consistency vs forward compatibility.

[not at google - send to devlin, 2015-01-21 22:21:19]

I'm still reviewing this, but I'm not sure it's worth it overall. Adding
documentation like "this function is optional from Chrome 41 onward" is accurate
but cognitive noise for developers. I'm ok conceding that making it required was
a mistake that we need to live with forever (and there are other APIs with
required callbacks that shouldn't be). Either way it would be good to submit the
better error messages and update callers of lastError.run.

https://codereview.chromium.org/855813002/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/notifications_custom_bindings.js
(right):

https://codereview.chromium.org/855813002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/notifications_custom_bindings.js:113: //
TODO(dewittj): Remove this hack. This is used as a way to deep
On 2015/01/20 22:58:44, robwu wrote:
> On 2015/01/20 22:46:51, dewittj wrote:
> > On 2015/01/20 22:34:01, robwu wrote:
> > > On 2015/01/20 17:41:35, kalman wrote:
> > > > The best way to deep copy would be to expose a native function which
uses
> > > > WebSerializedScriptValue.serialize(obj).deserialize(). I think the TODO
> > should
> > > > be changed to reflect that. The main advantage over JSON is that
recursive
> > > > objects wouldn't crash; it may or may not be any quicker.
> > > 
> > > Recursive objects are invalid for this API, so that is not something that
we
> > > have to worry about.
> > > 
> > > I *think* that the purpose of deep cloning the object is to make sure that
> > > changing the object that is being passed in to the function after the
> function
> > > call does not change the behavior of the first function call.
> > > 
> > > var buttons = [{ ... }, { ... }];
> > > chrome.notifications.create(..., { buttons: buttons, ... });
> > > buttons.length = 0; // Should not cause buttons to not appear in the
> > > notification.
> > 
> > The JSON copy has 2 important properties:
> > 1) Custom bindings can change the copy without affecting client data
> structures
> > 2) Client program can run while the API is asynchronously downloading
images,
> > and change the input object without affecting the data passed to Chrome in
the
> > custom bindings.
> > 
> > At the time, extension bindings did not work with prototype properties; i.e.
> > given var b = Object.create(a), passing b to the bindings would cause a
crash
> > since the schema validator would work but the c++ code would not grab the
> > properties from a.  Even if it did work, it would be susceptible to #2
above.
> 
> There are many other extension APIs that suffer from 1 or 2, see
> https://crbug.com/436588. I believe that we should not develop custom
> serialization solutions for each potentially affected extension API, but make
> deep cloning a part of the extension parameter validator. If you want to
discuss
> this further, just follow up on the related bug instead of here to avoid
> fragmentation of the discussion.

I'm sure that copying is necessary. I was addressing the fact that I presume the
TODO is to avoid copying via JSON and instead copying via <some other method>,
and a good <some other method> is using SerializedScriptValue (the same
serialization scheme that postMessage and IndexedDB use).

https://codereview.chromium.org/855813002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/notifications_custom_bindings.js:131: }
You could avoid needing to expose reportUncheckedLastError publicly with the
equivalent of

lastError.run(name, error, stack, failure_function, [callback || function(){},
id])

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/last_error.js (right):

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/last_error.js:116: if (!hasAccessed(targetChrome))
On 2015/01/20 22:34:01, robwu wrote:
> On 2015/01/20 17:41:35, kalman wrote:
> > This doesn't look right, it'll always print an error no matter whether the
> > function succeeded or failed.
> 
> This is correct. Function run sets chrome.runtime.lastError, which should be
> printed to the console if the user did not check its value.

What am I missing here? If I do:

chrome.someNamespace.someFunction.get(function() {
  // code
});

and that implementation uses lastError.run (most don't), then if it *succeeds*
with an unchecked error then i'll still print "Unchecked runtime.lastError...".

[robwu, 2015-01-21 23:21:09]

On 2015/01/21 22:21:19, kalman wrote:
> I'm still reviewing this, but I'm not sure it's worth it overall. Adding
> documentation like "this function is optional from Chrome 41 onward" is
accurate
> but cognitive noise for developers. I'm ok conceding that making it required
was
> a mistake that we need to live with forever (and there are other APIs with
> required callbacks that shouldn't be).

The only disadvantage of making this parameter optional is that extensions which
have been designed and tested in the then-current version of Chrome does not
work in old versions of Chrome. This disadvantage becomes non-relevant within a
few release cycles, because users should not use old versions of Chrome any way,
and developers who care about supporting outdated Chrome versions have to
actually test their extension in that old version.

The main advantage of making this parameter optional is API consistency. I
haven't yet seen an extension API where the callback was required even though it
did not make sense.

When I used this API for the first time, I *expected* the callback to be
optional. The fact that it did not was confusing: "Perhaps there is something
that I have to check after creating the notification? No. Hmm. Then why is it
required?"

https://codereview.chromium.org/855813002/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/notifications_custom_bindings.js
(right):

https://codereview.chromium.org/855813002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/notifications_custom_bindings.js:113: //
TODO(dewittj): Remove this hack. This is used as a way to deep
On 2015/01/21 22:21:19, kalman wrote:
> On 2015/01/20 22:58:44, robwu wrote:
> > On 2015/01/20 22:46:51, dewittj wrote:
> > > On 2015/01/20 22:34:01, robwu wrote:
> > > > On 2015/01/20 17:41:35, kalman wrote:
> > > > > The best way to deep copy would be to expose a native function which
> uses
> > > > > WebSerializedScriptValue.serialize(obj).deserialize(). I think the
TODO
> > > should
> > > > > be changed to reflect that. The main advantage over JSON is that
> recursive
> > > > > objects wouldn't crash; it may or may not be any quicker.
> > > > 
> > > > Recursive objects are invalid for this API, so that is not something
that
> we
> > > > have to worry about.
> > > > 
> > > > I *think* that the purpose of deep cloning the object is to make sure
that
> > > > changing the object that is being passed in to the function after the
> > function
> > > > call does not change the behavior of the first function call.
> > > > 
> > > > var buttons = [{ ... }, { ... }];
> > > > chrome.notifications.create(..., { buttons: buttons, ... });
> > > > buttons.length = 0; // Should not cause buttons to not appear in the
> > > > notification.
> > > 
> > > The JSON copy has 2 important properties:
> > > 1) Custom bindings can change the copy without affecting client data
> > structures
> > > 2) Client program can run while the API is asynchronously downloading
> images,
> > > and change the input object without affecting the data passed to Chrome in
> the
> > > custom bindings.
> > > 
> > > At the time, extension bindings did not work with prototype properties;
i.e.
> > > given var b = Object.create(a), passing b to the bindings would cause a
> crash
> > > since the schema validator would work but the c++ code would not grab the
> > > properties from a.  Even if it did work, it would be susceptible to #2
> above.
> > 
> > There are many other extension APIs that suffer from 1 or 2, see
> > https://crbug.com/436588. I believe that we should not develop custom
> > serialization solutions for each potentially affected extension API, but
make
> > deep cloning a part of the extension parameter validator. If you want to
> discuss
> > this further, just follow up on the related bug instead of here to avoid
> > fragmentation of the discussion.
> 
> I'm sure that copying is necessary. I was addressing the fact that I presume
the
> TODO is to avoid copying via JSON and instead copying via <some other method>,
> and a good <some other method> is using SerializedScriptValue (the same
> serialization scheme that postMessage and IndexedDB use).

Yep, I understood that, and I pointed out that copying is just a quick hack to
get immutability of the parameters. A way to achieve this goal is to make
immutable properties a part of the validator, i.e. let the validator return a
(partial) copy of the sanitized parameters.

https://codereview.chromium.org/855813002/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/notifications_custom_bindings.js:131: }
On 2015/01/21 22:21:19, kalman wrote:
> You could avoid needing to expose reportUncheckedLastError publicly with the
> equivalent of
> 
> lastError.run(name, error, stack, failure_function, [callback || function(){},
> id])

Done. Though reportUncheckedLastError has to be public anyway, because of the
use in send_request.js and within last_error.js itself.

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/last_error.js (right):

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/last_error.js:116: if (!hasAccessed(targetChrome))
On 2015/01/21 22:21:19, kalman wrote:
> On 2015/01/20 22:34:01, robwu wrote:
> > On 2015/01/20 17:41:35, kalman wrote:
> > > This doesn't look right, it'll always print an error no matter whether the
> > > function succeeded or failed.
> > 
> > This is correct. Function run sets chrome.runtime.lastError, which should be
> > printed to the console if the user did not check its value.
> 
> What am I missing here? If I do:
> 
> chrome.someNamespace.someFunction.get(function() {
>   // code
> });
> 
> and that implementation uses lastError.run (most don't), then if it *succeeds*
> with an unchecked error then i'll still print "Unchecked
runtime.lastError...".

callback is a user-supplied callback. That "// code" ought to read the
chrome.runtime.lastError property. If the code does not, then we have to assume
that the developer does not know about the error and print the error to the
console.

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/last_error.js:130: function
reportUncheckedLastError(name, message, stack) {
On 2015/01/20 22:34:01, robwu wrote:
> On 2015/01/20 17:41:35, kalman wrote:
> > Given that this method is under the "lastError" namespace, "reportUnchecked"
> > would be more accurate. Also, I think it should assert that there actually
is
> a
> > last error at this point.
> 
> If we couple this method with the chrome object, then we'd better use the
> following instead, to make sure that the reported error is consistent to
what's
> assigned to lastError:
> 
> function reportIfUnchecked(name, targetChrome, stack) {
>   var error = hasAccessed(targetChrome) && chrome.runtime.lastError;
>   if (!error)
>     return;
>   console.error("Unchecked runtime.lastError while running " +
>       (name || "unknown") + ": " + error.message + (stack ? "\n" + stack :
""));
> }

Done.

[not at google - send to devlin, 2015-01-21 23:24:32]

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/last_error.js (right):

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/last_error.js:116: if (!hasAccessed(targetChrome))
On 2015/01/21 23:21:09, robwu wrote:
> On 2015/01/21 22:21:19, kalman wrote:
> > On 2015/01/20 22:34:01, robwu wrote:
> > > On 2015/01/20 17:41:35, kalman wrote:
> > > > This doesn't look right, it'll always print an error no matter whether
the
> > > > function succeeded or failed.
> > > 
> > > This is correct. Function run sets chrome.runtime.lastError, which should
be
> > > printed to the console if the user did not check its value.
> > 
> > What am I missing here? If I do:
> > 
> > chrome.someNamespace.someFunction.get(function() {
> >   // code
> > });
> > 
> > and that implementation uses lastError.run (most don't), then if it
*succeeds*
> > with an unchecked error then i'll still print "Unchecked
> runtime.lastError...".
> 
> callback is a user-supplied callback. That "// code" ought to read the
> chrome.runtime.lastError property. If the code does not, then we have to
assume
> that the developer does not know about the error and print the error to the
> console.

but if there isn't an error, it'll still print a non-existent error.

[robwu, 2015-01-21 23:28:18]

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/last_error.js (right):

https://codereview.chromium.org/855813002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/last_error.js:116: if (!hasAccessed(targetChrome))
On 2015/01/21 23:24:32, kalman wrote:
> On 2015/01/21 23:21:09, robwu wrote:
> > On 2015/01/21 22:21:19, kalman wrote:
> > > On 2015/01/20 22:34:01, robwu wrote:
> > > > On 2015/01/20 17:41:35, kalman wrote:
> > > > > This doesn't look right, it'll always print an error no matter whether
> the
> > > > > function succeeded or failed.
> > > > 
> > > > This is correct. Function run sets chrome.runtime.lastError, which
should
> be
> > > > printed to the console if the user did not check its value.
> > > 
> > > What am I missing here? If I do:
> > > 
> > > chrome.someNamespace.someFunction.get(function() {
> > >   // code
> > > });
> > > 
> > > and that implementation uses lastError.run (most don't), then if it
> *succeeds*
> > > with an unchecked error then i'll still print "Unchecked
> > runtime.lastError...".
> > 
> > callback is a user-supplied callback. That "// code" ought to read the
> > chrome.runtime.lastError property. If the code does not, then we have to
> assume
> > that the developer does not know about the error and print the error to the
> > console.
> 
> but if there isn't an error, it'll still print a non-existent error.

A few lines earlier, chrome.runtime.lastError is set (set(name, message, ...)).
That's the whole point of calling this function, not? So, if lastError is not
checked, there is certainly a real error that needs to be reported.

[not at google - send to devlin, 2015-01-21 23:32:45]

Ok you're right, I misread run(). This would be a lot simpler if we didn't try
to modify the notifications API though.

[robwu, 2015-01-21 23:42:45]

On 2015/01/21 23:32:45, kalman wrote:
> Ok you're right, I misread run(). This would be a lot simpler if we didn't try
> to modify the notifications API though.

The main objective of this patch was declaring the notifications callbacks as
optional.
I also improved the lastError API as I found flaws in its implementation while
looking over it. If you want to, I can split up the patch so you can review it
independently.

[not at google - send to devlin, 2015-01-21 23:51:02]

that would help, thanks.

[robwu, 2015-01-22 14:23:40]

On 2015/01/21 23:51:02, kalman wrote:
> that would help, thanks.

Moved changes to lastError to https://codereview.chromium.org/865083003/.
The new patch is just a few lines (see comment 10 for my reply to your question
about the merits of this patch).

WDYT of the current patch?

[robwu, 2015-01-27 22:32:38]

On 2015/01/22 14:23:40, robwu wrote:
> On 2015/01/21 23:51:02, kalman wrote:
> > that would help, thanks.
> 
> Moved changes to lastError to https://codereview.chromium.org/865083003/.
> The new patch is just a few lines (see comment 10 for my reply to your
question
> about the merits of this patch).
> 
> WDYT of the current patch?

Ping Ben?

[not at google - send to devlin, 2015-01-28 17:48:01]

lgtm

https://codereview.chromium.org/855813002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/api/notifications.idl (right):

https://codereview.chromium.org/855813002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/notifications.idl:133: // that represents the
created notification.
Could you also add "optional since Chrome 42" to each of these? We can also file
a bug to remove that once 42 hits stable (good enough).

[robwu, 2015-01-28 18:06:46]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2015-01-28 18:07:31]

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

[robwu, 2015-01-28 18:22:19]

The CQ bit was unchecked by rob@robwu.nl

[robwu, 2015-01-28 18:24:10]

kalman@, while we are at it, we can as well mark the notificationId parameter of
the create/update methods as optional.
Marking it as optional makes much more sense than specifying an empty string.
WDYT?

[not at google - send to devlin, 2015-01-28 18:32:54]

Alright, sure why not, with the same warning I suppose.

[robwu, 2015-02-02 23:32:17]

kalman@: I've marked ID as optional and added a test. PTAL.

[not at google - send to devlin, 2015-02-03 18:08:20]

lgtm

https://codereview.chromium.org/855813002/diff/120001/chrome/common/extension...
File chrome/common/extensions/api/notifications.idl (right):

https://codereview.chromium.org/855813002/diff/120001/chrome/common/extension...
chrome/common/extensions/api/notifications.idl:131: // The parameter was
required before Chrome 42.
s/was/is/g

same below.

could you also put it on its own line so that the schema generation picks it up?
I'd rather it render as its own paragraph in the docs.

[robwu, 2015-02-03 18:13:08]

https://codereview.chromium.org/855813002/diff/120001/chrome/common/extension...
File chrome/common/extensions/api/notifications.idl (right):

https://codereview.chromium.org/855813002/diff/120001/chrome/common/extension...
chrome/common/extensions/api/notifications.idl:131: // The parameter was
required before Chrome 42.
On 2015/02/03 18:08:20, kalman wrote:
> s/was/is/g
> 
> same below.
> 
> could you also put it on its own line so that the schema generation picks it
up?
> I'd rather it render as its own paragraph in the docs.

Done.

[robwu, 2015-02-03 18:13:15]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2015-02-03 18:13:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/855813002/140001

[commit-bot: I haz the power, 2015-02-03 19:31:38]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-03 19:32:47]

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