Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
194079:
renderer process notifications support (Closed)
|
Created:
11 years, 3 months ago by John Gregg Modified:
9 years ago CC:
chromium-reviews_googlegroups.com, michaeln Visibility:
Public. |
DescriptionThis part contains the renderer-side logic for desktop notifications in pages. The code which tracks active notifications (by maintaining a bi-directional map between int notification_ids used on IPC and WebKit notification objects) is put in common because it will be reused by worker notifications in part 2.
Also note this patch sets ENABLE_NOTIFICATIONS=1 for the WebKit build, which is necessary to access the conditionally built WebCore objects -- part 1.b. contains a command line flag which will hide notifications from pages by default; that patch is separate to ease review and because it depends on a webkit change (https://bugs.webkit.org/show_bug.cgi?id=28930)
Checked in as http://src.chromium.org/viewvc/chrome?view=rev&revision=27973
Patch Set 1 #Patch Set 2 : lint fixes #Patch Set 3 : style cleanup in IPC definitions #
Total comments: 53
Patch Set 4 : fix threading issues & style #
Total comments: 31
Patch Set 5 : make hash_map work #
Total comments: 24
Patch Set 6 : address feedback #Patch Set 7 : "more style changes" #
Total comments: 28
Patch Set 8 : last change for code review #
Total comments: 1
Messages
Total messages: 22 (0 generated)
http://codereview.chromium.org/194079/diff/9001/9003 File chrome/common/desktop_notifications/active_notification_tracker.cc (right): http://codereview.chromium.org/194079/diff/9001/9003#newcode8 Line 8: #include "webkit/api/public/WebNotificationPermissionCallback.h" What's the right order? You did it differently in the other file. http://codereview.chromium.org/194079/diff/9001/9003#newcode16 Line 16: if (iter != reverse_notification_table_.end()) { if (blah) return false; id = ... return true; it a tiny bit prettier to me. http://codereview.chromium.org/194079/diff/9001/9003#newcode27 Line 27: if (lookup) { do it the same way here as you do above http://codereview.chromium.org/194079/diff/9001/9003#newcode45 Line 45: if (notification) { once again, should this ever happen? if not, maybe a CHECK...or if you don't want it to crash, DCHECK + if http://codereview.chromium.org/194079/diff/9001/9004 File chrome/common/desktop_notifications/active_notification_tracker.h (right): http://codereview.chromium.org/194079/diff/9001/9004#newcode19 Line 19: class ActiveNotificationTracker { On what threads can this be touched? Please document it here. http://codereview.chromium.org/194079/diff/9001/9004#newcode40 Line 40: typedef std::map<WebKit::WebNotification, int>::iterator ReverseTableIterator; Have you seen it done this way elsewhere? From what I've seen, this is usually done like this: typedef std::map<WebKit::WebNotification, int> ReverseTable; ReverseTable reverse_notificaiton_table_; And then later, you'd just do ReverseTable::iterator. Personally, I think doing it that way is a bit cleaner, but if it's done this way elsewhere, I'm OK with it. Also, is there any reason you used this and not a hash_map? Once again, I've typically seen the later, but it's not a super big deal to me. If you want to use hash_map, include <base/hash_tables.h> and use base::hash_map<> http://codereview.chromium.org/194079/diff/9001/9006 File chrome/renderer/notification_provider.cc (right): http://codereview.chromium.org/194079/diff/9001/9006#newcode58 Line 58: if (notification.isHTML()) { no need for { }'s http://codereview.chromium.org/194079/diff/9001/9006#newcode69 Line 69: GURL(view_->webview()->GetMainFrame()->url()), space in to match up with ( http://codereview.chromium.org/194079/diff/9001/9006#newcode78 Line 78: GURL(view_->webview()->GetMainFrame()->url()), space in to match up with ( http://codereview.chromium.org/194079/diff/9001/9006#newcode87 Line 87: if (manager_.GetId(notification, id)) { This should probably be a DCHECK + this if statement or a CHECK if you're pretty sure it should never happen. http://codereview.chromium.org/194079/diff/9001/9006#newcode92 Line 92: void NotificationProvider::objectDestroyed(const WebNotification& notification) { 80 cols http://codereview.chromium.org/194079/diff/9001/9006#newcode99 Line 99: WebKit::WebNotificationPresenter::Permission NotificationProvider::checkPermission( 80 cols http://codereview.chromium.org/194079/diff/9001/9006#newcode170 Line 170: new NotificationCloseEventTask(notification, by_user)); I think you can do this with RunableMethod and it'd be much cleaner. If that doesn't work, I think doing it the way I did DOMStorageDispatcherHost is still cleaner. http://codereview.chromium.org/194079/diff/9001/9006#newcode177 Line 177: WebNotificationPermissionCallback* client) { Hmm...what's this for? Maybe add a comment? http://codereview.chromium.org/194079/diff/9001/9006#newcode186 Line 186: if (callback) { Should we DCHECK this? (Even if so, we should probably leave in this if statement.) http://codereview.chromium.org/194079/diff/9001/9006#newcode191 Line 191: manager_.OnPermissionRequestComplete(id); Is it ok to do this on the IO thread? It's usually easier if you only touch datastructures on one thread or another. Either way, document where things can be touched. http://codereview.chromium.org/194079/diff/9001/9007 File chrome/renderer/notification_provider.h (right): http://codereview.chromium.org/194079/diff/9001/9007#newcode15 Line 15: #include "webkit/api/public/WebNotification.h" does . come after P? (not sure) http://codereview.chromium.org/194079/diff/9001/9007#newcode19 Line 19: using WebKit::WebNotification; Don't do this in .h files. http://codereview.chromium.org/194079/diff/9001/9007#newcode26 Line 26: static NotificationProvider* create(RenderView* view) { What's the point of this? http://codereview.chromium.org/194079/diff/9001/9007#newcode42 Line 42: bool showHTML(const WebNotification& notification, int id); Are 32 bits enough, even in your wildest immagination? It prob makes sense to just make this 64, I would think. Maybe this ID should be a typedef to int/int64 so it's more readable? http://codereview.chromium.org/194079/diff/9001/9007#newcode46 Line 46: void OnDisplay(int id); The order of methods in both files should be the same. http://codereview.chromium.org/194079/diff/9001/9007#newcode54 Line 54: explicit NotificationProvider(RenderView* view) : view_(view) {} It seems like there's no reason to inline this. http://codereview.chromium.org/194079/diff/9001/9007#newcode57 Line 57: RenderView* view_; Why are these grouped together? Overall, the ordering of functions in this file aren't super intuitive to me. But if you can justify them in your mind, it's fine. http://codereview.chromium.org/194079/diff/9001/9007#newcode60 Line 60: }; Needs a disallow copy and assign. http://codereview.chromium.org/194079/diff/9001/9010 File webkit/api/public/WebNotification.h (right): http://codereview.chromium.org/194079/diff/9001/9010#newcode60 Line 60: bool lessThan(const WebNotification& other) const { return (uintptr_t)m_private < (uintptr_t)other.m_private; } use reinterpret_cast<> I've never seen this type before. Is it used similarly elsewhere? What's this operator needed for? Maybe add a comment?
http://codereview.chromium.org/194079/diff/9001/9003 File chrome/common/desktop_notifications/active_notification_tracker.cc (right): http://codereview.chromium.org/194079/diff/9001/9003#newcode8 Line 8: #include "webkit/api/public/WebNotificationPermissionCallback.h" On 2009/09/15 20:56:46, Jeremy Orlow wrote: > What's the right order? You did it differently in the other file. Done. http://codereview.chromium.org/194079/diff/9001/9003#newcode16 Line 16: if (iter != reverse_notification_table_.end()) { On 2009/09/15 20:56:46, Jeremy Orlow wrote: > if (blah) > return false; > > id = ... > return true; > > it a tiny bit prettier to me. Done. http://codereview.chromium.org/194079/diff/9001/9003#newcode27 Line 27: if (lookup) { On 2009/09/15 20:56:46, Jeremy Orlow wrote: > do it the same way here as you do above Done. http://codereview.chromium.org/194079/diff/9001/9003#newcode45 Line 45: if (notification) { On 2009/09/15 20:56:46, Jeremy Orlow wrote: > once again, should this ever happen? if not, maybe a CHECK...or if you don't > want it to crash, DCHECK + if Done. http://codereview.chromium.org/194079/diff/9001/9004 File chrome/common/desktop_notifications/active_notification_tracker.h (right): http://codereview.chromium.org/194079/diff/9001/9004#newcode40 Line 40: typedef std::map<WebKit::WebNotification, int>::iterator ReverseTableIterator; hash_map is awkward because i'm using an object rather than a number/pointer/string as my key, so i'd have to define hash functions manually. with just the == and < operators defined on WebKit::WebNotifications it's enough to put in a map. I will change the typedef style, it does seem more consistent with the codebase. http://codereview.chromium.org/194079/diff/9001/9006 File chrome/renderer/notification_provider.cc (right): http://codereview.chromium.org/194079/diff/9001/9006#newcode58 Line 58: if (notification.isHTML()) { On 2009/09/15 20:56:46, Jeremy Orlow wrote: > no need for { }'s Done. http://codereview.chromium.org/194079/diff/9001/9006#newcode69 Line 69: GURL(view_->webview()->GetMainFrame()->url()), On 2009/09/15 20:56:46, Jeremy Orlow wrote: > space in to match up with ( Done. http://codereview.chromium.org/194079/diff/9001/9006#newcode78 Line 78: GURL(view_->webview()->GetMainFrame()->url()), On 2009/09/15 20:56:46, Jeremy Orlow wrote: > space in to match up with ( Done. http://codereview.chromium.org/194079/diff/9001/9006#newcode87 Line 87: if (manager_.GetId(notification, id)) { On 2009/09/15 20:56:46, Jeremy Orlow wrote: > This should probably be a DCHECK + this if statement or a CHECK if you're pretty > sure it should never happen. Done. http://codereview.chromium.org/194079/diff/9001/9006#newcode87 Line 87: if (manager_.GetId(notification, id)) { On 2009/09/15 20:56:46, Jeremy Orlow wrote: > This should probably be a DCHECK + this if statement or a CHECK if you're pretty > sure it should never happen. Done. http://codereview.chromium.org/194079/diff/9001/9006#newcode99 Line 99: WebKit::WebNotificationPresenter::Permission NotificationProvider::checkPermission( On 2009/09/15 20:56:46, Jeremy Orlow wrote: > 80 cols Done. http://codereview.chromium.org/194079/diff/9001/9006#newcode99 Line 99: WebKit::WebNotificationPresenter::Permission NotificationProvider::checkPermission( On 2009/09/15 20:56:46, Jeremy Orlow wrote: > 80 cols Done. http://codereview.chromium.org/194079/diff/9001/9006#newcode170 Line 170: new NotificationCloseEventTask(notification, by_user)); I wanted to do RunnableMethod here but I can't because the type I'm operating on is not a heap pointer, it's a regular object. So I would have to alloc a pointer and copy, and then find a place to clean it up (which is not obvious). Fortunately, the threading changes I needed also solved my problem here, so all these tasks go away. http://codereview.chromium.org/194079/diff/9001/9006#newcode177 Line 177: WebNotificationPermissionCallback* client) { On 2009/09/15 20:56:46, Jeremy Orlow wrote: > Hmm...what's this for? Maybe add a comment? Also goes away by switching threads. http://codereview.chromium.org/194079/diff/9001/9006#newcode186 Line 186: if (callback) { On 2009/09/15 20:56:46, Jeremy Orlow wrote: > Should we DCHECK this? (Even if so, we should probably leave in this if > statement.) Done. http://codereview.chromium.org/194079/diff/9001/9006#newcode191 Line 191: manager_.OnPermissionRequestComplete(id); On 2009/09/15 20:56:46, Jeremy Orlow wrote: > Is it ok to do this on the IO thread? It's usually easier if you only touch > datastructures on one thread or another. Either way, document where things can > be touched. No, you are correct. I need to change the threading here and I added some assertions to keep me honest... http://codereview.chromium.org/194079/diff/9001/9007 File chrome/renderer/notification_provider.h (right): http://codereview.chromium.org/194079/diff/9001/9007#newcode15 Line 15: #include "webkit/api/public/WebNotification.h" On 2009/09/15 20:56:46, Jeremy Orlow wrote: > does . come after P? (not sure) Good call. I think . should go first. http://codereview.chromium.org/194079/diff/9001/9007#newcode19 Line 19: using WebKit::WebNotification; On 2009/09/15 20:56:46, Jeremy Orlow wrote: > Don't do this in .h files. Done. http://codereview.chromium.org/194079/diff/9001/9007#newcode26 Line 26: static NotificationProvider* create(RenderView* view) { On 2009/09/15 20:56:46, Jeremy Orlow wrote: > What's the point of this? gone. http://codereview.chromium.org/194079/diff/9001/9007#newcode42 Line 42: bool showHTML(const WebNotification& notification, int id); I do think 32 bits is enough; we're tracking active notifications and it's safe to reuse the id after that notification has been closed. Plus switching to int64 means I can't use the functionality of base/id_map.h which takes care of a lot. http://codereview.chromium.org/194079/diff/9001/9007#newcode46 Line 46: void OnDisplay(int id); On 2009/09/15 20:56:46, Jeremy Orlow wrote: > The order of methods in both files should be the same. Done. http://codereview.chromium.org/194079/diff/9001/9007#newcode54 Line 54: explicit NotificationProvider(RenderView* view) : view_(view) {} On 2009/09/15 20:56:46, Jeremy Orlow wrote: > It seems like there's no reason to inline this. Done. http://codereview.chromium.org/194079/diff/9001/9007#newcode57 Line 57: RenderView* view_; On 2009/09/15 20:56:46, Jeremy Orlow wrote: > Why are these grouped together? Overall, the ordering of functions in this file > aren't super intuitive to me. But if you can justify them in your mind, it's > fine. Done. http://codereview.chromium.org/194079/diff/9001/9010 File webkit/api/public/WebNotification.h (right): http://codereview.chromium.org/194079/diff/9001/9010#newcode60 Line 60: bool lessThan(const WebNotification& other) const { return (uintptr_t)m_private < (uintptr_t)other.m_private; } uintptr_t is just an integer type that represents a pointer in a portable way. a quick grep shows a few other uses for similar reasons. the purpose of the operator is so that I can use WebNotification as the key of a std::map; for that it needs to be orderable. i'll add a comment.
So my biggest question is whether all of your error handling is really necessary or paranoid. If you're just being paranoid, I'd get rid of all the if statements and do DCHECKS. Btw, DCHECK is fine whenever the next instruction would crash the program. CHECK is really only necessary if you're SURE you want an error to crash the program and the problem would make it crash in a subtle/non-obvious way in the future. http://codereview.chromium.org/194079/diff/9001/9004 File chrome/common/desktop_notifications/active_notification_tracker.h (right): http://codereview.chromium.org/194079/diff/9001/9004#newcode40 Line 40: typedef std::map<WebKit::WebNotification, int>::iterator ReverseTableIterator; On 2009/09/17 03:22:00, John Gregg wrote: > hash_map is awkward because i'm using an object rather than a > number/pointer/string as my key, so i'd have to define hash functions manually. > with just the == and < operators defined on WebKit::WebNotifications it's enough > to put in a map. > > I will change the typedef style, it does seem more consistent with the codebase. Take a look at this to see how hash_maps are typically used on objects (where comparing pointers is enough): src/chrome/browser/in_process_webkit/storage_area.h Actually, now that I think about it, I'm not sure your logic will work. == compares values while < only compares pointers, right? If so, that's not going to work. I did a search and found map only used in one other place, so I kind of think you should go with hash map. http://codereview.chromium.org/194079/diff/9001/9006 File chrome/renderer/notification_provider.cc (right): http://codereview.chromium.org/194079/diff/9001/9006#newcode170 Line 170: new NotificationCloseEventTask(notification, by_user)); On 2009/09/17 03:22:00, John Gregg wrote: > I wanted to do RunnableMethod here but I can't because the type I'm operating on > is not a heap pointer, it's a regular object. So I would have to alloc a > pointer and copy, and then find a place to clean it up (which is not obvious). That's essentially what you were doing anyway tho...it's just that the code was all in a class. > Fortunately, the threading changes I needed also solved my problem here, so all > these tasks go away. http://codereview.chromium.org/194079/diff/11001/12002 File chrome/common/desktop_notifications/active_notification_tracker.cc (right): http://codereview.chromium.org/194079/diff/11001/12002#newcode19 Line 19: return false; This seems like something to CHECK or DCHECK + if statement (depending on whether it seems horribly impossible or just impossible). Same goes with many places in the code. http://codereview.chromium.org/194079/diff/11001/12002#newcode32 Line 32: *notification = *lookup; Hmm...so this copies the notification....does that make sense? Should it be possible to not find it? Maybe you should be returning a const WebNotification&? http://codereview.chromium.org/194079/diff/11001/12002#newcode48 Line 48: notification_table_.Remove(id); Maybe dcheck to make sure it existed? http://codereview.chromium.org/194079/diff/11001/12003 File chrome/common/desktop_notifications/active_notification_tracker.h (right): http://codereview.chromium.org/194079/diff/11001/12003#newcode21 Line 21: // the main thread. In the browser, what does "main" thread mean? UI? http://codereview.chromium.org/194079/diff/11001/12005 File chrome/renderer/notification_provider.cc (right): http://codereview.chromium.org/194079/diff/11001/12005#newcode18 Line 18: : view_(view) { 4 spaces in http://codereview.chromium.org/194079/diff/11001/12005#newcode37 Line 37: void NotificationProvider::objectDestroyed(const WebNotification& notification) { 80 cols http://codereview.chromium.org/194079/diff/11001/12005#newcode48 Line 48: no space, I think. http://codereview.chromium.org/194079/diff/11001/12005#newcode61 Line 61: const WebString& origin, WebNotificationPermissionCallback* callback) { could go on the previous line probably? not sure if it's worht it. http://codereview.chromium.org/194079/diff/11001/12005#newcode65 Line 65: view_->routing_id(), line up with (...no need for so many lines http://codereview.chromium.org/194079/diff/11001/12005#newcode71 Line 71: int id) { could be lined up with ( http://codereview.chromium.org/194079/diff/11001/12005#newcode79 Line 79: int id) { could be lined up with ( http://codereview.chromium.org/194079/diff/11001/12005#newcode138 Line 138: if (callback) { no {}'s http://codereview.chromium.org/194079/diff/11001/12006 File chrome/renderer/notification_provider.h (right): http://codereview.chromium.org/194079/diff/11001/12006#newcode57 Line 57: RenderView* view_; Are there any details one should be aware of with these vars? http://codereview.chromium.org/194079/diff/11001/12009 File webkit/api/public/WebNotification.h (right): http://codereview.chromium.org/194079/diff/11001/12009#newcode61 Line 61: bool lessThan(const WebNotification& other) const { return reinterpret_cast<uintptr_t>(m_private) < reinterpret_cast<uintptr_t>(other.m_private); } Doing this on one line is kinda excessive.
http://codereview.chromium.org/194079/diff/9001/9004 File chrome/common/desktop_notifications/active_notification_tracker.h (right): http://codereview.chromium.org/194079/diff/9001/9004#newcode40 Line 40: typedef std::map<WebKit::WebNotification, int>::iterator ReverseTableIterator; > Take a look at this to see how hash_maps are typically used on objects (where > comparing pointers is enough): > src/chrome/browser/in_process_webkit/storage_area.h > > Actually, now that I think about it, I'm not sure your logic will work. == > compares values while < only compares pointers, right? If so, that's not going > to work. WebNotification is just a wrapper around a pointer. both == and < are comparing the m_private pointers, so that much is okay. But comparing the addresses of two WebNotifications instead of the m_private pointers would cause problems, so I can't use that same hash function. > > I did a search and found map only used in one other place, so I kind of think > you should go with hash map. I still would need to hash based on the m_private pointers though, so I would still have to add a new hash() function to WebKit::WebNotification rather than lessThan and operator<. http://codereview.chromium.org/194079/diff/11001/12002 File chrome/common/desktop_notifications/active_notification_tracker.cc (right): http://codereview.chromium.org/194079/diff/11001/12002#newcode30 Line 30: return false; These "not found" conditions are actually not illegal states, or at least this one isn't. The main reason is that a page could show a notification, then that page is gets closed. WebKit will tell the renderer that the JS Notification object has gone away, but we don't want to tear down that notification from the screen. When in the browser the user closes the toast, and we would go to fire the onClose event on the JS Notification object, we need to fail somewhere. I'm opting to handle this by removing from the Tracker on the renderer side, and when anything comes from the browser with that notification ID, it just gets dropped here. We could try to deal with it by pushing the information about a deleted JS Notification object up to the browser, but that seems more complicated and potentially racy. http://codereview.chromium.org/194079/diff/11001/12002#newcode48 Line 48: notification_table_.Remove(id); On 2009/09/18 21:24:02, Jeremy Orlow wrote: > Maybe dcheck to make sure it existed? that's on the next line. http://codereview.chromium.org/194079/diff/11001/12003 File chrome/common/desktop_notifications/active_notification_tracker.h (right): http://codereview.chromium.org/194079/diff/11001/12003#newcode21 Line 21: // the main thread. On 2009/09/18 21:24:02, Jeremy Orlow wrote: > In the browser, what does "main" thread mean? UI? as the comment said, this class should be used in the renderer or worker process, where i think "main" thread is the right word: ChildThread* ChildProcess::main_thread(), etc. this class isn't used in the browser process. http://codereview.chromium.org/194079/diff/11001/12005 File chrome/renderer/notification_provider.cc (right): http://codereview.chromium.org/194079/diff/11001/12005#newcode18 Line 18: : view_(view) { On 2009/09/18 21:24:02, Jeremy Orlow wrote: > 4 spaces in Done. http://codereview.chromium.org/194079/diff/11001/12005#newcode37 Line 37: void NotificationProvider::objectDestroyed(const WebNotification& notification) { On 2009/09/18 21:24:02, Jeremy Orlow wrote: > 80 cols Done. http://codereview.chromium.org/194079/diff/11001/12005#newcode48 Line 48: On 2009/09/18 21:24:02, Jeremy Orlow wrote: > no space, I think. Done. http://codereview.chromium.org/194079/diff/11001/12005#newcode61 Line 61: const WebString& origin, WebNotificationPermissionCallback* callback) { On 2009/09/18 21:24:02, Jeremy Orlow wrote: > could go on the previous line probably? not sure if it's worht it. looks equally awkward (actually worse, to me) http://codereview.chromium.org/194079/diff/11001/12005#newcode65 Line 65: view_->routing_id(), On 2009/09/18 21:24:02, Jeremy Orlow wrote: > line up with (...no need for so many lines Done. http://codereview.chromium.org/194079/diff/11001/12005#newcode71 Line 71: int id) { On 2009/09/18 21:24:02, Jeremy Orlow wrote: > could be lined up with ( Done. http://codereview.chromium.org/194079/diff/11001/12005#newcode79 Line 79: int id) { On 2009/09/18 21:24:02, Jeremy Orlow wrote: > could be lined up with ( Done. http://codereview.chromium.org/194079/diff/11001/12005#newcode138 Line 138: if (callback) { On 2009/09/18 21:24:02, Jeremy Orlow wrote: > no {}'s Done. http://codereview.chromium.org/194079/diff/11001/12006 File chrome/renderer/notification_provider.h (right): http://codereview.chromium.org/194079/diff/11001/12006#newcode57 Line 57: RenderView* view_; On 2009/09/18 21:24:02, Jeremy Orlow wrote: > Are there any details one should be aware of with these vars? adding... http://codereview.chromium.org/194079/diff/11001/12009 File webkit/api/public/WebNotification.h (right): http://codereview.chromium.org/194079/diff/11001/12009#newcode61 Line 61: bool lessThan(const WebNotification& other) const { return reinterpret_cast<uintptr_t>(m_private) < reinterpret_cast<uintptr_t>(other.m_private); } On 2009/09/18 21:24:02, Jeremy Orlow wrote: > Doing this on one line is kinda excessive. isn't this WebKit style?
http://codereview.chromium.org/194079/diff/9001/9004 File chrome/common/desktop_notifications/active_notification_tracker.h (right): http://codereview.chromium.org/194079/diff/9001/9004#newcode40 Line 40: typedef std::map<WebKit::WebNotification, int>::iterator ReverseTableIterator; On 2009/09/18 21:49:52, John Gregg wrote: > > Take a look at this to see how hash_maps are typically used on objects (where > > comparing pointers is enough): > > src/chrome/browser/in_process_webkit/storage_area.h > > > > Actually, now that I think about it, I'm not sure your logic will work. == > > compares values while < only compares pointers, right? If so, that's not > going > > to work. > > WebNotification is just a wrapper around a pointer. both == and < are comparing > the m_private pointers, so that much is okay. But comparing the addresses of > two WebNotifications instead of the m_private pointers would cause problems, so > I can't use that same hash function. > > > > > I did a search and found map only used in one other place, so I kind of think > > you should go with hash map. > > I still would need to hash based on the m_private pointers though, so I would > still have to add a new hash() function to WebKit::WebNotification rather than > lessThan and operator<. Yes. Personally, I think this is cleaner. And it's probably faster. If map is only used in one or two other places, I'd vote for changing it. http://codereview.chromium.org/194079/diff/11001/12002 File chrome/common/desktop_notifications/active_notification_tracker.cc (right): http://codereview.chromium.org/194079/diff/11001/12002#newcode30 Line 30: return false; On 2009/09/18 21:49:52, John Gregg wrote: > These "not found" conditions are actually not illegal states, or at least this > one isn't. > > The main reason is that a page could show a notification, then that page is gets > closed. WebKit will tell the renderer that the JS Notification object has gone > away, but we don't want to tear down that notification from the screen. > > When in the browser the user closes the toast, and we would go to fire the > onClose event on the JS Notification object, we need to fail somewhere. > > I'm opting to handle this by removing from the Tracker on the renderer side, and > when anything comes from the browser with that notification ID, it just gets > dropped here. > > We could try to deal with it by pushing the information about a deleted JS > Notification object up to the browser, but that seems more complicated and > potentially racy. Sounds good. Please do double check all the cases in this CL and I'll just assume they're good. http://codereview.chromium.org/194079/diff/11001/12003 File chrome/common/desktop_notifications/active_notification_tracker.h (right): http://codereview.chromium.org/194079/diff/11001/12003#newcode21 Line 21: // the main thread. On 2009/09/18 21:49:52, John Gregg wrote: > On 2009/09/18 21:24:02, Jeremy Orlow wrote: > > In the browser, what does "main" thread mean? UI? > > as the comment said, this class should be used in the renderer or worker > process, where i think "main" thread is the right word: > ChildThread* ChildProcess::main_thread(), etc. > > this class isn't used in the browser process. Got it. http://codereview.chromium.org/194079/diff/11001/12005 File chrome/renderer/notification_provider.cc (right): http://codereview.chromium.org/194079/diff/11001/12005#newcode61 Line 61: const WebString& origin, WebNotificationPermissionCallback* callback) { On 2009/09/18 21:49:52, John Gregg wrote: > On 2009/09/18 21:24:02, Jeremy Orlow wrote: > > could go on the previous line probably? not sure if it's worht it. > > looks equally awkward (actually worse, to me) k...it's fine http://codereview.chromium.org/194079/diff/11001/12009 File webkit/api/public/WebNotification.h (right): http://codereview.chromium.org/194079/diff/11001/12009#newcode61 Line 61: bool lessThan(const WebNotification& other) const { return reinterpret_cast<uintptr_t>(m_private) < reinterpret_cast<uintptr_t>(other.m_private); } On 2009/09/18 21:49:52, John Gregg wrote: > On 2009/09/18 21:24:02, Jeremy Orlow wrote: > > Doing this on one line is kinda excessive. > > isn't this WebKit style? Well, >80 columns is webkit stype, but this seems a bit excessive....I guess it's ok though.
it was a bit of a hassle getting the hash_map to work on both msvc and gcc, but i think i've accomplished this in the current CL.
Pretty much done. http://codereview.chromium.org/194079/diff/19001/19003 File chrome/common/desktop_notifications/active_notification_tracker.cc (right): http://codereview.chromium.org/194079/diff/19001/19003#newcode32 Line 32: *notification = *lookup; Do you really mean to copy it rather than returning a pointer? http://codereview.chromium.org/194079/diff/19001/19003#newcode50 Line 50: if (notification) { no {}s http://codereview.chromium.org/194079/diff/19001/19004 File chrome/common/desktop_notifications/active_notification_tracker.h (right): http://codereview.chromium.org/194079/diff/19001/19004#newcode62 Line 62: #if defined(COMPILER_MSVC) There's got to be a better way to do this. Maybe we should ask chromium dev? http://codereview.chromium.org/194079/diff/19001/19006 File chrome/renderer/notification_provider.cc (right): http://codereview.chromium.org/194079/diff/19001/19006#newcode37 Line 37: void NotificationProvider::objectDestroyed(const WebNotification& notification) { >80 http://codereview.chromium.org/194079/diff/19001/19006#newcode55 Line 55: msg->EnableMessagePumping(); We can't make this async...seems like we could, given that the spec isn't yet written. :-) http://codereview.chromium.org/194079/diff/19001/19006#newcode65 Line 65: view_->routing_id(), doesn't need to be split out like this. http://codereview.chromium.org/194079/diff/19001/19006#newcode70 Line 70: bool NotificationProvider::ShowHTML(const WebNotification& notification, It'd be nice if you had asserts about what thread we're on in these methods...if nothing else, it's a good form of documentation. http://codereview.chromium.org/194079/diff/19001/19006#newcode104 Line 104: void NotificationProvider::OnPermissionRequestComplete(int id) { It'd be nice if you had asserts about what thread we're on in these methods...if nothing else, it's a good form of documentation. http://codereview.chromium.org/194079/diff/19001/19006#newcode126 Line 126: void NotificationProvider::HandleOnClose(int id, bool by_user) { It'd be nice if you had asserts about what thread we're on in these methods...if nothing else, it's a good form of documentation. http://codereview.chromium.org/194079/diff/19001/19006#newcode138 Line 138: if (callback) { no {}'s http://codereview.chromium.org/194079/diff/19001/19008 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/194079/diff/19001/19008#newcode341 Line 341: Maybe remove this space. http://codereview.chromium.org/194079/diff/19001/19010 File webkit/api/public/WebNotification.h (right): http://codereview.chromium.org/194079/diff/19001/19010#newcode61 Line 61: bool lessThan(const WebNotification& other) const { no longer needed, right? http://codereview.chromium.org/194079/diff/19001/19010#newcode65 Line 65: size_t hash() const { return reinterpret_cast<size_t>(m_private); } This is a great way to do it. Thanks! http://codereview.chromium.org/194079/diff/19001/19010#newcode110 Line 110: inline bool operator<(const WebNotification& a, const WebNotification& b) no longer needed, right?
http://codereview.chromium.org/194079/diff/19001/19004 File chrome/common/desktop_notifications/active_notification_tracker.h (right): http://codereview.chromium.org/194079/diff/19001/19004#newcode62 Line 62: #if defined(COMPILER_MSVC) Do we really need a hash_map here? In most cases, a regular std::map is fine: it's O(ln n) so for less than a few thousand items it won't matter.
I wrote it as a std::map originally, but the concern was raised that std::map isn't widely used in chrome code and hash_map is the convention.
simplified things by going back to std::map, and addressed the style points. http://codereview.chromium.org/194079/diff/19001/19003 File chrome/common/desktop_notifications/active_notification_tracker.cc (right): http://codereview.chromium.org/194079/diff/19001/19003#newcode32 Line 32: *notification = *lookup; On 2009/09/24 00:42:17, Jeremy Orlow wrote: > Do you really mean to copy it rather than returning a pointer? I do actually. One more fun thing about working with these non-pointer wrappers. Remember copying a WebNotification is really just copying the WebCore::Notification* pointer inside it. specifically, because base::IDMap uses pointers, this tracker class makes a heap-allocated copy when adding a notification, and it owns that pointer; returning it would be a potential race condition. http://codereview.chromium.org/194079/diff/19001/19004 File chrome/common/desktop_notifications/active_notification_tracker.h (right): http://codereview.chromium.org/194079/diff/19001/19004#newcode62 Line 62: #if defined(COMPILER_MSVC) On 2009/09/24 00:42:17, Jeremy Orlow wrote: > There's got to be a better way to do this. Maybe we should ask chromium dev? > okay, i will send out an email. i agree this is getting crazy, but from looking in base/hash_tables.h and other docs on stdext::hash_map, i think this is the way. i could always go back to std::map! http://codereview.chromium.org/194079/diff/19001/19006 File chrome/renderer/notification_provider.cc (right): http://codereview.chromium.org/194079/diff/19001/19006#newcode37 Line 37: void NotificationProvider::objectDestroyed(const WebNotification& notification) { On 2009/09/24 00:42:17, Jeremy Orlow wrote: > >80 Done. http://codereview.chromium.org/194079/diff/19001/19006#newcode55 Line 55: msg->EnableMessagePumping(); On 2009/09/24 00:42:17, Jeremy Orlow wrote: > We can't make this async...seems like we could, given that the spec isn't yet > written. :-) I just think checkPermission as an API should be sync; if the only reason is to make the impl easier i don't know if that's the right move. You're right though, I don't need MessagePumping anymore because I moved the handler to the IO thread. http://codereview.chromium.org/194079/diff/19001/19006#newcode65 Line 65: view_->routing_id(), On 2009/09/24 00:42:17, Jeremy Orlow wrote: > doesn't need to be split out like this. Done. http://codereview.chromium.org/194079/diff/19001/19006#newcode138 Line 138: if (callback) { On 2009/09/24 00:42:17, Jeremy Orlow wrote: > no {}'s Done. http://codereview.chromium.org/194079/diff/19001/19010 File webkit/api/public/WebNotification.h (right): http://codereview.chromium.org/194079/diff/19001/19010#newcode61 Line 61: bool lessThan(const WebNotification& other) const { On 2009/09/24 00:42:17, Jeremy Orlow wrote: > no longer needed, right? actually the stdext::hash_compare implementation requires a comparator as well, so i need this unless the hashing can be simplified. http://codereview.chromium.org/194079/diff/19001/19010#newcode110 Line 110: inline bool operator<(const WebNotification& a, const WebNotification& b) On 2009/09/24 00:42:17, Jeremy Orlow wrote: > no longer needed, right? yeah, it stays or goes with lessThan(); see above.
I'll try to review this today. Please switch back to std::map though, per brettw. http://codereview.chromium.org/194079/diff/19001/19004 File chrome/common/desktop_notifications/active_notification_tracker.h (right): http://codereview.chromium.org/194079/diff/19001/19004#newcode62 Line 62: #if defined(COMPILER_MSVC) On 2009/09/28 19:12:56, John Gregg wrote: > On 2009/09/24 00:42:17, Jeremy Orlow wrote: > > There's got to be a better way to do this. Maybe we should ask chromium dev? > > > > okay, i will send out an email. i agree this is getting crazy, but from looking > in base/hash_tables.h and other docs on stdext::hash_map, i think this is the > way. > > i could always go back to std::map! > I think going back is the right thing to do. I said this somewhere in the comments, but there were a lot so it could have gotten lost. Sorry for leading you astray.
On 2009/09/28 20:44:58, Jeremy Orlow wrote: > I'll try to review this today. Please switch back to std::map though, per > brettw. > > http://codereview.chromium.org/194079/diff/19001/19004 > File chrome/common/desktop_notifications/active_notification_tracker.h (right): > > http://codereview.chromium.org/194079/diff/19001/19004#newcode62 > Line 62: #if defined(COMPILER_MSVC) > On 2009/09/28 19:12:56, John Gregg wrote: > > On 2009/09/24 00:42:17, Jeremy Orlow wrote: > > > There's got to be a better way to do this. Maybe we should ask chromium > dev? > > > > > > > okay, i will send out an email. i agree this is getting crazy, but from > looking > > in base/hash_tables.h and other docs on stdext::hash_map, i think this is the > > way. > > > > i could always go back to std::map! > > > > I think going back is the right thing to do. I said this somewhere in the > comments, but there were a lot so it could have gotten lost. Sorry for leading > you astray. Sorry, that comment of mine was stale. I have switched back to std::map in the latest version of the CL.
Looks pretty good to me, but it'd be good to have another set of eyes double check the logic side of things. Michael, can you take a look too? http://codereview.chromium.org/194079/diff/28001/28005 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/194079/diff/28001/28005#newcode1623 Line 1623: string16 /* origin */, line up with ( http://codereview.chromium.org/194079/diff/28001/28006 File chrome/renderer/notification_provider.cc (right): http://codereview.chromium.org/194079/diff/28001/28006#newcode18 Line 18: : view_(view) { 4 spaces in http://codereview.chromium.org/194079/diff/28001/28006#newcode38 Line 38: const WebNotification& notification) { this can't go on the previous line? http://codereview.chromium.org/194079/diff/28001/28006#newcode134 Line 134: if (callback) I'm kind of starting to wonder if we need these. I know it'll crash the renderer if we don't, but maybe that's the right thing to do...rather than error checking for something that should never happen. http://codereview.chromium.org/194079/diff/28001/28006#newcode147 Line 147: IPC_MESSAGE_HANDLER(ViewMsg_PostErrorToNotificationObject, if any of these can be done on one line instead of 2, please do so http://codereview.chromium.org/194079/diff/28001/28007 File chrome/renderer/notification_provider.h (right): http://codereview.chromium.org/194079/diff/28001/28007#newcode14 Line 14: #include "webkit/api/public/WebNotificationPermissionCallback.h" You could just forward declare this rather than including it. http://codereview.chromium.org/194079/diff/28001/28007#newcode57 Line 57: RenderView* view_; Maybe put a comment. Include details on ownership. http://codereview.chromium.org/194079/diff/28001/28008 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/194079/diff/28001/28008#newcode48 Line 48: #include "chrome/renderer/notification_provider.h" alphabetical http://codereview.chromium.org/194079/diff/28001/28009 File chrome/renderer/render_view.h (right): http://codereview.chromium.org/194079/diff/28001/28009#newcode33 Line 33: #include "chrome/renderer/notification_provider.h" alphabetical in future patches, you might want to be more careful about this...I've seen it a bunch of places
Never mind, Michael. It was the other that I wanted a sanity check on. :-) Found a few more issues. Once these are addressed, I'm ready to LGTM without another review. http://codereview.chromium.org/194079/diff/28001/28003 File chrome/common/desktop_notifications/active_notification_tracker.cc (right): http://codereview.chromium.org/194079/diff/28001/28003#newcode47 Line 47: WebNotification* notification = notification_table_.Lookup(id); use a scoped_ptr http://codereview.chromium.org/194079/diff/28001/28003#newcode50 Line 50: if (notification) { don't need {}'s http://codereview.chromium.org/194079/diff/28001/28003#newcode53 Line 53: delete notification; Avoid doing deletes at ALL costs. This will lead to programming practices (like using scoped_ptr) that avoid deletes and minimize the chances for adding leaks. http://codereview.chromium.org/194079/diff/28001/28006 File chrome/renderer/notification_provider.cc (right): http://codereview.chromium.org/194079/diff/28001/28006#newcode51 Line 51: return static_cast<WebKit::WebNotificationPresenter::Permission>(permission); Don't need WebKit:: http://codereview.chromium.org/194079/diff/28001/28006#newcode134 Line 134: if (callback) On 2009/09/30 21:22:16, Jeremy Orlow wrote: > I'm kind of starting to wonder if we need these. I know it'll crash the > renderer if we don't, but maybe that's the right thing to do...rather than error > checking for something that should never happen. And, if you go this route, then GetNotifiactions can probably just return the notification rather than a boolean 'found'.
http://codereview.chromium.org/194079/diff/28001/28003 File chrome/common/desktop_notifications/active_notification_tracker.cc (right): http://codereview.chromium.org/194079/diff/28001/28003#newcode47 Line 47: WebNotification* notification = notification_table_.Lookup(id); On 2009/09/30 21:35:45, Jeremy Orlow wrote: > use a scoped_ptr okay. http://codereview.chromium.org/194079/diff/28001/28003#newcode50 Line 50: if (notification) { On 2009/09/30 21:35:45, Jeremy Orlow wrote: > don't need {}'s Done. http://codereview.chromium.org/194079/diff/28001/28003#newcode53 Line 53: delete notification; On 2009/09/30 21:35:45, Jeremy Orlow wrote: > Avoid doing deletes at ALL costs. This will lead to programming practices (like > using scoped_ptr) that avoid deletes and minimize the chances for adding leaks. Done. http://codereview.chromium.org/194079/diff/28001/28005 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/194079/diff/28001/28005#newcode1623 Line 1623: string16 /* origin */, On 2009/09/30 21:22:16, Jeremy Orlow wrote: > line up with ( Done. http://codereview.chromium.org/194079/diff/28001/28006 File chrome/renderer/notification_provider.cc (right): http://codereview.chromium.org/194079/diff/28001/28006#newcode18 Line 18: : view_(view) { On 2009/09/30 21:22:16, Jeremy Orlow wrote: > 4 spaces in Done. http://codereview.chromium.org/194079/diff/28001/28006#newcode38 Line 38: const WebNotification& notification) { On 2009/09/30 21:22:16, Jeremy Orlow wrote: > this can't go on the previous line? it's 81 chars long that way :( http://codereview.chromium.org/194079/diff/28001/28006#newcode51 Line 51: return static_cast<WebKit::WebNotificationPresenter::Permission>(permission); On 2009/09/30 21:35:45, Jeremy Orlow wrote: > Don't need WebKit:: Done. http://codereview.chromium.org/194079/diff/28001/28006#newcode134 Line 134: if (callback) I can do without the check in this spot. But for notifications i definitely need the found, and now that I think about it it's an error to DCHECK for it, since it's a valid situation for a WebNotification to be removed from the map while an IPC is in flight (see earlier comment). http://codereview.chromium.org/194079/diff/28001/28006#newcode147 Line 147: IPC_MESSAGE_HANDLER(ViewMsg_PostErrorToNotificationObject, On 2009/09/30 21:22:16, Jeremy Orlow wrote: > if any of these can be done on one line instead of 2, please do so done for 3 of 4. http://codereview.chromium.org/194079/diff/28001/28007 File chrome/renderer/notification_provider.h (right): http://codereview.chromium.org/194079/diff/28001/28007#newcode14 Line 14: #include "webkit/api/public/WebNotificationPermissionCallback.h" On 2009/09/30 21:22:16, Jeremy Orlow wrote: > You could just forward declare this rather than including it. Done. http://codereview.chromium.org/194079/diff/28001/28007#newcode57 Line 57: RenderView* view_; On 2009/09/30 21:22:16, Jeremy Orlow wrote: > Maybe put a comment. Include details on ownership. Done. http://codereview.chromium.org/194079/diff/28001/28008 File chrome/renderer/render_view.cc (right): http://codereview.chromium.org/194079/diff/28001/28008#newcode48 Line 48: #include "chrome/renderer/notification_provider.h" On 2009/09/30 21:22:16, Jeremy Orlow wrote: > alphabetical Done. http://codereview.chromium.org/194079/diff/28001/28009 File chrome/renderer/render_view.h (right): http://codereview.chromium.org/194079/diff/28001/28009#newcode33 Line 33: #include "chrome/renderer/notification_provider.h" On 2009/09/30 21:22:16, Jeremy Orlow wrote: > alphabetical > > in future patches, you might want to be more careful about this...I've seen it a > bunch of places Done. Point taken; I'm confident I add them in the right place initially, but need to watch out when merging from trunk into my branch.
LGTM http://codereview.chromium.org/194079/diff/28001/28009 File chrome/renderer/render_view.h (right): http://codereview.chromium.org/194079/diff/28001/28009#newcode33 Line 33: #include "chrome/renderer/notification_provider.h" On 2009/10/01 16:31:12, John Gregg wrote: > On 2009/09/30 21:22:16, Jeremy Orlow wrote: > > alphabetical > > > > in future patches, you might want to be more careful about this...I've seen it > a > > bunch of places > > Done. Point taken; I'm confident I add them in the right place initially, but > need to watch out when merging from trunk into my branch. Happens to me too. :-)
This change seems to have broken at least the Vista builds, for both trunk and bleeding-edge V8 versions. It looks like it might break Vista and Windows 7, but not XP, and not Mac or Linux.
Is there an error log you can link to? Everything on the build bots seemed fine.
I guess I am looking at the experimental builds, not the waterfall ones. In the controls on the upper left of the buildbot waterfall page, click "experimental". A number of these builds stopped compiling with this change. I'm not sure how they are set up differently, but I think it is important. It is hard for people to notice, since some of them are usually red, but a lot of ones that were green turned red with this change, and the same error is showing up in them all. http://build.chromium.org/buildbot/waterfall.fyi/waterfall?builder=Webkit%20V... http://build.chromium.org/buildbot/waterfall.fyi/builders/Webkit%20Vista%20(d... The errors are 4>Linking... 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved external symbol "public: void __thiscall WebCore::Notification::show(void)" (?show@Notification@WebCore@@QAEXXZ) referenced in function "class v8::Handle<class v8::Value> __cdecl WebCore::NotificationInternal::showCallback(class v8::Arguments const &)" (?showCallback@NotificationInternal@WebCore@@YA?AV?$Handle@VValue@v8@@@v8@@ABVArguments@4@@Z) 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved external symbol "public: void __thiscall WebCore::Notification::cancel(void)" (?cancel@Notification@WebCore@@QAEXXZ) referenced in function "class v8::Handle<class v8::Value> __cdecl WebCore::NotificationInternal::cancelCallback(class v8::Arguments const &)" (?cancelCallback@NotificationInternal@WebCore@@YA?AV?$Handle@VValue@v8@@@v8@@ABVArguments@4@@Z) 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved external symbol "public: static class v8::Handle<class v8::Value> __cdecl WebCore::V8Custom::v8NotificationRemoveEventListenerCallback(class v8::Arguments const &)" (?v8NotificationRemoveEventListenerCallback@V8Custom@WebCore@@SA?AV?$Handle@VValue@v8@@@v8@@ABVArguments@4@@Z) referenced in function "class v8::Persistent<class v8::FunctionTemplate> __cdecl WebCore::ConfigureV8NotificationTemplate(class v8::Persistent<class v8::FunctionTemplate>)" (?ConfigureV8NotificationTemplate@WebCore@@YA?AV?$Persistent@VFunctionTemplate@v8@@@v8@@V23@@Z) 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved external symbol "public: static class v8::Handle<class v8::Value> __cdecl WebCore::V8Custom::v8NotificationAddEventListenerCallback(class v8::Arguments const &)" (?v8NotificationAddEventListenerCallback@V8Custom@WebCore@@SA?AV?$Handle@VValue@v8@@@v8@@ABVArguments@4@@Z) referenced in function "class v8::Persistent<class v8::FunctionTemplate> __cdecl WebCore::ConfigureV8NotificationTemplate(class v8::Persistent<class v8::FunctionTemplate>)" (?ConfigureV8NotificationTemplate@WebCore@@YA?AV?$Persistent@VFunctionTemplate@v8@@@v8@@V23@@Z) 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved external symbol "public: int __thiscall WebCore::NotificationCenter::checkPermission(void)" (?checkPermission@NotificationCenter@WebCore@@QAEHXZ) referenced in function "class v8::Handle<class v8::Value> __cdecl WebCore::NotificationCenterInternal::checkPermissionCallback(class v8::Arguments const &)" (?checkPermissionCallback@NotificationCenterInternal@WebCore@@YA?AV?$Handle@VValue@v8@@@v8@@ABVArguments@4@@Z) 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved external symbol "public: static class v8::Handle<class v8::Value> __cdecl WebCore::V8Custom::v8NotificationCenterRequestPermissionCallback(class v8::Arguments const &)" (?v8NotificationCenterRequestPermissionCallback@V8Custom@WebCore@@SA?AV?$Handle@VValue@v8@@@v8@@ABVArguments@4@@Z) referenced in function "class v8::Persistent<class v8::FunctionTemplate> __cdecl WebCore::ConfigureV8NotificationCenterTemplate(class v8::Persistent<class v8::FunctionTemplate>)" (?ConfigureV8NotificationCenterTemplate@WebCore@@YA?AV?$Persistent@VFunctionTemplate@v8@@@v8@@V23@@Z) 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved external symbol "public: static class v8::Handle<class v8::Value> __cdecl WebCore::V8Custom::v8NotificationCenterCreateNotificationCallback(class v8::Arguments const &)" (?v8NotificationCenterCreateNotificationCallback@V8Custom@WebCore@@SA?AV?$Handle@VValue@v8@@@v8@@ABVArguments@4@@Z) referenced in function "class v8::Persistent<class v8::FunctionTemplate> __cdecl WebCore::ConfigureV8NotificationCenterTemplate(class v8::Persistent<class v8::FunctionTemplate>)" (?ConfigureV8NotificationCenterTemplate@WebCore@@YA?AV?$Persistent@VFunctionTemplate@v8@@@v8@@V23@@Z) 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved external symbol "public: static class v8::Handle<class v8::Value> __cdecl WebCore::V8Custom::v8NotificationCenterCreateHTMLNotificationCallback(class v8::Arguments const &)" (?v8NotificationCenterCreateHTMLNotificationCallback@V8Custom@WebCore@@SA?AV?$Handle@VValue@v8@@@v8@@ABVArguments@4@@Z) referenced in function "class v8::Persistent<class v8::FunctionTemplate> __cdecl WebCore::ConfigureV8NotificationCenterTemplate(class v8::Persistent<class v8::FunctionTemplate>)" (?ConfigureV8NotificationCenterTemplate@WebCore@@YA?AV?$Persistent@VFunctionTemplate@v8@@@v8@@V23@@Z) 5>c:\b\slave\webkit-dbg-vista\build\src\webkit\Release\test_shell.exe : fatal error LNK1120: 8 unresolved externals
The compile errors that I was worried about are not due to this changelist. I see now that they are due to a webkit.org change, not a change in the chromium repository, and that I was confused because the Linux 64 webkit.orgbuildbot column is not adjacent to other platforms' webkit.org buildbot columns, and it does not say (webkit.org) in parenthese= s in its title, the way the others do. This made me think that it was part o= f the v8-latest group, and had the same configuration. On Tue, Oct 6, 2009 at 11:45 AM, <whesse@chromium.org> wrote: > I guess I am looking at the experimental builds, not the waterfall ones. = In > the > controls on the upper left of the buildbot waterfall page, click > "experimental". > A number of these builds stopped compiling with this change. I'm not sur= e > how > they are set up differently, but I think it is important. > > It is hard for people to notice, since some of them are usually red, but = a > lot > of ones that were green turned red with this change, and the same error i= s > showing up in them all. > > > http://build.chromium.org/buildbot/waterfall.fyi/waterfall?builder=3DWebk= it%20Vista%20(dbg)&last_time=3D1254775033 > > http://build.chromium.org/buildbot/waterfall.fyi/builders/Webkit%20Vista%= 20(dbg)/builds/3303/steps/compile/logs/stdio > > The errors are > 4>Linking... > 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved > external > symbol "public: void __thiscall WebCore::Notification::show(void)" > (?show@Notification@WebCore@@QAEXXZ) referenced in function "class > v8::Handle<class v8::Value> __cdecl > WebCore::NotificationInternal::showCallback(class v8::Arguments const &)" > (?showCallback@NotificationInternal@WebCore@@YA?AV?$Handle@VValue@v8@@@v8= @ > @ABVArguments@4@@Z) > 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved > external > symbol "public: void __thiscall WebCore::Notification::cancel(void)" > (?cancel@Notification@WebCore@@QAEXXZ) referenced in function "class > v8::Handle<class v8::Value> __cdecl > WebCore::NotificationInternal::cancelCallback(class v8::Arguments const &= )" > (?cancelCallback@NotificationInternal@WebCore@@YA?AV?$Handle@VValue@v8@ > @@v8@@ABVArguments@4@@Z) > 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved > external > symbol "public: static class v8::Handle<class v8::Value> __cdecl > WebCore::V8Custom::v8NotificationRemoveEventListenerCallback(class > v8::Arguments > const &)" > (?v8NotificationRemoveEventListenerCallback@V8Custom@WebCore@ > @SA?AV?$Handle@VValue@v8@@@v8@@ABVArguments@4@@Z) > referenced in function "class v8::Persistent<class v8::FunctionTemplate> > __cdecl > WebCore::ConfigureV8NotificationTemplate(class v8::Persistent<class > v8::FunctionTemplate>)" > (?ConfigureV8NotificationTemplate@WebCore > @@YA?AV?$Persistent@VFunctionTemplate@v8@@@v8@@V23@@Z) > 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved > external > symbol "public: static class v8::Handle<class v8::Value> __cdecl > WebCore::V8Custom::v8NotificationAddEventListenerCallback(class > v8::Arguments > const &)" > (?v8NotificationAddEventListenerCallback@V8Custom@WebCore@ > @SA?AV?$Handle@VValue@v8@@@v8@@ABVArguments@4@@Z) > referenced in function "class v8::Persistent<class v8::FunctionTemplate> > __cdecl > WebCore::ConfigureV8NotificationTemplate(class v8::Persistent<class > v8::FunctionTemplate>)" > (?ConfigureV8NotificationTemplate@WebCore > @@YA?AV?$Persistent@VFunctionTemplate@v8@@@v8@@V23@@Z) > 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved > external > symbol "public: int __thiscall > WebCore::NotificationCenter::checkPermission(void)" > (?checkPermission@NotificationCenter@WebCore@@QAEHXZ) referenced in > function > "class v8::Handle<class v8::Value> __cdecl > WebCore::NotificationCenterInternal::checkPermissionCallback(class > v8::Arguments > const &)" > (?checkPermissionCallback@NotificationCenterInternal@WebCore@ > @YA?AV?$Handle@VValue@v8@@@v8@@ABVArguments@4@@Z) > 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved > external > symbol "public: static class v8::Handle<class v8::Value> __cdecl > WebCore::V8Custom::v8NotificationCenterRequestPermissionCallback(class > v8::Arguments const &)" > (?v8NotificationCenterRequestPermissionCallback@V8Custom@WebCore@ > @SA?AV?$Handle@VValue@v8@@@v8@@ABVArguments@4@@Z) > referenced in function "class v8::Persistent<class v8::FunctionTemplate> > __cdecl > WebCore::ConfigureV8NotificationCenterTemplate(class v8::Persistent<class > v8::FunctionTemplate>)" > (?ConfigureV8NotificationCenterTemplate@WebCore > @@YA?AV?$Persistent@VFunctionTemplate@v8@@@v8@@V23@@Z) > 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved > external > symbol "public: static class v8::Handle<class v8::Value> __cdecl > WebCore::V8Custom::v8NotificationCenterCreateNotificationCallback(class > v8::Arguments const &)" > (?v8NotificationCenterCreateNotificationCallback@V8Custom@WebCore@ > @SA?AV?$Handle@VValue@v8@@@v8@@ABVArguments@4@@Z) > referenced in function "class v8::Persistent<class v8::FunctionTemplate> > __cdecl > WebCore::ConfigureV8NotificationCenterTemplate(class v8::Persistent<class > v8::FunctionTemplate>)" > (?ConfigureV8NotificationCenterTemplate@WebCore > @@YA?AV?$Persistent@VFunctionTemplate@v8@@@v8@@V23@@Z) > 5>webcore.lib(DerivedSourcesAllInOne.obj) : error LNK2019: unresolved > external > symbol "public: static class v8::Handle<class v8::Value> __cdecl > WebCore::V8Custom::v8NotificationCenterCreateHTMLNotificationCallback(cla= ss > v8::Arguments const &)" > (?v8NotificationCenterCreateHTMLNotificationCallback@V8Custom@WebCore@ > @SA?AV?$Handle@VValue@v8@@@v8@@ABVArguments@4@@Z) > referenced in function "class v8::Persistent<class v8::FunctionTemplate> > __cdecl > WebCore::ConfigureV8NotificationCenterTemplate(class v8::Persistent<class > v8::FunctionTemplate>)" > (?ConfigureV8NotificationCenterTemplate@WebCore > @@YA?AV?$Persistent@VFunctionTemplate@v8@@@v8@@V23@@Z) > 5>c:\b\slave\webkit-dbg-vista\build\src\webkit\Release\test_shell.exe : > fatal > error LNK1120: 8 unresolved externals > > > > > http://codereview.chromium.org/194079 > --=20 William Hesse Software Engineer whesse@google.com Google Denmark ApS Frederiksborggade 20B, 1 sal 1360 K=F8benhavn K Denmark CVR nr. 28 86 69 84 If you received this communication by mistake, please don't forward it to anyone else (it may contain confidential or privileged information), please erase all copies of it, including all attachments, and please let the sende= r know it went to the wrong person. Thanks.
Sorry I missed this earlier, but this isn't designed correctly (see below). It's currently leading to the number 2 renderer crash. I think it should be reverted in the meantime. http://codereview.chromium.org/194079/diff/34001/33006 File chrome/renderer/notification_provider.cc (right): http://codereview.chromium.org/194079/diff/34001/33006#newcode143 Line 143: if (message.routing_id() != view_->routing_id()) This isn't thread safe. IPC::ChannelProxy::MessageFilters run on the IO thread, and I don't see a reason why you need to do this, since all you end up doing is posting a task to the UI thread. You should just have an object that lives only on the main thread, and that way you don't need to intercept the message on the IO thread only to dispatch it to the UI thread manually. RenderView is not thread safe and should only be used on the main thread. There's a race condition here when RenderView is destructed, since the filter is removed via a PostTask. This is leading to the number 2 renderer crash now: http://chromedashboard.mtv.corp.google.com/#view=wincrash
Also, it looks like part of the reason that this was missed by other reviewers is that the default CC list from watchlists was removed. Please keep it in the future, as it serves a useful purpose which in this case would have caught this earlier. On 2009/10/14 20:33:28, John Abd-El-Malek wrote: > Sorry I missed this earlier, but this isn't designed correctly (see below). > It's currently leading to the number 2 renderer crash. I think it should be > reverted in the meantime. > > http://codereview.chromium.org/194079/diff/34001/33006 > File chrome/renderer/notification_provider.cc (right): > > http://codereview.chromium.org/194079/diff/34001/33006#newcode143 > Line 143: if (message.routing_id() != view_->routing_id()) > This isn't thread safe. IPC::ChannelProxy::MessageFilters run on the IO thread, > and I don't see a reason why you need to do this, since all you end up doing is > posting a task to the UI thread. You should just have an object that lives only > on the main thread, and that way you don't need to intercept the message on the > IO thread only to dispatch it to the UI thread manually. > > RenderView is not thread safe and should only be used on the main thread. > There's a race condition here when RenderView is destructed, since the filter is > removed via a PostTask. This is leading to the number 2 renderer crash now: > http://chromedashboard.mtv.corp.google.com/#view=wincrash |
