Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
275503003:
Remove deprecated extension notification and deprecated extension_service functions. (Closed)
|
Created:
6 years, 7 months ago by limasdf Modified:
6 years, 7 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUse ExtensionRegistryObserver instead of deprecated extension notification.
and Use ExtensionRegistry functions instead of deprecated ExtensionService functions.
R=xiyuan@chromium.org,rdevlin.cronin@chromium.org
BUG=354046, 354458
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272861
Patch Set 1 #Patch Set 2 : #
Total comments: 18
Patch Set 3 : address review comments #
Total comments: 4
Patch Set 4 : address review comment from xiyuan #
Messages
Total messages: 35 (0 generated)
Pleae take a look
Updated, and added @Cronin who has strong extension knowledge. PTAL.
Looks like most of the comments from https://codereview.chromium.org/280853004/ (where there was a slight CL mixup) have been integrated into this, so not too much. Biggest thing is that it doesn't look like this has been compiled on ChromeOS, which is important to do since many of these methods are *only* compiled in a CrOS build. If you're on Linux, check out http://www.chromium.org/developers/how-tos/build-instructions-chromeos for how to build CrOS on your local machine - it's pretty straight-forward, and can save a few of try-bot runs. :) https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/app_list/start_page_handler.cc (right): https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/app_list/start_page_handler.cc:12: #include "chrome/browser/extensions/extension_service.h" Safe to remove this include, I think. https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/app_list/start_page_handler.cc:140: if (!registry) ExtensionRegistry can never be NULL, unless something is very wrong. https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/app_list/start_page_handler.cc:142: const extensions::Extension& hotword_extension = registry->GetExtensionById( ExtensionRegistry::GetExtensionById() returns a const Extension*, not const Extension&. https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/app_list/start_page_handler.cc:143: extension_misc::kHotwordExtensionId, false /* include_disabled */); Hmm... does this compile? ExtensionRegistry::GetExtensionById takes an enum for the second parameter, not a boolean. If it compiles, that would be a subtle bug indeed (since it would convert the boolean -> int 0 -> ExtensionRegistry::NONE) - but I'd hope that clang just yells instead. In either case, please change this to be GetExtensionById(id, ExtensionRegistry::ENABLED) https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/command_handler.cc (right): https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/command_handler.cc:50: extensions::ExtensionRegistry::Get(profile_)); This is all in extensions namespace, so please remove extensions:: prefix. https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/command_handler.h (right): https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/command_handler.h:23: class Command; These can be moved to around line 32, so we don't have two "namespace extensions" blocks. https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/command_handler.h:28: class Extension; This should be forward-declared in the extensions namespace, not in the global namespace. https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/command_handler.h:50: const extensions::Extension* extension) OVERRIDE; In extensions namespace, so don't need extensions::.
@cronin ptal https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/app_list/start_page_handler.cc (right): https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/app_list/start_page_handler.cc:12: #include "chrome/browser/extensions/extension_service.h" On 2014/05/19 16:04:11, D Cronin wrote: > Safe to remove this include, I think. Done. https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/app_list/start_page_handler.cc:140: if (!registry) On 2014/05/19 16:04:11, D Cronin wrote: > ExtensionRegistry can never be NULL, unless something is very wrong. Done. I was a little worried about because of https://code.google.com/p/chromium/codesearch#chromium/src/components/keyed_s... But there must be exisiting |profile| here. https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/app_list/start_page_handler.cc:142: const extensions::Extension& hotword_extension = registry->GetExtensionById( On 2014/05/19 16:04:11, D Cronin wrote: > ExtensionRegistry::GetExtensionById() returns a const Extension*, not const > Extension&. Done. https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/app_list/start_page_handler.cc:142: const extensions::Extension& hotword_extension = registry->GetExtensionById( On 2014/05/19 16:04:11, D Cronin wrote: > ExtensionRegistry::GetExtensionById() returns a const Extension*, not const > Extension&. Done. shame on me :( https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/app_list/start_page_handler.cc:143: extension_misc::kHotwordExtensionId, false /* include_disabled */); On 2014/05/19 16:04:11, D Cronin wrote: > Hmm... does this compile? ExtensionRegistry::GetExtensionById takes an enum for > the second parameter, not a boolean. If it compiles, that would be a subtle bug > indeed (since it would convert the boolean -> int 0 -> ExtensionRegistry::NONE) > - but I'd hope that clang just yells instead. In either case, please change > this to be > GetExtensionById(id, ExtensionRegistry::ENABLED) Done. https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/command_handler.cc (right): https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/command_handler.cc:50: extensions::ExtensionRegistry::Get(profile_)); On 2014/05/19 16:04:11, D Cronin wrote: > This is all in extensions namespace, so please remove extensions:: prefix. Done. https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/command_handler.h (right): https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/command_handler.h:23: class Command; On 2014/05/19 16:04:11, D Cronin wrote: > These can be moved to around line 32, so we don't have two "namespace > extensions" blocks. Done. https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/command_handler.h:28: class Extension; On 2014/05/19 16:04:11, D Cronin wrote: > This should be forward-declared in the extensions namespace, not in the global > namespace. Done. https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/command_handler.h:50: const extensions::Extension* extension) OVERRIDE; On 2014/05/19 16:04:11, D Cronin wrote: > In extensions namespace, so don't need extensions::. Done.
lgtm! https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/app_list/start_page_handler.cc (right): https://codereview.chromium.org/275503003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/app_list/start_page_handler.cc:142: const extensions::Extension& hotword_extension = registry->GetExtensionById( On 2014/05/20 04:04:49, limasdf wrote: > On 2014/05/19 16:04:11, D Cronin wrote: > > ExtensionRegistry::GetExtensionById() returns a const Extension*, not const > > Extension&. > > Done. shame on me :( No worries :)
@pam, could you take a look as OWNER of c/b/ui/webui ?
@pam kindly ping
@pam friendly ping
On Sat, May 24, 2014 at 12:42 AM, <limasdf@gmail.com> wrote: > @pam friendly ping > > Please, ask another webui reviewer/owner. Pam does not work on Chromium full-time anymore. -- Thiago Farina To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
added xiyuan for the webui. @xiyuan ptal. - @tfarnia, thanks!
lgtm https://codereview.chromium.org/275503003/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/app_list/start_page_handler.cc (right): https://codereview.chromium.org/275503003/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/app_list/start_page_handler.cc:64: : recommended_apps_(NULL), extension_registry_observer_(this) { nit: one member per line https://codereview.chromium.org/275503003/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/command_handler.cc (right): https://codereview.chromium.org/275503003/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/command_handler.cc:24: : profile_(profile), extension_registry_observer_(this) { nit: one per line
Thanks for the review https://codereview.chromium.org/275503003/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/app_list/start_page_handler.cc (right): https://codereview.chromium.org/275503003/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/app_list/start_page_handler.cc:64: : recommended_apps_(NULL), extension_registry_observer_(this) { On 2014/05/24 05:46:59, xiyuan wrote: > nit: one member per line Done. https://codereview.chromium.org/275503003/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/command_handler.cc (right): https://codereview.chromium.org/275503003/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/command_handler.cc:24: : profile_(profile), extension_registry_observer_(this) { On 2014/05/24 05:46:59, xiyuan wrote: > nit: one per line Done.
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/limasdf@gmail.com/275503003/100001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/limasdf@gmail.com/275503003/100001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/limasdf@gmail.com/275503003/100001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
I do still work on Chromium full-time. But I'm not generally reading work email on the weekends, or on Monday this week since it's a holiday, so if you want faster turn-around, someone not in the US would be fine too. - Pam On Fri, May 23, 2014 at 9:06 PM, Thiago Farina <tfarina@chromium.org> wrote: > > > On Sat, May 24, 2014 at 12:42 AM, <limasdf@gmail.com> wrote: > >> @pam friendly ping >> >> Please, ask another webui reviewer/owner. Pam does not work on Chromium > full-time anymore. > > -- > Thiago Farina > -- Google Germany GmbH Dienerstr. 12, 80331 München AG Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth FloresTax ID: 48/725/00206 VAT ID: DE813741370 To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Oh, I see -- this was sent a while ago. Sorry about that: I do often miss review requests in my email. That's why my nickname includes "IM for reviews", but I realize not everyone can contact me that way. My apologies for that. I'll try to get to the review tonight. - Pam On Sun, May 25, 2014 at 5:56 PM, Pam Greene <pam@chromium.org> wrote: > I do still work on Chromium full-time. > > But I'm not generally reading work email on the weekends, or on Monday > this week since it's a holiday, so if you want faster turn-around, someone > not in the US would be fine too. > > - Pam > > > > On Fri, May 23, 2014 at 9:06 PM, Thiago Farina <tfarina@chromium.org> > wrote: > >> >> >> On Sat, May 24, 2014 at 12:42 AM, <limasdf@gmail.com> wrote: >> >>> @pam friendly ping >>> >>> Please, ask another webui reviewer/owner. Pam does not work on Chromium >> full-time anymore. >> >> -- >> Thiago Farina >> > > -- > Google Germany GmbH > Dienerstr. 12, 80331 München > AG Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, > Christine Elizabeth FloresTax ID: 48/725/00206 > VAT ID: DE813741370 > -- Google Germany GmbH Dienerstr. 12, 80331 München AG Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth FloresTax ID: 48/725/00206 VAT ID: DE813741370 To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Sunday, May 25, 2014, Pam Greene <pam@chromium.org> wrote: > I do still work on Chromium full-time. > > I was under the impression that you worked more in the internal/private code than the public, as I don't see you doing many public reviews. :) I haven't found a way to say that without saying you don't work on Chromium full-time, sorry. -- Thiago Farina To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Sunday, May 25, 2014, Pam Greene <pam@chromium.org> wrote: > Oh, I see -- this was sent a while ago. Sorry about that: I do often miss > review requests in my email. That's why my nickname includes "IM for > reviews", but I realize not everyone can contact me that way. My apologies > for that. I'll try to get to the review tonight. > No need Pam. Xiyuan already took it for webui/, thanks. -- Thiago Farina To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OK, glad to hear it's been taken care of. Sorry for the delay. In the future, feel free to ping me by IM if I don't reply to a review quickly, or ask someone else to. I'm still completely happy to review WebUI changes. - Pam On Sun, May 25, 2014 at 7:35 PM, Thiago Farina <tfarina@chromium.org> wrote: > > > On Sunday, May 25, 2014, Pam Greene <pam@chromium.org> wrote: > >> Oh, I see -- this was sent a while ago. Sorry about that: I do often miss >> review requests in my email. That's why my nickname includes "IM for >> reviews", but I realize not everyone can contact me that way. My apologies >> for that. I'll try to get to the review tonight. >> > > No need Pam. Xiyuan already took it for webui/, thanks. > > > -- > Thiago Farina > > -- Google Germany GmbH Dienerstr. 12, 80331 München AG Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth FloresTax ID: 48/725/00206 VAT ID: DE813741370 To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/limasdf@gmail.com/275503003/100001
Message was sent while issue was closed.
Change committed as 272861 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
