Introduce an extension parameter which is used to customize the extension. It's available for exter…

Introduce an extension parameter which is used to customize the extension. It's available for external extensions on Windows by using a registry key. The installer can set this parameter per user.

BUG=267510
TBR=nkostylev@chromium.org,estade@chromium.org

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

[vasilii, 2014-03-12 19:23:20]

Hi guys,

please review the CL.

[not at google - send to devlin, 2014-03-13 03:10:26]

I'm OOO for the next couple of days, Yoyo would you be able to review this?

[Finnur, 2014-03-13 16:37:51]

I got in late in the meeting where this was discussed, so I'm not entirely in on
the concept we're dealing with here. Couple of questions:

What is the purpose of the InstallParameter?
How are extension developers supposed to use it?
On what platforms do you expect it to work?

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.cc:189: std::string extension_id
= extension->id();
Out of curiosity... What necessitates the const removal?

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.cc:194: // Reenable the
extension if needed
nit:
s/Reenable/Re-enable/
... and end the comment with period.

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.cc:199:
service->ReloadExtension(extension_id);
Maybe document why this is needed?

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_browsertest.h (right):

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.h:93: const std::string&
install_param);
I think we should add comments for at least these two last functions...

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_provider_impl.h (right):

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/external_provider_impl.h:67: static const char
kInstallParam[];
nit: Umm... Alphabetical ordering? :)

https://codereview.chromium.org/196663003/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/settings_override/manifest.json (right):

https://codereview.chromium.org/196663003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/settings_override/manifest.json:7: "homepage":
"http://www.homepage.com/__PARAM__",
Why is the param part of the path and not the query like with the other params
(except startup pages)?

[vasilii, 2014-03-14 10:54:57]

The InstallParameter allows to customize extension per user. It's supposed to be
used by installers of external extensions. Extension developers can use it to
differentiate users. Given that we have "1 extension = 1 purpose" policy, it's
the only available mechanism. Currently the API is available for Win only. We
may extend it to other platforms in the future.

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.cc:189: std::string extension_id
= extension->id();
On 2014/03/13 16:37:51, Finnur wrote:
> Out of curiosity... What necessitates the const removal?

I wanted to remove the reference here..

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.cc:194: // Reenable the
extension if needed
On 2014/03/13 16:37:51, Finnur wrote:
> nit:
> s/Reenable/Re-enable/
> ... and end the comment with period.

Done.

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.cc:199:
service->ReloadExtension(extension_id);
On 2014/03/13 16:37:51, Finnur wrote:
> Maybe document why this is needed?

Done.

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_browsertest.h (right):

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.h:93: const std::string&
install_param);
On 2014/03/13 16:37:51, Finnur wrote:
> I think we should add comments for at least these two last functions...

Done.

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_provider_impl.h (right):

https://codereview.chromium.org/196663003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/external_provider_impl.h:67: static const char
kInstallParam[];
On 2014/03/13 16:37:51, Finnur wrote:
> nit: Umm... Alphabetical ordering? :)

They were before renaming ;-)

https://codereview.chromium.org/196663003/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/settings_override/manifest.json (right):

https://codereview.chromium.org/196663003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/settings_override/manifest.json:7: "homepage":
"http://www.homepage.com/__PARAM__",
On 2014/03/13 16:37:51, Finnur wrote:
> Why is the param part of the path and not the query like with the other params
> (except startup pages)?

Done.

[Finnur, 2014-03-14 12:11:08]

The bug you point to is restricted so I think it would be good to list in the CL
description how you expect people to use this and either change the doc files
for this API or the deployment doc
(http://developer.chrome.com/extensions/external_extensions) to show this new
option. If we plan on extending this to more APIs then the latter is probably
the right place. Otherwise, the former, I suspect.

https://codereview.chromium.org/196663003/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/settings_override/manifest.json (right):

https://codereview.chromium.org/196663003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/settings_override/manifest.json:7: "homepage":
"http://www.homepage.com/__PARAM__",
Did you forget to update the CL?

https://codereview.chromium.org/196663003/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/settings_override/manifest.json (right):

https://codereview.chromium.org/196663003/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/settings_override/manifest.json:22:
"http://www.noo.__MSG_url_domain__/s?q={searchTerms}&id=__PARAM__"
I'm a little worried about the inflexibility of this param. For example: |id| is
super generic so you are very likely to have conflicts with what the service
that it points to already defines as its params. Imagine if Google Search, or
whatever search engine that wants to use this, already defines id= to mean
something else. It might not be easy for them to change their API to make room
for this param you are introducing...

Instead of hard-coding 'id', why not make the developers specify a pair, such as
"key|value" so that they can control both the param name and its value?

[battre, 2014-03-14 12:13:50]

https://codereview.chromium.org/196663003/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/settings_override/manifest.json (right):

https://codereview.chromium.org/196663003/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/settings_override/manifest.json:22:
"http://www.noo.__MSG_url_domain__/s?q={searchTerms}&id=__PARAM__"
On 2014/03/14 12:11:09, Finnur wrote:
> I'm a little worried about the inflexibility of this param. For example: |id|
is
> super generic so you are very likely to have conflicts with what the service
> that it points to already defines as its params. Imagine if Google Search, or
> whatever search engine that wants to use this, already defines id= to mean
> something else. It might not be easy for them to change their API to make room
> for this param you are introducing...
> 
> Instead of hard-coding 'id', why not make the developers specify a pair, such
as
> "key|value" so that they can control both the param name and its value?

I think that __PARAM__ can be an arbitrary string (like "id=foobar"). I don't
see that structuring this is necessary.

[Finnur, 2014-03-14 14:19:47]

The __PARAM__ isn't what I'm worried about, but after reading your reply again I
think I get what you meant to say, which is that id= part is not hard-coded. If
that's the case then fine.

[vasilii, 2014-03-14 14:30:24]

'id' isn't hardcoded. The only hardcoded placeholder is '__PARAM__'.

https://codereview.chromium.org/196663003/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/settings_override/manifest.json (right):

https://codereview.chromium.org/196663003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/settings_override/manifest.json:7: "homepage":
"http://www.homepage.com/__PARAM__",
On 2014/03/14 12:11:09, Finnur wrote:
> Did you forget to update the CL?

Done.

[Finnur, 2014-03-14 16:48:42]

LGTM

[vasilii, 2014-03-14 17:13:14]

Yoyo, can you review?

[vasilii, 2014-03-17 18:53:22]

Yoyo, friendly ping.

[Yoyo Zhou, 2014-03-18 00:07:50]

Looks mostly okay, but is there a test that the install parameter is actually
available to the extension? I don't see one in this CL.

https://chromiumcodereview.appspot.com/196663003/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://chromiumcodereview.appspot.com/196663003/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_browsertest.cc:200: // observers may access
|install_param|.
I'm struggling to understand the reason for this. Can we call SetInstallParam
earlier, say at line 153?

[vasilii, 2014-03-18 10:01:22]

The install parameter isn't available to the extension. It's used to change the
manifest only.

https://codereview.chromium.org/196663003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://codereview.chromium.org/196663003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_browsertest.cc:200: // observers may access
|install_param|.
On 2014/03/18 00:07:51, Yoyo Zhou wrote:
> I'm struggling to understand the reason for this. Can we call SetInstallParam
> earlier, say at line 153?

We don't know its id yet. It'd be ideal if we could install the extension, set
the parameter and then load it.

[vasilii, 2014-03-20 18:17:23]

Yoyo, do you have more comments?

[Yoyo Zhou, 2014-03-20 18:19:39]

LGTM, sorry for the delay.

[vasilii, 2014-03-20 18:29:11]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-03-20 18:32:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/196663003/40001

[commit-bot: I haz the power, 2014-03-20 18:33:18]

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

[commit-bot: I haz the power, 2014-03-20 18:33:19]

Failed to apply patch for extensions/browser/pending_extension_info.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: extensions/browser/pending_extension_info.cc
  |diff --git a/extensions/browser/pending_extension_info.cc
b/extensions/browser/pending_extension_info.cc
  |index
0abcdf61c85fe61c3bd6309b17fc8d1143c1cbce..d1503b9ae09f80c12660afaca12100180918ffd5
100644
  |--- a/extensions/browser/pending_extension_info.cc
  |+++ b/extensions/browser/pending_extension_info.cc
  --------------------------
  No file to patch.  Skipping patch.
  3 out of 3 hunks ignored

Patch:       extensions/browser/pending_extension_info.cc
Index: extensions/browser/pending_extension_info.cc
diff --git a/extensions/browser/pending_extension_info.cc
b/extensions/browser/pending_extension_info.cc
index
0abcdf61c85fe61c3bd6309b17fc8d1143c1cbce..d1503b9ae09f80c12660afaca12100180918ffd5
100644
--- a/extensions/browser/pending_extension_info.cc
+++ b/extensions/browser/pending_extension_info.cc
@@ -10,6 +10,7 @@ namespace extensions {
 
 PendingExtensionInfo::PendingExtensionInfo(
     const std::string& id,
+    const std::string& install_parameter,
     const GURL& update_url,
     const Version& version,
     ShouldAllowInstallPredicate should_allow_install,
@@ -21,6 +22,7 @@ PendingExtensionInfo::PendingExtensionInfo(
     : id_(id),
       update_url_(update_url),
       version_(version),
+      install_parameter_(install_parameter),
       should_allow_install_(should_allow_install),
       is_from_sync_(is_from_sync),
       install_silently_(install_silently),
@@ -35,6 +37,8 @@ PendingExtensionInfo::PendingExtensionInfo()
       install_silently_(false),
       install_source_(Manifest::INVALID_LOCATION) {}
 
+PendingExtensionInfo::~PendingExtensionInfo() {}
+
 bool PendingExtensionInfo::operator==(const PendingExtensionInfo& rhs) const {
   return id_ == rhs.id_;
 }

[vasilii, 2014-03-21 11:35:38]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-03-21 11:35:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/196663003/80001

[commit-bot: I haz the power, 2014-03-21 12:44:33]

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

[commit-bot: I haz the power, 2014-03-21 12:44:33]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[battre, 2014-03-21 16:50:55]

The CQ bit was checked by battre@chromium.org

[commit-bot: I haz the power, 2014-03-21 16:51:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/196663003/80001

[vasilii, 2014-03-21 17:01:58]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-03-21 17:02:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/196663003/100001

[commit-bot: I haz the power, 2014-03-21 17:05:36]

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

[commit-bot: I haz the power, 2014-03-21 17:05:42]

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

[battre, 2014-03-21 17:07:23]

The CQ bit was checked by battre@chromium.org

[commit-bot: I haz the power, 2014-03-21 17:08:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/196663003/100001

[commit-bot: I haz the power, 2014-03-21 17:10:48]

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

[commit-bot: I haz the power, 2014-03-21 17:10:49]

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

[vasilii, 2014-03-21 17:19:54]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-03-21 17:20:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/196663003/100001

[commit-bot: I haz the power, 2014-03-21 17:23:39]

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

[commit-bot: I haz the power, 2014-03-21 17:23:39]

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

[vasilii, 2014-03-24 09:42:51]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-03-24 09:42:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/196663003/100001

[commit-bot: I haz the power, 2014-03-24 09:44:29]

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

[commit-bot: I haz the power, 2014-03-24 09:44:30]

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

[vasilii, 2014-03-24 13:15:12]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-03-24 13:15:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/196663003/100001

[commit-bot: I haz the power, 2014-03-24 13:16:09]

Change committed as 258919