Disable caching "Chrome Hotword Shared Module" extension (temporary).

Disable caching "Chrome Hotword Shared Module" extension (temporary).

BUG=447077
Committed: https://crrev.com/643dd0dab732ccfc1a1930d38fc30727a63ccfd6
Cr-Commit-Position: refs/heads/master@{#311707}

[ginkage, 2015-01-15 05:47:53]

ginkage@google.com changed reviewers:
+ dpolukhin@chromium.org

[ginkage, 2015-01-15 05:48:45]

ginkage@google.com changed reviewers:
+ ginkage@google.com

[ginkage, 2015-01-15 05:48:45]

Should it look like this?

[Dmitry Polukhin, 2015-01-15 09:40:38]

LGTM

In case you haven't found it: add command line switch
--force-fieldtrials="VoiceTrigger/Experimental/" to make this extensions
EXTERNAL_COMPONENT otherwise it won't be cached.

https://codereview.chromium.org/832683004/diff/1/chrome/browser/extensions/up...
File chrome/browser/extensions/updater/extension_cache_impl.cc (right):

https://codereview.chromium.org/832683004/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/extension_cache_impl.cc:25: // TODO: Implement
id/hash-based map instead of id/version.
Please use TODO(ginkage)... as in style guide and move it to AllowCaching.

https://codereview.chromium.org/832683004/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/extension_cache_impl.cc:26: const char
kBlacklistedExt[] = "lccekmodgklaepjeofjdjpbminllajkg";
This constant already exists kHotwordSharedModuleId please use it instead.

https://codereview.chromium.org/832683004/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/extension_cache_impl.cc:87: if (cache_ && id
!= kBlacklistedExt)
I would check if caching allowed like this 'ContainsKey(allowed_extensions_,
id)' or even make this check separate function to call it from PutExtension and
GetExtension.

[ginkage, 2015-01-15 15:02:24]

Shuffled around a bit.

https://codereview.chromium.org/832683004/diff/1/chrome/browser/extensions/up...
File chrome/browser/extensions/updater/extension_cache_impl.cc (right):

https://codereview.chromium.org/832683004/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/extension_cache_impl.cc:87: if (cache_ && id
!= kBlacklistedExt)
On 2015/01/15 09:40:38, Dmitry Polukhin wrote:
> I would check if caching allowed like this 'ContainsKey(allowed_extensions_,
> id)' or even make this check separate function to call it from PutExtension
and
> GetExtension.

I'm not really sure: I've added this check for cases when the extension is
already in the cache, but we should not use it (like, after a system upgrade).
Otherwise, AllowCaching check is sufficient.

[Dmitry Polukhin, 2015-01-15 15:07:48]

lgtm

https://codereview.chromium.org/832683004/diff/1/chrome/browser/extensions/up...
File chrome/browser/extensions/updater/extension_cache_impl.cc (right):

https://codereview.chromium.org/832683004/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/extension_cache_impl.cc:87: if (cache_ && id
!= kBlacklistedExt)
On 2015/01/15 15:02:24, ginkage wrote:
> On 2015/01/15 09:40:38, Dmitry Polukhin wrote:
> > I would check if caching allowed like this 'ContainsKey(allowed_extensions_,
> > id)' or even make this check separate function to call it from PutExtension
> and
> > GetExtension.
> 
> I'm not really sure: I've added this check for cases when the extension is
> already in the cache, but we should not use it (like, after a system upgrade).
> Otherwise, AllowCaching check is sufficient.

Yes, check in GetExtension required to recover if wrong variant is already in
the cache. Repeating check in GetExtension should generalize cases like when
extension shouldn't be cache and used from cache in any case.

[ginkage, 2015-01-15 16:23:55]

ginkage@google.com changed reviewers:
+ asargent@chromium.org

[ginkage, 2015-01-15 16:23:55]

asargent@, please owner review.

[asargent_no_longer_on_chrome, 2015-01-15 17:39:48]

lgtm

[ginkage, 2015-01-15 18:00:35]

The CQ bit was checked by ginkage@google.com

[commit-bot: I haz the power, 2015-01-15 18:02:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/832683004/20001

[commit-bot: I haz the power, 2015-01-15 18:58:01]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-15 18:59:55]

Patchset 2 (id:??) landed as
https://crrev.com/643dd0dab732ccfc1a1930d38fc30727a63ccfd6
Cr-Commit-Position: refs/heads/master@{#311707}

[Ivan Podogov, 2015-02-03 10:30:43]

ginkage@chromium.org changed reviewers:
- ginkage@google.com