Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11316133:
Added implementation of SyncEventObserver to route events to Javascript Extension. (Closed)
|
Created:
8 years, 1 month ago by calvinlo Modified:
8 years ago CC:
chromium-reviews, tzik+watch_chromium.org, Aaron Boodman, kinuko+watch, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded implementation of SyncEventObserver to route events to Javascript Extension.
BUG=160939
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170427
Patch Set 1 #
Total comments: 20
Patch Set 2 : Rebase after SyncEventObserver Interface updated #Patch Set 3 : Fix comment typo #Patch Set 4 : Fixed test to match updated SyncEventObserver interface #
Total comments: 8
Patch Set 5 : Kinuko review #2 #Patch Set 6 : Kinuko review #2 (after little fixups) #
Total comments: 10
Patch Set 7 : Kinuko Review #3 and rebase #
Total comments: 2
Patch Set 8 : Kinuko Review #4 #
Total comments: 6
Patch Set 9 : Benwells Review #1 #Patch Set 10 : Rebase + lint fixes #Patch Set 11 : Fix compile #Patch Set 12 : Fix destructor for compile #
Messages
Total messages: 23 (0 generated)
Hi Kinuko, can you please take a first pass to see if I'm on the right track? I'm not sure if this code follows your intended design.
Looks good, I added some minor comments. https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc (right): https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:22: SYNC_FILE_SYSTEM_SYNC_STATE_STATUS_INITIALIZING; Wow is this line-break allowed? (If it compiles it's fine) https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:44: profile_(profile) {} nit: four spaces before ':' is the common pattern ... Profile* profile) : extension_id_(extension_id), profile_(profile) {} https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:50: api::sync_file_system::SyncState syncState; syncState -> sync_state while you're in chrome c++ code :) https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:51: syncState.service_name = "drive"; Maybe this should be also given in constructor. Or maybe even better I think we can make the constructor just take a single pointer to the SyncEventManager and the manager can hold profile_ and service_name_ fields (since they will be shared by observers). class ExtensionSyncEventManager { public: ExtensionSyncEventManager(Profile* profile, const std::string& service_name); // Getters for SyncEventObserver. Profile* profile() { return profile_; } const std::string& service_name() const { return service_name_; } ... } class ExtensionSyncEventObserver { public: explicit ExtensionSyncEventObserver(ExtensionSyncEventManager* manager); ... } If you're not going to have Manager code in this CL yet you can leave TODO comment instead https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:9: #include "chrome/browser/extensions/event_router.h" Not necessary (yet)? https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:14: // Handles sending JavaScript events to exactly one extension ID nit: '.' at the end of comment https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:18: ExtensionSyncEventObserver(std::string extension_id, Profile* profile); nit: use const reference for extension_id (to save data copy) const std::string& extension_id https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:21: DISALLOW_COPY_AND_ASSIGN(ExtensionSyncEventObserver); This needs to be placed in private: (Usually we write this at the end of the class) https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:32: } // namespace extensions nit: extra space after '//'? https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/eve... File chrome/browser/extensions/event_names.h (right): https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/eve... chrome/browser/extensions/event_names.h:88: // Sync File System '.' at the end of comment
Implements onSyncStateChanged() from SyncEventObserver and enum conversion from C++ to JavaScript (IDL) version only. https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc (right): https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:22: SYNC_FILE_SYSTEM_SYNC_STATE_STATUS_INITIALIZING; On 2012/11/21 10:59:04, kinuko wrote: > Wow is this line-break allowed? (If it compiles it's fine) Yes it actually compiles. I wasn't sure how else to make this shorter. I tried using a "using" statement to shorten this but then the compiler complained. If you have suggestions on how to shorten this, I'd really like to know. I'm not familiar with how to do this in C++ (versus Java). https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:44: profile_(profile) {} On 2012/11/21 10:59:04, kinuko wrote: > nit: four spaces before ':' is the common pattern > > ... > Profile* profile) > : extension_id_(extension_id), > profile_(profile) {} Done. https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:50: api::sync_file_system::SyncState syncState; On 2012/11/21 10:59:04, kinuko wrote: > syncState -> sync_state while you're in chrome c++ code :) Done. https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:51: syncState.service_name = "drive"; Ok, finally able to get back to this. Based on our conversation, I don't think we need a manager anymore since the app_origin/extension ID value is now passed as a param to each function in the SyncEventObserver interface. I still need to add set::extension IDs that are listening and ignore sending the event if the extension_id isn't listening here. Do you think it's a good idea for me to submit this first with TODOs and add all the OnEventListener and NOTIFICATION_EXTENSION_UNLOADED hookups in another patch or continue to do everything in this patch? https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:9: #include "chrome/browser/extensions/event_router.h" On 2012/11/21 10:59:04, kinuko wrote: > Not necessary (yet)? You're right. I've added a forward declaration of Profile so it still compiles. https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:14: // Handles sending JavaScript events to exactly one extension ID On 2012/11/21 10:59:04, kinuko wrote: > nit: '.' at the end of comment Done. https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:18: ExtensionSyncEventObserver(std::string extension_id, Profile* profile); On 2012/11/21 10:59:04, kinuko wrote: > nit: use const reference for extension_id (to save data copy) > > const std::string& extension_id Done. https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:21: DISALLOW_COPY_AND_ASSIGN(ExtensionSyncEventObserver); On 2012/11/21 10:59:04, kinuko wrote: > This needs to be placed in private: > (Usually we write this at the end of the class) Done. https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:32: } // namespace extensions On 2012/11/21 10:59:04, kinuko wrote: > nit: extra space after '//'? Done.
https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc (right): https://codereview.chromium.org/11316133/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:51: syncState.service_name = "drive"; On 2012/11/28 06:01:45, calvinlo wrote: > Ok, finally able to get back to this. Based on our conversation, I don't think > we need a manager anymore since the app_origin/extension ID value is now passed > as a param to each function in the SyncEventObserver interface. > > I still need to add set::extension IDs that are listening and ignore sending the > event if the extension_id isn't listening here. > > Do you think it's a good idea for me to submit this first with TODOs and add all > the OnEventListener and NOTIFICATION_EXTENSION_UNLOADED hookups in another patch > or continue to do everything in this patch? I feel that EXTENSION_UNLOADED can surely come later. OnListenerAdded may be crucial so I might want to see it together but please feel free to leave TODOs when you think the patch has enough meaningful content. https://codereview.chromium.org/11316133/diff/8/chrome/browser/extensions/api... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc (right): https://codereview.chromium.org/11316133/diff/8/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:45: const std::string& extension_id, This is given as GURL right? I think you can get the app or extension for given GURL by doing something like this (please look for the original definition for more details): Extension* app = ExtensionSystem::Get(profile_)->extension_service()->GetInstalledApp(app_origin); Extension class has id() accessor. (Please DCHECK before accessing it) https://codereview.chromium.org/11316133/diff/8/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:53: sync_state.service_name = "drive"; sync_state.service_name = service_name_; ? https://codereview.chromium.org/11316133/diff/8/chrome/browser/extensions/api... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/8/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:26: const std::string& description) OVERRIDE; You'll also need to have OnFileSynced() override here (since it's pure virtual method) The method body can be empty (but I think it's also ok to put its impl in this change) https://codereview.chromium.org/11316133/diff/8/chrome/browser/sync_file_syst... File chrome/browser/sync_file_system/sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/8/chrome/browser/sync_file_syst... chrome/browser/sync_file_system/sync_event_observer.h:10: #include "googleurl/src/gurl.h" I think forward decl would work class GURL;
If it is ok with you, I would like to send this patch in first as it's already ~200 lines. I will then do this: 1. Implement OnListenerAdded including JS Api Test 2. Convert DOMString filepath returned in OnFileSynced event to Webkit FileEntry type. 3. Implement NOTIFICATION_EXTENSION_UNLOADED https://codereview.chromium.org/11316133/diff/8/chrome/browser/extensions/api... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc (right): https://codereview.chromium.org/11316133/diff/8/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:45: const std::string& extension_id, On 2012/11/28 07:41:13, kinuko wrote: > This is given as GURL right? > > I think you can get the app or extension for given GURL by doing something like > this (please look for the original definition for more details): > > Extension* app = > ExtensionSystem::Get(profile_)->extension_service()->GetInstalledApp(app_origin); > > Extension class has id() accessor. (Please DCHECK before accessing it) Done. https://codereview.chromium.org/11316133/diff/8/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:53: sync_state.service_name = "drive"; On 2012/11/28 07:41:13, kinuko wrote: > sync_state.service_name = service_name_; ? Done. Sorry my mistake. https://codereview.chromium.org/11316133/diff/8/chrome/browser/extensions/api... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/8/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:26: const std::string& description) OVERRIDE; On 2012/11/28 07:41:13, kinuko wrote: > You'll also need to have OnFileSynced() override here (since it's pure virtual > method) > > The method body can be empty (but I think it's also ok to put its impl in this > change) Ok I've added this now but just using a DOMString for the file path until I figure out how to make it into a Webkit.FileEntry object. https://codereview.chromium.org/11316133/diff/8/chrome/browser/sync_file_syst... File chrome/browser/sync_file_system/sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/8/chrome/browser/sync_file_syst... chrome/browser/sync_file_system/sync_event_observer.h:10: #include "googleurl/src/gurl.h" On 2012/11/28 07:41:13, kinuko wrote: > I think forward decl would work > > class GURL; Done.
looking good. https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc (right): https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:5: #include "chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h" nit: please insert one empty line after this https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:67: const std::string ExtensionSyncEventObserver::GetExtensionId( you probably meant const std::string& ? (as Extension::id() returns const ref) https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:16: // Handles sending JavaScript events to exactly one extension ID. nit: 'exactly one' may not be correct if we get broadcasting events? https://codereview.chromium.org/11316133/diff/11003/chrome/browser/sync_file_... File chrome/browser/sync_file_system/sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/11003/chrome/browser/sync_file_... chrome/browser/sync_file_system/sync_event_observer.h:55: const fileapi::FileSystemURL& url) = 0; When you rebase the code you'll see the new code, the new signature takes parameters in (url, operation) order but not (operation, url). https://codereview.chromium.org/11316133/diff/11003/chrome/common/extensions/... File chrome/common/extensions/api/sync_file_system.idl (right): https://codereview.chromium.org/11316133/diff/11003/chrome/common/extensions/... chrome/common/extensions/api/sync_file_system.idl:20: FILE_DELETED I was looking at other idl files that use enum and noticed that (at least for input side) most of them use lower-capital names (like 'tcp, 'udp' for socket type) and in Javascript layer they're actually recognized as string enum. Maybe to follow the convention we should also use lower-case enum values like: { add, update, delete } ? (And if we do so we should also do same for SyncStateStatus)
If this is all ok now, I will send to Ben next for Extension owner review https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc (right): https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:5: #include "chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h" On 2012/11/28 11:06:40, kinuko wrote: > nit: please insert one empty line after this Done. https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:67: const std::string ExtensionSyncEventObserver::GetExtensionId( On 2012/11/28 11:06:40, kinuko wrote: > you probably meant const std::string& ? (as Extension::id() returns const ref) Done. https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:16: // Handles sending JavaScript events to exactly one extension ID. Sorry forgot to update this. You are correct as there is no longer going to be a manager. https://codereview.chromium.org/11316133/diff/11003/chrome/browser/sync_file_... File chrome/browser/sync_file_system/sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/11003/chrome/browser/sync_file_... chrome/browser/sync_file_system/sync_event_observer.h:55: const fileapi::FileSystemURL& url) = 0; Thanks for the tip. Rebased and updated. https://codereview.chromium.org/11316133/diff/11003/chrome/common/extensions/... File chrome/common/extensions/api/sync_file_system.idl (right): https://codereview.chromium.org/11316133/diff/11003/chrome/common/extensions/... chrome/common/extensions/api/sync_file_system.idl:20: FILE_DELETED On 2012/11/28 11:06:40, kinuko wrote: > I was looking at other idl files that use enum and noticed that (at least for > input side) most of them use lower-capital names (like 'tcp, 'udp' for socket > type) and in Javascript layer they're actually recognized as string enum. > > Maybe to follow the convention we should also use lower-case enum values like: { > add, update, delete } ? > > (And if we do so we should also do same for SyncStateStatus) Done.
If this is all ok now, I will send to Ben next for Extension owner review https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc (right): https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:5: #include "chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h" On 2012/11/28 11:06:40, kinuko wrote: > nit: please insert one empty line after this Done. https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:67: const std::string ExtensionSyncEventObserver::GetExtensionId( On 2012/11/28 11:06:40, kinuko wrote: > you probably meant const std::string& ? (as Extension::id() returns const ref) Done. https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/11003/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:16: // Handles sending JavaScript events to exactly one extension ID. Sorry forgot to update this. You are correct as there is no longer going to be a manager. https://codereview.chromium.org/11316133/diff/11003/chrome/browser/sync_file_... File chrome/browser/sync_file_system/sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/11003/chrome/browser/sync_file_... chrome/browser/sync_file_system/sync_event_observer.h:55: const fileapi::FileSystemURL& url) = 0; Thanks for the tip. Rebased and updated. https://codereview.chromium.org/11316133/diff/11003/chrome/common/extensions/... File chrome/common/extensions/api/sync_file_system.idl (right): https://codereview.chromium.org/11316133/diff/11003/chrome/common/extensions/... chrome/common/extensions/api/sync_file_system.idl:20: FILE_DELETED On 2012/11/28 11:06:40, kinuko wrote: > I was looking at other idl files that use enum and noticed that (at least for > input side) most of them use lower-capital names (like 'tcp, 'udp' for socket > type) and in Javascript layer they're actually recognized as string enum. > > Maybe to follow the convention we should also use lower-case enum values like: { > add, update, delete } ? > > (And if we do so we should also do same for SyncStateStatus) Done.
Ok lgtm, I think it's ready for owners review https://codereview.chromium.org/11316133/diff/8006/chrome/browser/extensions/... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc (right): https://codereview.chromium.org/11316133/diff/8006/chrome/browser/extensions/... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:106: const std::string filePath = url.virtual_path().MaybeAsASCII(); Please use this instead: url.path().AsUTF8Unsafe() (virtual_path and path are kinda confusing but here path() has the one we're interested in; and converting ASCII will drop any non-ascii chars)
https://codereview.chromium.org/11316133/diff/8006/chrome/browser/extensions/... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc (right): https://codereview.chromium.org/11316133/diff/8006/chrome/browser/extensions/... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:106: const std::string filePath = url.virtual_path().MaybeAsASCII(); On 2012/11/28 12:37:02, kinuko wrote: > Please use this instead: > url.path().AsUTF8Unsafe() > > (virtual_path and path are kinda confusing but here path() has the one we're > interested in; and converting ASCII will drop any non-ascii chars) Done.
Hi Ben, Can you please take a look for Extension ownership. Please note this patch is just the beginning and several things are missing. I plan to do this next: 1. Implement OnListenerAdded including JS Api Test 2. Convert DOMString filepath returned in OnFileSynced event to Webkit FileEntry type. 3. Implement NOTIFICATION_EXTENSION_UNLOADED so SyncFS knows to clean itself up.
https://codereview.chromium.org/11316133/diff/14001/chrome/browser/extensions... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc (right): https://codereview.chromium.org/11316133/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:101: const std::string extension_id = GetExtensionId(url.origin()); Nit: Index << 2 https://codereview.chromium.org/11316133/diff/14001/chrome/browser/extensions... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:33: Profile* profile_; Nit: Fields should go after functions. https://codereview.chromium.org/11316133/diff/14001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/14001/chrome/browser/sync_file_... chrome/browser/sync_file_system/sync_event_observer.h:12: class GURL; Why is this needed?
https://codereview.chromium.org/11316133/diff/14001/chrome/browser/extensions... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc (right): https://codereview.chromium.org/11316133/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.cc:101: const std::string extension_id = GetExtensionId(url.origin()); On 2012/11/29 04:37:58, benwells wrote: > Nit: Index << 2 Done. https://codereview.chromium.org/11316133/diff/14001/chrome/browser/extensions... File chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/extension_sync_event_observer.h:33: Profile* profile_; On 2012/11/29 04:37:58, benwells wrote: > Nit: Fields should go after functions. Done. https://codereview.chromium.org/11316133/diff/14001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/sync_event_observer.h (right): https://codereview.chromium.org/11316133/diff/14001/chrome/browser/sync_file_... chrome/browser/sync_file_system/sync_event_observer.h:12: class GURL; On 2012/11/29 04:37:58, benwells wrote: > Why is this needed? It's for line 48. Previously this header wasn't being included anywhere so it wasn't tested for compilation. So this line just fixes the compile now.
lgtm
Hi stig, can you please review the additions to chrome_browser_extensions.gypi. Thanks.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calvinlo@chromium.org/11316133/7024
Sorry for I got bad news for ya. Compile failed with a clobber build on win_aura. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_aura&n... 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/calvinlo@chromium.org/11316133/7024
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calvinlo@chromium.org/11316133/2007
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan... 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/calvinlo@chromium.org/11316133/9016
Message was sent while issue was closed.
Change committed as 170427 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
