|
|
Created:
6 years, 7 months ago by limasdf Modified:
6 years, 3 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, chromium-apps-reviews_chromium.org, markusheintz_, extensions-reviews_chromium.org, tapted Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionremove deprecated extension notification from content_settings.
Use ExtensionRegistryObserver instead.
R=bauerb@chromium.org
BUG=354046, 354458
TEST=unit_tests
Committed: https://crrev.com/76ad58487578183de69a8caeb48b8a22dccad652
Cr-Commit-Position: refs/heads/master@{#294768}
Patch Set 1 : #Patch Set 2 : fix unit_tests #
Total comments: 4
Patch Set 3 : review #
Total comments: 2
Patch Set 4 : remove unwanted header #Patch Set 5 : remove observer from ShutdownOnUIThread #
Total comments: 2
Patch Set 6 : #
Total comments: 1
Messages
Total messages: 33 (6 generated)
please take a look when you have time.
LGTM, thanks!
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/276573011/20001
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: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
Bernhard, Could you take a look again? This fixes unit_tests.(Remove observer when ExtensionRegistry shutdown first).
https://codereview.chromium.org/276573011/diff/60001/chrome/browser/content_s... File chrome/browser/content_settings/content_settings_internal_extension_provider.cc (right): https://codereview.chromium.org/276573011/diff/60001/chrome/browser/content_s... chrome/browser/content_settings/content_settings_internal_extension_provider.cc:43: extension_registry_ = extensions::ExtensionRegistry::Get(profile); Could you initialize this in the initializer list above? https://codereview.chromium.org/276573011/diff/60001/chrome/browser/content_s... chrome/browser/content_settings/content_settings_internal_extension_provider.cc:88: if (extension_registry_) { When can it happen that |extension_registry_| is NULL?
https://codereview.chromium.org/276573011/diff/60001/chrome/browser/content_s... File chrome/browser/content_settings/content_settings_internal_extension_provider.cc (right): https://codereview.chromium.org/276573011/diff/60001/chrome/browser/content_s... chrome/browser/content_settings/content_settings_internal_extension_provider.cc:43: extension_registry_ = extensions::ExtensionRegistry::Get(profile); On 2014/06/10 08:57:05, Bernhard Bauer wrote: > Could you initialize this in the initializer list above? Done. https://codereview.chromium.org/276573011/diff/60001/chrome/browser/content_s... chrome/browser/content_settings/content_settings_internal_extension_provider.cc:88: if (extension_registry_) { On 2014/06/10 08:57:05, Bernhard Bauer wrote: > When can it happen that |extension_registry_| is NULL? Hm, it seems doesn't necessary. Removed if statement.
lgtm https://codereview.chromium.org/276573011/diff/80001/chrome/browser/content_s... File chrome/browser/content_settings/content_settings_internal_extension_provider.h (right): https://codereview.chromium.org/276573011/diff/80001/chrome/browser/content_s... chrome/browser/content_settings/content_settings_internal_extension_provider.h:9: #include "base/scoped_observer.h" This isn't necessary anymore.
Thank you for the review! https://codereview.chromium.org/276573011/diff/80001/chrome/browser/content_s... File chrome/browser/content_settings/content_settings_internal_extension_provider.h (right): https://codereview.chromium.org/276573011/diff/80001/chrome/browser/content_s... chrome/browser/content_settings/content_settings_internal_extension_provider.h:9: #include "base/scoped_observer.h" On 2014/06/10 09:34:16, Bernhard Bauer wrote: > This isn't necessary anymore. 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/276573011/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
Patchset #6 (id:180001) has been deleted
Patchset #6 (id:200001) has been deleted
Patchset #5 (id:160001) has been deleted
Sorry to bother you again. This finally seems to fix. PTAL
LGTM, just one nit: https://codereview.chromium.org/276573011/diff/220001/chrome/browser/content_... File chrome/browser/content_settings/content_settings_internal_extension_provider.cc (right): https://codereview.chromium.org/276573011/diff/220001/chrome/browser/content_... chrome/browser/content_settings/content_settings_internal_extension_provider.cc:75: DCHECK_EQ(type, extensions::NOTIFICATION_EXTENSION_HOST_CREATED); Nit: Put the expected value first for a more informative error message on failure.
Thank you for the review https://codereview.chromium.org/276573011/diff/220001/chrome/browser/content_... File chrome/browser/content_settings/content_settings_internal_extension_provider.cc (right): https://codereview.chromium.org/276573011/diff/220001/chrome/browser/content_... chrome/browser/content_settings/content_settings_internal_extension_provider.cc:75: DCHECK_EQ(type, extensions::NOTIFICATION_EXTENSION_HOST_CREATED); On 2014/09/14 14:04:04, Bernhard Bauer wrote: > Nit: Put the expected value first for a more informative error message on > failure. Done.
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/276573011/240001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/276573011/240001
Message was sent while issue was closed.
Committed patchset #6 (id:240001) as 31a96f8898f73bd7decd56c02aa0dff5877852a6
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/76ad58487578183de69a8caeb48b8a22dccad652 Cr-Commit-Position: refs/heads/master@{#294768}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:240001) has been created in https://codereview.chromium.org/553263004/ by limasdf@gmail.com. The reason for reverting is: Becuase this CL seems to break linux asan test http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_....
Message was sent while issue was closed.
https://codereview.chromium.org/276573011/diff/240001/chrome/browser/content_... File chrome/browser/content_settings/content_settings_internal_extension_provider.h (right): https://codereview.chromium.org/276573011/diff/240001/chrome/browser/content_... chrome/browser/content_settings/content_settings_internal_extension_provider.h:81: extensions::ExtensionRegistry* extension_registry_; // Not owned. This extra state might not be necessary once https://codereview.chromium.org/556173002/ lands. (waiting for that CL might even help with the asan issue too) |