Desktop Notifications API

This code is for the Desktop Notifications API being worked on in Chromium.  

WebKit support for the Javascript API is still pending with a patch out for review (see https://bugs.webkit.org/show_bug.cgi?id=25463), so the current code can't actually be checked in, but I would like preliminary feedback.

[jorlow, 2009-08-05 23:55:58]

I haven't reviewed any of the browser/ code yet.

http://codereview.appspot.com/104044/diff/1008/112
File chrome/app/theme/theme_resources.grd (right):

http://codereview.appspot.com/104044/diff/1008/112#newcode284
Line 284: 
Interesting...we're not going to try to do this with vector graphics and/or
allow this to be tweaked by users/themes?

http://codereview.appspot.com/104044/diff/1008/136
File chrome/common/notification_type.h (right):

http://codereview.appspot.com/104044/diff/1008/136#newcode646
Line 646: 
Why doesn't the next sentence continue here?

http://codereview.appspot.com/104044/diff/1008/139
File chrome/common/render_messages_internal.h (right):

http://codereview.appspot.com/104044/diff/1008/139#newcode613
Line 613: 
These should be plausible variable names.  (I.e. no spaces and concise)

http://codereview.appspot.com/104044/diff/1008/139#newcode624
Line 624: 
-'s and ?'s are not allowed for that reason

http://codereview.appspot.com/104044/diff/1008/140
File chrome/renderer/notification_provider.cc (right):

http://codereview.appspot.com/104044/diff/1008/140#newcode26
Line 26: GURL(view_->webview()->GetMainFrame()->GetURL()),
indent these 1 space more

either it should be 

blah_blah_blah(
    start here,
    continue here)

or 

blah_blah_blah(start here,
               continue here)

http://codereview.appspot.com/104044/diff/1008/140#newcode34
Line 34: const string16& title,
If you can fit more than one of these per line (while maintaining the indenting
you have) we typically do so.

http://codereview.appspot.com/104044/diff/1008/140#newcode98
Line 98: RendererNotificationManager::PermissionRequestDone);
Typically these functions are named OnBlahBlah rather than just BlahBlah

http://codereview.appspot.com/104044/diff/1008/140#newcode121
Line 121: void RunnableMethodTraits<WebNotification>::RetainCallee(
Hm...this should probably be in render_messages.h right?

http://codereview.appspot.com/104044/diff/1008/140#newcode133
Line 133: NewRunnableMethod(notification, &WebNotification::display));
indent 4 spaces, not 2

http://codereview.appspot.com/104044/diff/1008/140#newcode150
Line 150: proxy_table_.Remove(id);
improperly indented

http://codereview.appspot.com/104044/diff/1008/140#newcode158
Line 158: callback);
4 not 2

http://codereview.appspot.com/104044/diff/1008/140#newcode159
Line 159: callback_table_.Remove(id);
0 not 2

http://codereview.appspot.com/104044/diff/1008/141
File chrome/renderer/notification_provider.h (right):

http://codereview.appspot.com/104044/diff/1008/141#newcode46
Line 46: RenderView* view_;
Use DISALLOW_blah blah blah?

http://codereview.appspot.com/104044/diff/1008/141#newcode52
Line 52: return Singleton<RendererNotificationManager>::get();
Singletons should be avoided whenever possible since they make shutdown less
deterministic.  Is there something that can create this and own it?  I know that
in the browser process, we avoid singletons at pretty much any cost.

http://codereview.appspot.com/104044/diff/1008/141#newcode70
Line 70: };
Use DISALLOW_blah blah blah?

http://codereview.appspot.com/104044/diff/1008/143
File chrome/renderer/render_view.h (right):

http://codereview.appspot.com/104044/diff/1008/143#newcode828
Line 828: 
typo

http://codereview.appspot.com/104044/diff/1008/144
File webkit/build/V8Bindings/build-generated-files.sh (right):

http://codereview.appspot.com/104044/diff/1008/144#newcode46
Line 46: 
Are you sure this is needed?  I didn't have to touch this for DOM_STORAGE...I
think it was just the webkit.gyp stuff that took care of it.

http://codereview.appspot.com/104044/diff/1008/145
File webkit/build/webkit_common_defines.vsprops (right):

http://codereview.appspot.com/104044/diff/1008/145#newcode1
Line 1: <?xml version="1.0" encoding="Windows-1252"?>
Hmm....why is this file needed?

http://codereview.appspot.com/104044/diff/1008/148
File webkit/glue/webnotification.h (right):

http://codereview.appspot.com/104044/diff/1008/148#newcode5
Line 5: #ifndef WEBKIT_GLUE_NOTIFICATION_H_
:-(  Does this have to be done in Glue because of the hooks into WebView?  Have
you tried putting it in the WebKit API?

Note that if you need to access the web view delegate for a couple tiny things
(that'll go away once it's in the WebKit API) then you could use
webkit/api/src/TemporaryGlue.h like the media player code does.

http://codereview.appspot.com/104044/diff/1008/148#newcode8
Line 8: #include "base/string16.h"
If all else is equal, you should probably go ahead and have this use
WebString.h's to begin with...because, even if it doesn't go there now, it will
go into the WebKit API...which can't depend on base.

http://codereview.appspot.com/104044/diff/1008/148#newcode12
Line 12: public:
glue code needs to follow google style.  This means public is indented 1 space
and the rest 1 more.

In WebKit style (if you're in the API) public is not indented but everything
else is 4 spaces in.

http://codereview.appspot.com/104044/diff/1008/150
File webkit/glue/webnotification_impl.h (right):

http://codereview.appspot.com/104044/diff/1008/150#newcode23
Line 23: int id() { return id_; }
int id() const { ...

http://codereview.appspot.com/104044/diff/1008/150#newcode27
Line 27: WebCore::Notification* inner_;
This name is not very descriptive.

http://codereview.appspot.com/104044/diff/1008/152
File webkit/glue/webnotificationpresenter_impl.cc (right):

http://codereview.appspot.com/104044/diff/1008/152#newcode9
Line 9: #include "webnotificationpresenter_impl.h"
Shouldn't this go right under config.h?

http://codereview.appspot.com/104044/diff/1008/152#newcode20
Line 20: if (webworker_) {
{}'s seem unneeded

http://codereview.appspot.com/104044/diff/1008/152#newcode98
Line 98: 
no extra new line

http://codereview.appspot.com/104044/diff/1008/152#newcode106
Line 106: 
no extra new line

http://codereview.appspot.com/104044/diff/1008/152#newcode110
Line 110: if (result == WebNotificationPresenter::PERMISSION_ALLOWED) {
{}'s are unneeded

http://codereview.appspot.com/104044/diff/1008/153
File webkit/glue/webnotificationpresenter_impl.h (right):

http://codereview.appspot.com/104044/diff/1008/153#newcode14
Line 14: 
extra newline?

[johnnyg, 2009-08-06 00:29:28]

http://codereview.appspot.com/104044/diff/1008/112
File chrome/app/theme/theme_resources.grd (right):

http://codereview.appspot.com/104044/diff/1008/112#newcode284
Line 284: 
On 2009/08/05 23:55:58, jorlow wrote:
> Interesting...we're not going to try to do this with vector graphics and/or
> allow this to be tweaked by users/themes?

Good point.  All the UI will definitely need review, so I didn't get too deep
into it just to re-write it later.  This was written well before themes, but I
agree we should hook into that now: we want to make these look like they came
from Chrome, so they should match the main windows.

http://codereview.appspot.com/104044/diff/1008/139
File chrome/common/render_messages_internal.h (right):

http://codereview.appspot.com/104044/diff/1008/139#newcode613
Line 613: 
On 2009/08/05 23:55:58, jorlow wrote:
> These should be plausible variable names.  (I.e. no spaces and concise)

Thanks, good to know.

http://codereview.appspot.com/104044/diff/1008/141
File chrome/renderer/notification_provider.h (right):

http://codereview.appspot.com/104044/diff/1008/141#newcode52
Line 52: return Singleton<RendererNotificationManager>::get();
On 2009/08/05 23:55:58, jorlow wrote:
> Singletons should be avoided whenever possible since they make shutdown less
> deterministic.  Is there something that can create this and own it?  I know
that
> in the browser process, we avoid singletons at pretty much any cost.

Whoops... Good to know! I used singletons in both processes. I will explore some
more to find a place to own it or can you suggest a good candidate?

http://codereview.appspot.com/104044/diff/1008/148
File webkit/glue/webnotification.h (right):

http://codereview.appspot.com/104044/diff/1008/148#newcode5
Line 5: #ifndef WEBKIT_GLUE_NOTIFICATION_H_
On 2009/08/05 23:55:58, jorlow wrote:
> :-(  Does this have to be done in Glue because of the hooks into WebView? 
Have
> you tried putting it in the WebKit API?
> 
> Note that if you need to access the web view delegate for a couple tiny things
> (that'll go away once it's in the WebKit API) then you could use
> webkit/api/src/TemporaryGlue.h like the media player code does.

Yes, this is sort of a high-level merge conflict, since I wrote all this before
a lot of things got moved into the WebKit API.  I would like to put this in the
WebKit API as well.

[jorlow, 2009-08-06 00:39:03]

http://codereview.appspot.com/104044/diff/1008/141
File chrome/renderer/notification_provider.h (right):

http://codereview.appspot.com/104044/diff/1008/141#newcode52
Line 52: return Singleton<RendererNotificationManager>::get();
On 2009/08/06 00:29:28, johnnyg wrote:
> On 2009/08/05 23:55:58, jorlow wrote:
> > Singletons should be avoided whenever possible since they make shutdown less
> > deterministic.  Is there something that can create this and own it?  I know
> that
> > in the browser process, we avoid singletons at pretty much any cost.
> 
> Whoops... Good to know! I used singletons in both processes. I will explore
some
> more to find a place to own it or can you suggest a good candidate?

Unfortunately, I'm not sure.  Maybe look for something that all renderviewhost's
have access to?

[darin, 2009-08-06 04:03:58]

http://codereview.appspot.com/104044/diff/1008/148
File webkit/glue/webnotification.h (right):

http://codereview.appspot.com/104044/diff/1008/148#newcode11
Line 11: class WebNotification {
This interface should not be defined in glue.  We need it to live in the WebKit
API eventually, so it should be defined there.  If the implementation has too
many connections back to WebView / WebFrame, then it is OK for the
implementation to live in glue temporarily.  Otherwise, if you only need a very
little bit from WebView / WebFrame, we might be able to have the implementation
also live in webkit/api/src, possibly by adding necessary connections back to
glue via TemporaryGlue.h (this file will be deleted once WebView / WebFrame make
their way into api/src).

[PhistucK, 2009-08-06 04:37:03]

Just copyright nits :P

http://codereview.appspot.com/104044/diff/1008/114
File chrome/browser/notifications/balloon.h (right):

http://codereview.appspot.com/104044/diff/1008/114#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009?

http://codereview.appspot.com/104044/diff/1008/115
File chrome/browser/notifications/balloon_contents_win.cc (right):

http://codereview.appspot.com/104044/diff/1008/115#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009?

http://codereview.appspot.com/104044/diff/1008/116
File chrome/browser/notifications/balloon_contents_win.h (right):

http://codereview.appspot.com/104044/diff/1008/116#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009?

http://codereview.appspot.com/104044/diff/1008/117
File chrome/browser/notifications/balloon_view_win.cc (right):

http://codereview.appspot.com/104044/diff/1008/117#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009?

http://codereview.appspot.com/104044/diff/1008/118
File chrome/browser/notifications/balloon_view_win.h (right):

http://codereview.appspot.com/104044/diff/1008/118#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009?

http://codereview.appspot.com/104044/diff/1008/119
File chrome/browser/notifications/balloon_win.cc (right):

http://codereview.appspot.com/104044/diff/1008/119#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009?

http://codereview.appspot.com/104044/diff/1008/125
File chrome/browser/notifications/notification.h (right):

http://codereview.appspot.com/104044/diff/1008/125#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009?

http://codereview.appspot.com/104044/diff/1008/126
File chrome/browser/notifications/notification_manager.h (right):

http://codereview.appspot.com/104044/diff/1008/126#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009?

http://codereview.appspot.com/104044/diff/1008/127
File chrome/browser/notifications/notification_manager_win.cc (right):

http://codereview.appspot.com/104044/diff/1008/127#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009?

http://codereview.appspot.com/104044/diff/1008/129
File chrome/browser/notifications/notification_object_proxy.h (right):

http://codereview.appspot.com/104044/diff/1008/129#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009?

[johnnyg, 2009-08-06 18:52:42]

http://codereview.appspot.com/104044/diff/1008/114
File chrome/browser/notifications/balloon.h (right):

http://codereview.appspot.com/104044/diff/1008/114#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/08/06 04:37:03, PhistucK wrote:
> 2009?

Done.

http://codereview.appspot.com/104044/diff/1008/115
File chrome/browser/notifications/balloon_contents_win.cc (right):

http://codereview.appspot.com/104044/diff/1008/115#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/08/06 04:37:03, PhistucK wrote:
> 2009?

Done.

http://codereview.appspot.com/104044/diff/1008/116
File chrome/browser/notifications/balloon_contents_win.h (right):

http://codereview.appspot.com/104044/diff/1008/116#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/08/06 04:37:03, PhistucK wrote:
> 2009?

Done.

http://codereview.appspot.com/104044/diff/1008/117
File chrome/browser/notifications/balloon_view_win.cc (right):

http://codereview.appspot.com/104044/diff/1008/117#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/08/06 04:37:03, PhistucK wrote:
> 2009?

Done.

http://codereview.appspot.com/104044/diff/1008/118
File chrome/browser/notifications/balloon_view_win.h (right):

http://codereview.appspot.com/104044/diff/1008/118#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/08/06 04:37:03, PhistucK wrote:
> 2009?

Done.

http://codereview.appspot.com/104044/diff/1008/119
File chrome/browser/notifications/balloon_win.cc (right):

http://codereview.appspot.com/104044/diff/1008/119#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/08/06 04:37:03, PhistucK wrote:
> 2009?

Done.

http://codereview.appspot.com/104044/diff/1008/139
File chrome/common/render_messages_internal.h (right):

http://codereview.appspot.com/104044/diff/1008/139#newcode613
Line 613: 
On 2009/08/06 00:29:28, johnnyg wrote:
> On 2009/08/05 23:55:58, jorlow wrote:
> > These should be plausible variable names.  (I.e. no spaces and concise)
> 
> Thanks, good to know.

Done.

http://codereview.appspot.com/104044/diff/1008/139#newcode624
Line 624: 
On 2009/08/05 23:55:58, jorlow wrote:
> -'s and ?'s are not allowed for that reason

Done.

http://codereview.appspot.com/104044/diff/1008/140
File chrome/renderer/notification_provider.cc (right):

http://codereview.appspot.com/104044/diff/1008/140#newcode26
Line 26: GURL(view_->webview()->GetMainFrame()->GetURL()),
On 2009/08/05 23:55:58, jorlow wrote:
> indent these 1 space more
> 
> either it should be 
> 
> blah_blah_blah(
>     start here,
>     continue here)
> 
> or 
> 
> blah_blah_blah(start here,
>                continue here)

Done.

http://codereview.appspot.com/104044/diff/1008/140#newcode34
Line 34: const string16& title,
On 2009/08/05 23:55:58, jorlow wrote:
> If you can fit more than one of these per line (while maintaining the
indenting
> you have) we typically do so.

Done.

http://codereview.appspot.com/104044/diff/1008/140#newcode98
Line 98: RendererNotificationManager::PermissionRequestDone);
On 2009/08/05 23:55:58, jorlow wrote:
> Typically these functions are named OnBlahBlah rather than just BlahBlah 

Done.

http://codereview.appspot.com/104044/diff/1008/140#newcode121
Line 121: void RunnableMethodTraits<WebNotification>::RetainCallee(
On 2009/08/05 23:55:58, jorlow wrote:
> Hm...this should probably be in render_messages.h right?

I see this pattern a few other places, generally nearby to the specific class
that's being used in a task.

http://codereview.appspot.com/104044/diff/1008/140#newcode133
Line 133: NewRunnableMethod(notification, &WebNotification::display));
On 2009/08/05 23:55:58, jorlow wrote:
> indent 4 spaces, not 2

Done.

http://codereview.appspot.com/104044/diff/1008/140#newcode150
Line 150: proxy_table_.Remove(id);
On 2009/08/05 23:55:58, jorlow wrote:
> improperly indented

Done.

http://codereview.appspot.com/104044/diff/1008/140#newcode158
Line 158: callback);
On 2009/08/05 23:55:58, jorlow wrote:
> 4 not 2

Done.

http://codereview.appspot.com/104044/diff/1008/140#newcode159
Line 159: callback_table_.Remove(id);
On 2009/08/05 23:55:58, jorlow wrote:
> 0 not 2

Done.

http://codereview.appspot.com/104044/diff/1008/141
File chrome/renderer/notification_provider.h (right):

http://codereview.appspot.com/104044/diff/1008/141#newcode46
Line 46: RenderView* view_;
On 2009/08/05 23:55:58, jorlow wrote:
> Use DISALLOW_blah blah blah?

Done.

http://codereview.appspot.com/104044/diff/1008/141#newcode70
Line 70: };
On 2009/08/05 23:55:58, jorlow wrote:
> Use DISALLOW_blah blah blah?

Done.

http://codereview.appspot.com/104044/diff/1008/143
File chrome/renderer/render_view.h (right):

http://codereview.appspot.com/104044/diff/1008/143#newcode828
Line 828: 
On 2009/08/05 23:55:58, jorlow wrote:
> typo

Done.

http://codereview.appspot.com/104044/diff/1008/150
File webkit/glue/webnotification_impl.h (right):

http://codereview.appspot.com/104044/diff/1008/150#newcode23
Line 23: int id() { return id_; }
On 2009/08/05 23:55:58, jorlow wrote:
> int id() const { ...

Done.

http://codereview.appspot.com/104044/diff/1008/150#newcode27
Line 27: WebCore::Notification* inner_;
On 2009/08/05 23:55:58, jorlow wrote:
> This name is not very descriptive.

Done.

http://codereview.appspot.com/104044/diff/1008/152
File webkit/glue/webnotificationpresenter_impl.cc (right):

http://codereview.appspot.com/104044/diff/1008/152#newcode9
Line 9: #include "webnotificationpresenter_impl.h"
On 2009/08/05 23:55:58, jorlow wrote:
> Shouldn't this go right under config.h?

Done.

http://codereview.appspot.com/104044/diff/1008/152#newcode20
Line 20: if (webworker_) {
On 2009/08/05 23:55:58, jorlow wrote:
> {}'s seem unneeded

Done.

http://codereview.appspot.com/104044/diff/1008/152#newcode98
Line 98: 
On 2009/08/05 23:55:58, jorlow wrote:
> no extra new line

Done.

http://codereview.appspot.com/104044/diff/1008/152#newcode106
Line 106: 
On 2009/08/05 23:55:58, jorlow wrote:
> no extra new line

Done.