[Hotword, Extensions] Removing EXTERNAL_COMPONENT from IsPolicyLocation, adding a function to check…

[Hotword, Extensions] Removing EXTERNAL_COMPONENT from IsPolicyLocation, adding a function to check if an extension is a component extension, and updating places IsPolicyLocation was used.

This is necessary for the correct look for hotwording.

BUG=341611, 325439, 289023
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250614

[rpetterson, 2014-02-06 23:23:26]

atwilson@chromium.org: Please review changes in browser/background

mek@chromium.org: Please review changes in extensions/*

Marijn, the one instance I wasn't sure of was in extension_service.cc line 2185
where IsPolicyLocation is used to determine if the extension should be
Acknowledged. I'm not sure what purpose Acknowledgement of the service serves so
I wasn't sure how or if to modify that call.

[Marijn Kruisselbrink, 2014-02-06 23:37:08]

On 2014/02/06 23:23:26, rpetterson wrote:
> Marijn, the one instance I wasn't sure of was in extension_service.cc line
2185
> where IsPolicyLocation is used to determine if the extension should be
> Acknowledged. I'm not sure what purpose Acknowledgement of the service serves
so
> I wasn't sure how or if to modify that call.
The Acknowledgement part almost certainly is required for EXTERNAL_COMPONENT to
work correctly. I would expect that without it the
ExternalProviderImplTest.InAppPayments unit test would even fail.

https://codereview.chromium.org/156153005/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_contents_service.cc (right):

https://codereview.chromium.org/156153005/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_contents_service.cc:436:
extension->location() == extensions::Manifest::EXTERNAL_COMPONENT ||
I'm not entirely sure if this should be included here, and if it should be than
I can't think of a reason why regular COMPONENT extensions shouldn't also
auto-restart...

[rpetterson, 2014-02-07 00:23:38]

On 2014/02/06 23:37:08, Marijn Kruisselbrink wrote:
> On 2014/02/06 23:23:26, rpetterson wrote:
> > Marijn, the one instance I wasn't sure of was in extension_service.cc line
> 2185
> > where IsPolicyLocation is used to determine if the extension should be
> > Acknowledged. I'm not sure what purpose Acknowledgement of the service
serves
> so
> > I wasn't sure how or if to modify that call.
> The Acknowledgement part almost certainly is required for EXTERNAL_COMPONENT
to
> work correctly. I would expect that without it the
> ExternalProviderImplTest.InAppPayments unit test would even fail.

Surprisingly, it did not. But I've updated that check to include
EXTERNAL_COMPONENT as well.

[rpetterson, 2014-02-07 00:23:51]

https://codereview.chromium.org/156153005/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_contents_service.cc (right):

https://codereview.chromium.org/156153005/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_contents_service.cc:436:
extension->location() == extensions::Manifest::EXTERNAL_COMPONENT ||
On 2014/02/06 23:37:09, Marijn Kruisselbrink wrote:
> I'm not entirely sure if this should be included here, and if it should be
than
> I can't think of a reason why regular COMPONENT extensions shouldn't also
> auto-restart...

Seems reasonable. I've updated it to include both types of component extensions.

[rpetterson, 2014-02-10 17:52:16]

Monday morning ping.

[Marijn Kruisselbrink, 2014-02-10 19:51:14]

lgtm

[Andrew T Wilson (Slow), 2014-02-11 15:56:31]

LGTM with one comment/question

https://codereview.chromium.org/156153005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/156153005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:2186: extension->location() ==
Manifest::EXTERNAL_COMPONENT)
Just making sure that it's intentional that you aren't using
IsComponentLocation() here...

[rpetterson, 2014-02-11 18:34:17]

https://codereview.chromium.org/156153005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/156153005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:2186: extension->location() ==
Manifest::EXTERNAL_COMPONENT)
On 2014/02/11 15:56:32, Andrew T Wilson wrote:
> Just making sure that it's intentional that you aren't using
> IsComponentLocation() here...

Yes this is intentional. I'm keeping the same behavior that was previously
there. Previously IsPolicyLocation included external components but not regular
components which IsComponentLocation includes. The response to this case is to
AcknowledgeExternalExtensions and my understanding is that regular component
extensions aren't considered external.

[rpetterson, 2014-02-11 23:17:43]

The CQ bit was checked by rlp@chromium.org

[commit-bot: I haz the power, 2014-02-11 23:18:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/156153005/60001

[commit-bot: I haz the power, 2014-02-12 02:21:38]

Change committed as 250614