Add a Create method to DesktopNotificationHandler and stubs for the notification objects.

Adds a GYP flag for desktop notifications (enabled by default) and stub out relevant code if set to 0.

In chrome/browser/task_manager I moved notifications related code to their own files and allow exclusion and stubbing via GYP. Similarly I have added stubs for other parts including a chrome/browser/notifications/notification_stubs.cc which takes care of classes within that directory.

I also created some static methods in DesktopNotificationService to wrap commonly used notification related code so that we reduce the amount of stubs required.

BUG=none
TEST=existing notification tests succeed

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

[Satish, 2011-05-31 15:05:40]

Looks like I missed some parts of the change, uploading a new patch shortly..
please hold off on reviewing this one.

[Torne, 2011-06-02 16:18:35]

LGTM generally

http://codereview.chromium.org/7053041/diff/3001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/7053041/diff/3001/build/common.gypi#newcode437
build/common.gypi:437: 'enable_desktop_notifications%': 0,
I assume you have this set to 0 just for trybots and will set it to 1 to submit?

http://codereview.chromium.org/7053041/diff/3001/chrome/browser/desktop_notif...
File chrome/browser/desktop_notification_handler.cc (right):

http://codereview.chromium.org/7053041/diff/3001/chrome/browser/desktop_notif...
chrome/browser/desktop_notification_handler.cc:24: // static
Reorder the constructor/create method in this file to match header order?

[John Gregg, 2011-06-03 00:13:54]

http://codereview.chromium.org/7053041/diff/3023/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.h (right):

http://codereview.chromium.org/7053041/diff/3023/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.h:136: Profile*
profile);
This doesn't make sense to me.  Perhaps this class is over-broadly named, but
the point of it is to connect web pages to the notification UI.

Other places in browser code (which might call your new function), could just
call into the notification UI themselves, basically what the impl of this
function does.  Why doesn't that work?

[Satish, 2011-06-03 07:17:03]

http://codereview.chromium.org/7053041/diff/3023/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.h (right):

http://codereview.chromium.org/7053041/diff/3023/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.h:136: Profile*
profile);
On 2011/06/03 00:13:54, John Gregg wrote:
> This doesn't make sense to me.  Perhaps this class is over-broadly named, but
> the point of it is to connect web pages to the notification UI.
> 
> Other places in browser code (which might call your new function), could just
> call into the notification UI themselves, basically what the impl of this
> function does.  Why doesn't that work?

I moved this code here to reduce surface area of calls to the notification code
and stubs required. Do you have any suggestions where else I could move this
method?

[Satish, 2011-06-03 12:29:54]

New patch uploaded addressing all concerns so far, please take a look.

http://codereview.chromium.org/7053041/diff/3023/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.h (right):

http://codereview.chromium.org/7053041/diff/3023/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.h:136: Profile*
profile);
On 2011/06/03 00:13:54, John Gregg wrote:
> This doesn't make sense to me.  Perhaps this class is over-broadly named, but
> the point of it is to connect web pages to the notification UI.
> 
> Other places in browser code (which might call your new function), could just
> call into the notification UI themselves, basically what the impl of this
> function does.  Why doesn't that work?

I have now removed this and rolled back the calling code to what they were
before. It required adding just 2 more mock methods which seems fine.

[John Gregg, 2011-06-06 17:12:27]

http://codereview.chromium.org/7053041/diff/15023/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/7053041/diff/15023/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.cc:596: return
prefs_cache()->HasPermission(origin.GetOrigin());
In what context is this function called?  See the comments for prefs_cache() --
it's meant literally as a cache for the use of the IPC handler without jumping
to the UI thread.  If you're on the UI thread here, you should look directly at
the content settings.

http://codereview.chromium.org/7053041/diff/15023/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager.cc (right):

http://codereview.chromium.org/7053041/diff/15023/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager.cc:75: PrefService*
local_state, BalloonCollection* balloons) {
nit: i think the style preference here is one per line

http://codereview.chromium.org/7053041/diff/15023/content/browser/renderer_ho...
File content/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/7053041/diff/15023/content/browser/renderer_ho...
content/browser/renderer_host/render_message_filter.cc:631:
WebKit::WebNotificationPresenter::PermissionNotAllowed;
I think this is fine, but it would be good to carefully document the thread
behavior that you expect.  So add a comment and a DCHECK that
DesktopNotificationService::HasPermission is only expected to be called on the
IO thread.

[Satish, 2011-06-06 20:36:43]

Thanks for the review. Replies inline and a new patch uploaded.

http://codereview.chromium.org/7053041/diff/15023/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/7053041/diff/15023/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.cc:596: return
prefs_cache()->HasPermission(origin.GetOrigin());
On 2011/06/06 17:12:27, John Gregg wrote:
> In what context is this function called?  See the comments for prefs_cache()
--
> it's meant literally as a cache for the use of the IPC handler without jumping
> to the UI thread.  If you're on the UI thread here, you should look directly
at
> the content settings.

This should be called from the IO thread alone, added a DCHECK here.

http://codereview.chromium.org/7053041/diff/15023/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager.cc (right):

http://codereview.chromium.org/7053041/diff/15023/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager.cc:75: PrefService*
local_state, BalloonCollection* balloons) {
On 2011/06/06 17:12:27, John Gregg wrote:
> nit: i think the style preference here is one per line

Done.

http://codereview.chromium.org/7053041/diff/15023/content/browser/renderer_ho...
File content/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/7053041/diff/15023/content/browser/renderer_ho...
content/browser/renderer_host/render_message_filter.cc:631:
WebKit::WebNotificationPresenter::PermissionNotAllowed;
On 2011/06/06 17:12:27, John Gregg wrote:
> I think this is fine, but it would be good to carefully document the thread
> behavior that you expect.  So add a comment and a DCHECK that
> DesktopNotificationService::HasPermission is only expected to be called on the
> IO thread.

Done, added DCHECK as mentioned above.

[John Gregg, 2011-06-06 20:44:22]

LGTM

On 2011/06/06 20:36:43, Satish wrote:
> Thanks for the review. Replies inline and a new patch uploaded.
> 
>
http://codereview.chromium.org/7053041/diff/15023/chrome/browser/notification...
> File chrome/browser/notifications/desktop_notification_service.cc (right):
> 
>
http://codereview.chromium.org/7053041/diff/15023/chrome/browser/notification...
> chrome/browser/notifications/desktop_notification_service.cc:596: return
> prefs_cache()->HasPermission(origin.GetOrigin());
> On 2011/06/06 17:12:27, John Gregg wrote:
> > In what context is this function called?  See the comments for prefs_cache()
> --
> > it's meant literally as a cache for the use of the IPC handler without
jumping
> > to the UI thread.  If you're on the UI thread here, you should look directly
> at
> > the content settings.
> 
> This should be called from the IO thread alone, added a DCHECK here.
> 
>
http://codereview.chromium.org/7053041/diff/15023/chrome/browser/notification...
> File chrome/browser/notifications/notification_ui_manager.cc (right):
> 
>
http://codereview.chromium.org/7053041/diff/15023/chrome/browser/notification...
> chrome/browser/notifications/notification_ui_manager.cc:75: PrefService*
> local_state, BalloonCollection* balloons) {
> On 2011/06/06 17:12:27, John Gregg wrote:
> > nit: i think the style preference here is one per line
> 
> Done.
> 
>
http://codereview.chromium.org/7053041/diff/15023/content/browser/renderer_ho...
> File content/browser/renderer_host/render_message_filter.cc (right):
> 
>
http://codereview.chromium.org/7053041/diff/15023/content/browser/renderer_ho...
> content/browser/renderer_host/render_message_filter.cc:631:
> WebKit::WebNotificationPresenter::PermissionNotAllowed;
> On 2011/06/06 17:12:27, John Gregg wrote:
> > I think this is fine, but it would be good to carefully document the thread
> > behavior that you expect.  So add a comment and a DCHECK that
> > DesktopNotificationService::HasPermission is only expected to be called on
the
> > IO thread.
> 
> Done, added DCHECK as mentioned above.

[Satish, 2011-06-06 21:03:45]

Adding John Abd-El-Malek to look and approve the files under
content/browser/renderer_host

[jam, 2011-06-06 21:16:22]

lgtm

http://codereview.chromium.org/7053041/diff/13023/content/browser/renderer_ho...
File content/browser/renderer_host/render_message_filter.cc (left):

http://codereview.chromium.org/7053041/diff/13023/content/browser/renderer_ho...
content/browser/renderer_host/render_message_filter.cc:21: #include
"chrome/browser/notifications/notifications_prefs_cache.h"
woohoo!  please take out this file from content\browser\DEPS as well

[jam, 2011-06-08 17:49:48]

hmm, I had only looked at the content change earlier.  Now that I see this, I
really dislike this approach of adding empty stubs to avoid linker errors.  I
think we should revert this change, and come up with a better way of removing
features at compile time.  This approach is not really maintainable, i.e.
everyone has to remember to edit another file when they add/remove functions, or
else another builder fails.  It also seems like a quick hack to get things
linking, which we should avoid putting in trunk chrome.