Small refactoring extension_toolbar_model.cc

This CL does small refactoring extension_toolbar_model.cc before removing NOTIFICATION_EXTENSION_UNLOADED and NOTIFICATION_EXTENSION_LOADED.

BUG=354046
TEST=browser_tests --gtest_filter=ExtensionToolbarModelTest.*

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

[limasdf, 2014-04-25 14:44:02]

could you take a look? before removing
OTIFICATION_EXTENSION_UNLOADED_DEPRECATED, I did small refactoring.

[not at google - send to devlin, 2014-04-25 16:53:34]

Nice!

https://codereview.chromium.org/256783003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/256783003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:189: ExtensionService*
extension_service =
it looks like this whole function early-exits if there's no extension service.
that shouldn't even really be happening since it's extension service that sends
these notifications, but my point is, this refactoring does change the existing
logic.

however, this class doesn't even need an extension service, just an extension
registry, apart from that "is_ready" check, and that you can get from here:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

so I'd make that your refactor here.
- what you have
- replace ES usage with ER and ready() and take it back to the start of Observe

[not at google - send to devlin, 2014-04-25 16:54:33]

https://codereview.chromium.org/256783003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/256783003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:189: ExtensionService*
extension_service =
On 2014/04/25 16:53:34, kalman wrote:
> it looks like this whole function early-exits if there's no extension service.
> that shouldn't even really be happening since it's extension service that
sends
> these notifications, but my point is, this refactoring does change the
existing
> logic.
> 
> however, this class doesn't even need an extension service, just an extension
> registry, apart from that "is_ready" check, and that you can get from here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> 
> so I'd make that your refactor here.
> - what you have
> - replace ES usage with ER and ready() and take it back to the start of
Observe

another option: just pull the ES back to the start, don't use ER yet, but
DCHECK(ES) rather than guarding against it being null.

[limasdf, 2014-04-25 17:42:54]

Thanks for the review as always! :)
please take a look my feedback.

And perhaps,
ER = ExtensionRegistry.
ES = ExtensionSystem.   ??

https://codereview.chromium.org/256783003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/256783003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.cc:189: ExtensionService*
extension_service =
On 2014/04/25 16:54:34, kalman wrote:
> On 2014/04/25 16:53:34, kalman wrote:
> > it looks like this whole function early-exits if there's no extension
service.
> > that shouldn't even really be happening since it's extension service that
> sends
> > these notifications, but my point is, this refactoring does change the
> existing
> > logic.
> > 
> > however, this class doesn't even need an extension service, just an
extension
> > registry, apart from that "is_ready" check, and that you can get from here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> > 
> > so I'd make that your refactor here.
> > - what you have
> > - replace ES usage with ER and ready() and take it back to the start of
> Observe
> 
> another option: just pull the ES back to the start, don't use ER yet, but
> DCHECK(ES) rather than guarding against it being null.

I chose second method to keep exisitng code as much as I can.
Done.

[not at google - send to devlin, 2014-04-25 20:17:46]

yes that's what I meant by those acronyms :)

lgtm

could you attach a BUG though.

[limasdf, 2014-04-25 20:28:01]

On 2014/04/25 20:17:46, kalman wrote:
> yes that's what I meant by those acronyms :)
> 
> lgtm
> 
> could you attach a BUG though.

hehe.. Thank you. I'm happy that I can run build bot before I go to bed :)
BUG is attached.

[limasdf, 2014-04-25 20:44:55]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2014-04-25 22:04:23]

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

[commit-bot: I haz the power, 2014-04-25 22:55:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 22:55:40]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[limasdf, 2014-04-26 02:32:27]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2014-04-26 02:33:27]

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

[commit-bot: I haz the power, 2014-04-26 10:06:45]

Change committed as 266349

[limasdf, 2014-04-26 13:52:51]

@Kalman, I have one dumb question.
ExtensionSystem and ExtensionRegistry they both has similar function which is
checking extension is loaded. (is_ready() and ready()). 
how about removing one of them?