Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
543693003:
Cleanup: Remove some deprecated ExtensionService usage. (Closed)
DescriptionCleanup: Remove some deprecated ExtensionService usage.
Committed: https://crrev.com/7b4bd93458ccde4b07b4aa63bc61148ef95f517e
Cr-Commit-Position: refs/heads/master@{#294023}
Patch Set 1 #
Total comments: 7
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : add comment #Patch Set 4 : rebase #
Messages
Total messages: 14 (4 generated)
thestig@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
https://codereview.chromium.org/543693003/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/543693003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_util.cc:67: return extension_id; This line means that this is "ReloadExtensionIfNotEnabled", which won't work as this function is used. https://codereview.chromium.org/543693003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_util.cc:106: registry->enabled_extensions().GetByID(extension_id); I think this should probably be registry->GetExtensionById(EVERYTHING) (so that it includes disabled/etc extensions). https://codereview.chromium.org/543693003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_util.cc:137: extension = registry->GetExtensionById(id, ExtensionRegistry::EVERYTHING); Maybe have ReloadExtensionIfEnabled return a valid pointer to the extension, instead?
https://codereview.chromium.org/543693003/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/543693003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_util.cc:67: return extension_id; On 2014/09/04 20:50:00, Devlin wrote: > This line means that this is "ReloadExtensionIfNotEnabled", which won't work as > this function is used. I meant if (!extension_is_enabled) ... https://codereview.chromium.org/543693003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_util.cc:106: registry->enabled_extensions().GetByID(extension_id); On 2014/09/04 20:50:00, Devlin wrote: > I think this should probably be registry->GetExtensionById(EVERYTHING) (so that > it includes disabled/etc extensions). Bleh, I looked up the GetExtensionById() conversion. https://codereview.chromium.org/543693003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_util.cc:137: extension = registry->GetExtensionById(id, ExtensionRegistry::EVERYTHING); On 2014/09/04 20:50:00, Devlin wrote: > Maybe have ReloadExtensionIfEnabled return a valid pointer to the extension, > instead? In the SetAllowFileAccess() use case, the returned extension pointer would go unused. That's more wasted work.
lgtm https://codereview.chromium.org/543693003/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/543693003/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_util.cc:137: extension = registry->GetExtensionById(id, ExtensionRegistry::EVERYTHING); On 2014/09/05 06:07:45, Lei Zhang wrote: > On 2014/09/04 20:50:00, Devlin wrote: > > Maybe have ReloadExtensionIfEnabled return a valid pointer to the extension, > > instead? > > In the SetAllowFileAccess() use case, the returned extension pointer would go > unused. That's more wasted work. Fair. I think what would be best would actually be for ExtensionService to return a pointer to the reloaded extension, and then it wouldn't be any extra work. But, that's obviously a bigger change, so don't worry about it. https://codereview.chromium.org/543693003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/543693003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_util.cc:136: // Reloading the extension invalidates the |extension| pointer. This is actually a really good note. Would you mind duplicating it in the .h file in the function comments?
https://codereview.chromium.org/543693003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/543693003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_util.cc:136: // Reloading the extension invalidates the |extension| pointer. On 2014/09/05 15:38:09, Devlin wrote: > This is actually a really good note. Would you mind duplicating it in the .h > file in the function comments? Done.
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/543693003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/53589) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/58554) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/543693003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 389908cd76a03128883115476a91e73e64a147c1
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7b4bd93458ccde4b07b4aa63bc61148ef95f517e Cr-Commit-Position: refs/heads/master@{#294023} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
