Hook up API to notification_ui_manager.

Another interim checkin. This CL completes the plumbing between our nascent API and the existing desktop notification UI.

Next step will be to create a new peer class to the existing desktop notification class, then create a new IPC message (or alter the existing one) to invoke it. Then we will have a subsystem that's entirely our own to start UI iteration.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161993

[miket_OOO, 2012-10-12 18:27:33]

http://codereview.chromium.org/11116016/diff/8/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/api_function.h (right):

http://codereview.chromium.org/11116016/diff/8/chrome/browser/extensions/api/...
chrome/browser/extensions/api/api_function.h:15: class ApiFunction : public
UIThreadExtensionFunction {
mihaip and aa have inquired about the future of this family of extension
functions. I'm following up on that separately and might do a grand unifying CL
that refactors all those classes.

[dharcourt, 2012-10-12 22:38:50]

LGTM. A couple of questions for my education in the comments.

http://codereview.chromium.org/11116016/diff/8/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/notification/notification_api.cc (right):

http://codereview.chromium.org/11116016/diff/8/chrome/browser/extensions/api/...
chrome/browser/extensions/api/notification/notification_api.cc:78: src_id_ =
ExtractSrcId(options_dict.get());
Presumably src_id will be used by the NotificationDelegate to send notifications
to the JavaScript... is that right?

http://codereview.chromium.org/11116016/diff/8/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/notification/notification_api.h (right):

http://codereview.chromium.org/11116016/diff/8/chrome/browser/extensions/api/...
chrome/browser/extensions/api/notification/notification_api.h:26: //
UIThreadExtensionFunction:
For my education: Both the ApiFunction superclass and the
UIThreadExtensionFunction class in the comment have a RunImpl() method, but I
can't see how the two are tied together so that this would be an override of the
UIThreadExtensionFunction RunImpl() rather than the ApiFunction one. How are
they associated?

[miket_OOO, 2012-10-15 17:13:11]

http://codereview.chromium.org/11116016/diff/8/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/notification/notification_api.cc (right):

http://codereview.chromium.org/11116016/diff/8/chrome/browser/extensions/api/...
chrome/browser/extensions/api/notification/notification_api.cc:78: src_id_ =
ExtractSrcId(options_dict.get());
On 2012/10/12 22:38:50, Charles d.Harcourt wrote:
> Presumably src_id will be used by the NotificationDelegate to send
notifications
> to the JavaScript... is that right?

That's right. When an ApiResource is created, the infrastructure assigns a
unique srcId that binds the resource to the owning extension/app, and that id is
used to report stuff back from the resource to the owner. The exact mechanism is
easier to explain in person, but in a nutshell it piggybacks the id on the
options dictionary passed in to whichever API creates the resource, so the C++
side of that API can look into the dictionary and see a srcId field that
magically appears even though the JS caller didn't pass anything in.

http://codereview.chromium.org/11116016/diff/8/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/notification/notification_api.h (right):

http://codereview.chromium.org/11116016/diff/8/chrome/browser/extensions/api/...
chrome/browser/extensions/api/notification/notification_api.h:26: //
UIThreadExtensionFunction:
On 2012/10/12 22:38:50, Charles d.Harcourt wrote:
> For my education: Both the ApiFunction superclass and the
> UIThreadExtensionFunction class in the comment have a RunImpl() method, but I
> can't see how the two are tied together so that this would be an override of
the
> UIThreadExtensionFunction RunImpl() rather than the ApiFunction one. How are
> they associated?

I think you're asking a general C++ question, but in the chance I'm
misinterpreting and you've actually pointed out a bug, please read with a
skeptical eye.

The virtual keyword indicates that the decision which version the overridden
function to call is made at runtime depending on which class the object actually
is. So in this case, if it's an ApiFunction object, it'll call the ApiFunction's
implementation of that method.

It's worth pointing out that ApiFunction doesn't implement that method, so if a
derived class doesn't implement it either, it'll go up the class hierarchy until
it finds an implementation (if it were a pure virtual, meaning the base class
specified that it must be implemented, then it wouldn't have compiled). This is
in contrast to your "the ApiFunction superclass ... [has] a RunImpl() method,"
which isn't the case.

Does this answer your question?

[commit-bot: I haz the power, 2012-10-15 18:20:31]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[miket_OOO, 2012-10-15 18:22:42]

Hi Antony.

[asargent_no_longer_on_chrome, 2012-10-15 19:26:25]

lgtm

https://chromiumcodereview.appspot.com/11116016/diff/8/chrome/browser/extensi...
File chrome/browser/extensions/api/notification/notification_api.cc (right):

https://chromiumcodereview.appspot.com/11116016/diff/8/chrome/browser/extensi...
chrome/browser/extensions/api/notification/notification_api.cc:96:
SendResponse(true);
nit: Since you always call SendResponse here, can't this class just inherit from
SyncExtensionFunction instead of UIThreadExtensionFunction?

https://chromiumcodereview.appspot.com/11116016/diff/8/chrome/browser/extensi...
File chrome/browser/extensions/api/notification/notification_api.h (right):

https://chromiumcodereview.appspot.com/11116016/diff/8/chrome/browser/extensi...
chrome/browser/extensions/api/notification/notification_api.h:22:
DECLARE_EXTENSION_FUNCTION_NAME("experimental.notification.show")
style nit: I think we usually put the DECLARE_EXTENSION_FUNCTION_NAME macro at
the bottom of the class. Consider slavish adherence to this convention.

[commit-bot: I haz the power, 2012-10-15 20:01:08]

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

[commit-bot: I haz the power, 2012-10-15 23:16:34]

Change committed as 161993