Use ExtensionRegistryObserver instead of deprecated extension notification from autocomplete

Use extensions::ExtensionRegistryObserver instead of deprecated extension notification from autocomplete

R=pkasting@chromium.org
CC=kalman@chromium.org
BUG=354046, 354458
TEST=unit_tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270268

[limasdf, 2014-05-11 17:16:19]

I'm cleaning deprecatex extension notification.
Please take a look when you have time.

[not at google - send to devlin, 2014-05-12 17:59:00]

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/extension_app_provider_unittest.cc (right):

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider_unittest.cc:30:
app_provider_(new ExtensionAppProvider(NULL, profile_.get())),
typically we do leave this sort of initialization until SetUp. is there a
specific reason you moved it into the constructor?

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_provider_unittest.cc (right):

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider_unittest.cc:303: profile_(new
TestingProfile) {
ditto, though in this case it's basically the same behaviour; any reason you
needed to change this to a pointer?

[Peter Kasting, 2014-05-12 19:46:55]

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/extension_app_provider.cc (right):

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.cc:42: //
ExtensionRegistryObserver handles use non-incognito profile itself.
Is this comment trying to say "ExtensionRegistryObserver will handle getting the
original profile itself as necessary" or something similar?

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.cc:199: DCHECK_EQ(type,
chrome::NOTIFICATION_EXTENSION_UNINSTALLED);
Nit: (expected, actual)

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/extension_app_provider.h (right):

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.h:86: // Overridden from
extensions::ExtensionRegistryObserver.
Nit: It looks like file style is to say "extensions::ExtensionRegistryObserver
implementation:".  Also, this should be listed below Observe(), to keep override
order matching the parent class declaration order atop the file.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.h:98: // Listen to extension
load notifications.
Remove this comment; it's not useful.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/extension_app_provider_unittest.cc (right):

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider_unittest.cc:30:
app_provider_(new ExtensionAppProvider(NULL, profile_.get())),
On 2014/05/12 17:59:00, kalman wrote:
> typically we do leave this sort of initialization until SetUp. is there a
> specific reason you moved it into the constructor?

The two should be equivalent, and if you _can_ init something in the constructor
initializer list, I think it's better to do so.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_backend.cc:86:
extensions::ExtensionRegistry::Get(profile));
Nit: Move this after the line below, to keep initialization order matching
declaration order.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_backend.cc:168: void
ShortcutsBackend::OnExtensionUnloaded(
Nit: Function definition order should match declaration order.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_backend.h (right):

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_backend.h:114: // Overridden from
extensions::ExtensionRegistryObserver.
Nit: File style seems to be "extensions::ExtensionRegistryObserver:"

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_backend.h:161: // Listen to extension
load, unload notifications.
Remove this comment; it's both inaccurate (you're not caring about load
notifications) and not useful.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_provider_unittest.cc (right):

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider_unittest.cc:303: profile_(new
TestingProfile) {
On 2014/05/12 17:59:00, kalman wrote:
> any reason you
> needed to change this to a pointer?

I'm wondering this too.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider_unittest.cc:314:
ASSERT_TRUE(profile_.get()->CreateHistoryService(true, false));
There's no need to use "get()" before "->".

[limasdf, 2014-05-13 14:38:44]

PTAL.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/extension_app_provider.cc (right):

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.cc:42: //
ExtensionRegistryObserver handles use non-incognito profile itself.
On 2014/05/12 19:46:55, Peter Kasting wrote:
> Is this comment trying to say "ExtensionRegistryObserver will handle getting
the
> original profile itself as necessary" or something similar?
Yes, Exactly. I blame my English T.T
Updated.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.cc:199: DCHECK_EQ(type,
chrome::NOTIFICATION_EXTENSION_UNINSTALLED);
On 2014/05/12 19:46:55, Peter Kasting wrote:
> Nit: (expected, actual)

Done.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/extension_app_provider.h (right):

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.h:86: // Overridden from
extensions::ExtensionRegistryObserver.
On 2014/05/12 19:46:55, Peter Kasting wrote:
> Nit: It looks like file style is to say "extensions::ExtensionRegistryObserver
> implementation:".  Also, this should be listed below Observe(), to keep
override
> order matching the parent class declaration order atop the file.

Done.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.h:98: // Listen to extension
load notifications.
On 2014/05/12 19:46:55, Peter Kasting wrote:
> Remove this comment; it's not useful.

Done.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/extension_app_provider_unittest.cc (right):

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider_unittest.cc:30:
app_provider_(new ExtensionAppProvider(NULL, profile_.get())),
On 2014/05/12 17:59:00, kalman wrote:
> typically we do leave this sort of initialization until SetUp. is there a
> specific reason you moved it into the constructor?

Interestingly, the result is different. the original code fail from test.
|ExtensionRegistry::RemoveObserver| is called with improper observer.
Perhaps, the reason is inside of gtest::SetUp. or scoped_ptr. because of my
ignorance for them,
I just patched the issue with this method.

* Result of original code
[ RUN      ] ExtensionAppProviderTest.BasicMatching
Received signal 11 <unknown> 000000000000
#0 0x0000040a68c5 base::debug::StackTrace::StackTrace()
#1 0x0000040a61f1 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f142bdf3cb0 <unknown>
#3 0x7f142a5bd156 <unknown>
#4 0x7f142a5bd1b2 <unknown>
#5 0x00000045045c __gnu_debug::_Safe_iterator_base::_Safe_iterator_base()
#6 0x0000088d9780 __gnu_debug::_Safe_iterator<>::_Safe_iterator()
#7 0x0000088d880b std::__debug::vector<>::end()
#8 0x0000088d82b3 ObserverListBase<>::RemoveObserver()
#9 0x0000088d7a8c extensions::ExtensionRegistry::RemoveObserver()
#10 0x00000214d269 ScopedObserver<>::RemoveAll()
#11 0x00000214c676 ScopedObserver<>::~ScopedObserver()
#12 0x00000214be62 ExtensionAppProvider::~ExtensionAppProvider()
#13 0x00000214bedc ExtensionAppProvider::~ExtensionAppProvider()

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_backend.cc:86:
extensions::ExtensionRegistry::Get(profile));
On 2014/05/12 19:46:55, Peter Kasting wrote:
> Nit: Move this after the line below, to keep initialization order matching
> declaration order.

Done.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_backend.cc:168: void
ShortcutsBackend::OnExtensionUnloaded(
On 2014/05/12 19:46:55, Peter Kasting wrote:
> Nit: Function definition order should match declaration order.

Done.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_backend.h (right):

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_backend.h:114: // Overridden from
extensions::ExtensionRegistryObserver.
On 2014/05/12 19:46:55, Peter Kasting wrote:
> Nit: File style seems to be "extensions::ExtensionRegistryObserver:"

Done.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_backend.h:161: // Listen to extension
load, unload notifications.
On 2014/05/12 19:46:55, Peter Kasting wrote:
> Remove this comment; it's both inaccurate (you're not caring about load
> notifications) and not useful.

Done.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_provider_unittest.cc (right):

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider_unittest.cc:303: profile_(new
TestingProfile) {
On 2014/05/12 17:59:00, kalman wrote:
> ditto, though in this case it's basically the same behaviour; any reason you
> needed to change this to a pointer?

for consistent with extension_app_provider_unittest.cc.
But on second thought, I shouldn't touch this which is not a part of purpose of
this CL.
back to original, not pointer. Done.

https://codereview.chromium.org/271393003/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider_unittest.cc:314:
ASSERT_TRUE(profile_.get()->CreateHistoryService(true, false));
On 2014/05/12 19:46:55, Peter Kasting wrote:
> There's no need to use "get()" before "->".
back to original. Done.

[Peter Kasting, 2014-05-13 17:59:28]

LGTM

https://codereview.chromium.org/271393003/diff/90001/chrome/browser/autocompl...
File chrome/browser/autocomplete/extension_app_provider.h (right):

https://codereview.chromium.org/271393003/diff/90001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.h:91: //
extensions::ExtensionRegistryObserver implementation.
Nit: Trailing colon, not period

[limasdf, 2014-05-13 18:04:05]

Thanks very much for the kind review!

https://codereview.chromium.org/271393003/diff/90001/chrome/browser/autocompl...
File chrome/browser/autocomplete/extension_app_provider.h (right):

https://codereview.chromium.org/271393003/diff/90001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.h:91: //
extensions::ExtensionRegistryObserver implementation.
On 2014/05/13 17:59:28, Peter Kasting wrote:
> Nit: Trailing colon, not period

Yep, my bad.
One question, why some kind of comment finish with colon? 
AFAIK, comment should be ended with period.

[limasdf, 2014-05-13 18:04:33]

.

https://codereview.chromium.org/271393003/diff/90001/chrome/browser/autocompl...
File chrome/browser/autocomplete/extension_app_provider.h (right):

https://codereview.chromium.org/271393003/diff/90001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.h:91: //
extensions::ExtensionRegistryObserver implementation.
On 2014/05/13 18:04:06, sunglim wrote:
> On 2014/05/13 17:59:28, Peter Kasting wrote:
> > Nit: Trailing colon, not period
> 
> Yep, my bad.
> One question, why some kind of comment finish with colon? 
> AFAIK, comment should be ended with period.

Done.

[limasdf, 2014-05-13 18:08:01]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2014-05-13 18:08:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/limasdf@gmail.com/271393003/110001

[Peter Kasting, 2014-05-13 18:15:26]

https://codereview.chromium.org/271393003/diff/90001/chrome/browser/autocompl...
File chrome/browser/autocomplete/extension_app_provider.h (right):

https://codereview.chromium.org/271393003/diff/90001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.h:91: //
extensions::ExtensionRegistryObserver implementation.
On 2014/05/13 18:04:06, sunglim wrote:
> On 2014/05/13 17:59:28, Peter Kasting wrote:
> > Nit: Trailing colon, not period
> 
> Yep, my bad.
> One question, why some kind of comment finish with colon? 
> AFAIK, comment should be ended with period.

It's basically trying to say "The following methods all come from class X:
<...>".

[limasdf, 2014-05-13 18:18:06]

yay

https://codereview.chromium.org/271393003/diff/90001/chrome/browser/autocompl...
File chrome/browser/autocomplete/extension_app_provider.h (right):

https://codereview.chromium.org/271393003/diff/90001/chrome/browser/autocompl...
chrome/browser/autocomplete/extension_app_provider.h:91: //
extensions::ExtensionRegistryObserver implementation.
On 2014/05/13 18:15:26, Peter Kasting wrote:
> On 2014/05/13 18:04:06, sunglim wrote:
> > On 2014/05/13 17:59:28, Peter Kasting wrote:
> > > Nit: Trailing colon, not period
> > 
> > Yep, my bad.
> > One question, why some kind of comment finish with colon? 
> > AFAIK, comment should be ended with period.
> 
> It's basically trying to say "The following methods all come from class X:
> <...>".

good notation. thanks!

[commit-bot: I haz the power, 2014-05-13 22:11:11]

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...)

[commit-bot: I haz the power, 2014-05-14 00:35:41]

Change committed as 270268

[limasdf, 2014-05-15 18:31:30]

A revert of this CL has been created in
https://codereview.chromium.org/290043003/ by limasdf@gmail.com.

The reason for reverting is: Crash is reported.
See https://code.google.com/p/chromium/issues/detail?id=373624.