Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
194108:
adds DesktopNotificationService to Profile (Closed)
|
Created:
11 years, 3 months ago by John Gregg Modified:
9 years ago CC:
chromium-reviews_googlegroups.com Visibility:
Public. |
Descriptionadds DesktopNotificationService to Profile
part of the browser-side code for desktop notifications. UI code is in separate CL.
Patch Set 1 #Patch Set 2 : lint fixes #
Total comments: 80
Patch Set 3 : deal with threading #Patch Set 4 : fix merge problem #
Total comments: 5
Patch Set 5 : address feedback #
Total comments: 43
Patch Set 6 : address feedback #Patch Set 7 : untabify #
Total comments: 54
Patch Set 8 : now with more GURLs #
Total comments: 35
Patch Set 9 : code review #
Total comments: 19
Patch Set 10 : extract prefs cache, other feedback #Patch Set 11 : more feedback #
Total comments: 10
Messages
Total messages: 41 (0 generated)
This patch is still bigger than I like to review. I think the profile stuff can be split out from the actual manager implementation. But here's a first pass. http://codereview.chromium.org/194108/diff/1004/1010 File chrome/browser/notifications/desktop_notification_service.cc (right): http://codereview.chromium.org/194108/diff/1004/1010#newcode38 Line 38: int route_id, more on one line http://codereview.chromium.org/194108/diff/1004/1010#newcode46 Line 46: RenderViewHost::FromID(process_id_, route_id_)->Send(new if possible, new on the next line http://codereview.chromium.org/194108/diff/1004/1011 File chrome/browser/notifications/desktop_notification_service.h (right): http://codereview.chromium.org/194108/diff/1004/1011#newcode22 Line 22: explicit DesktopNotificationService(NotificationUIManager* ui_manager) : : on next line http://codereview.chromium.org/194108/diff/1004/1011#newcode23 Line 23: ui_manager_(ui_manager) {} } on next line http://codereview.chromium.org/194108/diff/1004/1011#newcode47 Line 47: bool ShowDesktopNotification(Profile* profile, Not sure, but I think it's better to do bool Blahblahblah(First first, Second second, Third third, Fourth ..., ...); since these are so long http://codereview.chromium.org/194108/diff/1004/1011#newcode66 Line 66: friend class NotificationPermissionInfoBarDelegate; Are you sure this is necessary? Would be nice if we could avoid it. http://codereview.chromium.org/194108/diff/1004/1012 File chrome/browser/notifications/desktop_notification_service_mac.mm (right): http://codereview.chromium.org/194108/diff/1004/1012#newcode7 Line 7: #include "Cocoa/cocoa.h" <>'s instead of ""s? http://codereview.chromium.org/194108/diff/1004/1012#newcode29 Line 29: const string16& text, put more ont he same line....i'm sure this also applies elsewhere http://codereview.chromium.org/194108/diff/1004/1012#newcode34 Line 34: // TODO(johnnyg): Integration with Growl will go here. Coming soon. NOTIMPLEMENTED() http://codereview.chromium.org/194108/diff/1004/1013 File chrome/browser/notifications/desktop_notification_service_win.cc (right): http://codereview.chromium.org/194108/diff/1004/1013#newcode17 Line 17: #include "chrome/common/child_process_host.h" alphabetical http://codereview.chromium.org/194108/diff/1004/1013#newcode23 Line 23: // No empty //'s http://codereview.chromium.org/194108/diff/1004/1013#newcode29 Line 29: const string16& title, you can fit more on each line http://codereview.chromium.org/194108/diff/1004/1013#newcode77 Line 77: SiteInstance* site_instance = SiteInstance::CreateSiteInstance(profile_); So this has to happen on the UI thread? http://codereview.chromium.org/194108/diff/1004/1013#newcode103 Line 103: const GURL& url, don't need to all be on their own lines http://codereview.chromium.org/194108/diff/1004/1013#newcode111 Line 111: route_id, see comment below http://codereview.chromium.org/194108/diff/1004/1013#newcode118 Line 118: space probably unnecessary http://codereview.chromium.org/194108/diff/1004/1013#newcode127 Line 127: const GURL& origin, don't need to all be on their own lines http://codereview.chromium.org/194108/diff/1004/1013#newcode137 Line 137: process_id, More of these can go on either line....you're call if the 2nd line lines up with ( or 4 spaces in from new http://codereview.chromium.org/194108/diff/1004/1014 File chrome/browser/notifications/notification.h (right): http://codereview.chromium.org/194108/diff/1004/1014#newcode56 Line 56: void operator=(const Notification&); Just use the macro maybe? http://codereview.chromium.org/194108/diff/1004/1015 File chrome/browser/notifications/notification_object_proxy.cc (right): http://codereview.chromium.org/194108/diff/1004/1015#newcode11 Line 11: #include "chrome/common/worker_messages.h" alphabetical http://codereview.chromium.org/194108/diff/1004/1015#newcode55 Line 55: io_loop_->PostTask(FROM_HERE, Can you prove that this will never be called after the io_loop_ has been shut down? If so, please document it in the header files somewhere. If not, it needs to be. :-) http://codereview.chromium.org/194108/diff/1004/1015#newcode79 Line 79: io_loop_->PostTask(FROM_HERE, It seems like there'd be a standard way to do this...like RunnableMethod http://codereview.chromium.org/194108/diff/1004/1016 File chrome/browser/notifications/notification_object_proxy.h (right): http://codereview.chromium.org/194108/diff/1004/1016#newcode15 Line 15: // attached JS listeners will be invoked in the renderer or worker process. Your classes should all explain what threads they're intended to be used on. http://codereview.chromium.org/194108/diff/1004/1016#newcode27 Line 27: virtual void display(); Why virtual? http://codereview.chromium.org/194108/diff/1004/1016#newcode48 Line 48: #endif // #ifndef CHROME_BROWSER_NOTIFICATIONS_NOTIFICATION_OBJECT_PROXY_H_ Don't need the #ifndef part http://codereview.chromium.org/194108/diff/1004/1017 File chrome/browser/notifications/notification_ui_manager.h (right): http://codereview.chromium.org/194108/diff/1004/1017#newcode19 Line 19: typedef std::deque<QueuedNotification*> QueuedNotifications; This doesn't really sound like a type name....NotificationsDeque maybe? http://codereview.chromium.org/194108/diff/1004/1017#newcode30 Line 30: BalloonCollection* collection = new BalloonCollection(); Why couldnt' this just be done in the constructor? Either way, why is it inline? http://codereview.chromium.org/194108/diff/1004/1017#newcode41 Line 41: SiteInstance* site_instance) { put on previous line http://codereview.chromium.org/194108/diff/1004/1017#newcode50 Line 50: NotificationUIManager() : balloons_observer_(0), balloon_collection_(0) { } Are there any examples of this elsewhere? I've always seen it like Blah : blah_(), blah() { } Even when it's not necessary (like here) http://codereview.chromium.org/194108/diff/1004/1018 File chrome/browser/profile.cc (right): http://codereview.chromium.org/194108/diff/1004/1018#newcode429 Line 429: return profile_->GetDesktopNotificationService(); So incognito has no difference in how the service works? http://codereview.chromium.org/194108/diff/1004/1018#newcode1283 Line 1283: if (!desktop_notification_service_.get()) { is there any way to DCHECK that we're on the UI thread? http://codereview.chromium.org/194108/diff/1004/1018#newcode1289 Line 1289: if (io_thread) { don't need {}'s http://codereview.chromium.org/194108/diff/1004/1018#newcode1290 Line 1290: desktop_notification_service_->Initialize(io_thread->message_loop()); Why does it need to be initialized like this? It should be able to grab the message loop via ChromeThread::GetMessageLoop, right? And are you guarenteed that it'll be shut down (so it won't try to use the loop anymore) before the io thread goes away? http://codereview.chromium.org/194108/diff/1004/1020 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/194108/diff/1004/1020#newcode842 Line 842: IPC_MESSAGE_HANDLER(ViewHostMsg_CheckNotificationPermission, This is supposed to be sync, right!? How can this work? http://codereview.chromium.org/194108/diff/1004/1020#newcode1657 Line 1657: DesktopNotificationService* serv = Maybe create a helper function to do this for you (in this class). http://codereview.chromium.org/194108/diff/1004/1020#newcode1664 Line 1664: int callback_context) { this can go on the above line, right? http://codereview.chromium.org/194108/diff/1004/1021 File chrome/browser/renderer_host/render_view_host.h (right): http://codereview.chromium.org/194108/diff/1004/1021#newcode583 Line 583: void OnCheckNotificationPermission(const string16& origin, int*); put a variable name here http://codereview.chromium.org/194108/diff/1004/1021#newcode584 Line 584: void OnRequestNotificationPermission(const string16& origin, int); put a variable name here http://codereview.chromium.org/194108/diff/1004/1028 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/194108/diff/1004/1028#newcode667 Line 667: // Used to inform the renderer that the browser has displayed its This file is in the other CL....so whichever wins should commit it, I guess.
http://codereview.chromium.org/194108/diff/1004/1028 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/194108/diff/1004/1028#newcode1599 Line 1599: IPC_SYNC_MESSAGE_ROUTED1_1(ViewHostMsg_CheckNotificationPermission, We avoid sync messages to the UI thread at all costs since it can lead to deadlock. Can you modify the logic in the renderer so that this is an async message?
I had trouble with this particular IPC message -- I did get an assert when I first created it that told me I had to pump messages in order to handle a sync IPC on the UI thread without deadlocks. So I followed those instructions and added msg->EnableMessagePumping() at the creation site (which is in my other pending changelist 194079). I guess that's not enough? I was surprised that there was no way to access a profile preference without being on the UI thread, which is what this message needs to do. That would be another way to fix the problem, if it can be done.
On Tue, Sep 15, 2009 at 3:39 PM, <johnnyg@chromium.org> wrote: > Reviewers: Jeremy Orlow, John Abd-El-Malek, > > Message: > > I had trouble with this particular IPC message -- I did get an assert when > I > first created it that told me I had to pump messages in order to handle a > sync > IPC on the UI thread without deadlocks. So I followed those instructions > and > added msg->EnableMessagePumping() at the creation site (which is in my > other > pending changelist 194079). I guess that's not enough? > > I was surprised that there was no way to access a profile preference > without > being on the UI thread, which is what this message needs to do. That would > be > another way to fix the problem, if it can be done. > Given that you're doing UI related things, I suspect you need to be on the UI thread. If not, DOM Storage is a good example of how you can save off any profile info you might need for access elsewhere. > > Description: > adds DesktopNotificationService to Profile > > part of the browser-side code for desktop notifications. UI code is in > separate > CL. > > Please review this at http://codereview.chromium.org/194108 > > Affected files: > M chrome/app/chromium_strings.grd > M chrome/app/generated_resources.grd > M chrome/browser/automation/automation_profile_impl.h > M chrome/browser/browser_resources.grd > A chrome/browser/notifications/balloons.h > A chrome/browser/notifications/desktop_notification_service.h > A chrome/browser/notifications/desktop_notification_service.cc > A chrome/browser/notifications/desktop_notification_service_mac.mm > A chrome/browser/notifications/desktop_notification_service_win.cc > A chrome/browser/notifications/notification.h > A chrome/browser/notifications/notification_object_proxy.h > A chrome/browser/notifications/notification_object_proxy.cc > A chrome/browser/notifications/notification_ui_manager.h > M chrome/browser/profile.h > M chrome/browser/profile.cc > M chrome/browser/renderer_host/render_view_host.h > M chrome/browser/renderer_host/render_view_host.cc > A chrome/browser/resources/notification.html > M chrome/chrome.gyp > M chrome/common/chrome_switches.h > M chrome/common/chrome_switches.cc > M chrome/common/pref_names.h > M chrome/common/pref_names.cc > M chrome/common/render_messages_internal.h > M chrome/test/testing_profile.h > > >
On Tue, Sep 15, 2009 at 3:39 PM, <johnnyg@chromium.org> wrote: > > Reviewers: Jeremy Orlow, John Abd-El-Malek, > > Message: > > I had trouble with this particular IPC message -- I did get an assert when > I > first created it that told me I had to pump messages in order to handle a > sync > IPC on the UI thread without deadlocks. So I followed those instructions > and > added msg->EnableMessagePumping() at the creation site (which is in my > other > pending changelist 194079). I guess that's not enough? > No. This means you're running a nested message loop which opens up the door to all sorts of strange bugs. Can't you change the logic in the renderer to do this asynchronously? It is always harder to do this, but we've managed to do it for all the use cases except the ones that need it by design (i.e. showModalDialog). > > I was surprised that there was no way to access a profile preference > without > being on the UI thread, which is what this message needs to do. That would > be > another way to fix the problem, if it can be done. > > Description: > adds DesktopNotificationService to Profile > > part of the browser-side code for desktop notifications. UI code is in > separate > CL. > > Please review this at http://codereview.chromium.org/194108 > > Affected files: > M chrome/app/chromium_strings.grd > M chrome/app/generated_resources.grd > M chrome/browser/automation/automation_profile_impl.h > M chrome/browser/browser_resources.grd > A chrome/browser/notifications/balloons.h > A chrome/browser/notifications/desktop_notification_service.h > A chrome/browser/notifications/desktop_notification_service.cc > A chrome/browser/notifications/desktop_notification_service_mac.mm > A chrome/browser/notifications/desktop_notification_service_win.cc > A chrome/browser/notifications/notification.h > A chrome/browser/notifications/notification_object_proxy.h > A chrome/browser/notifications/notification_object_proxy.cc > A chrome/browser/notifications/notification_ui_manager.h > M chrome/browser/profile.h > M chrome/browser/profile.cc > M chrome/browser/renderer_host/render_view_host.h > M chrome/browser/renderer_host/render_view_host.cc > A chrome/browser/resources/notification.html > M chrome/chrome.gyp > M chrome/common/chrome_switches.h > M chrome/common/chrome_switches.cc > M chrome/common/pref_names.h > M chrome/common/pref_names.cc > M chrome/common/render_messages_internal.h > M chrome/test/testing_profile.h > > >
> No. This means you're running a nested message loop which opens up the door > to all sorts of strange bugs. > > Can't you change the logic in the renderer to do this asynchronously? It is > always harder to do this, but we've managed to do it for all the use cases > except the ones that need it by design (i.e. showModalDialog). I'm sure I can -- do you know a specific example I can look at where the JS api is synchronous but it's handled via an async IPC?
On Tue, Sep 15, 2009 at 4:41 PM, <johnnyg@chromium.org> wrote: > > No. This means you're running a nested message loop which opens up the >> door >> to all sorts of strange bugs. >> > > Can't you change the logic in the renderer to do this asynchronously? It >> is >> always harder to do this, but we've managed to do it for all the use cases >> except the ones that need it by design (i.e. showModalDialog). >> > > I'm sure I can -- do you know a specific example I can look at where the JS > api > is synchronous but it's handled via an async IPC? If JS requires the result synchronously, there is no way to send this using an async message. Since this is a prototype, can the API be changed to be asynchronous? If not, can whatever data that's required be available/cached on the IO thread? > > http://codereview.chromium.org/194108 >
thanks for all the comments! I've incorporated most of them, and am working on replacing my UI-thread sync IPC. looks like my other issue is how to deal with io_thread shutdown and cleanly tearing down my objects, would appreciate pointers to what is the best practice on that. http://codereview.chromium.org/194108/diff/1004/1010 File chrome/browser/notifications/desktop_notification_service.cc (right): http://codereview.chromium.org/194108/diff/1004/1010#newcode38 Line 38: int route_id, On 2009/09/15 22:23:01, Jeremy Orlow wrote: > more on one line Done. http://codereview.chromium.org/194108/diff/1004/1010#newcode46 Line 46: RenderViewHost::FromID(process_id_, route_id_)->Send(new On 2009/09/15 22:23:01, Jeremy Orlow wrote: > if possible, new on the next line Done. http://codereview.chromium.org/194108/diff/1004/1011 File chrome/browser/notifications/desktop_notification_service.h (right): http://codereview.chromium.org/194108/diff/1004/1011#newcode22 Line 22: explicit DesktopNotificationService(NotificationUIManager* ui_manager) : On 2009/09/15 22:23:01, Jeremy Orlow wrote: > : on next line moved this ctor to the .cc file http://codereview.chromium.org/194108/diff/1004/1011#newcode23 Line 23: ui_manager_(ui_manager) {} On 2009/09/15 22:23:01, Jeremy Orlow wrote: > } on next line Done. http://codereview.chromium.org/194108/diff/1004/1011#newcode47 Line 47: bool ShowDesktopNotification(Profile* profile, On 2009/09/15 22:23:01, Jeremy Orlow wrote: > Not sure, but I think it's better to do > > bool Blahblahblah(First first, Second second, > Third third, Fourth ..., > ...); > > since these are so long Done. http://codereview.chromium.org/194108/diff/1004/1011#newcode66 Line 66: friend class NotificationPermissionInfoBarDelegate; On 2009/09/15 22:23:01, Jeremy Orlow wrote: > Are you sure this is necessary? Would be nice if we could avoid it. I can make the 2 methods the info-bar calls public. http://codereview.chromium.org/194108/diff/1004/1013 File chrome/browser/notifications/desktop_notification_service_win.cc (right): http://codereview.chromium.org/194108/diff/1004/1013#newcode17 Line 17: #include "chrome/common/child_process_host.h" On 2009/09/15 22:23:01, Jeremy Orlow wrote: > alphabetical Done. http://codereview.chromium.org/194108/diff/1004/1013#newcode23 Line 23: // On 2009/09/15 22:23:01, Jeremy Orlow wrote: > No empty //'s Done. http://codereview.chromium.org/194108/diff/1004/1013#newcode29 Line 29: const string16& title, On 2009/09/15 22:23:01, Jeremy Orlow wrote: > you can fit more on each line Done. http://codereview.chromium.org/194108/diff/1004/1013#newcode77 Line 77: SiteInstance* site_instance = SiteInstance::CreateSiteInstance(profile_); On 2009/09/15 22:23:01, Jeremy Orlow wrote: > So this has to happen on the UI thread? Yes. This tasks calls the notification ui manager to actually show a notification, so it's on the UI thread. Is there a particular problem with doing it, or do I just need better documentation about threading? http://codereview.chromium.org/194108/diff/1004/1014 File chrome/browser/notifications/notification.h (right): http://codereview.chromium.org/194108/diff/1004/1014#newcode56 Line 56: void operator=(const Notification&); On 2009/09/15 22:23:01, Jeremy Orlow wrote: > Just use the macro maybe? I want to allow copying, just not assigning. the macro prevents both. http://codereview.chromium.org/194108/diff/1004/1015 File chrome/browser/notifications/notification_object_proxy.cc (right): http://codereview.chromium.org/194108/diff/1004/1015#newcode11 Line 11: #include "chrome/common/worker_messages.h" On 2009/09/15 22:23:01, Jeremy Orlow wrote: > alphabetical Done. http://codereview.chromium.org/194108/diff/1004/1015#newcode55 Line 55: io_loop_->PostTask(FROM_HERE, On 2009/09/15 22:23:01, Jeremy Orlow wrote: > Can you prove that this will never be called after the io_loop_ has been shut > down? If so, please document it in the header files somewhere. If not, it > needs to be. :-) i probably can't guarantee it right now. can I detect the shutdown of the IO thread somehow? http://codereview.chromium.org/194108/diff/1004/1015#newcode79 Line 79: io_loop_->PostTask(FROM_HERE, On 2009/09/15 22:23:01, Jeremy Orlow wrote: > It seems like there'd be a standard way to do this...like RunnableMethod i can use a static method and RunnableFunction rather than a task, but i can't use a RunnableMethod with a this pointer because in the case of close(), the proxy object might be deleted afterwards, so it can't safely defer a call on itself. http://codereview.chromium.org/194108/diff/1004/1016 File chrome/browser/notifications/notification_object_proxy.h (right): http://codereview.chromium.org/194108/diff/1004/1016#newcode27 Line 27: virtual void display(); On 2009/09/15 22:23:01, Jeremy Orlow wrote: > Why virtual? yeah, not necessary here http://codereview.chromium.org/194108/diff/1004/1016#newcode48 Line 48: #endif // #ifndef CHROME_BROWSER_NOTIFICATIONS_NOTIFICATION_OBJECT_PROXY_H_ On 2009/09/15 22:23:01, Jeremy Orlow wrote: > Don't need the #ifndef part Done. http://codereview.chromium.org/194108/diff/1004/1017 File chrome/browser/notifications/notification_ui_manager.h (right): http://codereview.chromium.org/194108/diff/1004/1017#newcode19 Line 19: typedef std::deque<QueuedNotification*> QueuedNotifications; On 2009/09/15 22:23:01, Jeremy Orlow wrote: > This doesn't really sound like a type name....NotificationsDeque maybe? okay. http://codereview.chromium.org/194108/diff/1004/1017#newcode30 Line 30: BalloonCollection* collection = new BalloonCollection(); On 2009/09/15 22:23:01, Jeremy Orlow wrote: > Why couldnt' this just be done in the constructor? > > Either way, why is it inline? I wanted the constructor to be uninitializing so I could initialize with mock objects instead in unit tests. But I can move it to the impl file. http://codereview.chromium.org/194108/diff/1004/1017#newcode41 Line 41: SiteInstance* site_instance) { On 2009/09/15 22:23:01, Jeremy Orlow wrote: > put on previous line Done. http://codereview.chromium.org/194108/diff/1004/1017#newcode50 Line 50: NotificationUIManager() : balloons_observer_(0), balloon_collection_(0) { } On 2009/09/15 22:23:01, Jeremy Orlow wrote: > Are there any examples of this elsewhere? I've always seen it like > > Blah > : blah_(), blah() { > } > > Even when it's not necessary (like here) I have seen this before, but i'll change it. http://codereview.chromium.org/194108/diff/1004/1018 File chrome/browser/profile.cc (right): http://codereview.chromium.org/194108/diff/1004/1018#newcode429 Line 429: return profile_->GetDesktopNotificationService(); On 2009/09/15 22:23:01, Jeremy Orlow wrote: > So incognito has no difference in how the service works? not currently, but i need to figure out a plan for incognito. http://codereview.chromium.org/194108/diff/1004/1018#newcode1283 Line 1283: if (!desktop_notification_service_.get()) { On 2009/09/15 22:23:01, Jeremy Orlow wrote: > is there any way to DCHECK that we're on the UI thread? Done. http://codereview.chromium.org/194108/diff/1004/1018#newcode1289 Line 1289: if (io_thread) { On 2009/09/15 22:23:01, Jeremy Orlow wrote: > don't need {}'s Done. http://codereview.chromium.org/194108/diff/1004/1018#newcode1290 Line 1290: desktop_notification_service_->Initialize(io_thread->message_loop()); On 2009/09/15 22:23:01, Jeremy Orlow wrote: > Why does it need to be initialized like this? It should be able to grab the > message loop via ChromeThread::GetMessageLoop, right? > > And are you guarenteed that it'll be shut down (so it won't try to use the loop > anymore) before the io thread goes away? I may need some pointers on how to deal with that. How can I detect the shutdown? http://codereview.chromium.org/194108/diff/1004/1020 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/194108/diff/1004/1020#newcode842 Line 842: IPC_MESSAGE_HANDLER(ViewHostMsg_CheckNotificationPermission, On 2009/09/15 22:23:01, Jeremy Orlow wrote: > This is supposed to be sync, right!? How can this work? yeah that needs to change... i'm pumping messages which allows it work, but i guess that's not a good way. http://codereview.chromium.org/194108/diff/1004/1020#newcode1664 Line 1664: int callback_context) { On 2009/09/15 22:23:01, Jeremy Orlow wrote: > this can go on the above line, right? Done. http://codereview.chromium.org/194108/diff/1004/1021 File chrome/browser/renderer_host/render_view_host.h (right): http://codereview.chromium.org/194108/diff/1004/1021#newcode583 Line 583: void OnCheckNotificationPermission(const string16& origin, int*); On 2009/09/15 22:23:01, Jeremy Orlow wrote: > put a variable name here Done. http://codereview.chromium.org/194108/diff/1004/1021#newcode584 Line 584: void OnRequestNotificationPermission(const string16& origin, int); On 2009/09/15 22:23:01, Jeremy Orlow wrote: > put a variable name here Done.
> looks like my other issue is how to deal with io_thread shutdown and cleanly > tearing down my objects, would appreciate pointers to what is the best practice > on that. What exact issues are you worried about? That some objects might be leaked because the code that calls them is on the UI thread, and when it runs the IO thread might have gone away? Generally we don't care too much about leaks in that case, since the browser is shutting down. But if there's something that you really need to do, you can call MessageLoop::AddDestructionObserver to get notified (on the IO thread) before it goes away. If you're trying to use the io thread from a non-ui thread (i.e. file or other thread) and you want to know when the message loop pointer is valid, what we do is go to the ui thread and from there check if io_thread() is valid. http://codereview.chromium.org/194108/diff/1004/1015 File chrome/browser/notifications/notification_object_proxy.cc (right): http://codereview.chromium.org/194108/diff/1004/1015#newcode79 Line 79: io_loop_->PostTask(FROM_HERE, On 2009/09/18 19:11:33, John Gregg wrote: > On 2009/09/15 22:23:01, Jeremy Orlow wrote: > > It seems like there'd be a standard way to do this...like RunnableMethod > > i can use a static method and RunnableFunction rather than a task, but i can't > use a RunnableMethod with a this pointer because in the case of close(), the > proxy object might be deleted afterwards, so it can't safely defer a call on > itself. why not just make the proxy object ref counted? Then RunnableMethod will work fine and all this file will be cleaner.
> What exact issues are you worried about? That some objects might be leaked > because the code that calls them is on the UI thread, and when it runs the IO > thread might have gone away? Generally we don't care too much about leaks in > that case, since the browser is shutting down. But if there's something that > you really need to do, you can call MessageLoop::AddDestructionObserver to get > notified (on the IO thread) before it goes away. > > If you're trying to use the io thread from a non-ui thread (i.e. file or other > thread) and you want to know when the message loop pointer is valid, what we do > is go to the ui thread and from there check if io_thread() is valid. I have some objects (like the NotificationObjectProxy) which are caching a pointer to the IO MessageLoop, and the concern was that when that object is used on the UI thread and tries to send off an IPC, that pointer might be invalid. Sounds like instead of caching that pointer at all I should check g_browser_process->io_thread() and grab the message loop from there ? > why not just make the proxy object ref counted? Then RunnableMethod will work > fine and all this file will be cleaner. oh, yes, actually the proxy is already ref counted. so I can just use RunnableMethod. thanks!
On Fri, Sep 18, 2009 at 12:50 PM, <johnnyg@chromium.org> wrote: > What exact issues are you worried about? That some objects might be leaked >> because the code that calls them is on the UI thread, and when it runs the >> IO >> thread might have gone away? Generally we don't care too much about leaks >> in >> that case, since the browser is shutting down. But if there's something >> that >> you really need to do, you can call MessageLoop::AddDestructionObserver to >> get >> notified (on the IO thread) before it goes away. >> > > If you're trying to use the io thread from a non-ui thread (i.e. file or >> other >> thread) and you want to know when the message loop pointer is valid, what >> we >> > do > >> is go to the ui thread and from there check if io_thread() is valid. >> > > I have some objects (like the NotificationObjectProxy) which are caching a > pointer to the IO MessageLoop, and the concern was that when that object is > used > on the UI thread and tries to send off an IPC, that pointer might be > invalid. > Sounds like instead of caching that pointer at all I should check > g_browser_process->io_thread() and grab the message loop from there ? yep, caching that pointer isn't safe, so always use g_browser_process->io_thread() > > why not just make the proxy object ref counted? Then RunnableMethod will >> work >> fine and all this file will be cleaner. >> > > oh, yes, actually the proxy is already ref counted. so I can just use > RunnableMethod. thanks! np :) > > > http://codereview.chromium.org/194108 >
On Fri, Sep 18, 2009 at 1:46 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Fri, Sep 18, 2009 at 12:50 PM, <johnnyg@chromium.org> wrote: > >> What exact issues are you worried about? That some objects might be >>> leaked >>> because the code that calls them is on the UI thread, and when it runs >>> the IO >>> thread might have gone away? Generally we don't care too much about >>> leaks in >>> that case, since the browser is shutting down. But if there's something >>> that >>> you really need to do, you can call MessageLoop::AddDestructionObserver >>> to get >>> notified (on the IO thread) before it goes away. >>> >> >> If you're trying to use the io thread from a non-ui thread (i.e. file or >>> other >>> thread) and you want to know when the message loop pointer is valid, what >>> we >>> >> do >> >>> is go to the ui thread and from there check if io_thread() is valid. >>> >> >> I have some objects (like the NotificationObjectProxy) which are caching a >> pointer to the IO MessageLoop, and the concern was that when that object >> is used >> on the UI thread and tries to send off an IPC, that pointer might be >> invalid. >> Sounds like instead of caching that pointer at all I should check >> g_browser_process->io_thread() and grab the message loop from there ? > > > yep, caching that pointer isn't safe, so always use > g_browser_process->io_thread() > Yeah. The IO thread is killed by the UI thread, so doing so is safe. > > >> >> why not just make the proxy object ref counted? Then RunnableMethod will >>> work >>> fine and all this file will be cleaner. >>> >> >> oh, yes, actually the proxy is already ref counted. so I can just use >> RunnableMethod. thanks! > > > np :) > >> >> >> http://codereview.chromium.org/194108 >> > >
http://codereview.chromium.org/194108/diff/1004/1011 File chrome/browser/notifications/desktop_notification_service.h (right): http://codereview.chromium.org/194108/diff/1004/1011#newcode66 Line 66: friend class NotificationPermissionInfoBarDelegate; On 2009/09/18 19:11:33, John Gregg wrote: > On 2009/09/15 22:23:01, Jeremy Orlow wrote: > > Are you sure this is necessary? Would be nice if we could avoid it. > > I can make the 2 methods the info-bar calls public. I'd lean towards doing that. http://codereview.chromium.org/194108/diff/1004/1012 File chrome/browser/notifications/desktop_notification_service_mac.mm (right): http://codereview.chromium.org/194108/diff/1004/1012#newcode7 Line 7: #include "Cocoa/cocoa.h" On 2009/09/15 22:23:01, Jeremy Orlow wrote: > <>'s instead of ""s? Ping on the comments in this file... http://codereview.chromium.org/194108/diff/1004/1013 File chrome/browser/notifications/desktop_notification_service_win.cc (right): http://codereview.chromium.org/194108/diff/1004/1013#newcode77 Line 77: SiteInstance* site_instance = SiteInstance::CreateSiteInstance(profile_); On 2009/09/18 19:11:33, John Gregg wrote: > On 2009/09/15 22:23:01, Jeremy Orlow wrote: > > So this has to happen on the UI thread? > > Yes. This tasks calls the notification ui manager to actually show a > notification, so it's on the UI thread. Is there a particular problem with > doing it, or do I just need better documentation about threading? Better documentation. http://codereview.chromium.org/194108/diff/1004/1015 File chrome/browser/notifications/notification_object_proxy.cc (right): http://codereview.chromium.org/194108/diff/1004/1015#newcode55 Line 55: io_loop_->PostTask(FROM_HERE, On 2009/09/18 19:11:33, John Gregg wrote: > On 2009/09/15 22:23:01, Jeremy Orlow wrote: > > Can you prove that this will never be called after the io_loop_ has been shut > > down? If so, please document it in the header files somewhere. If not, it > > needs to be. :-) > > i probably can't guarantee it right now. can I detect the shutdown of the IO > thread somehow? Yes. You can get a notification on the IO thread of when it's going away, but this is probably not the best approach. Even better is looking at the shutdown sequence on the UI thread. http://cs/p?sa=N&cd=1&ct=rc#chrome/trunk/src/chrome/browser/browser_process_i... We can talk about this more if you'd like. It's a pretty subtle and tricky thing. :-) ....of course, now that I think about it, because the UI thread shuts the IO thread down, doing g_browser_process->io_thread() is always safe. Lucky you. :-) http://codereview.chromium.org/194108/diff/1004/1015#newcode79 Line 79: io_loop_->PostTask(FROM_HERE, On 2009/09/18 19:34:20, John Abd-El-Malek wrote: > On 2009/09/18 19:11:33, John Gregg wrote: > > On 2009/09/15 22:23:01, Jeremy Orlow wrote: > > > It seems like there'd be a standard way to do this...like RunnableMethod > > > > i can use a static method and RunnableFunction rather than a task, but i can't > > use a RunnableMethod with a this pointer because in the case of close(), the > > proxy object might be deleted afterwards, so it can't safely defer a call on > > itself. > > why not just make the proxy object ref counted? Then RunnableMethod will work > fine and all this file will be cleaner. The only problem with this is that, if you're not careful, it means your class could be destroyed on one of multiple threads. If at all possible, keep shutdown deterministic. Adding a Shutdown() method that's always called on the same thread that does 99% of the shutdown work can help. a lot of the classes in /chrome/browser/in_process_webkit/ deals with these subtleties if you'd like to see some examples. (That said, I don't deal with the UI thread.) http://codereview.chromium.org/194108/diff/1004/1017 File chrome/browser/notifications/notification_ui_manager.h (right): http://codereview.chromium.org/194108/diff/1004/1017#newcode30 Line 30: BalloonCollection* collection = new BalloonCollection(); On 2009/09/18 19:11:33, John Gregg wrote: > On 2009/09/15 22:23:01, Jeremy Orlow wrote: > > Why couldnt' this just be done in the constructor? > > > > Either way, why is it inline? > > I wanted the constructor to be uninitializing so I could initialize with mock > objects instead in unit tests. > > But I can move it to the impl file. Fine. http://codereview.chromium.org/194108/diff/1004/1017#newcode50 Line 50: NotificationUIManager() : balloons_observer_(0), balloon_collection_(0) { } On 2009/09/18 19:11:33, John Gregg wrote: > On 2009/09/15 22:23:01, Jeremy Orlow wrote: > > Are there any examples of this elsewhere? I've always seen it like > > > > Blah > > : blah_(), blah() { > > } > > > > Even when it's not necessary (like here) > > I have seen this before, but i'll change it. I'm not too worried if there is precedent. http://codereview.chromium.org/194108/diff/1004/1018 File chrome/browser/profile.cc (right): http://codereview.chromium.org/194108/diff/1004/1018#newcode1290 Line 1290: desktop_notification_service_->Initialize(io_thread->message_loop()); On 2009/09/18 19:11:33, John Gregg wrote: > On 2009/09/15 22:23:01, Jeremy Orlow wrote: > > Why does it need to be initialized like this? It should be able to grab the > > message loop via ChromeThread::GetMessageLoop, right? > > > > And are you guarenteed that it'll be shut down (so it won't try to use the > loop > > anymore) before the io thread goes away? > > I may need some pointers on how to deal with that. How can I detect the > shutdown? > I replied to this elsewhere. I'm happy to talk about it via vc or something. http://codereview.chromium.org/194108/diff/1004/1020 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/194108/diff/1004/1020#newcode842 Line 842: IPC_MESSAGE_HANDLER(ViewHostMsg_CheckNotificationPermission, On 2009/09/18 19:11:33, John Gregg wrote: > On 2009/09/15 22:23:01, Jeremy Orlow wrote: > > This is supposed to be sync, right!? How can this work? > > yeah that needs to change... i'm pumping messages which allows it work, but i > guess that's not a good way. Well, I was talking more about the mechanics....your message handler here is an async one but the message is supposed to be sync. But yeah...I think you need to get rid of the sync call anyway.
new code posted which deals with threading issues. http://codereview.chromium.org/194108/diff/1004/1020 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/194108/diff/1004/1020#newcode842 Line 842: IPC_MESSAGE_HANDLER(ViewHostMsg_CheckNotificationPermission, I need to be a sync call because I still want the JS interface to be synchronous, I'm just going to handle it on the IO thread using a preference cache rather than the UI thread.
http://codereview.chromium.org/194108/diff/1004/1020 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/194108/diff/1004/1020#newcode842 Line 842: IPC_MESSAGE_HANDLER(ViewHostMsg_CheckNotificationPermission, On 2009/09/21 17:54:09, John Gregg wrote: > I need to be a sync call because I still want the JS interface to be > synchronous, I'm just going to handle it on the IO thread using a preference > cache rather than the UI thread. cool (didn't review the rest of the change in detail) http://codereview.chromium.org/194108/diff/11004/11010 File chrome/browser/notifications/desktop_notification_service.cc (right): http://codereview.chromium.org/194108/diff/11004/11010#newcode277 Line 277: g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE, need to check io_thread() anywhere you use it since it could be null http://codereview.chromium.org/194108/diff/11004/11030 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/194108/diff/11004/11030#newcode1621 Line 1621: string16 /* origin */, nit: update spacing
http://codereview.chromium.org/194108/diff/11004/11010 File chrome/browser/notifications/desktop_notification_service.cc (right): http://codereview.chromium.org/194108/diff/11004/11010#newcode277 Line 277: g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE, On 2009/09/21 22:36:57, John Abd-El-Malek wrote: > need to check io_thread() anywhere you use it since it could be null okay, i'll change that (makes perfect sense). but fyi I've seen this many places and if you grep the src for io_thread()->message_loop() there are a lot of hits. http://codereview.chromium.org/194108/diff/11004/11030 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/194108/diff/11004/11030#newcode1621 Line 1621: string16 /* origin */, On 2009/09/21 22:36:57, John Abd-El-Malek wrote: > nit: update spacing Done.
My brain's mush....I'll try to finish reviewing the service tomorrow. http://codereview.chromium.org/194108/diff/11004/11010 File chrome/browser/notifications/desktop_notification_service.cc (right): http://codereview.chromium.org/194108/diff/11004/11010#newcode277 Line 277: g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE, On 2009/09/22 18:24:57, John Gregg wrote: > On 2009/09/21 22:36:57, John Abd-El-Malek wrote: > > need to check io_thread() anywhere you use it since it could be null > > okay, i'll change that (makes perfect sense). > > but fyi I've seen this many places and if you grep the src for > io_thread()->message_loop() there are a lot of hits. Well, if you can guarantee it's not null, then you don't need to check. :-) I have a lot of code, for example, where I do this, but I can tell you in each case why it's impossible for the IO thread to have shut down at that point. If you can "prove" it here, then it's fine. http://codereview.chromium.org/194108/diff/12004/12010#newcode310 Line 310: Can you sprinkle in more DCHECKs for specific threads? (Or at least add comments to the header files for each function about where it hsould be called from) http://codereview.chromium.org/194108/diff/12004/12012 File chrome/browser/notifications/desktop_notification_service_mac.mm (right): http://codereview.chromium.org/194108/diff/12004/12012#newcode15 Line 15: } TODO + bug number http://codereview.chromium.org/194108/diff/12004/12012#newcode26 Line 26: // TODO(johnnyg): Integration with Growl will go here. Coming soon. Got a bug number? http://codereview.chromium.org/194108/diff/12004/12013 File chrome/browser/notifications/desktop_notification_service_win.cc (right): http://codereview.chromium.org/194108/diff/12004/12013#newcode65 Line 65: NotificationUIManager* ui_manager, you can do this on 2 lines instead of 3 http://codereview.chromium.org/194108/diff/12004/12013#newcode113 Line 113: MessageLoopForUI::current()->PostTask(FROM_HERE, Why do you need to do it this way rather than a direct call? We're already on the UI thread. http://codereview.chromium.org/194108/diff/12004/12014 File chrome/browser/notifications/notification.h (right): http://codereview.chromium.org/194108/diff/12004/12014#newcode15 Line 15: Notification(const GURL& origin_url, needn't be on separate lines http://codereview.chromium.org/194108/diff/12004/12014#newcode19 Line 19: content_url_(content_url), as far as I know, this is the ONLY place that we commonly put on multiple lines....otherwise vertical space is more important http://codereview.chromium.org/194108/diff/12004/12014#newcode23 Line 23: Notification(const Notification& notification) I don't see where this is used. http://codereview.chromium.org/194108/diff/12004/12014#newcode30 Line 30: return content_url_; needn't be on multiple lines http://codereview.chromium.org/194108/diff/12004/12014#newcode33 Line 33: const GURL& origin_url() const { needn't be on multiple lines http://codereview.chromium.org/194108/diff/12004/12015 File chrome/browser/notifications/notification_object_proxy.cc (right): http://codereview.chromium.org/194108/diff/12004/12015#newcode24 Line 24: // TODO(johnnyg): Worker support coming soon. bug #? http://codereview.chromium.org/194108/diff/12004/12015#newcode55 Line 55: if (!io_thread) if you can prove the io thread is still alive, this is not needed for example if this lives on the io thread and/or in the profile, then you're safe....but if something outside of those can hold a reference, you might not be http://codereview.chromium.org/194108/diff/12004/12015#newcode69 Line 69: if (host) host->Send(message); 2 lines http://codereview.chromium.org/194108/diff/12004/12016 File chrome/browser/notifications/notification_object_proxy.h (right): http://codereview.chromium.org/194108/diff/12004/12016#newcode12 Line 12: class Message; no indent http://codereview.chromium.org/194108/diff/12004/12020 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/194108/diff/12004/12020#newcode1650 Line 1650: process()->id(), no need for it to all be on different lines http://codereview.chromium.org/194108/diff/12004/12020#newcode1657 Line 1657: const GURL& icon, no need for it to all be on different lines http://codereview.chromium.org/194108/diff/12004/12020#newcode1666 Line 1666: routing_id(), no need for it to all be on different lines http://codereview.chromium.org/194108/diff/12004/12020#newcode1667 Line 1667: false, this is difficult to read....an enum would be better http://codereview.chromium.org/194108/diff/12004/12022 File chrome/browser/renderer_host/resource_message_filter.cc (right): http://codereview.chromium.org/194108/diff/12004/12022#newcode1032 Line 1032: void ResourceMessageFilter::OnCheckNotificationPermission( Maybe group it with the other On* methods? http://codereview.chromium.org/194108/diff/12004/12025 File chrome/chrome.gyp (right): http://codereview.chromium.org/194108/diff/12004/12025#newcode1615 Line 1615: 'browser/notifications/desktop_notification_service.h', Alphabetical order.
some brief comments: http://codereview.chromium.org/194108/diff/12004/12015 File chrome/browser/notifications/notification_object_proxy.cc (right): http://codereview.chromium.org/194108/diff/12004/12015#newcode60 Line 60: NewRunnableMethod(this, &NotificationObjectProxy::Send, message)); big time oops: |this| does not implement thread-safe addref and release. http://codereview.chromium.org/194108/diff/12004/12016 File chrome/browser/notifications/notification_object_proxy.h (right): http://codereview.chromium.org/194108/diff/12004/12016#newcode29 Line 29: void display(); please use google style. display -> Display, etc.
addressed the style points and fixed the thread safety mistake; also eliminated some tasks which are no longer necessary since the threading was improved. http://codereview.chromium.org/194108/diff/12004/12010 File chrome/browser/notifications/desktop_notification_service.cc (right): http://codereview.chromium.org/194108/diff/12004/12010#newcode310 Line 310: // Schedule a task to show the infobar. On 2009/09/24 01:44:49, Jeremy Orlow wrote: > Can you sprinkle in more DCHECKs for specific threads? (Or at least add > comments to the header files for each function about where it hsould be called > from) another Task that's not needed. http://codereview.chromium.org/194108/diff/12004/12012 File chrome/browser/notifications/desktop_notification_service_mac.mm (right): http://codereview.chromium.org/194108/diff/12004/12012#newcode15 Line 15: } On 2009/09/24 01:44:49, Jeremy Orlow wrote: > TODO + bug number Done. http://codereview.chromium.org/194108/diff/12004/12012#newcode26 Line 26: // TODO(johnnyg): Integration with Growl will go here. Coming soon. On 2009/09/24 01:44:49, Jeremy Orlow wrote: > Got a bug number? Done. http://codereview.chromium.org/194108/diff/12004/12013 File chrome/browser/notifications/desktop_notification_service_win.cc (right): http://codereview.chromium.org/194108/diff/12004/12013#newcode65 Line 65: NotificationUIManager* ui_manager, On 2009/09/24 01:44:49, Jeremy Orlow wrote: > you can do this on 2 lines instead of 3 Done. http://codereview.chromium.org/194108/diff/12004/12013#newcode113 Line 113: MessageLoopForUI::current()->PostTask(FROM_HERE, On 2009/09/24 01:44:49, Jeremy Orlow wrote: > Why do you need to do it this way rather than a direct call? We're already on > the UI thread. Yeah, you're right, I can get rid of this task... it's a leftover when when these were handled on the IO thread. http://codereview.chromium.org/194108/diff/12004/12014 File chrome/browser/notifications/notification.h (right): http://codereview.chromium.org/194108/diff/12004/12014#newcode15 Line 15: Notification(const GURL& origin_url, On 2009/09/24 01:44:49, Jeremy Orlow wrote: > needn't be on separate lines Done. http://codereview.chromium.org/194108/diff/12004/12014#newcode23 Line 23: Notification(const Notification& notification) On 2009/09/24 01:44:49, Jeremy Orlow wrote: > I don't see where this is used. We put these in a std::deque, which requires that they be copyable. http://codereview.chromium.org/194108/diff/12004/12014#newcode30 Line 30: return content_url_; On 2009/09/24 01:44:49, Jeremy Orlow wrote: > needn't be on multiple lines Done. http://codereview.chromium.org/194108/diff/12004/12014#newcode33 Line 33: const GURL& origin_url() const { On 2009/09/24 01:44:49, Jeremy Orlow wrote: > needn't be on multiple lines Done. http://codereview.chromium.org/194108/diff/12004/12015 File chrome/browser/notifications/notification_object_proxy.cc (right): http://codereview.chromium.org/194108/diff/12004/12015#newcode24 Line 24: // TODO(johnnyg): Worker support coming soon. On 2009/09/24 01:44:49, Jeremy Orlow wrote: > bug #? Done. http://codereview.chromium.org/194108/diff/12004/12015#newcode60 Line 60: NewRunnableMethod(this, &NotificationObjectProxy::Send, message)); On 2009/09/24 05:33:28, darin wrote: > big time oops: |this| does not implement thread-safe addref and release. I changed NotificationObjectProxy to inherit RefCountedThreadSafe. http://codereview.chromium.org/194108/diff/12004/12015#newcode69 Line 69: if (host) host->Send(message); On 2009/09/24 01:44:49, Jeremy Orlow wrote: > 2 lines Done. http://codereview.chromium.org/194108/diff/12004/12016 File chrome/browser/notifications/notification_object_proxy.h (right): http://codereview.chromium.org/194108/diff/12004/12016#newcode12 Line 12: class Message; On 2009/09/24 01:44:49, Jeremy Orlow wrote: > no indent Done. http://codereview.chromium.org/194108/diff/12004/12016#newcode29 Line 29: void display(); On 2009/09/24 05:33:28, darin wrote: > please use google style. display -> Display, etc. Done. http://codereview.chromium.org/194108/diff/12004/12020 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/194108/diff/12004/12020#newcode1657 Line 1657: const GURL& icon, On 2009/09/24 01:44:49, Jeremy Orlow wrote: > no need for it to all be on different lines can't fit two on a line without changing the indentation, which seems to be (-aligned in the whole file. http://codereview.chromium.org/194108/diff/12004/12020#newcode1666 Line 1666: routing_id(), On 2009/09/24 01:44:49, Jeremy Orlow wrote: > no need for it to all be on different lines Done. http://codereview.chromium.org/194108/diff/12004/12020#newcode1667 Line 1667: false, On 2009/09/24 01:44:49, Jeremy Orlow wrote: > this is difficult to read....an enum would be better okay it's now an enum http://codereview.chromium.org/194108/diff/12004/12022 File chrome/browser/renderer_host/resource_message_filter.cc (right): http://codereview.chromium.org/194108/diff/12004/12022#newcode1032 Line 1032: void ResourceMessageFilter::OnCheckNotificationPermission( On 2009/09/24 01:44:49, Jeremy Orlow wrote: > Maybe group it with the other On* methods? ok I moved it up a bit. http://codereview.chromium.org/194108/diff/12004/12025 File chrome/chrome.gyp (right): http://codereview.chromium.org/194108/diff/12004/12025#newcode1615 Line 1615: 'browser/notifications/desktop_notification_service.h', On 2009/09/24 01:44:49, Jeremy Orlow wrote: > Alphabetical order. okay
Addressing comments. Will do another review in a bit. http://codereview.chromium.org/194108/diff/12004/12014 File chrome/browser/notifications/notification.h (right): http://codereview.chromium.org/194108/diff/12004/12014#newcode23 Line 23: Notification(const Notification& notification) On 2009/09/28 19:14:04, John Gregg wrote: > On 2009/09/24 01:44:49, Jeremy Orlow wrote: > > I don't see where this is used. > > We put these in a std::deque, which requires that they be copyable. I'm not super worried about it, but you could store pointers and not worry about it, right? Of course then you'd have to make the destructor delete them. (There's a function in base to do that.) http://codereview.chromium.org/194108/diff/12004/12020 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/194108/diff/12004/12020#newcode1657 Line 1657: const GURL& icon, On 2009/09/28 19:14:04, John Gregg wrote: > On 2009/09/24 01:44:49, Jeremy Orlow wrote: > > no need for it to all be on different lines > > can't fit two on a line without changing the indentation, which seems to be > (-aligned in the whole file. Please do change it. Not wasting vertical space is more important than lining stuff up, even if it's done often in the rest of the file.
Jeremy asked to borrow my eyes on this one. Some initial comments. Generally looks pretty neat. What about support for this in test_shell? Do we anticipate LayoutTests for these new scriptable interfaces. http://codereview.chromium.org/194108/diff/20003/20009 File chrome/browser/notifications/desktop_notification_service.cc (right): http://codereview.chromium.org/194108/diff/20003/20009#newcode46 Line 46: RenderViewHost::FromID(process_id_, route_id_)->Send( What guarantees the RenderViewHost is still around? http://codereview.chromium.org/194108/diff/20003/20009#newcode60 Line 60: explicit NotificationPermissionInfoBarDelegate( don't think you need explicit for multi-arg ctors http://codereview.chromium.org/194108/diff/20003/20009#newcode122 Line 122: string16 origin_; GURL? http://codereview.chromium.org/194108/diff/20003/20009#newcode131 Line 131: int callback_context_; what is behind the int value... wondering if there is a more appropiate 'type' that could be used? http://codereview.chromium.org/194108/diff/20003/20009#newcode142 Line 142: std::wstring origin; GURL? (or string16 depending on how you decide to represent origins) http://codereview.chromium.org/194108/diff/20003/20009#newcode191 Line 191: std::wstring worigin = UTF16ToWide(origin); The more string conversions i see, the more convinced i am that GURL is a better representation. http://codereview.chromium.org/194108/diff/20003/20010 File chrome/browser/notifications/desktop_notification_service.h (right): http://codereview.chromium.org/194108/diff/20003/20010#newcode41 Line 41: int HasPermission(const string16& origin); Would a GURL be a better representation of an 'origin'? http://codereview.chromium.org/194108/diff/20003/20010#newcode53 Line 53: std::set<std::wstring> allowed_origins_; Why is this not std::set<string16>? http://codereview.chromium.org/194108/diff/20003/20010#newcode75 Line 75: bool ShowDesktopNotification(const GURL& origin, const GURL& url, Ah... here you do use GURL for origin. You should be consistent about how 'origin' is represented. I like GURL myself. http://codereview.chromium.org/194108/diff/20003/20010#newcode76 Line 76: int process_id, int route_id, NotificationSource source, int notification_id); line length http://codereview.chromium.org/194108/diff/20003/20012 File chrome/browser/notifications/desktop_notification_service_win.cc (right): http://codereview.chromium.org/194108/diff/20003/20012#newcode26 Line 26: string16 CreateDataUrl(const GURL& icon_url, const string16& title, Should this either be a 'static' or in an anon namespace, the name of this function is very generic for the global scope. Also, would it make sense for the return value to be a GURL? http://codereview.chromium.org/194108/diff/20003/20012#newcode74 Line 74: notification_id, source == WorkerNotification); line length http://codereview.chromium.org/194108/diff/20003/20012#newcode88 Line 88: notification_id, source == WorkerNotification); line length http://codereview.chromium.org/194108/diff/20003/20013 File chrome/browser/notifications/notification.h (right): http://codereview.chromium.org/194108/diff/20003/20013#newcode39 Line 39: // The URL of the page/worker which created this notification. Either the comment or the data member is off? Is this a pageURL or the originURL (== pageURL.GetOrigin()). http://codereview.chromium.org/194108/diff/20003/20022 File chrome/browser/renderer_host/resource_message_filter.h (right): http://codereview.chromium.org/194108/diff/20003/20022#newcode189 Line 189: void OnCheckNotificationPermission(const string16& origin, int* permission_level); line length http://codereview.chromium.org/194108/diff/20003/20022#newcode349 Line 349: scoped_refptr<DesktopNotificationService::PrefsCache> notification_prefs_; The class name and the purpose it serves kind of clash here... a 'PrefsCache' that handles messages? http://codereview.chromium.org/194108/diff/20003/20029 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/194108/diff/20003/20029#newcode707 Line 707: // Informs the renderer that the one if its notifications has closed. copy/paste mistake on the comment? This is the same comment you have for the ViewMsg_PostCloseToNotificationObject message. http://codereview.chromium.org/194108/diff/20003/20029#newcode1643 Line 1643: string16 /* origin */, GURL a few lines up, string16 here... s/b consistent
http://codereview.chromium.org/194108/diff/20003/20009 File chrome/browser/notifications/desktop_notification_service.cc (right): http://codereview.chromium.org/194108/diff/20003/20009#newcode181 Line 181: } nit: chrome style would generally not have the curly braces around one-line blocks http://codereview.chromium.org/194108/diff/20003/20010 File chrome/browser/notifications/desktop_notification_service.h (right): http://codereview.chromium.org/194108/diff/20003/20010#newcode31 Line 31: class PrefsCache : public base::RefCounted<PrefsCache> { Ah... i see why this seperate helper is refcounted while the 'service' is not... nice way to deal with the IO vs UI thread access issues. I think you need to use base::RefCountedThreadSafe here since references are taken and released on both threads. Maybe a class comment that this classes methods should only be called on the IO thread. http://codereview.chromium.org/194108/diff/20003/20021 File chrome/browser/renderer_host/resource_message_filter.cc (right): http://codereview.chromium.org/194108/diff/20003/20021#newcode686 Line 686: *result = notification_prefs_->HasPermission(source_origin); I see, it doesn't really 'handle messages'. Maybe rephrase the comment in the .h file.
Thanks for helping out Michael! I'm working on a new patch that addresses your comments, and also when my other patch lands I can remove some of the overlapping code from this one. http://codereview.chromium.org/194108/diff/20003/20009 File chrome/browser/notifications/desktop_notification_service.cc (right): http://codereview.chromium.org/194108/diff/20003/20009#newcode46 Line 46: RenderViewHost::FromID(process_id_, route_id_)->Send( On 2009/10/01 01:29:20, michaeln wrote: > What guarantees the RenderViewHost is still around? added a check http://codereview.chromium.org/194108/diff/20003/20009#newcode60 Line 60: explicit NotificationPermissionInfoBarDelegate( On 2009/10/01 01:29:20, michaeln wrote: > don't think you need explicit for multi-arg ctors Done. http://codereview.chromium.org/194108/diff/20003/20009#newcode131 Line 131: int callback_context_; On 2009/10/01 01:29:20, michaeln wrote: > what is behind the int value... wondering if there is a more appropiate 'type' > that could be used? it's just an id to deal for the possibility of a renderer having more than one request pending -- there's a map between this ID and a javascript callback in the renderer process. but to this code it's opaque. http://codereview.chromium.org/194108/diff/20003/20009#newcode181 Line 181: } On 2009/10/01 18:17:06, michaeln wrote: > nit: chrome style would generally not have the curly braces around one-line > blocks Done. http://codereview.chromium.org/194108/diff/20003/20009#newcode191 Line 191: std::wstring worigin = UTF16ToWide(origin); On 2009/10/01 01:29:20, michaeln wrote: > The more string conversions i see, the more convinced i am that GURL is a better > representation. Okay, I think that makes sense. I will convert to GURLs throughout. http://codereview.chromium.org/194108/diff/20003/20010 File chrome/browser/notifications/desktop_notification_service.h (right): http://codereview.chromium.org/194108/diff/20003/20010#newcode31 Line 31: class PrefsCache : public base::RefCounted<PrefsCache> { On 2009/10/01 18:17:06, michaeln wrote: > Ah... i see why this seperate helper is refcounted while the 'service' is not... > nice way to deal with the IO vs UI thread access issues. > > I think you need to use base::RefCountedThreadSafe here since references are > taken and released on both threads. > > Maybe a class comment that this classes methods should only be called on the IO > thread. I'll move the constructor's comment up so this is more clear. I think technically it's not used across threads, just constructed on one and then handed off to the other, but I can use RefCountedThreadSafe anyway. http://codereview.chromium.org/194108/diff/20003/20010#newcode53 Line 53: std::set<std::wstring> allowed_origins_; On 2009/10/01 01:29:20, michaeln wrote: > Why is this not std::set<string16>? I think just because the preferences code deals with std::wstrings, so I have to convert somewhere. I'll put all the conversions adjacent to the prefs logic so that this interface will be consistent. http://codereview.chromium.org/194108/diff/20003/20010#newcode76 Line 76: int process_id, int route_id, NotificationSource source, int notification_id); On 2009/10/01 01:29:20, michaeln wrote: > line length Done. http://codereview.chromium.org/194108/diff/20003/20022 File chrome/browser/renderer_host/resource_message_filter.h (right): http://codereview.chromium.org/194108/diff/20003/20022#newcode349 Line 349: scoped_refptr<DesktopNotificationService::PrefsCache> notification_prefs_; On 2009/10/01 01:29:20, michaeln wrote: > The class name and the purpose it serves kind of clash here... a 'PrefsCache' > that handles messages? Just fitting in with the surrounding comments. I will elaborate.
Oops...I forgot to publish my last set of comments. As it turns out, Michael caught 90% of of them (which is a good sign). Here's a couple more though. As far as I can tell, this is good to go. So once Michael gives you a LGTM, you can assume one from me too. http://codereview.chromium.org/194108/diff/20003/20009 File chrome/browser/notifications/desktop_notification_service.cc (right): http://codereview.chromium.org/194108/diff/20003/20009#newcode46 Line 46: RenderViewHost::FromID(process_id_, route_id_)->Send( On 2009/10/02 18:02:01, John Gregg wrote: > On 2009/10/01 01:29:20, michaeln wrote: > > What guarantees the RenderViewHost is still around? > > added a check Just to be clear, this should "never happen" though...right? http://codereview.chromium.org/194108/diff/20003/20009#newcode181 Line 181: } On 2009/10/02 18:02:01, John Gregg wrote: > On 2009/10/01 18:17:06, michaeln wrote: > > nit: chrome style would generally not have the curly braces around one-line > > blocks > > Done. Note that I've found this a lot of places, so it's something to be mindful of when writing code from now on. http://codereview.chromium.org/194108/diff/20003/20010 File chrome/browser/notifications/desktop_notification_service.h (right): http://codereview.chromium.org/194108/diff/20003/20010#newcode31 Line 31: class PrefsCache : public base::RefCounted<PrefsCache> { On 2009/10/02 18:02:01, John Gregg wrote: > On 2009/10/01 18:17:06, michaeln wrote: > > Ah... i see why this seperate helper is refcounted while the 'service' is > not... > > nice way to deal with the IO vs UI thread access issues. > > > > I think you need to use base::RefCountedThreadSafe here since references are > > taken and released on both threads. > > > > Maybe a class comment that this classes methods should only be called on the > IO > > thread. > > I'll move the constructor's comment up so this is more clear. I think > technically it's not used across threads, just constructed on one and then > handed off to the other, but I can use RefCountedThreadSafe anyway. > I bet that if we looked REALLY hard at the code we could prove there will be at least one memory barrier between when this is last used on one thread and first used on another, but it probably just makes sense to use the threadsafe class since this is not performance intensive. A comment would be good too. http://codereview.chromium.org/194108/diff/20003/20012 File chrome/browser/notifications/desktop_notification_service_win.cc (right): http://codereview.chromium.org/194108/diff/20003/20012#newcode26 Line 26: string16 CreateDataUrl(const GURL& icon_url, const string16& title, On 2009/10/01 01:29:20, michaeln wrote: > Should this either be a 'static' or in an anon namespace, the name of this > function is very generic for the global scope. > > Also, would it make sense for the return value to be a GURL? Static seems like the right way to go. http://codereview.chromium.org/194108/diff/20003/20016 File chrome/browser/notifications/notification_ui_manager.h (right): http://codereview.chromium.org/194108/diff/20003/20016#newcode55 Line 55: BalloonCollectionObserver* balloons_observer_; short comments here would probably be good. http://codereview.chromium.org/194108/diff/20003/20017 File chrome/browser/profile.cc (right): http://codereview.chromium.org/194108/diff/20003/20017#newcode453 Line 453: return profile_->GetDesktopNotificationService(); Sanity check: it's ok that incognito is sharing the same service, right? I think so, but just want to be super sure. http://codereview.chromium.org/194108/diff/20003/20019 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/194108/diff/20003/20019#newcode1630 Line 1630: serv->ShowDesktopNotification(source_origin, url, process()->id(), routing_id(), >80 you may want to figure out how to make your IDE show you when you go over. in xcode, you can add a guide mark which is quite nice http://codereview.chromium.org/194108/diff/20003/20020 File chrome/browser/renderer_host/render_view_host.h (right): http://codereview.chromium.org/194108/diff/20003/20020#newcode574 Line 574: const GURL& url, Arg...please stop doing this. Only line up with the ( when it's not going to have a major impact on the vertical space. http://codereview.chromium.org/194108/diff/20003/20021 File chrome/browser/renderer_host/resource_message_filter.cc (right): http://codereview.chromium.org/194108/diff/20003/20021#newcode1051 Line 1051: not needed http://codereview.chromium.org/194108/diff/20003/20022 File chrome/browser/renderer_host/resource_message_filter.h (right): http://codereview.chromium.org/194108/diff/20003/20022#newcode24 Line 24: #include "chrome/browser/notifications/desktop_notification_service.h" alphabetical http://codereview.chromium.org/194108/diff/20003/20023 File chrome/browser/resources/notification.html (right): http://codereview.chromium.org/194108/diff/20003/20023#newcode1 Line 1: <html> I don't see anything wrong, but if you're not sure this is correct, you should have someone else take a quick look. http://codereview.chromium.org/194108/diff/20003/20029 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/194108/diff/20003/20029#newcode1634 Line 1634: IPC_MESSAGE_ROUTED5(ViewHostMsg_ShowDesktopNotificationText, Need comments on each.
On 2009/10/02 18:02:01, John Gregg wrote: > Thanks for helping out Michael! I'm working on a new patch that addresses your > comments, and also when my other patch lands I can remove some of the > overlapping code from this one. Ok, give me a ping when its ready for another look.
new code posted addressing the comments. http://codereview.chromium.org/194108/diff/20003/20009 File chrome/browser/notifications/desktop_notification_service.cc (right): http://codereview.chromium.org/194108/diff/20003/20009#newcode46 Line 46: RenderViewHost::FromID(process_id_, route_id_)->Send( On 2009/10/02 18:57:32, Jeremy Orlow wrote: > On 2009/10/02 18:02:01, John Gregg wrote: > > On 2009/10/01 01:29:20, michaeln wrote: > > > What guarantees the RenderViewHost is still around? > > > > added a check > > Just to be clear, this should "never happen" though...right? It should never happen, because it would mean a user clicking on an info bar after the tab showing the infobar went away. http://codereview.chromium.org/194108/diff/20003/20010 File chrome/browser/notifications/desktop_notification_service.h (right): http://codereview.chromium.org/194108/diff/20003/20010#newcode41 Line 41: int HasPermission(const string16& origin); On 2009/10/01 01:29:20, michaeln wrote: > Would a GURL be a better representation of an 'origin'? GURLs throughout now. http://codereview.chromium.org/194108/diff/20003/20012 File chrome/browser/notifications/desktop_notification_service_win.cc (right): http://codereview.chromium.org/194108/diff/20003/20012#newcode26 Line 26: string16 CreateDataUrl(const GURL& icon_url, const string16& title, On 2009/10/02 18:57:32, Jeremy Orlow wrote: > On 2009/10/01 01:29:20, michaeln wrote: > > Should this either be a 'static' or in an anon namespace, the name of this > > function is very generic for the global scope. > > > > Also, would it make sense for the return value to be a GURL? > > Static seems like the right way to go. Done. http://codereview.chromium.org/194108/diff/20003/20012#newcode74 Line 74: notification_id, source == WorkerNotification); On 2009/10/01 01:29:20, michaeln wrote: > line length Done. http://codereview.chromium.org/194108/diff/20003/20012#newcode88 Line 88: notification_id, source == WorkerNotification); On 2009/10/01 01:29:20, michaeln wrote: > line length Done. http://codereview.chromium.org/194108/diff/20003/20013 File chrome/browser/notifications/notification.h (right): http://codereview.chromium.org/194108/diff/20003/20013#newcode39 Line 39: // The URL of the page/worker which created this notification. On 2009/10/01 01:29:20, michaeln wrote: > Either the comment or the data member is off? Is this a pageURL or the originURL > (== pageURL.GetOrigin()). Yep, it's an origin. http://codereview.chromium.org/194108/diff/20003/20016 File chrome/browser/notifications/notification_ui_manager.h (right): http://codereview.chromium.org/194108/diff/20003/20016#newcode55 Line 55: BalloonCollectionObserver* balloons_observer_; On 2009/10/02 18:57:32, Jeremy Orlow wrote: > short comments here would probably be good. Added some throughout. http://codereview.chromium.org/194108/diff/20003/20019 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/194108/diff/20003/20019#newcode1630 Line 1630: serv->ShowDesktopNotification(source_origin, url, process()->id(), routing_id(), On 2009/10/02 18:57:32, Jeremy Orlow wrote: > >80 > > you may want to figure out how to make your IDE show you when you go over. in > xcode, you can add a guide mark which is quite nice Done. http://codereview.chromium.org/194108/diff/20003/20020 File chrome/browser/renderer_host/render_view_host.h (right): http://codereview.chromium.org/194108/diff/20003/20020#newcode574 Line 574: const GURL& url, On 2009/10/02 18:57:32, Jeremy Orlow wrote: > Arg...please stop doing this. Only line up with the ( when it's not going to > have a major impact on the vertical space. I can compress it a few lines since some lines will fit 2 args, but the vast majority of the file is formatted this way. http://codereview.chromium.org/194108/diff/20003/20022 File chrome/browser/renderer_host/resource_message_filter.h (right): http://codereview.chromium.org/194108/diff/20003/20022#newcode24 Line 24: #include "chrome/browser/notifications/desktop_notification_service.h" On 2009/10/02 18:57:32, Jeremy Orlow wrote: > alphabetical Done. http://codereview.chromium.org/194108/diff/20003/20022#newcode189 Line 189: void OnCheckNotificationPermission(const string16& origin, int* permission_level); On 2009/10/01 01:29:20, michaeln wrote: > line length Done. http://codereview.chromium.org/194108/diff/20003/20023 File chrome/browser/resources/notification.html (right): http://codereview.chromium.org/194108/diff/20003/20023#newcode1 Line 1: <html> On 2009/10/02 18:57:32, Jeremy Orlow wrote: > I don't see anything wrong, but if you're not sure this is correct, you should > have someone else take a quick look. This is a simple template pending a proper design from UI reviewers.
Also, in response to the point about test_shell support. Yes, this is required and it's in this CL, which brings test_shell up to parity with DumpRenderTree for the notification layout tests in WebKit: http://codereview.chromium.org/256091/show
I still haven't made it all the way thru... the biggest material thing i have are questions about are around the show methods of the service and thread safety. http://codereview.chromium.org/194108/diff/30002/22008 File chrome/browser/notifications/balloons.h (right): http://codereview.chromium.org/194108/diff/30002/22008#newcode38 Line 38: class BalloonCollectionInterface { Does this really need to be abstract... just checking. http://codereview.chromium.org/194108/diff/30002/22008#newcode52 Line 52: typedef std::vector<BalloonCollectionObserver*> BalloonObservers; You may be able to use ObserverList<T> for this. http://codereview.chromium.org/194108/diff/30002/22008#newcode78 Line 78: virtual void Close(); are these also virtual for testing purposes? http://codereview.chromium.org/194108/diff/30002/22008#newcode98 Line 98: void AddObserver(BalloonCollectionObserver* observer); Where is RemoveObserver, or why is there no need for one? http://codereview.chromium.org/194108/diff/30002/22008#newcode156 Line 156: gfx::Point GetOrigin() const; ah... another flavor of 'origin'... maybe GetScreenOrigin or GetOriginalScreenPosition or FirstPosition (or something) to avoid confusion with scheme://host:port origin. http://codereview.chromium.org/194108/diff/30002/22009 File chrome/browser/notifications/desktop_notification_service.cc (right): http://codereview.chromium.org/194108/diff/30002/22009#newcode116 Line 116: UMA_HISTOGRAM_COUNTS("NotificationPermissionRequest.Denied", 1); UMA sweet http://codereview.chromium.org/194108/diff/30002/22010 File chrome/browser/notifications/desktop_notification_service.h (right): http://codereview.chromium.org/194108/diff/30002/22010#newcode74 Line 74: // this notification. If these methods are supposed to be called from the IO thread, i think this class may want to be RefCountedThreadSafe so you can ensure its not deleted when the IO thread tries to call it. http://codereview.chromium.org/194108/diff/30002/22014 File chrome/browser/notifications/notification.h (right): http://codereview.chromium.org/194108/diff/30002/22014#newcode12 Line 12: // Representation of an HTML notification. The doc comment is a little misleading, i was immediately wondering "what about text notifications" only to discover in the details that text notifs are handled as data URLs. http://codereview.chromium.org/194108/diff/30002/22015 File chrome/browser/notifications/notification_object_proxy.cc (right): http://codereview.chromium.org/194108/diff/30002/22015#newcode24 Line 24: // TODO(johnnyg): http://crbug.com/23065 Worker support coming soon. coming soon... cool http://codereview.chromium.org/194108/diff/30002/22015#newcode55 Line 55: if (!io_thread) is this test ever expected to fail? There are other ChromeThread static methods to get particular message loops by thread type that may be of use here. http://codereview.chromium.org/194108/diff/30002/22016 File chrome/browser/notifications/notification_object_proxy.h (right): http://codereview.chromium.org/194108/diff/30002/22016#newcode18 Line 18: // attached JS listeners will be invoked in the renderer or worker process. So this is used on the browser process, and results in messages being sent to child processes. It can be real head-clearing to hear "used on this|that process" in doc comments. Consider rewording "stands in for" verbage. Also, its very unclear what the "actual" notification refers to, do you mean an instance of what in the main process or the child process? Terminology like JS Notification would help. http://codereview.chromium.org/194108/diff/30002/22016#newcode24 Line 24: int notification_id_, bool worker); nit: no trailing underbar on notification_id http://codereview.chromium.org/194108/diff/30002/22017 File chrome/browser/notifications/notification_ui_manager.h (right): http://codereview.chromium.org/194108/diff/30002/22017#newcode29 Line 29: // is useful for unit tests. How is this instanced for chrome, does chrome not use this method? Oh... there's no .cc file yet. So is this temporary scaffolding for test cases until... http://codereview.chromium.org/194108/diff/30002/22018 File chrome/browser/profile.cc (right): http://codereview.chromium.org/194108/diff/30002/22018#newcode1338 Line 1338: this, NULL)); i see, the ui_manager is not instanced in chrome yet http://codereview.chromium.org/194108/diff/30002/22020 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/194108/diff/30002/22020#newcode1644 Line 1644: DesktopNotificationService* serv = service http://codereview.chromium.org/194108/diff/30002/22020#newcode1645 Line 1645: process()->profile()->GetDesktopNotificationService(); what thread are we on here?
http://codereview.chromium.org/194108/diff/30002/22008 File chrome/browser/notifications/balloons.h (right): http://codereview.chromium.org/194108/diff/30002/22008#newcode38 Line 38: class BalloonCollectionInterface { On 2009/10/08 06:46:44, michaeln wrote: > Does this really need to be abstract... just checking. I override this in my unit test. http://codereview.chromium.org/194108/diff/30002/22008#newcode78 Line 78: virtual void Close(); On 2009/10/08 06:46:44, michaeln wrote: > are these also virtual for testing purposes? Not currently overriding these; I can make them non-virtual. http://codereview.chromium.org/194108/diff/30002/22008#newcode98 Line 98: void AddObserver(BalloonCollectionObserver* observer); On 2009/10/08 06:46:44, michaeln wrote: > Where is RemoveObserver, or why is there no need for one? This may be over-generalized for the current use, really there is just one observer right now which is the UI manager. I could simplify that if you think it's awkward. http://codereview.chromium.org/194108/diff/30002/22008#newcode156 Line 156: gfx::Point GetOrigin() const; On 2009/10/08 06:46:44, michaeln wrote: > ah... another flavor of 'origin'... maybe GetScreenOrigin or > GetOriginalScreenPosition or FirstPosition (or something) to avoid confusion > with scheme://host:port origin. Done. http://codereview.chromium.org/194108/diff/30002/22010 File chrome/browser/notifications/desktop_notification_service.h (right): http://codereview.chromium.org/194108/diff/30002/22010#newcode74 Line 74: // this notification. Maybe the comment is misleading. Conceptually it's handling an IPC, but does so on the UI thread from RenderViewHost. The only part of this class touched on the IO thread is the PrefsCache, which is RefCountedThreadSafe already. http://codereview.chromium.org/194108/diff/30002/22014 File chrome/browser/notifications/notification.h (right): http://codereview.chromium.org/194108/diff/30002/22014#newcode12 Line 12: // Representation of an HTML notification. On 2009/10/08 06:46:44, michaeln wrote: > The doc comment is a little misleading, i was immediately wondering "what about > text notifications" only to discover in the details that text notifs are handled > as data URLs. I expanded the comments. http://codereview.chromium.org/194108/diff/30002/22015 File chrome/browser/notifications/notification_object_proxy.cc (right): http://codereview.chromium.org/194108/diff/30002/22015#newcode55 Line 55: if (!io_thread) This is the way I was told to look it up by jam/jorlow. In a unit test the IO thread will be null. http://codereview.chromium.org/194108/diff/30002/22016 File chrome/browser/notifications/notification_object_proxy.h (right): http://codereview.chromium.org/194108/diff/30002/22016#newcode18 Line 18: // attached JS listeners will be invoked in the renderer or worker process. I edited it in an attempt to be more precise. http://codereview.chromium.org/194108/diff/30002/22016#newcode24 Line 24: int notification_id_, bool worker); On 2009/10/08 06:46:44, michaeln wrote: > nit: no trailing underbar on notification_id Done. http://codereview.chromium.org/194108/diff/30002/22017 File chrome/browser/notifications/notification_ui_manager.h (right): http://codereview.chromium.org/194108/diff/30002/22017#newcode29 Line 29: // is useful for unit tests. This is where my large patch was split in half. The implementation of this file is in http://codereview.chromium.org/208068, and a NotificationUIManager is owned by BrowserProcess. http://codereview.chromium.org/194108/diff/30002/22018 File chrome/browser/profile.cc (right): http://codereview.chromium.org/194108/diff/30002/22018#newcode1338 Line 1338: this, NULL)); On 2009/10/08 06:46:44, michaeln wrote: > i see, the ui_manager is not instanced in chrome yet yep, this is also on the seam between my two CLs. http://codereview.chromium.org/194108/diff/30002/22020 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/194108/diff/30002/22020#newcode1644 Line 1644: DesktopNotificationService* serv = On 2009/10/08 06:46:44, michaeln wrote: > service Done here and below. http://codereview.chromium.org/194108/diff/30002/22020#newcode1645 Line 1645: process()->profile()->GetDesktopNotificationService(); On 2009/10/08 06:46:44, michaeln wrote: > what thread are we on here? UI thread. This whole RenderViewHost class is, no? I can add documentation/DCHECK but I thought it would be superfluous.
I didn't re-read the updated code, but assuming you didn't do anything besides what was in the comments, it still seems OK to me. http://codereview.chromium.org/194108/diff/30002/22015 File chrome/browser/notifications/notification_object_proxy.cc (right): http://codereview.chromium.org/194108/diff/30002/22015#newcode55 Line 55: if (!io_thread) On 2009/10/08 21:54:35, John Gregg wrote: > This is the way I was told to look it up by jam/jorlow. In a unit test the IO > thread will be null. > > Well....Jam said to do it, I said to do it unless you can prove that there's no way this would ever be called after the io thread shuts down. But the fact it's hit during testing makes it fairly important to do it this way. wait....can io_thread_ ever return non-null but then io_loop return null? I actually think you can just do ChromeThread:: get message loop (or something) here though. It'll save you a bit of code. Not a big deal tho. http://codereview.chromium.org/194108/diff/30002/22020 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/194108/diff/30002/22020#newcode1645 Line 1645: process()->profile()->GetDesktopNotificationService(); On 2009/10/08 21:54:35, John Gregg wrote: > On 2009/10/08 06:46:44, michaeln wrote: > > what thread are we on here? > > UI thread. This whole RenderViewHost class is, no? I can add > documentation/DCHECK but I thought it would be superfluous. Yeah...they don't seem necessary.
Its tricky to review this half without seeing the other half. http://codereview.chromium.org/194108/diff/30002/22008 File chrome/browser/notifications/balloons.h (right): http://codereview.chromium.org/194108/diff/30002/22008#newcode98 Line 98: void AddObserver(BalloonCollectionObserver* observer); > > Where is RemoveObserver, or why is there no need for one? > > This may be over-generalized for the current use, really there is just one > observer right now which is the UI manager. I could simplify that if you think > it's awkward. Its more typical to have a "listener" with a simple getter/setter when there is only one client. Then there's no need for the std::vector, and no confusion about the observer pattern being slightly off. http://codereview.chromium.org/194108/diff/30002/22015 File chrome/browser/notifications/notification_object_proxy.cc (right): http://codereview.chromium.org/194108/diff/30002/22015#newcode55 Line 55: if (!io_thread) Since jeremy added ChromeThread::GetMessageLoop (i think jeremy added that), newer code has been using it. It is less code and easier to read. MessageLoop* loop = ChromeThread::GetMessageLoop(ChromeThread::IO); if (loop) go(loopy); You can check for NULL, and the GetMessageLoop method doesn't have the side effect of trying to start the IO thread if it isn't running, g_browser_process->io_thread() has that side effect. I'm didn't see jam's comments, if the above doesn't address whatever concerns john had, feel free to ignore the suggestion to use GetMessageLoop. But if there is no conflict between using GetMessageLoop and john's concerns... would be nice to make the switch. http://codereview.chromium.org/194108/diff/33007/34018 File chrome/browser/notifications/notification_ui_manager.h (right): http://codereview.chromium.org/194108/diff/33007/34018#newcode23 Line 23: // a queue of pending notifications when space becomes constrained. When you get around to instancing one of these in chrome, you may want to look at the Singleton<T> class. http://codereview.chromium.org/194108/diff/33007/34018#newcode24 Line 24: class NotificationUIManager : public BalloonCollectionObserver { public BalloonCollectionListener? http://codereview.chromium.org/194108/diff/33007/34018#newcode29 Line 29: // is useful for unit tests. Its a little tricky to review this CL since the rest of it is elsewhere ;) Can the unit tests just new one for themselves on the heap or on the stack? Take a look at the FRIEND_TEST(x,y) macro and usage of it. http://codereview.chromium.org/194108/diff/33007/34018#newcode40 Line 40: NOTREACHED(); but there's code that calls this method in your _win.cc file? http://codereview.chromium.org/194108/diff/33007/34018#newcode44 Line 44: virtual void OnBalloonSpaceChanged(); Unless you intend for this method to be called by clients of NotificationUIManager thru a NotificationUIManager* ptr, consider moving this override into the private section of this class. The BalloonCollection can still call it (thru a BalloonCollectionObserver* ptr) w/o having to clutter the public interface of this class. http://codereview.chromium.org/194108/diff/33007/34018#newcode61 Line 61: BalloonCollectionObserver* balloons_observer_; I'm fuzzy on why this class implements the BalloonCollectionObserver interface, and also keeps a ptr to a different one. How does this data member get set? I dont see any method of the public interface that looks like it would have the side-effect of setting it. http://codereview.chromium.org/194108/diff/33007/34023 File chrome/browser/renderer_host/resource_message_filter.cc (right): http://codereview.chromium.org/194108/diff/33007/34023#newcode694 Line 694: *result = notification_prefs_->HasPermission(GURL(source_origin)); Why instance a new GURL object here? Does HasPermission(source_origin) not work? http://codereview.chromium.org/194108/diff/33007/34024 File chrome/browser/renderer_host/resource_message_filter.h (right): http://codereview.chromium.org/194108/diff/33007/34024#newcode25 Line 25: #include "chrome/browser/notifications/desktop_notification_service.h" Can you get away with just a forward declaration to the classes in here, and only include this .h file in resource_message_filter.cc? class DesktopNotificationService; class DesktopNotificationService::PrefsCache; Does that work here? Reducing the number of #includes in these much included .h file is a good thing.
http://codereview.chromium.org/194108/diff/30002/22008 File chrome/browser/notifications/balloons.h (right): http://codereview.chromium.org/194108/diff/30002/22008#newcode98 Line 98: void AddObserver(BalloonCollectionObserver* observer); Seems sufficient here; I will change to that. http://codereview.chromium.org/194108/diff/30002/22015 File chrome/browser/notifications/notification_object_proxy.cc (right): http://codereview.chromium.org/194108/diff/30002/22015#newcode55 Line 55: if (!io_thread) It appears ChromeThread::GetMessageLoop() results in the same check that I have, so it should be equivalent and easier to read. Switching. http://codereview.chromium.org/194108/diff/33007/34018 File chrome/browser/notifications/notification_ui_manager.h (right): http://codereview.chromium.org/194108/diff/33007/34018#newcode23 Line 23: // a queue of pending notifications when space becomes constrained. On 2009/10/09 03:23:38, michaeln wrote: > When you get around to instancing one of these in chrome, you may want to look > at the Singleton<T> class. I used to use a Singleton<> for this, but I was told Singletons are avoided at all cost for the browser process due to wonky teardown behavior, which is why I attached to the browser process (which I think works pretty well). http://codereview.chromium.org/194108/diff/33007/34018#newcode40 Line 40: NOTREACHED(); On 2009/10/09 03:23:38, michaeln wrote: > but there's code that calls this method in your _win.cc file? Well, the feature's not implemented yet. I can take out that line and just have it silently do nothing if someone tries to use it. http://codereview.chromium.org/194108/diff/33007/34018#newcode44 Line 44: virtual void OnBalloonSpaceChanged(); On 2009/10/09 03:23:38, michaeln wrote: > Unless you intend for this method to be called by clients of > NotificationUIManager thru a NotificationUIManager* ptr, consider moving this > override into the private section of this class. The BalloonCollection can still > call it (thru a BalloonCollectionObserver* ptr) w/o having to clutter the > public interface of this class. I like that idea. http://codereview.chromium.org/194108/diff/33007/34018#newcode61 Line 61: BalloonCollectionObserver* balloons_observer_; You're right, that's dead code, probably an older way I was doing it. Cutting it out. http://codereview.chromium.org/194108/diff/33007/34023 File chrome/browser/renderer_host/resource_message_filter.cc (right): http://codereview.chromium.org/194108/diff/33007/34023#newcode694 Line 694: *result = notification_prefs_->HasPermission(GURL(source_origin)); On 2009/10/09 03:23:38, michaeln wrote: > Why instance a new GURL object here? Does HasPermission(source_origin) not work? > just me being dumb. fixed. http://codereview.chromium.org/194108/diff/33007/34024 File chrome/browser/renderer_host/resource_message_filter.h (right): http://codereview.chromium.org/194108/diff/33007/34024#newcode25 Line 25: #include "chrome/browser/notifications/desktop_notification_service.h" I'll don't think you can forward declare an inner class like this. I'll make it a non-inner class to make this possible.
At this point, I think we should just start differing anything other than major concerns until the next patch. (I trust Johnny to keep careful track of such items and track them forward.) http://codereview.chromium.org/194108/diff/33007/34018 File chrome/browser/notifications/notification_ui_manager.h (right): http://codereview.chromium.org/194108/diff/33007/34018#newcode23 Line 23: // a queue of pending notifications when space becomes constrained. On 2009/10/09 21:51:34, John Gregg wrote: > On 2009/10/09 03:23:38, michaeln wrote: > > When you get around to instancing one of these in chrome, you may want to look > > at the Singleton<T> class. > > I used to use a Singleton<> for this, but I was told Singletons are avoided at > all cost for the browser process due to wonky teardown behavior, which is why I > attached to the browser process (which I think works pretty well). This is true. http://codereview.chromium.org/194108/diff/33007/34018#newcode24 Line 24: class NotificationUIManager : public BalloonCollectionObserver { On 2009/10/09 03:23:38, michaeln wrote: > public BalloonCollectionListener? That's one way to go. No matter what, though, I think your use of Balloon_____ names some times and Notification______ other times is a bit confusing. http://codereview.chromium.org/194108/diff/33007/34018#newcode29 Line 29: // is useful for unit tests. On 2009/10/09 03:23:38, michaeln wrote: > Its a little tricky to review this CL since the rest of it is elsewhere ;) > > Can the unit tests just new one for themselves on the heap or on the stack? Take > a look at the FRIEND_TEST(x,y) macro and usage of it. > > Actually, please remove the bits done purely for unit testing from this patch and add it in the next patch. Let's keep the scope from increasing any more. :-)
http://codereview.chromium.org/194108/diff/33007/34018 File chrome/browser/notifications/notification_ui_manager.h (right): http://codereview.chromium.org/194108/diff/33007/34018#newcode24 Line 24: class NotificationUIManager : public BalloonCollectionObserver { > That's one way to go. No matter what, though, I think your > use of Balloon_____ names some times and Notification______ > other times is a bit confusing. I changed this to be a BalloonSpaceChangedListener. Hopefully when the other patch comes in with the rest of the 'Balloon' code it will be more clear: balloons are the things drawn on the screen. Notifications represent the notification end-to-end, including the connection back to script, etc., and for the period of their lifetime when being displayed, they get put into a balloon. If there is a cleaner way to express this in code, I'm up for it! http://codereview.chromium.org/194108/diff/33007/34018#newcode29 Line 29: // is useful for unit tests. > Actually, please remove the bits done purely for unit testing from this patch > and add it in the next patch. Let's keep the scope from increasing any more. > :-) I don't think there's much to remove. But I agree and would just point out that this file is a stub as far as this patch goes. If there's a glaring problem, okay, but for other stuff we can change it when the actual impl comes in the next patch.
Let me know when you have a new snapshot to look at, we gotta get this in the bank ;)
On 2009/10/09 23:28:31, michaeln wrote: > Let me know when you have a new snapshot to look at, we gotta get this in the > bank ;) the latest is up now with all the revisions.
Modulo a couple of minor comments, LGTM I'd suggest deferring the addition of the UIManager.h class interface until your next CL. I think the rest of this CL hangs together w/o that class interface. In the next CL presumably there is a UIManager.cc file to look at as well, it would be good to put the .h and .cc file into the same commit. If that causes you grief (that i'm not seeing), please feel free to ignore that suggestion. http://codereview.chromium.org/194108/diff/41059/41064 File chrome/browser/notifications/balloons.h (right): http://codereview.chromium.org/194108/diff/41059/41064#newcode31 Line 31: class BalloonSpaceChangeListener { you probably want virtual empty dtor's on each of these listener interfaces http://codereview.chromium.org/194108/diff/41059/41064#newcode96 Line 96: explicit BalloonCollection(); is explicit needed? http://codereview.chromium.org/194108/diff/41059/41064#newcode98 Line 98: BalloonSpaceChangeListener* spaceChangeListener() { return listener_; } space_change_listener() set_space_cache_listener(x) http://codereview.chromium.org/194108/diff/41059/41065 File chrome/browser/notifications/desktop_notification_service.cc (right): http://codereview.chromium.org/194108/diff/41059/41065#newcode46 Line 46: virtual void Run() { maybe DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO)) to be a little more self-documenting http://codereview.chromium.org/194108/diff/41059/41065#newcode60 Line 60: class NotificationPermissionInfoBarDelegate : public ConfirmInfoBarDelegate { I'm not familiar with the infobar ui infrastructure, it looks really nice to use :) http://codereview.chromium.org/194108/diff/41059/41069 File chrome/browser/notifications/desktop_notification_service_win.cc (right): http://codereview.chromium.org/194108/diff/41059/41069#newcode64 Line 64: ui_manager_->Add(notification, profile_, site_instance); Right now, the instance will always be NULL in chrome afaict. If this is the only callsite for methods on the uimanager class, it may be better to comment this out for now and to not submit the UIManager.h file with this CL. http://codereview.chromium.org/194108/diff/41059/41073 File chrome/browser/notifications/notification_ui_manager.h (right): http://codereview.chromium.org/194108/diff/41059/41073#newcode35 Line 35: // Adds a notification to be displayed. From the callsite, it looks like ownership of site_instance is transfered to the manager class. Is that right? Doc comments would be good.
Okay I made these changes; I also took your suggestion and removed notification_ui_manager.h from this patch, and landed it.
http://codereview.chromium.org/194108/diff/41059/41069 File chrome/browser/notifications/desktop_notification_service_win.cc (right): http://codereview.chromium.org/194108/diff/41059/41069#newcode5 Line 5: #include "chrome/browser/notifications/desktop_notification_service.h" Also, there's nothing windowzy about this _win.cc file that I can see? Any platform specific differences seem like they can be handled in your UIManager class.
http://codereview.chromium.org/194108/diff/41059/41069 File chrome/browser/notifications/desktop_notification_service_win.cc (right): http://codereview.chromium.org/194108/diff/41059/41069#newcode5 Line 5: #include "chrome/browser/notifications/desktop_notification_service.h" The current design is that only Windows Chrome will use its own UI for notifications, and that Mac/Linux will go to a 3rd party presenter library -- so really only Windows has a UI manager at all. But from recent design feedback I will be changing this so that Chrome does the UI on all platforms, in which case it will get refactored like you say.
http://codereview.chromium.org/194108/diff/41059/41060 File chrome/app/chromium_strings.grd (right): http://codereview.chromium.org/194108/diff/41059/41060#newcode366 Line 366: Allow <ph name="site">$1<ex>mail.google.com</ex></ph> to show desktop notifications? Mark is fixing this, but... Why does this need to be in the branding file, it does not have branding? Adding to this file and not google_chrome_strings.grd also broke a lot of strings because now the indexes don't match for every resource after this between the two files. |
