Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11745024:
Synced Notification Sync Change Processor (Closed)
|
Created:
7 years, 11 months ago by Pete Williamson Modified:
7 years, 10 months ago CC:
chromium-reviews, Raghu Simha, haitaol1, akalin, tim (not reviewing) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionSynced Notification Sync Change Processor
This change is the first of several to process changes from sync of the "Synced Notifications" data type. It defines the SyncedNotification type more fully in synced_notification_specifics.proto, and implements the incoming interfaces needed to handle the data.
This change handles notifications coming in from the sync channel. These notifications will eventually call into the C++ backend supporting the Desktop Notification API (doc here: https://sites.google.com/a/chromium.org/dev/developers/design-documents/extensions/proposed-changes/apis-under-development/desktop-notification-api )
It is based closely on the change list for history delete directives: https://chromiumcodereview.appspot.com/10855037
BUG=168212
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181825
Patch Set 1 #
Total comments: 78
Patch Set 2 : SyncedNotifications - first round of CR comment fixes. #
Total comments: 142
Patch Set 3 : Synced Notifications - CR changes per Zea and DCheng #
Total comments: 10
Patch Set 4 : Synced Notifications Sync Change Processor with CR fixes per DCheng #
Total comments: 10
Patch Set 5 : Synced Notifications Change Processor - CR #
Total comments: 27
Patch Set 6 : Synced Notifications Change Processor more CR changes per DCheng #Patch Set 7 : Lint fixes #
Total comments: 27
Patch Set 8 : Synced #
Total comments: 4
Patch Set 9 : Synced Notifications Change Processor CR fixes per Zea #
Total comments: 3
Patch Set 10 : SyncedNotifications - fix conflicts with merge from master #Patch Set 11 : Synced Notifications Change Processor - fix trybot issues #Patch Set 12 : Synced Notification Change Processor with more try fixes #Patch Set 13 : Synced Notifications Change Processor - more trybot issues #Patch Set 14 : Synced Notification Change Processor - even more trybot fixes #Patch Set 15 : Synced Notification Change Processor - android disable #Patch Set 16 : Synced Notification Change Processor - more android #Patch Set 17 : Synded Notifications Change Processor - more adnroid disabling #Patch Set 18 : Synced Notifications Change Processor - more android disabling #Patch Set 19 : Synced Notifications Change Processor - more android disabling #Patch Set 20 : Synced Notification Change Processor - remove unused header #Messages
Total messages: 45 (0 generated)
Nico - Could you please look at the files under chrome/browser/notifier (they are all new) and chrome_browser.gypi, or suggest someone else to do so? I apologize for going so high level to find a reviewer, but I couldn't find a specific enough OWNERS file further down. Eventually I plan to add myself as an owner of chrome/browser/notifier to ease the burden on higher level reviewers. Nicolas - could you please take a look at everything here? Fred and Daniel - I have cc-ed you in case you are interested, but I choose Nicolas and Nico as the primary reviewers.
Some comments. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... File chrome/browser/notifier/chrome_notifier_service.cc (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:29: // to worry about re-entrancy. Re-entrancy doesn't really relate to thread safety. That said, Sync does guarantee these methods will only be called on the datatype's thread. Rather than adding a comment, if you want to enforce the lack of thread safety, base::NonThreadSafe might be useful. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... File chrome/browser/notifier/chrome_notifier_service.h (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:7: remove extra newline https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:16: namespace sync_pb { This shouldn't be inside of the sync_pb namespace. That's reserved for files generated directly from the protos. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:20: class SyncedNotification { What are you trying to accomplish with this class? If it's just a 1:1 c++ version of the proto, how is it different from sync_pb::SyncNotificationSpecifics? https://codereview.chromium.org/11745024/diff/1/sync/protocol/server_data.proto File sync/protocol/server_data.proto (right): https://codereview.chromium.org/11745024/diff/1/sync/protocol/server_data.pro... sync/protocol/server_data.proto:18: remove extra newline https://codereview.chromium.org/11745024/diff/1/sync/protocol/server_data.pro... sync/protocol/server_data.proto:24: message Identifier { Given that these are all going to be part of the same namespace, and their similarity to existing sync message, I'd prefer more specific names. For example: SyncedNotificationIdentifier, SyncedNotificationData, and SyncedNotificationCoalescedData. Same in server_render.proto https://codereview.chromium.org/11745024/diff/1/sync/protocol/sync.proto File sync/protocol/sync.proto (right): https://codereview.chromium.org/11745024/diff/1/sync/protocol/sync.proto#newc... sync/protocol/sync.proto:35: import "server_data.proto"; do these need to be here? https://codereview.chromium.org/11745024/diff/1/sync/protocol/synced_notifica... File sync/protocol/synced_notification_specifics.proto (right): https://codereview.chromium.org/11745024/diff/1/sync/protocol/synced_notifica... sync/protocol/synced_notification_specifics.proto:17: import "server_render.proto"; Are you planning to reuse these protos anywhere? If not, I think I'd prefer having them inlined. Otherwise, the files should be renamed to make it clearer that they're specifics to the SyncedNotification datatype. "Server Data" in particular sounds like it related to the sync engine.
https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... File chrome/browser/notifier/chrome_notifier_service.cc (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:16: using namespace sync_pb; It's much more common to just wrap it in namespace sync_pb {} rather than pulling in the namespace. Also, I believe Nicolas commented on this as well, but the namespace shouldn't be sync_pb (as this is not generated protobuf code) https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:43: for (std::vector<SyncedNotification*>::iterator iter = our_data_.begin(); Nit: "it" is much more common than "iter". https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:57: CHECK(incoming.get()); Do we really want to CHECK() here? Is it possible we sometimes receive erroneous data from the server? It seems much more robust to continue gracefully to the next entry on an error like this and log an error or warning message. Same comment applies everywhere else CHECK() appears here. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:60: external_id = incoming->get_first_external_id(); const std::string& external_id = https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:65: std::string title = incoming->title(); const std::string& title https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:87: DCHECK(NULL != *iter); Isn't it a little late to DCHECK() that *iter is non-NULL here? Remove please. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:99: (sync_processor.Pass())->ProcessSyncChanges(FROM_HERE, new_changes)); I don't understand why you call Pass() here. Usually this would indicate ownership is being transferred... but that's doesn't seem like what's happening here. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:199: SyncedNotification* ChromeNotifierService::GetNotification( Perhaps FindNotificationWithTitle() would be a more descriptive name. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:200: const std::string& value) { "value" is not a meaningful name. I would call rename this "title" https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:219: bool ChromeNotifierService::Add(SyncedNotification* notification) { scoped_ptr<SyncedNotification> https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... File chrome/browser/notifier/chrome_notifier_service.h (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:10: #include "base/logging.h" Why do you need this header here? https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:17: // represent the state of outstanding notifications for chrome. What is an outstanding notification? Is the "array" detail important (it doesn't feel like it is, it's just an implementation detail, so why mention it here?) https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:48: const syncer::SyncData& sync_data); These static methods don't really feel like they belong here. I'd prefer to see them moved into the .cc as helper functions. Opinions vary on this, but they feel like an implementation detail that does not need to be exposed in the .h. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:50: bool Add(sync_pb::SyncedNotification* notification); Who calls this? What does the return value mean? https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:57: void LogContents(); Please describe what this method does. Is it only for debugging? Do we want this in production code? (It feels like the answer to the latter question should be "no") https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:59: // For now keep data in a vector, later likely move to a map. I feel like this comment ought to be a TODO or simply removed. In addition, why would it move to a map? https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:60: ScopedVector<sync_pb::SyncedNotification> our_data_; While pronouns don't always need to avoided, I don't feel like "our_" adds useful information here. It would be more useful to describe what kind of data this vector holds... https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:61: }; DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:62: Extra new line. In general, you don't really need two newlines in a row anywhere. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... File chrome/browser/notifier/chrome_notifier_service_unittest.cc (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service_unittest.cc:23: Extra newline. Please remove. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service_unittest.cc:24: const char MY_APP_ID[] = "fboilmbenheemaomgaeehigklolhkhnf"; Wrong naming convention. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... File chrome/browser/notifier/synced_notification.cc (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.cc:54: Remove extra newlines at the beginning of all blocks please. Same comment on line 91 and elsewhere. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.cc:55: if (!sync_data_.GetSpecifics().synced_notification(). 1) This seems overly complicated. I think a high level comment would help for all of these helper functions. 2) Given the complexity of this method, I'm not really sure calling it "title()" is appropriate (as that is used for simple accessors, which this is definitely not). 3) Consider aliasing some variables: const SyncedNotificationSpecifics& specifics = sync_data_.GetSpecifics().synced_notification(); if (!specifics.has_coalesced_notification() || !specifics.coalesced_notification().has_render_info() || etc...) Alternatively, do we really need to nest so many protobufs? This seems to be a big reason you need to do so many has_* checks. Can we flatten the structure some? https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.cc:114: const std::string SyncedNotification::app_id() const { Similar comment about incorrect naming (these are not simple accessors) applies. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:28: const std::string title() const; Why return a const string? https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:44: bool has_local_changes() { This method should be const. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:48: syncer::SyncData* sync_data() { Should this return a const pointer instead? If not, this should probably be called mutable_sync_data(). https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:63: // This keeps the SyncNotificationSpecifics alive. How does it keep it alive?
+KevinLiu to check the protobuf changes. Nicolas and Daniel, this should answer all your issues. I've also added some new work to bring the patch up to date with my integration into Kevin's code. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... File chrome/browser/notifier/chrome_notifier_service.cc (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:16: using namespace sync_pb; On 2013/01/07 20:42:10, dcheng wrote: > It's much more common to just wrap it in namespace sync_pb {} rather than > pulling in the namespace. Also, I believe Nicolas commented on this as well, but > the namespace shouldn't be sync_pb (as this is not generated protobuf code) This now uses a different type of using statement, using sync_pb::SyncedNotification; Is that better? https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:29: // to worry about re-entrancy. On 2013/01/05 01:18:48, Nicolas Zea wrote: > Re-entrancy doesn't really relate to thread safety. > > That said, Sync does guarantee these methods will only be called on the > datatype's thread. Rather than adding a comment, if you want to enforce the lack > of thread safety, base::NonThreadSafe might be useful. Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:43: for (std::vector<SyncedNotification*>::iterator iter = our_data_.begin(); On 2013/01/07 20:42:10, dcheng wrote: > Nit: "it" is much more common than "iter". Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:57: CHECK(incoming.get()); On 2013/01/07 20:42:10, dcheng wrote: > Do we really want to CHECK() here? Is it possible we sometimes receive erroneous > data from the server? > > It seems much more robust to continue gracefully to the next entry on an error > like this and log an error or warning message. Same comment applies everywhere > else CHECK() appears here. After I looked at the other code I originally copied the template from, I changed all these to DCHECK() https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:60: external_id = incoming->get_first_external_id(); On 2013/01/07 20:42:10, dcheng wrote: > const std::string& external_id = Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:65: std::string title = incoming->title(); On 2013/01/07 20:42:10, dcheng wrote: > const std::string& title Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:87: DCHECK(NULL != *iter); On 2013/01/07 20:42:10, dcheng wrote: > Isn't it a little late to DCHECK() that *iter is non-NULL here? Remove please. Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:99: (sync_processor.Pass())->ProcessSyncChanges(FROM_HERE, new_changes)); On 2013/01/07 20:42:10, dcheng wrote: > I don't understand why you call Pass() here. Usually this would indicate > ownership is being transferred... but that's doesn't seem like what's happening > here. Oops, good catch, converted to get() https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:199: SyncedNotification* ChromeNotifierService::GetNotification( On 2013/01/07 20:42:10, dcheng wrote: > Perhaps FindNotificationWithTitle() would be a more descriptive name. Renamed to FindNotifictionById() https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:200: const std::string& value) { On 2013/01/07 20:42:10, dcheng wrote: > "value" is not a meaningful name. I would call rename this "title" Renamed to "id" since we now find by ID instead of title https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.cc:219: bool ChromeNotifierService::Add(SyncedNotification* notification) { On 2013/01/07 20:42:10, dcheng wrote: > scoped_ptr<SyncedNotification> Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... File chrome/browser/notifier/chrome_notifier_service.h (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:7: On 2013/01/05 01:18:48, Nicolas Zea wrote: > remove extra newline Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:10: #include "base/logging.h" On 2013/01/07 20:42:10, dcheng wrote: > Why do you need this header here? Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:17: // represent the state of outstanding notifications for chrome. On 2013/01/07 20:42:10, dcheng wrote: > What is an outstanding notification? Is the "array" detail important (it doesn't > feel like it is, it's just an implementation detail, so why mention it here?) Comment clarified to remove the word array and explain outstanding. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:17: // represent the state of outstanding notifications for chrome. On 2013/01/07 20:42:10, dcheng wrote: > What is an outstanding notification? Is the "array" detail important (it doesn't > feel like it is, it's just an implementation detail, so why mention it here?) Comment clarified, the words array and outstanding replaced https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:50: bool Add(sync_pb::SyncedNotification* notification); On 2013/01/07 20:42:10, dcheng wrote: > Who calls this? What does the return value mean? This is called by unit tests and by other functions in the file. I removed the return value https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:57: void LogContents(); On 2013/01/07 20:42:10, dcheng wrote: > Please describe what this method does. Is it only for debugging? Do we want this > in production code? (It feels like the answer to the latter question should be > "no") Removed. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:59: // For now keep data in a vector, later likely move to a map. On 2013/01/07 20:42:10, dcheng wrote: > I feel like this comment ought to be a TODO or simply removed. In addition, why > would it move to a map? Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:60: ScopedVector<sync_pb::SyncedNotification> our_data_; On 2013/01/07 20:42:10, dcheng wrote: > While pronouns don't always need to avoided, I don't feel like "our_" adds > useful information here. It would be more useful to describe what kind of data > this vector holds... Changed to "notification_data" https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:61: }; On 2013/01/07 20:42:10, dcheng wrote: > DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service.h:62: On 2013/01/07 20:42:10, dcheng wrote: > Extra new line. In general, you don't really need two newlines in a row > anywhere. Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... File chrome/browser/notifier/chrome_notifier_service_unittest.cc (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service_unittest.cc:23: On 2013/01/07 20:42:10, dcheng wrote: > Extra newline. Please remove. Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/chrom... chrome/browser/notifier/chrome_notifier_service_unittest.cc:24: const char MY_APP_ID[] = "fboilmbenheemaomgaeehigklolhkhnf"; On 2013/01/07 20:42:10, dcheng wrote: > Wrong naming convention. Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... File chrome/browser/notifier/synced_notification.cc (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.cc:54: On 2013/01/07 20:42:10, dcheng wrote: > Remove extra newlines at the beginning of all blocks please. Same comment on > line 91 and elsewhere. Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.cc:55: if (!sync_data_.GetSpecifics().synced_notification(). On 2013/01/07 20:42:10, dcheng wrote: > 1) This seems overly complicated. I think a high level comment would help for > all of these helper functions. > > 2) Given the complexity of this method, I'm not really sure calling it "title()" > is appropriate (as that is used for simple accessors, which this is definitely > not). > > 3) Consider aliasing some variables: > const SyncedNotificationSpecifics& specifics = > sync_data_.GetSpecifics().synced_notification(); > > if (!specifics.has_coalesced_notification() || > !specifics.coalesced_notification().has_render_info() || > etc...) > > Alternatively, do we really need to nest so many protobufs? This seems to be a > big reason you need to do so many has_* checks. Can we flatten the structure > some? All functions renamed so they no longer have a name like a a simple accessor. I tried aliasing at first, but for some reason that I wasn't quickly able to debug, the aliasing failed. I'll come back and try it later (There is a TODO(petewil) tracking this). The protobuf format is given to us by the server, I'm not in control of it. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.cc:114: const std::string SyncedNotification::app_id() const { On 2013/01/07 20:42:10, dcheng wrote: > Similar comment about incorrect naming (these are not simple accessors) applies. Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:16: namespace sync_pb { On 2013/01/05 01:18:48, Nicolas Zea wrote: > This shouldn't be inside of the sync_pb namespace. That's reserved for files > generated directly from the protos. What namespace would be better? None? A new namespace? Some pre-existing sync namespace? https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:20: class SyncedNotification { On 2013/01/05 01:18:48, Nicolas Zea wrote: > What are you trying to accomplish with this class? If it's just a 1:1 c++ > version of the proto, how is it different from > sync_pb::SyncNotificationSpecifics? The primary point of this class is to do all the grungy logic of getting things out of a deeply nested sync class and making them easily available to the rest of the code. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:28: const std::string title() const; On 2013/01/07 20:42:10, dcheng wrote: > Why return a const string? Would you prefer a const string&? I don't think I understand the intent of your question. The data souldn't change, hence the const. The data is in string form, hence the string. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:44: bool has_local_changes() { On 2013/01/07 20:42:10, dcheng wrote: > This method should be const. Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:48: syncer::SyncData* sync_data() { On 2013/01/07 20:42:10, dcheng wrote: > Should this return a const pointer instead? If not, this should probably be > called mutable_sync_data(). Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:63: // This keeps the SyncNotificationSpecifics alive. On 2013/01/07 20:42:10, dcheng wrote: > How does it keep it alive? Comment updated to explain how it is kept alive. https://codereview.chromium.org/11745024/diff/1/sync/protocol/server_data.proto File sync/protocol/server_data.proto (right): https://codereview.chromium.org/11745024/diff/1/sync/protocol/server_data.pro... sync/protocol/server_data.proto:18: On 2013/01/05 01:18:48, Nicolas Zea wrote: > remove extra newline Done. https://codereview.chromium.org/11745024/diff/1/sync/protocol/server_data.pro... sync/protocol/server_data.proto:24: message Identifier { On 2013/01/05 01:18:48, Nicolas Zea wrote: > Given that these are all going to be part of the same namespace, and their > similarity to existing sync message, I'd prefer more specific names. For > example: SyncedNotificationIdentifier, SyncedNotificationData, and > SyncedNotificationCoalescedData. > > Same in server_render.proto Done. https://codereview.chromium.org/11745024/diff/1/sync/protocol/sync.proto File sync/protocol/sync.proto (right): https://codereview.chromium.org/11745024/diff/1/sync/protocol/sync.proto#newc... sync/protocol/sync.proto:35: import "server_data.proto"; On 2013/01/05 01:18:48, Nicolas Zea wrote: > do these need to be here? Done. https://codereview.chromium.org/11745024/diff/1/sync/protocol/synced_notifica... File sync/protocol/synced_notification_specifics.proto (right): https://codereview.chromium.org/11745024/diff/1/sync/protocol/synced_notifica... sync/protocol/synced_notification_specifics.proto:17: import "server_render.proto"; On 2013/01/05 01:18:48, Nicolas Zea wrote: > Are you planning to reuse these protos anywhere? If not, I think I'd prefer > having them inlined. > > Otherwise, the files should be renamed to make it clearer that they're specifics > to the SyncedNotification datatype. "Server Data" in particular sounds like it > related to the sync engine. Yes, these are being matched by a similar set of protos on the server side.
FYI, a really helpful tool to catch some of these consistency issues is git cl
lint (or gcl lint if you're still on SVN) and the linter on Rietveld. I've gone
ahead and run the linter on your uploaded patch; git cl lint will probably catch
some other issues as well. Some errors can be ignored ("no new line at the end
of file" -> no one really cares) but others ought to be addressed (e.g. "You
don't need a ; after a }").
https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce...
File chrome/browser/notifier/synced_notification.cc (right):
https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce...
chrome/browser/notifier/synced_notification.cc:55: if
(!sync_data_.GetSpecifics().synced_notification().
On 2013/01/18 18:58:39, Pete Williamson wrote:
> The protobuf format is given to us by the server, I'm not in control of it.
Sure but we can still change it at this point in time...
I'm not convinced that having lots of deeply nested messages like we currently
have defined results in cleaner code (look at how many checks you have to do in
SyncedNotification!) so I'd really like to see it change, but if I'm alone in
that opinion, I will defer to the majority.
https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce...
File chrome/browser/notifier/synced_notification.h (right):
https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce...
chrome/browser/notifier/synced_notification.h:28: const std::string title()
const;
On 2013/01/18 18:58:39, Pete Williamson wrote:
> On 2013/01/07 20:42:10, dcheng wrote:
> > Why return a const string?
>
> Would you prefer a const string&?
> I don't think I understand the intent of your question.
> The data souldn't change, hence the const.
> The data is in string form, hence the string.
Returning a const string doesn't guarantee that, and it just makes things harder
for people that want a mutable string (as they just have to make another copy
then).
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
File chrome/browser/notifier/chrome_notifier_delegate.cc (right):
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_delegate.cc:7: // has been closed.
Duplicate comment? This would be a good place to talk about tricky
implementation details if needed.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_delegate.cc:11: #include <string>
You shouldn't need this #include here or the .h. If you do, that's a sign that a
header file elsewhere isn't including it when it should be.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
File chrome/browser/notifier/chrome_notifier_delegate.h (right):
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_delegate.h:7: // has been closed.
Typical convention is to place class comments above the class (in this case,
after line 13). Also, what is this class going to do eventually? It certainly
seems like it handles more things than just Close(), so how come we don't need
to implement any of the other methods?
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_delegate.h:17: id_(id) {}
Please move this to the .cc.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_delegate.h:27: }
Fix indent. Or just move all the empty bodies to the .cc.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_delegate.h:30: virtual
~ChromeNotifierDelegate() {}
Please move this to the .cc.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_delegate.h:32: std::string id_;
const? This Or can this change?
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
File chrome/browser/notifier/chrome_notifier_service.cc (right):
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.cc:29: NotificationUIManager*
manager) :
Incorrect formatting. : should be on the next line.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.cc:120: syncer::SyncDataList
local_sync_data;
Just call this sync_data?
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.cc:131: }
Indent.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.cc:157:
Add(new_notification.Pass());
Indent.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.cc:236:
Empty newlines here and elsewhere at the beginning of methods are not needed.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.cc:241: string16 title16 =
UTF8ToWide(title);
UTF8ToUTF16.
Despite the fact that wstring happens to be string16 on some platforms, wstring
is deprecated and should not be used. This also won't compile on, say, Linux.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.cc:241: string16 title16 =
UTF8ToWide(title);
Why not string16 title = UTF8ToUTF16(notification->get_title()). It can
potentially save a copy and it doesn't seem like the std::string variables are
really used for anything else.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.cc:251: NotificationDelegate*
delegate = new ChromeNotifierDelegate(notification_id);
Use scoped_refptr. Not sure this comment is needed either.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.cc:260: << " " << icon_url << "
" << notification_id;
Formatting.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
File chrome/browser/notifier/chrome_notifier_service.h (right):
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.h:18: // The
ChromeNotifierService holds notifications which
Optional/preference/nit: I'm just eyeballing this, but try to wrap comments at
80 columns, not before. 80 columns is very few already, no need to make it
harder!
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.h:23: public base::NonThreadSafe
{
Why not use ThreadChecker (which uses composition) instead of inheritance?
Also, note that you have to actually call CalledOnValidThread() for this to do
anything =)
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.h:47:
sync_pb::SyncedNotification& notification);
Pass by const reference.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.h:51: const syncer::SyncData&
sync_data);
Optional/preference/nit: I'd personally prefer to see this made private and
public ForTest() versions for the unit test to call. It doesn't seem like anyone
outside of those two use cases should care about calling this function. But I
also don't really care.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.h:59: // The caller must not
free the it.
s/the//
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service.h:63: // Backpointer to the
owning profile, do not free at shutdown.
Back pointer. I also feel that "do not free" is redundant, as we have a raw
pointer here anyway.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
File chrome/browser/notifier/chrome_notifier_service_factory.cc (right):
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_factory.cc:48: // TODO: This
assumes the notification_ui_manager remains valid throughout the
TODO's should have an associated name.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_factory.cc:58: return true;
It might be helpful to comment why we can't just use the default here.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
File chrome/browser/notifier/chrome_notifier_service_factory.h (right):
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_factory.h:15: // Profiles.
I'm not really sure this comment adds anything. Remove?
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_factory.h:21: static
ChromeNotifierService* GetForProfileIfExists(
Is there a reason we need GetForProfileIfExists() or
GetForProfileWithoutCreating()?
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
File chrome/browser/notifier/chrome_notifier_service_unittest.cc (right):
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:49: const unsigned
long long kFakeCreationTime = 42;
Use int64 from basictypes.h.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:71: }
Not needed.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:104: }
I don't think you need this.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:138:
DCHECK(ContainsId(id));
Don't DCHECK/CHECK in testing code. It causes the entire test binary to abort.
Instead, use the appropriate gtest macros (ASSERT_*, EXPECT_*, or
ADD_FAILURE()).
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:174:
Extra newline. Please remove.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:209:
scoped_ptr<SyncedNotification> scoped_notif(notification);
Don't use notif as an abbreviation please.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:221:
Remove extra newline.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:223: // on the
stack.
Not a helpful comment. You can remove all the comments in this function.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:271:
scoped_ptr<TestChangeProcessor> sync_processor_;
Nit: SyncChangeProcessor
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:272:
scoped_ptr<SyncChangeProcessorDelegate> sync_processor_delegate_;
Nit: SyncChangeProcessor
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:280:
scoped_ptr<SyncedNotification> notif1(
Ditto to not abbreviating notification.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:302: EXPECT_EQ(0U,
processor()->change_list_size());}
Formatting.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:330: EXPECT_EQ(3U,
notifier.GetAllSyncData(SYNCED_NOTIFICATIONS).size());
May want to consider verifying that the entries in the list have the expected
values.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:352: SyncData s1 =
CreateSyncData("4", kAppId4, kCoalescingKey4, "44");
Why is this the only one created differently?
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c...
chrome/browser/notifier/chrome_notifier_service_unittest.cc:365: // Ensure the
array now has all local and remote notifications.
Ensure the local store has records of all local and remote notifications.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
File chrome/browser/notifier/synced_notification.cc (right):
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification.cc:103: if
(SyncedNotificationRenderInfo_Layout_LayoutType_TITLE_AND_SUBTEXT
switch () { }?
It may be helpful in other areas to use switch() as well so it's obvious what
cases you're purposely ignoring.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification.cc:177: const std::string
SyncedNotification::get_origin_url() const {
Return GURL?
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification.cc:183: const std::string
SyncedNotification::get_icon_url() const {
Return GURL?
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification.cc:215: // We have an icon after
all, get the URL as a string.
Some of these comments seem misplaced and/or unnecessary. Similar remarks apply
to some of the other comments in this file.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification.cc:230: // If we have subtext data,
concat the text lines and return it.
concatenate.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
File chrome/browser/notifier/synced_notification.h (right):
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification.h:22: explicit
SyncedNotification(const syncer::SyncData sync_data);
Pass by const reference please.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification.h:28: const std::string get_title()
const;
GetTitle. Ditto for everything else.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification.h:67: // it into the contained sync
entity.
I really don't understand what this comment means. There's no other reference to
SyncedNotificationSpecifics in the .h. Maybe just remove this comment or move it
to where it applies in the .cc.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification.h:68: syncer::SyncData sync_data_;
Extra space.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification.h:70: bool has_local_changes_;
DISALLOW_COPY_AND_ASSIGN.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
File chrome/browser/notifier/synced_notification_unittest.cc (right):
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification_unittest.cc:14: using std::string;
Optional/preference/nit: I'd prefer not to have this using line for consistency
with most Chrome code (most of Chrome just uses the std:: qualifier).
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification_unittest.cc:23: const unsigned long
long FAKE_CREATION_TIME = 42;
Constants are named kFoo. Also use int64 instead of long long.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification_unittest.cc:39:
Unneeded newline.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification_unittest.cc:57: virtual void
TearDown() {
OVERRIDE on SetUp() and TearDown()?
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification_unittest.cc:60: // TODO: convert to
scoped_ptr
Please fix this before checking in. This will probably cause the Valgrind and/or
Tsan bots to turn red.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification_unittest.cc:65:
Remove unnecessary newlines please.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification_unittest.cc:72:
Ditto on newlines.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification_unittest.cc:74: // on the stack.
Unneeded comments.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification_unittest.cc:149:
EXPECT_TRUE(found_app_id == expected_app_id);
EXPECT_EQ
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification_unittest.cc:163:
EXPECT_TRUE(found_title == expected_title);
EXPECT_EQ
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification_unittest.cc:170:
EXPECT_TRUE(found_icon_url == expected_icon_url);
EXPECT_EQ.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s...
chrome/browser/notifier/synced_notification_unittest.cc:187:
EXPECT_TRUE(found_id == expected_id);
EXPECT_EQ.
https://codereview.chromium.org/11745024/diff/12001/sync/protocol/server_data...
File sync/protocol/server_data.proto (right):
https://codereview.chromium.org/11745024/diff/12001/sync/protocol/server_data...
sync/protocol/server_data.proto:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
Meta comment that the file could use a better name. "server_data" and
"server_render" aren't very descriptive.
https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:16: namespace sync_pb { On 2013/01/18 18:58:39, Pete Williamson wrote: > On 2013/01/05 01:18:48, Nicolas Zea wrote: > > This shouldn't be inside of the sync_pb namespace. That's reserved for files > > generated directly from the protos. > > What namespace would be better? None? A new namespace? Some pre-existing sync > namespace? Notifier perhaps? Might also make sense to have the other classes within browser/notifier be in a notifier namespace. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.cc (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.cc:85: if (!sync_data_.GetSpecifics().synced_notification(). I think you might be able to get away with just checking sync_data_.GetSpecifics().synced_notification(). coalesced_notification().render_info().layout().has_layout_type() If I recall correctly, protocol buffers will return a "default" empty message if the has_x is false. The empty message's fields will all return false for has_y. So even if there is no coalesced notification, you can still access its contents. I don't think this applies to repeated fields, for which you should do bounds checking. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:67: // it into the contained sync entity. Is the only reason you keep around the sync data the fact that all your get* methods query it directly? Would it be better to just have local variables that are parsed directly from the specifics? E.g. have a static ParseFromSyncData method that fills the local variables (title, app id, coalescing key, etc.), as well as a CreateSyncData method to create a new SyncData from all the local variables if local changes have been made. https://codereview.chromium.org/11745024/diff/12001/sync/protocol/server_data... File sync/protocol/server_data.proto (right): https://codereview.chromium.org/11745024/diff/12001/sync/protocol/server_data... sync/protocol/server_data.proto:38: message SyncedNotificationNotification { nit: perhaps just SyncedNotification and CoalescedSyncedNotification (notificationnotification seems odd)
This should answer all of DCheng and Zea's comments. It includes a new namespace (notifier), removing some unused code, renaming all the getter functions to match the coding conventions, and some clearer names in the protobuf files. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... File chrome/browser/notifier/synced_notification.cc (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.cc:55: if (!sync_data_.GetSpecifics().synced_notification(). On 2013/01/18 21:56:13, dcheng wrote: > On 2013/01/18 18:58:39, Pete Williamson wrote: > > The protobuf format is given to us by the server, I'm not in control of it. > > Sure but we can still change it at this point in time... > I'm not convinced that having lots of deeply nested messages like we currently > have defined results in cleaner code (look at how many checks you have to do in > SyncedNotification!) so I'd really like to see it change, but if I'm alone in > that opinion, I will defer to the majority. I used Nicolas's idea to eliminate a lot of the extra checks, see if you like it better now. If not, I can investigate changing the protobufs, but I have to do that in cooperation with the sync team. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:16: namespace sync_pb { On 2013/01/18 23:09:25, Nicolas Zea wrote: > On 2013/01/18 18:58:39, Pete Williamson wrote: > > On 2013/01/05 01:18:48, Nicolas Zea wrote: > > > This shouldn't be inside of the sync_pb namespace. That's reserved for files > > > generated directly from the protos. > > > > What namespace would be better? None? A new namespace? Some pre-existing > sync > > namespace? > > Notifier perhaps? Might also make sense to have the other classes within > browser/notifier be in a notifier namespace. Done. https://codereview.chromium.org/11745024/diff/1/chrome/browser/notifier/synce... chrome/browser/notifier/synced_notification.h:28: const std::string title() const; On 2013/01/18 21:56:13, dcheng wrote: > On 2013/01/18 18:58:39, Pete Williamson wrote: > > On 2013/01/07 20:42:10, dcheng wrote: > > > Why return a const string? > > > > Would you prefer a const string&? > > I don't think I understand the intent of your question. > > The data souldn't change, hence the const. > > The data is in string form, hence the string. > > Returning a const string doesn't guarantee that, and it just makes things harder > for people that want a mutable string (as they just have to make another copy > then). Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_delegate.cc (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_delegate.cc:7: // has been closed. On 2013/01/18 21:56:13, dcheng wrote: > Duplicate comment? This would be a good place to talk about tricky > implementation details if needed. Yep, this is a dup, removed. (my theory is that class comment duplication is good, I've removed it since you seem to disagree.) https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_delegate.cc:11: #include <string> On 2013/01/18 21:56:13, dcheng wrote: > You shouldn't need this #include here or the .h. If you do, that's a sign that a > header file elsewhere isn't including it when it should be. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_delegate.h (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_delegate.h:7: // has been closed. On 2013/01/18 21:56:13, dcheng wrote: > Typical convention is to place class comments above the class (in this case, > after line 13). Also, what is this class going to do eventually? It certainly > seems like it handles more things than just Close(), so how come we don't need > to implement any of the other methods? Comment moved. Actually, we don't do the display, we only need the closed notification so we can change the "read" flag on the notification. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_delegate.h:17: id_(id) {} On 2013/01/18 21:56:13, dcheng wrote: > Please move this to the .cc. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_delegate.h:27: } On 2013/01/18 21:56:13, dcheng wrote: > Fix indent. Or just move all the empty bodies to the .cc. indent fixed https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_delegate.h:32: std::string id_; On 2013/01/18 21:56:13, dcheng wrote: > const? This Or can this change? Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.cc (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:29: NotificationUIManager* manager) : On 2013/01/18 21:56:13, dcheng wrote: > Incorrect formatting. : should be on the next line. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:120: syncer::SyncDataList local_sync_data; On 2013/01/18 21:56:13, dcheng wrote: > Just call this sync_data? Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:131: } On 2013/01/18 21:56:13, dcheng wrote: > Indent. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:157: Add(new_notification.Pass()); On 2013/01/18 21:56:13, dcheng wrote: > Indent. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:236: On 2013/01/18 21:56:13, dcheng wrote: > Empty newlines here and elsewhere at the beginning of methods are not needed. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:241: string16 title16 = UTF8ToWide(title); On 2013/01/18 21:56:13, dcheng wrote: > UTF8ToUTF16. > > Despite the fact that wstring happens to be string16 on some platforms, wstring > is deprecated and should not be used. This also won't compile on, say, Linux. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:241: string16 title16 = UTF8ToWide(title); On 2013/01/18 21:56:13, dcheng wrote: > Why not string16 title = UTF8ToUTF16(notification->get_title()). It can > potentially save a copy and it doesn't seem like the std::string variables are > really used for anything else. I expect the compiler to be smart and not allocate an extra variable. Also, IMO, it makes the code easier to read, and helps with debugging. I do reuse the utf8 variables in the DVLOG statement. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:251: NotificationDelegate* delegate = new ChromeNotifierDelegate(notification_id); On 2013/01/18 21:56:13, dcheng wrote: > Use scoped_refptr. Not sure this comment is needed either. Done. I removed the TODO, but kept the rest of the comment. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:260: << " " << icon_url << " " << notification_id; On 2013/01/18 21:56:13, dcheng wrote: > Formatting. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service_factory.cc (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_factory.cc:48: // TODO: This assumes the notification_ui_manager remains valid throughout the On 2013/01/18 21:56:13, dcheng wrote: > TODO's should have an associated name. Instead, I just did the research and removed the TODO (I use a personal convention of not putting my name on anything that should be done before checkin, and this one slipped past me.) https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_factory.cc:58: return true; On 2013/01/18 21:56:13, dcheng wrote: > It might be helpful to comment why we can't just use the default here. Started an email thread to find the right setting and the reason for it. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service_factory.h (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_factory.h:15: // Profiles. On 2013/01/18 21:56:13, dcheng wrote: > I'm not really sure this comment adds anything. Remove? Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_factory.h:15: // Profiles. On 2013/01/18 21:56:13, dcheng wrote: > I'm not really sure this comment adds anything. Remove? Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_factory.h:21: static ChromeNotifierService* GetForProfileIfExists( On 2013/01/18 21:56:13, dcheng wrote: > Is there a reason we need GetForProfileIfExists() or > GetForProfileWithoutCreating()? Nope, removed. I thought it was inherited from the base class, but it seems not to have been. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service_unittest.cc (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:49: const unsigned long long kFakeCreationTime = 42; On 2013/01/18 21:56:13, dcheng wrote: > Use int64 from basictypes.h. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:71: } On 2013/01/18 21:56:13, dcheng wrote: > Not needed. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:104: } On 2013/01/18 21:56:13, dcheng wrote: > I don't think you need this. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:138: DCHECK(ContainsId(id)); On 2013/01/18 21:56:13, dcheng wrote: > Don't DCHECK/CHECK in testing code. It causes the entire test binary to abort. > > Instead, use the appropriate gtest macros (ASSERT_*, EXPECT_*, or > ADD_FAILURE()). Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:174: On 2013/01/18 21:56:13, dcheng wrote: > Extra newline. Please remove. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:209: scoped_ptr<SyncedNotification> scoped_notif(notification); On 2013/01/18 21:56:13, dcheng wrote: > Don't use notif as an abbreviation please. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:221: On 2013/01/18 21:56:13, dcheng wrote: > Remove extra newline. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:223: // on the stack. On 2013/01/18 21:56:13, dcheng wrote: > Not a helpful comment. You can remove all the comments in this function. I removed the rest, but I think this one is important for proper code maintenance, so I left it. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:271: scoped_ptr<TestChangeProcessor> sync_processor_; On 2013/01/18 21:56:13, dcheng wrote: > Nit: SyncChangeProcessor I presume you mean to change the type to SyncChangeProcessor from TestChangeProcessor. Why? The processor function is expeted to return a TestChangeProcessor, do you think the code is clearer or cleaner if we use SyncChangeProcessor and convert it when we hand it out? https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:280: scoped_ptr<SyncedNotification> notif1( On 2013/01/18 21:56:13, dcheng wrote: > Ditto to not abbreviating notification. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:302: EXPECT_EQ(0U, processor()->change_list_size());} On 2013/01/18 21:56:13, dcheng wrote: > Formatting. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:330: EXPECT_EQ(3U, notifier.GetAllSyncData(SYNCED_NOTIFICATIONS).size()); On 2013/01/18 21:56:13, dcheng wrote: > May want to consider verifying that the entries in the list have the expected > values. added TODO(petewil): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:352: SyncData s1 = CreateSyncData("4", kAppId4, kCoalescingKey4, "44"); On 2013/01/18 21:56:13, dcheng wrote: > Why is this the only one created differently? This is remote data. Today remote data and local data have different forms, and I need to test that we handle both forms correctly and merge them correctly. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:365: // Ensure the array now has all local and remote notifications. On 2013/01/18 21:56:13, dcheng wrote: > Ensure the local store has records of all local and remote notifications. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.cc (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.cc:85: if (!sync_data_.GetSpecifics().synced_notification(). On 2013/01/18 23:09:25, Nicolas Zea wrote: > I think you might be able to get away with just checking > sync_data_.GetSpecifics().synced_notification(). > coalesced_notification().render_info().layout().has_layout_type() > > If I recall correctly, protocol buffers will return a "default" empty message if > the has_x is false. The empty message's fields will all return false for has_y. > > So even if there is no coalesced notification, you can still access its > contents. I don't think this applies to repeated fields, for which you should do > bounds checking. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.cc:103: if (SyncedNotificationRenderInfo_Layout_LayoutType_TITLE_AND_SUBTEXT On 2013/01/18 21:56:13, dcheng wrote: > switch () { }? > > It may be helpful in other areas to use switch() as well so it's obvious what > cases you're purposely ignoring. changed here. In the other places I left the if statement in because only one type actually has the data we are looking for. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.cc:177: const std::string SyncedNotification::get_origin_url() const { On 2013/01/18 21:56:13, dcheng wrote: > Return GURL? Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.cc:183: const std::string SyncedNotification::get_icon_url() const { On 2013/01/18 21:56:13, dcheng wrote: > Return GURL? Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.cc:215: // We have an icon after all, get the URL as a string. On 2013/01/18 21:56:13, dcheng wrote: > Some of these comments seem misplaced and/or unnecessary. Similar remarks apply > to some of the other comments in this file. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.cc:230: // If we have subtext data, concat the text lines and return it. On 2013/01/18 21:56:13, dcheng wrote: > concatenate. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:22: explicit SyncedNotification(const syncer::SyncData sync_data); On 2013/01/18 21:56:13, dcheng wrote: > Pass by const reference please. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:28: const std::string get_title() const; On 2013/01/18 21:56:13, dcheng wrote: > GetTitle. Ditto for everything else. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:67: // it into the contained sync entity. On 2013/01/18 21:56:13, dcheng wrote: > I really don't understand what this comment means. There's no other reference to > SyncedNotificationSpecifics in the .h. Maybe just remove this comment or move it > to where it applies in the .cc. Moved comment to the .cc file https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:67: // it into the contained sync entity. On 2013/01/18 23:09:25, Nicolas Zea wrote: > Is the only reason you keep around the sync data the fact that all your get* > methods query it directly? Would it be better to just have local variables that > are parsed directly from the specifics? > > E.g. have a static ParseFromSyncData method that fills the local variables > (title, app id, coalescing key, etc.), as well as a CreateSyncData method to > create a new SyncData from all the local variables if local changes have been > made. My thinking in keeping it around is that I need to compare it (which comes in off the wire) with my local data, so I need two copies. If you think I don't need to do that, I can just copy the data. Also, if you think it is better, I could have two lists of my structure, one for data from the wire, one for local data. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:68: syncer::SyncData sync_data_; On 2013/01/18 21:56:13, dcheng wrote: > Extra space. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:70: bool has_local_changes_; On 2013/01/18 21:56:13, dcheng wrote: > DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification_unittest.cc (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification_unittest.cc:14: using std::string; On 2013/01/18 21:56:13, dcheng wrote: > Optional/preference/nit: I'd prefer not to have this using line for consistency > with most Chrome code (most of Chrome just uses the std:: qualifier). Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification_unittest.cc:23: const unsigned long long FAKE_CREATION_TIME = 42; On 2013/01/18 21:56:13, dcheng wrote: > Constants are named kFoo. Also use int64 instead of long long. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification_unittest.cc:39: On 2013/01/18 21:56:13, dcheng wrote: > Unneeded newline. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification_unittest.cc:57: virtual void TearDown() { On 2013/01/18 21:56:13, dcheng wrote: > OVERRIDE on SetUp() and TearDown()? Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification_unittest.cc:57: virtual void TearDown() { On 2013/01/18 21:56:13, dcheng wrote: > OVERRIDE on SetUp() and TearDown()? Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification_unittest.cc:60: // TODO: convert to scoped_ptr On 2013/01/18 21:56:13, dcheng wrote: > Please fix this before checking in. This will probably cause the Valgrind and/or > Tsan bots to turn red. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification_unittest.cc:72: On 2013/01/18 21:56:13, dcheng wrote: > Ditto on newlines. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification_unittest.cc:74: // on the stack. On 2013/01/18 21:56:13, dcheng wrote: > Unneeded comments. Removed most of the comments, left a few of the ones that I consider more important. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification_unittest.cc:170: EXPECT_TRUE(found_icon_url == expected_icon_url); On 2013/01/18 21:56:13, dcheng wrote: > EXPECT_EQ. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification_unittest.cc:187: EXPECT_TRUE(found_id == expected_id); On 2013/01/18 21:56:13, dcheng wrote: > EXPECT_EQ. Done. https://codereview.chromium.org/11745024/diff/12001/sync/protocol/server_data... File sync/protocol/server_data.proto (right): https://codereview.chromium.org/11745024/diff/12001/sync/protocol/server_data... sync/protocol/server_data.proto:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/01/18 21:56:13, dcheng wrote: > Meta comment that the file could use a better name. "server_data" and > "server_render" aren't very descriptive. The file is named to match its server side counterpart. I can rename it if you feel this is important, but that makes porting changes when they happen just a bit harder. https://codereview.chromium.org/11745024/diff/12001/sync/protocol/server_data... sync/protocol/server_data.proto:38: message SyncedNotificationNotification { On 2013/01/18 23:09:25, Nicolas Zea wrote: > nit: perhaps just SyncedNotification and CoalescedSyncedNotification > (notificationnotification seems odd) We already have a SyncedNotification class (it is the wrapper to provide good C++ semantics for accessing the protobuf.) So, I can only rename this to SyncedNotification if I rename that.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.cc (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:241: string16 title16 = UTF8ToWide(title); On 2013/01/23 01:45:55, Pete Williamson wrote: > On 2013/01/18 21:56:13, dcheng wrote: > > Why not string16 title = UTF8ToUTF16(notification->get_title()). It can > > potentially save a copy and it doesn't seem like the std::string variables are > > really used for anything else. > > I expect the compiler to be smart and not allocate an extra variable. Also, > IMO, it makes the code easier to read, and helps with debugging. I do reuse the > utf8 variables in the DVLOG statement. I personally find it harder to read a block of code when there are variables foo and foo16 and would prefer to see it avoided where not necessary. In this case, debugging seems to be limited to DVLOG()ing and I'm pretty sure string16 overrides operator<< so that it Just Works with LOG() statements. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.h (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:23: public base::NonThreadSafe { On 2013/01/18 21:56:13, dcheng wrote: > Why not use ThreadChecker (which uses composition) instead of inheritance? > > Also, note that you have to actually call CalledOnValidThread() for this to do > anything =) I think you missed the comments in this file. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service_unittest.cc (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:271: scoped_ptr<TestChangeProcessor> sync_processor_; On 2013/01/23 01:45:55, Pete Williamson wrote: > On 2013/01/18 21:56:13, dcheng wrote: > > Nit: SyncChangeProcessor > > I presume you mean to change the type to SyncChangeProcessor from > TestChangeProcessor. Why? The processor function is expeted to return a > TestChangeProcessor, do you think the code is clearer or cleaner if we use > SyncChangeProcessor and convert it when we hand it out? Yes. This removes the PassAs<> cast in PassProcessor(). https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:352: SyncData s1 = CreateSyncData("4", kAppId4, kCoalescingKey4, "44"); On 2013/01/23 01:45:55, Pete Williamson wrote: > On 2013/01/18 21:56:13, dcheng wrote: > > Why is this the only one created differently? > > This is remote data. Today remote data and local data have different forms, and > I need to test that we handle both forms correctly and merge them correctly. Sorry, I guess that was ambiguous. I mean line 352-353, you create the SyncData and bind it to a temporary and then push it into the vector; in the rest, you just push_back() the result of CreateSyncData() directly. Is there a reason for the inconsistency, and if not, can we make it consistent? https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_delegate.h (right): https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_delegate.h:18: explicit ChromeNotifierDelegate(const std::string& id); Formatting. https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service_unittest.cc (right): https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:107: change_map_.erase(change_map_.begin(), change_map_.end()); change_map_.clear(). https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.cc (right): https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.cc:158: return GURL(""); GURL() instead of GURL("") https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:70: Extra newlines. https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification_unittest.cc (right): https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification_unittest.cc:158: // TODO: test with a multi-line body TODO(petewil)
Another round of CR fixes addressing all of DCheng's most recent feedback. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.cc (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:241: string16 title16 = UTF8ToWide(title); On 2013/01/23 18:39:43, dcheng wrote: > On 2013/01/23 01:45:55, Pete Williamson wrote: > > On 2013/01/18 21:56:13, dcheng wrote: > > > Why not string16 title = UTF8ToUTF16(notification->get_title()). It can > > > potentially save a copy and it doesn't seem like the std::string variables > are > > > really used for anything else. > > > > I expect the compiler to be smart and not allocate an extra variable. Also, > > IMO, it makes the code easier to read, and helps with debugging. I do reuse > the > > utf8 variables in the DVLOG statement. > > I personally find it harder to read a block of code when there are variables foo > and foo16 and would prefer to see it avoided where not necessary. In this case, > debugging seems to be limited to DVLOG()ing and I'm pretty sure string16 > overrides operator<< so that it Just Works with LOG() statements. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.h (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:18: // The ChromeNotifierService holds notifications which On 2013/01/18 21:56:13, dcheng wrote: > Optional/preference/nit: I'm just eyeballing this, but try to wrap comments at > 80 columns, not before. 80 columns is very few already, no need to make it > harder! Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:23: public base::NonThreadSafe { On 2013/01/18 21:56:13, dcheng wrote: > Why not use ThreadChecker (which uses composition) instead of inheritance? > > Also, note that you have to actually call CalledOnValidThread() for this to do > anything =) OK, removed the inheritance from base::NonThreadSafe, I'll go back to my plan to use thread checker when I add support to the unit test files. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:47: sync_pb::SyncedNotification& notification); On 2013/01/18 21:56:13, dcheng wrote: > Pass by const reference. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:59: // The caller must not free the it. On 2013/01/18 21:56:13, dcheng wrote: > s/the// Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:63: // Backpointer to the owning profile, do not free at shutdown. On 2013/01/18 21:56:13, dcheng wrote: > Back pointer. I also feel that "do not free" is redundant, as we have a raw > pointer here anyway. Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service_unittest.cc (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:271: scoped_ptr<TestChangeProcessor> sync_processor_; On 2013/01/23 18:39:43, dcheng wrote: > On 2013/01/23 01:45:55, Pete Williamson wrote: > > On 2013/01/18 21:56:13, dcheng wrote: > > > Nit: SyncChangeProcessor > > > > I presume you mean to change the type to SyncChangeProcessor from > > TestChangeProcessor. Why? The processor function is expeted to return a > > TestChangeProcessor, do you think the code is clearer or cleaner if we use > > SyncChangeProcessor and convert it when we hand it out? > > Yes. This removes the PassAs<> cast in PassProcessor(). Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:272: scoped_ptr<SyncChangeProcessorDelegate> sync_processor_delegate_; On 2013/01/18 21:56:13, dcheng wrote: > Nit: SyncChangeProcessor Done. https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:352: SyncData s1 = CreateSyncData("4", kAppId4, kCoalescingKey4, "44"); On 2013/01/23 18:39:43, dcheng wrote: > On 2013/01/23 01:45:55, Pete Williamson wrote: > > On 2013/01/18 21:56:13, dcheng wrote: > > > Why is this the only one created differently? > > > > This is remote data. Today remote data and local data have different forms, > and > > I need to test that we handle both forms correctly and merge them correctly. > > Sorry, I guess that was ambiguous. I mean line 352-353, you create the SyncData > and bind it to a temporary and then push it into the vector; in the rest, you > just push_back() the result of CreateSyncData() directly. Is there a reason for > the inconsistency, and if not, can we make it consistent? Done. https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_delegate.h (right): https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_delegate.h:18: explicit ChromeNotifierDelegate(const std::string& id); On 2013/01/23 18:39:43, dcheng wrote: > Formatting. Done. https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service_unittest.cc (right): https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:107: change_map_.erase(change_map_.begin(), change_map_.end()); On 2013/01/23 18:39:43, dcheng wrote: > change_map_.clear(). Done. https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.cc (right): https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.cc:158: return GURL(""); On 2013/01/23 18:39:43, dcheng wrote: > GURL() instead of GURL("") Done. https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:70: On 2013/01/23 18:39:43, dcheng wrote: > Extra newlines. Done. https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification_unittest.cc (right): https://codereview.chromium.org/11745024/diff/25001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification_unittest.cc:158: // TODO: test with a multi-line body On 2013/01/23 18:39:43, dcheng wrote: > TODO(petewil) Done.
https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:67: // it into the contained sync entity. On 2013/01/23 01:45:55, Pete Williamson wrote: > On 2013/01/18 23:09:25, Nicolas Zea wrote: > > Is the only reason you keep around the sync data the fact that all your get* > > methods query it directly? Would it be better to just have local variables > that > > are parsed directly from the specifics? > > > > E.g. have a static ParseFromSyncData method that fills the local variables > > (title, app id, coalescing key, etc.), as well as a CreateSyncData method to > > create a new SyncData from all the local variables if local changes have been > > made. > > My thinking in keeping it around is that I need to compare it (which comes in > off the wire) with my local data, so I need two copies. If you think I don't > need to do that, I can just copy the data. Also, if you think it is better, I > could have two lists of my structure, one for data from the wire, one for local > data. I think it would be better to compare by having two separate SyncedNotification objects, which then have comparison functions (::Equals(const SyncedNotification& other)). Having an implicit copy of the data in here seems like a roundabout way of just having two independent objects. https://codereview.chromium.org/11745024/diff/12001/sync/protocol/server_data... File sync/protocol/server_data.proto (right): https://codereview.chromium.org/11745024/diff/12001/sync/protocol/server_data... sync/protocol/server_data.proto:38: message SyncedNotificationNotification { On 2013/01/23 01:45:55, Pete Williamson wrote: > On 2013/01/18 23:09:25, Nicolas Zea wrote: > > nit: perhaps just SyncedNotification and CoalescedSyncedNotification > > (notificationnotification seems odd) > > We already have a SyncedNotification class (it is the wrapper to provide good > C++ semantics for accessing the protobuf.) So, I can only rename this to > SyncedNotification if I rename that. They're within different namespaces, so the names won't conflict. https://codereview.chromium.org/11745024/diff/29003/sync/protocol/server_data... File sync/protocol/server_data.proto (right): https://codereview.chromium.org/11745024/diff/29003/sync/protocol/server_data... sync/protocol/server_data.proto:20: // IMPORTANT: Everything in this message may be sent to the client in clear text This comment is a bit odd to me (and all the IMPORTANT warnings below). I'm not sure what the concern about sending in "clear text" is. All transmissions are done via HTTPS. Certainly credit cards and such should not be synced (but for entirely different reasons). And all field names should be safe to have in an open source repository, so no internal/confidential products should be referenced. But, if there is a field reserved in the chromium protobuf, the server protobuf can use a different name if it prefers (the field number is all that matters). https://codereview.chromium.org/11745024/diff/29003/sync/protocol/server_data... sync/protocol/server_data.proto:39: // The unique identifier to identify the notification. s/to identify/of https://codereview.chromium.org/11745024/diff/29003/sync/protocol/server_data... sync/protocol/server_data.proto:79: // created (in Java time milliseconds). presumably this is milliseconds since unix epoch? I'd prefer specifying that. https://codereview.chromium.org/11745024/diff/29003/sync/protocol/server_rend... File sync/protocol/server_render.proto (right): https://codereview.chromium.org/11745024/diff/29003/sync/protocol/server_rend... sync/protocol/server_render.proto:39: optional TitleAndImageData title_and_image_data = 4; why is title encoded in both titleandsubtext and titleandimage? Can we pull title out? https://codereview.chromium.org/11745024/diff/29003/sync/protocol/server_rend... sync/protocol/server_render.proto:60: // If post_data is populated, this indicates that action_url should be newline above comment
CR changes per Zea - Changed the protobuf object names to be nicer, changed SyncedNotification to keep a copy of its data, more minor protobuf fixes https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/12001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:67: // it into the contained sync entity. On 2013/01/25 00:18:45, Nicolas Zea wrote: > On 2013/01/23 01:45:55, Pete Williamson wrote: > > On 2013/01/18 23:09:25, Nicolas Zea wrote: > > > Is the only reason you keep around the sync data the fact that all your get* > > > methods query it directly? Would it be better to just have local variables > > that > > > are parsed directly from the specifics? > > > > > > E.g. have a static ParseFromSyncData method that fills the local variables > > > (title, app id, coalescing key, etc.), as well as a CreateSyncData method to > > > create a new SyncData from all the local variables if local changes have > been > > > made. > > > > My thinking in keeping it around is that I need to compare it (which comes in > > off the wire) with my local data, so I need two copies. If you think I don't > > need to do that, I can just copy the data. Also, if you think it is better, I > > could have two lists of my structure, one for data from the wire, one for > local > > data. > > I think it would be better to compare by having two separate SyncedNotification > objects, which then have comparison functions (::Equals(const > SyncedNotification& other)). Having an implicit copy of the data in here seems > like a roundabout way of just having two independent objects. As it turns out, there is another reason to keep the sync_data around, we need to hand it back out in response to the GetAllSync data method, which requires us to hand it out as sync_data instead of as a SyncedNotification object. I still changed the class to use local variables and initialize them at startup as you suggest, so in case you can tell me that I don't need to support GetAllSyncData, I can remove the sync_data_ object from the SyncedNotification. https://codereview.chromium.org/11745024/diff/12001/sync/protocol/server_data... File sync/protocol/server_data.proto (right): https://codereview.chromium.org/11745024/diff/12001/sync/protocol/server_data... sync/protocol/server_data.proto:38: message SyncedNotificationNotification { On 2013/01/25 00:18:45, Nicolas Zea wrote: > On 2013/01/23 01:45:55, Pete Williamson wrote: > > On 2013/01/18 23:09:25, Nicolas Zea wrote: > > > nit: perhaps just SyncedNotification and CoalescedSyncedNotification > > > (notificationnotification seems odd) > > > > We already have a SyncedNotification class (it is the wrapper to provide good > > C++ semantics for accessing the protobuf.) So, I can only rename this to > > SyncedNotification if I rename that. > > They're within different namespaces, so the names won't conflict. Done. https://codereview.chromium.org/11745024/diff/29003/sync/protocol/server_data... File sync/protocol/server_data.proto (right): https://codereview.chromium.org/11745024/diff/29003/sync/protocol/server_data... sync/protocol/server_data.proto:20: // IMPORTANT: Everything in this message may be sent to the client in clear text On 2013/01/25 00:18:46, Nicolas Zea wrote: > This comment is a bit odd to me (and all the IMPORTANT warnings below). I'm not > sure what the concern about sending in "clear text" is. All transmissions are > done via HTTPS. Certainly credit cards and such should not be synced (but for > entirely different reasons). > > And all field names should be safe to have in an open source repository, so no > internal/confidential products should be referenced. But, if there is a field > reserved in the chromium protobuf, the server protobuf can use a different name > if it prefers (the field number is all that matters). Once again, I am copying the protobuf from the sync side. Ideally, we will make them as close as possible to ease porting changes back and forth between the chrome and server code bases. I think this wording is boilerplate from the server side code base. I will check with the server folks to see if we can remove it. https://codereview.chromium.org/11745024/diff/29003/sync/protocol/server_data... sync/protocol/server_data.proto:39: // The unique identifier to identify the notification. On 2013/01/25 00:18:46, Nicolas Zea wrote: > s/to identify/of Done. https://codereview.chromium.org/11745024/diff/29003/sync/protocol/server_data... sync/protocol/server_data.proto:79: // created (in Java time milliseconds). On 2013/01/25 00:18:46, Nicolas Zea wrote: > presumably this is milliseconds since unix epoch? I'd prefer specifying that. Done. https://codereview.chromium.org/11745024/diff/29003/sync/protocol/server_rend... File sync/protocol/server_render.proto (right): https://codereview.chromium.org/11745024/diff/29003/sync/protocol/server_rend... sync/protocol/server_render.proto:39: optional TitleAndImageData title_and_image_data = 4; On 2013/01/25 00:18:46, Nicolas Zea wrote: > why is title encoded in both titleandsubtext and titleandimage? Can we pull > title out? Started a thread about this with the server team. https://codereview.chromium.org/11745024/diff/29003/sync/protocol/server_rend... sync/protocol/server_render.proto:60: // If post_data is populated, this indicates that action_url should be On 2013/01/25 00:18:46, Nicolas Zea wrote: > newline above comment Done.
I still think that in Chrome, we should rename "server_data.proto" to "notification_data.proto" and "server_render.proto" to "notification_render_info.proto". I realize these names are inherited but there's no reason we can't have more meaningful file names in the Chrome code. Please fix the lint errors as well. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_delegate.cc (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_delegate.cc:11: ChromeNotifierDelegate:: ~ChromeNotifierDelegate() {} Remove extra space https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.cc (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:197: return notification; Nit: return a scoped_ptr<> https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:227: // Show it to the user (and complete taking ownership of the pointer). This comment seems wrong (there is no ownership taking here). I would just alias the pointer into a local and do notification.release() on line 225. Then use the aliased pointer in line 228. Otherwise, it looks like you're passing ownership to Show()--but you're really not. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:236: string16 title16 = UTF8ToUTF16(notification->title()); You don't need the 16 suffix anymore. It might make things a bit shorter. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.h (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:47: static const syncer::SyncData CreateSyncDataFromNotification( const here has no effect. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:55: void Add(scoped_ptr<notifier::SyncedNotification> notification); This should be made private. For testing purposes, you can expose a public AddForTest() version that forwards to Add(). https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:57: void Show(notifier::SyncedNotification* notification); Ditto. This should be private as well, unless there's a reason non-unit testing code will be calling this? https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service_factory.h (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_factory.h:34: virtual bool ServiceIsCreatedWithProfile() const OVERRIDE; I thought we were removing these 3 methods and just using the default. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service_unittest.cc (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:69: virtual void Add(const Notification& notification, Profile* profile) {} Do we need OVERRIDE annotations on virtual methods? I think this might break builds on some platforms. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.cc (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.cc:30: image_url_ = GetImageUrl(sync_data); Use initializer format for these? Then you can const-ify the members. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.cc:133: DCHECK(false); NOTREACHED() is more canonical. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:35: const std::string& body() const { return body_; } Formatting. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:67: // our local variables. Probably should be called ExtractFoo or ParseFoo then (extract is probably more specific since there's not really any 'parsing' per se).
This should address all outstanding issues brought up by DCheng and Zea. It includes a rename of the protobuf files. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_delegate.cc (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_delegate.cc:11: ChromeNotifierDelegate:: ~ChromeNotifierDelegate() {} On 2013/01/25 22:23:48, dcheng wrote: > Remove extra space Done. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.cc (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:197: return notification; On 2013/01/25 22:23:48, dcheng wrote: > Nit: return a scoped_ptr<> Done. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:227: // Show it to the user (and complete taking ownership of the pointer). On 2013/01/25 22:23:48, dcheng wrote: > This comment seems wrong (there is no ownership taking here). I would just alias > the pointer into a local and do notification.release() on line 225. Then use the > aliased pointer in line 228. Otherwise, it looks like you're passing ownership > to Show()--but you're really not. Done. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:236: string16 title16 = UTF8ToUTF16(notification->title()); On 2013/01/25 22:23:48, dcheng wrote: > You don't need the 16 suffix anymore. It might make things a bit shorter. Done. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.h (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:47: static const syncer::SyncData CreateSyncDataFromNotification( On 2013/01/25 22:23:48, dcheng wrote: > const here has no effect. Done. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:55: void Add(scoped_ptr<notifier::SyncedNotification> notification); On 2013/01/25 22:23:48, dcheng wrote: > This should be made private. For testing purposes, you can expose a public > AddForTest() version that forwards to Add(). Done. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:57: void Show(notifier::SyncedNotification* notification); On 2013/01/25 22:23:48, dcheng wrote: > Ditto. This should be private as well, unless there's a reason non-unit testing > code will be calling this? Done. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service_factory.h (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_factory.h:34: virtual bool ServiceIsCreatedWithProfile() const OVERRIDE; On 2013/01/25 22:23:48, dcheng wrote: > I thought we were removing these 3 methods and just using the default. Oops, done now, I forgot to revisit them once I had an answer to my email thread. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service_unittest.cc (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:69: virtual void Add(const Notification& notification, Profile* profile) {} On 2013/01/25 22:23:48, dcheng wrote: > Do we need OVERRIDE annotations on virtual methods? I think this might break > builds on some platforms. as I understand the OVERRIDE keyword, it merely gives the complier a bit of information to do an extra check to warn us if we get a function that has a signature similar to an overridden function, so nothing should break, but I've added the keyword anyhow. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.cc (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.cc:30: image_url_ = GetImageUrl(sync_data); On 2013/01/25 22:23:48, dcheng wrote: > Use initializer format for these? Then you can const-ify the members. Done. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.cc:133: DCHECK(false); On 2013/01/25 22:23:48, dcheng wrote: > NOTREACHED() is more canonical. Done. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:35: const std::string& body() const { return body_; } On 2013/01/25 22:23:48, dcheng wrote: > Formatting. Done. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:67: // our local variables. On 2013/01/25 22:23:48, dcheng wrote: > Probably should be called ExtractFoo or ParseFoo then (extract is probably more > specific since there's not really any 'parsing' per se). Done.
Please address the Rietveld lint warnings. You can see the results by clicking on the links in the column under "Lint" on this page. https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service_unittest.cc (right): https://codereview.chromium.org/11745024/diff/38001/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service_unittest.cc:69: virtual void Add(const Notification& notification, Profile* profile) {} On 2013/01/26 02:17:09, Pete Williamson wrote: > On 2013/01/25 22:23:48, dcheng wrote: > > Do we need OVERRIDE annotations on virtual methods? I think this might break > > builds on some platforms. > > as I understand the OVERRIDE keyword, it merely gives the complier a bit of > information to do an extra check to warn us if we get a function that has a > signature similar to an overridden function, so nothing should break, but I've > added the keyword anyhow. Some platforms enforce that the OVERRIDE keyword is present when we implement a virtual method (in particular, the clang plugin does this).
OK, this patch addresses the lint issues (except for the newlines at the end of files).
LGTM with the comments addressed. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.cc (right): https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:72: DCHECK_GT(id.length(), static_cast<size_t>(0)); FYI: 0U probably works. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:106: (sync_processor.get())->ProcessSyncChanges(FROM_HERE, new_changes)); Nit: remove extra parens. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:113: DCHECK_EQ(syncer::SYNCED_NOTIFICATIONS, type); Would be nice to have a TODO or a comment stating why we don't implement this (maybe this is obvious and doesn't need a comment) https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.h (right): https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:18: class NotificationUIManager; Minor nit: Most people sort these alphabetically. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:72: NotificationUIManager* notification_manager_; Nit: make these T* const so they don't get reset after construction. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.cc (right): https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.cc:251: if ( ii < subtext_lines - 1) Nit: extra space.
Answered DCheng's concerns. Since there were only a few straightforward changes, I'll wait for Zea and include the changes with the next patch that I make for him. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.cc (right): https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:72: DCHECK_GT(id.length(), static_cast<size_t>(0)); On 2013/01/28 23:35:09, dcheng wrote: > FYI: 0U probably works. Done. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:106: (sync_processor.get())->ProcessSyncChanges(FROM_HERE, new_changes)); On 2013/01/28 23:35:09, dcheng wrote: > Nit: remove extra parens. Done. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.cc:113: DCHECK_EQ(syncer::SYNCED_NOTIFICATIONS, type); On 2013/01/28 23:35:09, dcheng wrote: > Would be nice to have a TODO or a comment stating why we don't implement this > (maybe this is obvious and doesn't need a comment) Done. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.h (right): https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:18: class NotificationUIManager; On 2013/01/28 23:35:09, dcheng wrote: > Minor nit: Most people sort these alphabetically. Done. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:72: NotificationUIManager* notification_manager_; On 2013/01/28 23:35:09, dcheng wrote: > Nit: make these T* const so they don't get reset after construction. Not done, we use this to call the non-const NotificationUIManager::Add function which takes a non-const profile
https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.h (right): https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:72: NotificationUIManager* notification_manager_; On 2013/01/29 00:11:43, Pete Williamson wrote: > On 2013/01/28 23:35:09, dcheng wrote: > > Nit: make these T* const so they don't get reset after construction. > > Not done, we use this to call the non-const NotificationUIManager::Add function > which takes a non-const profile T* const is not the same thing as const T*. In particular, you can pass a T* const to a function that takes a T*.
https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... File chrome/browser/notifier/chrome_notifier_service.h (right): https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/c... chrome/browser/notifier/chrome_notifier_service.h:72: NotificationUIManager* notification_manager_; On 2013/01/29 00:28:34, dcheng wrote: > On 2013/01/29 00:11:43, Pete Williamson wrote: > > On 2013/01/28 23:35:09, dcheng wrote: > > > Nit: make these T* const so they don't get reset after construction. > > > > Not done, we use this to call the non-const NotificationUIManager::Add > function > > which takes a non-const profile > > T* const is not the same thing as const T*. In particular, you can pass a T* > const to a function that takes a T*. Oops, I misread your comment thinking you wanted me to have a const T*. Sure, a T* const will compile just fine. Done.
https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:17: #include "sync/protocol/synced_notification_specifics.pb.h" I think you can forward declare any protobuffers now (once you pass by ref below). You can probably then get rid of both these sync includes. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:64: // helper function to mark a notification as read or dismissed. helper -> Helper https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:66: sync_pb::CoalescedSyncedNotification_ReadState readState); pass as const ref https://codereview.chromium.org/11745024/diff/41006/chrome/browser/sync/profi... File chrome/browser/sync/profile_sync_components_factory_impl.cc (right): https://codereview.chromium.org/11745024/diff/41006/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_components_factory_impl.cc:229: // Synced Notifications sync is enabled by default. Register unless I don't think this should be enabled by default. The datatype is still in development, so it should be disabled by default, at the very least until the sync server supports it on both dev/stable builds. https://codereview.chromium.org/11745024/diff/41006/sync/protocol/synced_noti... File sync/protocol/synced_notification_data.proto (right): https://codereview.chromium.org/11745024/diff/41006/sync/protocol/synced_noti... sync/protocol/synced_notification_data.proto:64: // created (in milliseconds since the epoch). mention that this is linux epoch (chrome's base::Time internal representation is windows epoch). https://codereview.chromium.org/11745024/diff/41006/sync/protocol/synced_noti... File sync/protocol/synced_notification_specifics.proto (right): https://codereview.chromium.org/11745024/diff/41006/sync/protocol/synced_noti... sync/protocol/synced_notification_specifics.proto:18: import "synced_notification_data.proto"; put these in abc order
https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:66: sync_pb::CoalescedSyncedNotification_ReadState readState); Btw... I missed this, but naming (should be read_state).
This should address Zea's comments, and the remaining "lgtm" nits that DCheng had. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/s... File chrome/browser/notifier/synced_notification.h (right): https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:17: #include "sync/protocol/synced_notification_specifics.pb.h" On 2013/01/29 01:17:02, Nicolas Zea wrote: > I think you can forward declare any protobuffers now (once you pass by ref > below). You can probably then get rid of both these sync includes. Done. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:64: // helper function to mark a notification as read or dismissed. On 2013/01/29 01:17:02, Nicolas Zea wrote: > helper -> Helper Done. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:66: sync_pb::CoalescedSyncedNotification_ReadState readState); On 2013/01/29 01:33:48, dcheng wrote: > Btw... I missed this, but naming (should be read_state). Done. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/notifier/s... chrome/browser/notifier/synced_notification.h:66: sync_pb::CoalescedSyncedNotification_ReadState readState); On 2013/01/29 01:17:02, Nicolas Zea wrote: > pass as const ref Done. https://codereview.chromium.org/11745024/diff/41006/chrome/browser/sync/profi... File chrome/browser/sync/profile_sync_components_factory_impl.cc (right): https://codereview.chromium.org/11745024/diff/41006/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_components_factory_impl.cc:229: // Synced Notifications sync is enabled by default. Register unless On 2013/01/29 01:17:02, Nicolas Zea wrote: > I don't think this should be enabled by default. The datatype is still in > development, so it should be disabled by default, at the very least until the > sync server supports it on both dev/stable builds. Done. https://codereview.chromium.org/11745024/diff/41006/sync/protocol/synced_noti... File sync/protocol/synced_notification_data.proto (right): https://codereview.chromium.org/11745024/diff/41006/sync/protocol/synced_noti... sync/protocol/synced_notification_data.proto:64: // created (in milliseconds since the epoch). On 2013/01/29 01:17:02, Nicolas Zea wrote: > mention that this is linux epoch (chrome's base::Time internal representation is > windows epoch). Done. https://codereview.chromium.org/11745024/diff/41006/sync/protocol/synced_noti... File sync/protocol/synced_notification_specifics.proto (right): https://codereview.chromium.org/11745024/diff/41006/sync/protocol/synced_noti... sync/protocol/synced_notification_specifics.proto:18: import "synced_notification_data.proto"; On 2013/01/29 01:17:02, Nicolas Zea wrote: > put these in abc order Done.
https://codereview.chromium.org/11745024/diff/55003/chrome/browser/sync/profi... File chrome/browser/sync/profile_sync_components_factory_impl.cc (right): https://codereview.chromium.org/11745024/diff/55003/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_components_factory_impl.cc:229: // Synced Notifications sync is enabled by default. enabled -> disabled https://codereview.chromium.org/11745024/diff/55003/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_components_factory_impl.cc:231: if (!command_line_->HasSwitch(switches::kEnableSyncSyncedNotifications)) { presumably this should be if (has switch), not if (!has switch)?
This addresses Zea@'s comments. https://codereview.chromium.org/11745024/diff/55003/chrome/browser/sync/profi... File chrome/browser/sync/profile_sync_components_factory_impl.cc (right): https://codereview.chromium.org/11745024/diff/55003/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_components_factory_impl.cc:229: // Synced Notifications sync is enabled by default. On 2013/01/29 21:43:34, Nicolas Zea wrote: > enabled -> disabled Done. https://codereview.chromium.org/11745024/diff/55003/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_components_factory_impl.cc:231: if (!command_line_->HasSwitch(switches::kEnableSyncSyncedNotifications)) { On 2013/01/29 21:43:34, Nicolas Zea wrote: > presumably this should be if (has switch), not if (!has switch)? Done.
sync LGTM
My only real question: Why is this in chrome/browser/notifier and not in the existing chrome/browser/notifications? This is sync for notifications, right? https://codereview.chromium.org/11745024/diff/55005/chrome/browser/sync/profi... File chrome/browser/sync/profile_sync_components_factory_impl.cc (right): https://codereview.chromium.org/11745024/diff/55005/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_components_factory_impl.cc:230: // TODO(petewil): Switch to enabled by default once datatype support is done. add link to tracking bug
Thakis - thanks for the review! The focus of the "notifications" directory is somewhat different - that is mechanism for showing the UI and implements the NotificationUIManager interface, which my code calls as a client. The focus of my code is to get notifications from the sync service and forward them to the NotificationUIManager. The proper name of my component is the Chrome Notifier. I'll be happy to rename the directory if you have a name that fits better so there is less conflict with "notifications". https://codereview.chromium.org/11745024/diff/55005/chrome/browser/sync/profi... File chrome/browser/sync/profile_sync_components_factory_impl.cc (right): https://codereview.chromium.org/11745024/diff/55005/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_components_factory_impl.cc:230: // TODO(petewil): Switch to enabled by default once datatype support is done. On 2013/01/31 22:43:14, Nico wrote: > add link to tracking bug This is work on the server sync side - is it OK to put those bug numbers into chrome?
As far as I understand, this code is for syncing notifications which are displayed by the code in chrome/browser/notifications. Inventing a new name for this is I think confusing. Logical places are c/b/sync/(glue/)?notifications or c/b/notifications/sync. Neither stops your code from being its own component as far as I understand. zea: Which way round is this usually done? I think c/b/notifications/sync might be backwards from how we usually do dependencies? (since the UI is a client of this new code) https://codereview.chromium.org/11745024/diff/55005/chrome/browser/sync/profi... File chrome/browser/sync/profile_sync_components_factory_impl.cc (right): https://codereview.chromium.org/11745024/diff/55005/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_components_factory_impl.cc:230: // TODO(petewil): Switch to enabled by default once datatype support is done. On 2013/01/31 23:02:40, Pete Williamson wrote: > On 2013/01/31 22:43:14, Nico wrote: > > add link to tracking bug > > This is work on the server sync side - is it OK to put those bug numbers into > chrome? Ah. No, just add "server side" to the comment somewhere.
On 2013/01/31 23:28:20, Nico wrote: > As far as I understand, this code is for syncing notifications which are > displayed by the code in chrome/browser/notifications. Inventing a new name for > this is I think confusing. > > Logical places are c/b/sync/(glue/)?notifications or c/b/notifications/sync. > Neither stops your code from being its own component as far as I understand. > > zea: Which way round is this usually done? I think c/b/notifications/sync might > be backwards from how we usually do dependencies? (since the UI is a client of > this new code) > I don't think this should be part of the sync/ directory. Really there should only be one ProfileKeyedService in there, ProfileSyncService. We've been working towards moving as much as possible of the datatype specific logic in glue/ into the directories of the native services, so I'd rather follow that approach. In that vein, c/b/notifications/notifier makes sense, or maybe c/b/notifications/sync_notifier. WDYT? > https://codereview.chromium.org/11745024/diff/55005/chrome/browser/sync/profi... > File chrome/browser/sync/profile_sync_components_factory_impl.cc (right): > > https://codereview.chromium.org/11745024/diff/55005/chrome/browser/sync/profi... > chrome/browser/sync/profile_sync_components_factory_impl.cc:230: // > TODO(petewil): Switch to enabled by default once datatype support is done. > On 2013/01/31 23:02:40, Pete Williamson wrote: > > On 2013/01/31 22:43:14, Nico wrote: > > > add link to tracking bug > > > > This is work on the server sync side - is it OK to put those bug numbers into > > chrome? > > Ah. No, just add "server side" to the comment somewhere.
c/b/notifictions/sync_notifier is OK by me if Nico approves. I originally picked c/b/notifier since similar kind of code for history was in c/b/history. I think that c/b/notifications/notifier is insufficiently specific and might be confusing. I'd also be happy to go with c/b/notifications/sync_notifier, but I would prefer to avoid c/b/synced_notifications On Thu, Jan 31, 2013 at 4:10 PM, <zea@chromium.org> wrote: > On 2013/01/31 23:28:20, Nico wrote: > >> As far as I understand, this code is for syncing notifications which are >> displayed by the code in chrome/browser/notifications. Inventing a new >> name >> > for > >> this is I think confusing. >> > > Logical places are c/b/sync/(glue/)?notifications or >> c/b/notifications/sync. >> Neither stops your code from being its own component as far as I >> understand. >> > > zea: Which way round is this usually done? I think c/b/notifications/sync >> > might > >> be backwards from how we usually do dependencies? (since the UI is a >> client of >> this new code) >> > > > I don't think this should be part of the sync/ directory. Really there > should > only be one ProfileKeyedService in there, ProfileSyncService. We've been > working > towards moving as much as possible of the datatype specific logic in glue/ > into > the directories of the native services, so I'd rather follow that approach. > > In that vein, c/b/notifications/notifier makes sense, or maybe > c/b/notifications/sync_**notifier. WDYT? > > > > https://codereview.chromium.**org/11745024/diff/55005/** > chrome/browser/sync/profile_**sync_components_factory_impl.**cc<https://codereview.chromium.org/11745024/diff/55005/chrome/browser/sync/profile_sync_components_factory_impl.cc> > >> File chrome/browser/sync/profile_**sync_components_factory_impl.**cc >> (right): >> > > > https://codereview.chromium.**org/11745024/diff/55005/** > chrome/browser/sync/profile_**sync_components_factory_impl.**cc#newcode230<https://codereview.chromium.org/11745024/diff/55005/chrome/browser/sync/profile_sync_components_factory_impl.cc#newcode230> > >> chrome/browser/sync/profile_**sync_components_factory_impl.**cc:230: // >> TODO(petewil): Switch to enabled by default once datatype support is done. >> On 2013/01/31 23:02:40, Pete Williamson wrote: >> > On 2013/01/31 22:43:14, Nico wrote: >> > > add link to tracking bug >> > >> > This is work on the server sync side - is it OK to put those bug numbers >> > into > >> > chrome? >> > > Ah. No, just add "server side" to the comment somewhere. >> > > > > https://codereview.chromium.**org/11745024/<https://codereview.chromium.org/1... >
-Akalin@, +kevinliu On Thu, Jan 31, 2013 at 4:43 PM, Pete Williamson <petewil@google.com> wrote: > c/b/notifictions/sync_notifier is OK by me if Nico approves. I originally > picked c/b/notifier since similar kind of code for history was in > c/b/history. I think that c/b/notifications/notifier is insufficiently > specific and might be confusing. I'd also be happy to go with > c/b/notifications/sync_notifier, but I would prefer to avoid > c/b/synced_notifications > > > On Thu, Jan 31, 2013 at 4:10 PM, <zea@chromium.org> wrote: > >> On 2013/01/31 23:28:20, Nico wrote: >> >>> As far as I understand, this code is for syncing notifications which are >>> displayed by the code in chrome/browser/notifications. Inventing a new >>> name >>> >> for >> >>> this is I think confusing. >>> >> >> Logical places are c/b/sync/(glue/)?notifications or >>> c/b/notifications/sync. >>> Neither stops your code from being its own component as far as I >>> understand. >>> >> >> zea: Which way round is this usually done? I think c/b/notifications/sync >>> >> might >> >>> be backwards from how we usually do dependencies? (since the UI is a >>> client of >>> this new code) >>> >> >> >> I don't think this should be part of the sync/ directory. Really there >> should >> only be one ProfileKeyedService in there, ProfileSyncService. We've been >> working >> towards moving as much as possible of the datatype specific logic in >> glue/ into >> the directories of the native services, so I'd rather follow that >> approach. >> >> In that vein, c/b/notifications/notifier makes sense, or maybe >> c/b/notifications/sync_**notifier. WDYT? >> >> >> >> https://codereview.chromium.**org/11745024/diff/55005/** >> chrome/browser/sync/profile_**sync_components_factory_impl.**cc<https://codereview.chromium.org/11745024/diff/55005/chrome/browser/sync/profile_sync_components_factory_impl.cc> >> >>> File chrome/browser/sync/profile_**sync_components_factory_impl.**cc >>> (right): >>> >> >> >> https://codereview.chromium.**org/11745024/diff/55005/** >> chrome/browser/sync/profile_**sync_components_factory_impl.** >> cc#newcode230<https://codereview.chromium.org/11745024/diff/55005/chrome/browser/sync/profile_sync_components_factory_impl.cc#newcode230> >> >>> chrome/browser/sync/profile_**sync_components_factory_impl.**cc:230: // >>> TODO(petewil): Switch to enabled by default once datatype support is >>> done. >>> On 2013/01/31 23:02:40, Pete Williamson wrote: >>> > On 2013/01/31 22:43:14, Nico wrote: >>> > > add link to tracking bug >>> > >>> > This is work on the server sync side - is it OK to put those bug >>> numbers >>> >> into >> >>> > chrome? >>> >> >> Ah. No, just add "server side" to the comment somewhere. >>> >> >> >> >> https://codereview.chromium.**org/11745024/<https://codereview.chromium.org/1... >> > >
On 2013/02/01 00:10:14, Nicolas Zea wrote: > On 2013/01/31 23:28:20, Nico wrote: > > As far as I understand, this code is for syncing notifications which are > > displayed by the code in chrome/browser/notifications. Inventing a new name > for > > this is I think confusing. > > > > Logical places are c/b/sync/(glue/)?notifications or c/b/notifications/sync. > > Neither stops your code from being its own component as far as I understand. > > > > zea: Which way round is this usually done? I think c/b/notifications/sync > might > > be backwards from how we usually do dependencies? (since the UI is a client of > > this new code) > > > > I don't think this should be part of the sync/ directory. Really there should > only be one ProfileKeyedService in there, ProfileSyncService. We've been working > towards moving as much as possible of the datatype specific logic in glue/ into > the directories of the native services, so I'd rather follow that approach. > > In that vein, c/b/notifications/notifier makes sense, or maybe > c/b/notifications/sync_notifier. WDYT? +ben since I always confuse which was this is meant to be. I believe below chrome/, we allow subdirectories depend on their parent directories but not the other way round. It looks like this code does depend on c/b/notifications, putting it below c/b/notifications makes sense to me. This code effectively just implements syncing for notifications, right? What's wrong with c/b/notifications/sync?
On Thu, Jan 31, 2013 at 4:46 PM, <thakis@chromium.org> wrote: > On 2013/02/01 00:10:14, Nicolas Zea wrote: >> >> On 2013/01/31 23:28:20, Nico wrote: >> > As far as I understand, this code is for syncing notifications which are >> > displayed by the code in chrome/browser/notifications. Inventing a new >> > name >> for >> > this is I think confusing. >> > >> > Logical places are c/b/sync/(glue/)?notifications or >> > c/b/notifications/sync. >> > Neither stops your code from being its own component as far as I >> > understand. >> > >> > zea: Which way round is this usually done? I think >> > c/b/notifications/sync >> might >> > be backwards from how we usually do dependencies? (since the UI is a >> > client > > of >> >> > this new code) >> > > > >> I don't think this should be part of the sync/ directory. Really there >> should >> only be one ProfileKeyedService in there, ProfileSyncService. We've been > > working >> >> towards moving as much as possible of the datatype specific logic in glue/ > > into >> >> the directories of the native services, so I'd rather follow that >> approach. > > >> In that vein, c/b/notifications/notifier makes sense, or maybe >> c/b/notifications/sync_notifier. WDYT? > > > +ben since I always confuse which was this is meant to be. *which way > > I believe below chrome/, we allow subdirectories depend on their parent > directories but not the other way round. It looks like this code does depend > on > c/b/notifications, putting it below c/b/notifications makes sense to me. > > This code effectively just implements syncing for notifications, right? > What's > wrong with c/b/notifications/sync? > > https://codereview.chromium.org/11745024/
On 2013/02/01 00:47:09, Nico wrote: > On Thu, Jan 31, 2013 at 4:46 PM, <mailto:thakis@chromium.org> wrote: > > On 2013/02/01 00:10:14, Nicolas Zea wrote: > >> > >> On 2013/01/31 23:28:20, Nico wrote: > >> > As far as I understand, this code is for syncing notifications which are > >> > displayed by the code in chrome/browser/notifications. Inventing a new > >> > name > >> for > >> > this is I think confusing. > >> > > >> > Logical places are c/b/sync/(glue/)?notifications or > >> > c/b/notifications/sync. > >> > Neither stops your code from being its own component as far as I > >> > understand. > >> > > >> > zea: Which way round is this usually done? I think > >> > c/b/notifications/sync > >> might > >> > be backwards from how we usually do dependencies? (since the UI is a > >> > client > > > > of > >> > >> > this new code) > >> > > > > > > >> I don't think this should be part of the sync/ directory. Really there > >> should > >> only be one ProfileKeyedService in there, ProfileSyncService. We've been > > > > working > >> > >> towards moving as much as possible of the datatype specific logic in glue/ > > > > into > >> > >> the directories of the native services, so I'd rather follow that > >> approach. > > > > > >> In that vein, c/b/notifications/notifier makes sense, or maybe > >> c/b/notifications/sync_notifier. WDYT? > > > > > > +ben since I always confuse which was this is meant to be. > > *which way > > > > > I believe below chrome/, we allow subdirectories depend on their parent > > directories but not the other way round. It looks like this code does depend > > on > > c/b/notifications, putting it below c/b/notifications makes sense to me. > > > > This code effectively just implements syncing for notifications, right? > > What's > > wrong with c/b/notifications/sync? > > > > https://codereview.chromium.org/11745024/ That's fine with me as well. I just thought, since this "is a" notifier, that sync_notifier might make sense.
I'll go ahead and move this to c/b/notifications/sync_notifier – Ben, if you don't like this please speak now.
What's up with this "notifier" suffix by the way? This is just sync for notifications, right? Why do you want to repeat the "notification" part? (If there's a good reason I'm fine with that, I'm just curious.) On Fri, Feb 1, 2013 at 3:28 PM, <petewil@chromium.org> wrote: > I'll go ahead and move this to c/b/notifications/sync_notifier – Ben, if you > don't like this please speak now. > > https://codereview.chromium.org/11745024/
Zea seemed to prefer it, and I think it is more descriptive. The component is a sync notification processor. There are other kinds of notifications that are not sync notifications, and using the directory "sync" implies that this code ties all notifications to sync, where the reality is that a synced notification is a separate kind of beastie. I'll be happy to change it if you feel strongly.
On 2013/02/01 23:59:24, Pete Williamson wrote: > Zea seemed to prefer it, and I think it is more descriptive. The component is a > sync notification processor. There are other kinds of notifications that are > not sync notifications, and using the directory "sync" implies that this code > ties all notifications to sync, where the reality is that a synced notification > is a separate kind of beastie. I'll be happy to change it if you feel strongly. FWIW, I prefer "sync" to make the paths a bit shorter, since having notifier and notifications multiple in the same path feels redundant (e.g. chrome/browser/notifications/notifier/chrome_notifier_service.h). I don't feel a reader would assume that all notifications are synced just because there's a subdirectory called "sync"; it'd just be a sub-component of notifications.
Ping thakis. Now that ABarth has weighed in and the thread has died down, are we clear to proceed with the code review? Thanks!
Can you add a link to https://sites.google.com/a/chromium.org/dev/developers/design-documents/exten... to the CL description and mention that this is for this api? LGTM as is. Maybe we can move it to a less confusing place later, once it's clearer how this will turn out.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petewil@chromium.org/11745024/69001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_aura. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petewil@chromium.org/11745024/70051
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petewil@chromium.org/11745024/77043
Message was sent while issue was closed.
Change committed as 181825 |
