Added enhanced bookmarks extension as an external component extension

Added enhanced bookmarks extension as an external component extension

BUG=321393
TBR=cpu@chromium.org

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

[yefimt, 2013-11-20 00:39:52]

Please take a look
not sure if I got all owners, I'll add if needed later

[Yaron, 2013-11-20 01:40:04]

https://codereview.chromium.org/68173031/diff/30001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/68173031/diff/30001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6980: +        Disable Enhanced Bookmarks
Extension
Maybe something like: Provides an off switch if opted into the enhanced
bookmarks experiment?

https://codereview.chromium.org/68173031/diff/30001/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/68173031/diff/30001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:1835: "disable-enhanced-bookmarks",
Seems like this would show always though. Can we restrict it to when you're in
the experiment?

[Finnur, 2013-11-20 13:55:14]

I don't have access to the design doc in the bug.

Are there any tests for this extension?

https://codereview.chromium.org/68173031/diff/30001/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/68173031/diff/30001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:1840: },
I think we should strive towards having only enable flags here because disable
flags in settings are confusing, mainly when you want to enable something:
Instead of checking 'Enable' you un-check Disable, which is a double negative.

[Yaron, 2013-11-20 18:21:09]

On 2013/11/20 13:55:14, Finnur wrote:
> I don't have access to the design doc in the bug.
> 
> Are there any tests for this extension?

Yes, the extension codebase has lots of tests

> 
>
https://codereview.chromium.org/68173031/diff/30001/chrome/browser/about_flag...
> File chrome/browser/about_flags.cc (right):
> 
>
https://codereview.chromium.org/68173031/diff/30001/chrome/browser/about_flag...
> chrome/browser/about_flags.cc:1840: },
> I think we should strive towards having only enable flags here because disable
> flags in settings are confusing, mainly when you want to enable something:
> Instead of checking 'Enable' you un-check Disable, which is a double negative.

Doc should be visible to you now. I'm fine with it having the "enable" sense
defaulted to "enable" and you can disable it to go back.

[yefimt, 2013-11-21 22:40:39]

https://codereview.chromium.org/68173031/diff/30001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/68173031/diff/30001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6980: +        Disable Enhanced Bookmarks
Extension
On 2013/11/20 01:40:05, Yaron wrote:
> Maybe something like: Provides an off switch if opted into the enhanced
> bookmarks experiment?

Done.

https://codereview.chromium.org/68173031/diff/30001/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/68173031/diff/30001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:1835: "disable-enhanced-bookmarks",
On 2013/11/20 01:40:05, Yaron wrote:
> Seems like this would show always though. Can we restrict it to when you're in
> the experiment? 

Done.

https://codereview.chromium.org/68173031/diff/30001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:1840: },
On 2013/11/20 13:55:15, Finnur wrote:
> I think we should strive towards having only enable flags here because disable
> flags in settings are confusing, mainly when you want to enable something:
> Instead of checking 'Enable' you un-check Disable, which is a double negative.

Done.

[Yaron, 2013-11-21 23:47:00]

https://codereview.chromium.org/68173031/diff/130001/chrome/browser/extension...
File chrome/browser/extensions/external_component_loader.cc (right):

https://codereview.chromium.org/68173031/diff/130001/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.cc:35: Feature* feature =
feature_provider->GetFeature("metricsPrivate");
Actually, in thinking about this, I don't know if this really addresses your
concern about using a command line flag to force an experiment which downloads
an extension. There's another command line flag which allows you to add your
experiment to all whitelists so this doesn't really help. I think the first step
is to see if we need to worry or if we have sufficient control over finch
experiments to see if this is an actual issue

[yefimt, 2013-11-21 23:50:53]

https://codereview.chromium.org/68173031/diff/130001/chrome/browser/extension...
File chrome/browser/extensions/external_component_loader.cc (right):

https://codereview.chromium.org/68173031/diff/130001/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.cc:35: Feature* feature =
feature_provider->GetFeature("metricsPrivate");
Command line flag should work, it will get parameters (like id) from finch, the
only problem I found that chrome always need to be run with flag in command
line, not just one time. Removing flag from command line will remove extension.

On 2013/11/21 23:47:01, Yaron wrote:
> Actually, in thinking about this, I don't know if this really addresses your
> concern about using a command line flag to force an experiment which downloads
> an extension. There's another command line flag which allows you to add your
> experiment to all whitelists so this doesn't really help. I think the first
step
> is to see if we need to worry or if we have sufficient control over finch
> experiments to see if this is an actual issue

[Finnur, 2013-11-22 10:31:57]

https://codereview.chromium.org/68173031/diff/130001/chrome/browser/extension...
File chrome/browser/extensions/external_component_loader.h (right):

https://codereview.chromium.org/68173031/diff/130001/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.h:25: static bool
IsEnhancedBookmarksExperimentEnabled();
It feels a bit hacky to have this be part of the ExternalComponentLoader. Is
there no .cc file associated with the enhanced bookmark code that can own this
function?

https://codereview.chromium.org/68173031/diff/130001/chrome/common/extensions...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/68173031/diff/130001/chrome/common/extensions...
chrome/common/extensions/api/_permission_features.json:525:
"D57DE394F36DC1C3220E7604C575D29C51A6C495"  // http://crbug.com/319444
nit: Normally, I'd also ask you to end all comments with a period, but since
this file is full of non-compliance on that I'll let you off the hook. But one
more indentation space needed for comments.

[yefimt, 2013-11-22 19:01:42]

https://codereview.chromium.org/68173031/diff/130001/chrome/browser/extension...
File chrome/browser/extensions/external_component_loader.h (right):

https://codereview.chromium.org/68173031/diff/130001/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.h:25: static bool
IsEnhancedBookmarksExperimentEnabled();
I feel the same, but the problem is all code is in extension. So I could not
find better place.
If it helps in any way, this code will go away some time later because
Enh.bookmarks will be enabled for all users or removed completely.

On 2013/11/22 10:31:57, Finnur wrote:
> It feels a bit hacky to have this be part of the ExternalComponentLoader. Is
> there no .cc file associated with the enhanced bookmark code that can own this
> function?

https://codereview.chromium.org/68173031/diff/130001/chrome/common/extensions...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/68173031/diff/130001/chrome/common/extensions...
chrome/common/extensions/api/_permission_features.json:525:
"D57DE394F36DC1C3220E7604C575D29C51A6C495"  // http://crbug.com/319444
On 2013/11/22 10:31:57, Finnur wrote:
> nit: Normally, I'd also ask you to end all comments with a period, but since
> this file is full of non-compliance on that I'll let you off the hook. But one
> more indentation space needed for comments. 

Done.

[Finnur, 2013-11-22 19:04:48]

LGTM, with one nit.

https://codereview.chromium.org/68173031/diff/130001/chrome/browser/extension...
File chrome/browser/extensions/external_component_loader.h (right):

https://codereview.chromium.org/68173031/diff/130001/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.h:25: static bool
IsEnhancedBookmarksExperimentEnabled();
Can you document this function and the fact that it is temporary until we enable
this experiment for all or remove it?

[yefimt, 2013-11-22 22:53:15]

+cpu@ for chrome/app/generated_resources.grd

[cpu_(ooo_6.6-7.5), 2013-11-23 02:59:40]

see the owners file, you can just tbr.

[Yaron, 2013-11-25 23:49:33]

lgtm

I assume you'll have a separate CL to actually disable the extension if the flag
is unset? I don't see that here, it just looks like you wouldn't download
updates.

+palmer from security pov. Happy to discuss further or offline if needed

https://codereview.chromium.org/68173031/diff/300002/chrome/browser/extension...
File chrome/browser/extensions/external_component_loader.cc (right):

https://codereview.chromium.org/68173031/diff/300002/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.cc:24: // This functione
will be removed after experiment goes to all users or go away.
s/functione/function

https://codereview.chromium.org/68173031/diff/300002/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/68173031/diff/300002/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:332: const char kDisableDnsProbes[]            
 = "disable-dns-probes";
Merge error?

[yefimt, 2013-11-26 00:14:47]

https://codereview.chromium.org/68173031/diff/300002/chrome/browser/extension...
File chrome/browser/extensions/external_component_loader.cc (right):

https://codereview.chromium.org/68173031/diff/300002/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.cc:24: // This functione
will be removed after experiment goes to all users or go away.
On 2013/11/25 23:49:33, Yaron wrote:
> s/functione/function

Done.

https://codereview.chromium.org/68173031/diff/300002/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/68173031/diff/300002/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:332: const char kDisableDnsProbes[]            
 = "disable-dns-probes";
Yes, thank you
On 2013/11/25 23:49:33, Yaron wrote:
> Merge error?

[commit-bot: I haz the power, 2013-11-26 00:15:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yefim@chromium.org/68173031/570001

[Yaron, 2013-11-26 01:00:07]

+meacer per palmer's request

[palmer, 2013-11-26 01:03:38]

https://codereview.chromium.org/68173031/diff/300002/chrome/browser/extension...
File chrome/browser/extensions/external_component_loader.cc (right):

https://codereview.chromium.org/68173031/diff/300002/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.cc:37: return (feature &&
feature->IsIdInWhitelist(app_id));
Nit: don't need outer parentheses

[meacer, 2013-11-26 01:32:03]

Lgtm

https://codereview.chromium.org/68173031/diff/570001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/68173031/diff/570001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6985: +        Provides an off switch for
enhanced bookmarks experiment
Nit: "Provides an off switch" seems to be a remainder from the initial patch
where the flag disabled the experiment.
Maybe "Provides an on/off switch"?

https://codereview.chromium.org/68173031/diff/570001/chrome/browser/extension...
File chrome/browser/extensions/external_component_loader.cc (right):

https://codereview.chromium.org/68173031/diff/570001/chrome/browser/extension...
chrome/browser/extensions/external_component_loader.cc:51:
prefs_->SetString(app_id + ".external_update_url",
Can we restrict the update url to Web Store for all external components in
addition to this? Since now external components aren't going to be listed in the
extensions page, it would be good if we can make sure they are only updated
through the Web Store, as a hardening mechanism.

[commit-bot: I haz the power, 2013-11-26 03:06:43]

Change committed as 237229