Allow explicitly whitelisted apps/extensions in public sessions

Allow explicitly whitelisted apps/extensions in public sessions

This CL adds an extension management policy provider that allows
explicitly whitelisted apps/extensions to be installed in public sessions.
Right now, QuickOffice and all hosted apps are whitelisted.

BUG=296868
TEST=New browser and unit tests

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

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

[bartfab (slow), 2013-09-23 11:53:47]

Hi Anthony,

Could you please review this CL?

Hi Mattias,

Could you please review the policy part of this CL?

[Mattias Nissler (ping if slow), 2013-09-23 12:53:29]

https://chromiumcodereview-hr.appspot.com/24261010/diff/1/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview-hr.appspot.com/24261010/diff/1/chrome/app/generate...
chrome/app/generated_resources.grd:4302: +      <message
name="IDS_EXTENSION_CANT_INSTALL_IN_PUBLIC_SESSION" desc="Error message when a
user tries to install or the administrator tries to force-install through policy
an extension tjat is not allowed in a public session.">
*that

https://chromiumcodereview-hr.appspot.com/24261010/diff/1/chrome/browser/chro...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://chromiumcodereview-hr.appspot.com/24261010/diff/1/chrome/browser/chro...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:121: class
ExtensionInstallSuccessObserver
Is there any good reasons to make these classes if you can just base::Bind
CheckNotification?

https://chromiumcodereview-hr.appspot.com/24261010/diff/1/chrome/browser/chro...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:144: const
string16 id_;
Why is this a string16, while it is a std::string in line 130?

https://chromiumcodereview-hr.appspot.com/24261010/diff/1/chrome/browser/chro...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:187:
scoped_ptr<net::test_server::HttpResponse> HandleRequest(
I guess it would be good to factor this out so other tests that need to serve
update manifests can use the same code. Speaking of which: Are you sure this
doesn't exist somewhere else already?

https://chromiumcodereview-hr.appspot.com/24261010/diff/1/chrome/browser/chro...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:190: return
scoped_ptr<net::test_server::HttpResponse>();
nit: blank line here for readability

https://chromiumcodereview-hr.appspot.com/24261010/diff/1/chrome/browser/chro...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:193: while
((id_pos = request.relative_url.find("id%3D", id_pos)) !=
Can you use proper URL parsing, for GetValueForKeyInQuery in
net/base/url_util.h?

Also not that there is switches::kAppsGalleryURL, which can potentially make all
this significantly easier.

https://chromiumcodereview-hr.appspot.com/24261010/diff/1/chrome/browser/chro...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:205: content
+= ReplaceStringPlaceholders(
base::StringPrintf is probably simpler to use here.

https://chromiumcodereview-hr.appspot.com/24261010/diff/1/chrome/browser/chro...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:567:
forcelist->add_entries(ReplaceStringPlaceholders("$1;$2", parts, NULL));
StringPrintf is simpler

[bartfab (slow), 2013-09-23 17:03:05]

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/app/generated_r...
chrome/app/generated_resources.grd:4302: +      <message
name="IDS_EXTENSION_CANT_INSTALL_IN_PUBLIC_SESSION" desc="Error message when a
user tries to install or the administrator tries to force-install through policy
an extension tjat is not allowed in a public session.">
On 2013/09/23 12:53:29, Mattias Nissler wrote:
> *that

Done.

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:121: class
ExtensionInstallSuccessObserver
On 2013/09/23 12:53:29, Mattias Nissler wrote:
> Is there any good reasons to make these classes if you can just base::Bind
> CheckNotification?

CheckNotification() needs to access the NotificationDetails. This is not
normally possible from a WindowedNotificationObserver callback because the
callback has no pointer to the WindowedNotificationObserver it belongs to. I
could instantiate a callback with a pointer to a pointer argument, instantiate a
WindowedNotificationObserver that uses the callback and then inject a pointer to
the WindowedNotificationObserver via the argument from the first step. But I
think that this is clumsy and error-prone.

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:144: const
string16 id_;
On 2013/09/23 12:53:29, Mattias Nissler wrote:
> Why is this a string16, while it is a std::string in line 130?

They listen for different notification types. An installation success provides
the extension ID as an std::string, an installation failure provides the
extension ID as a string16.

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:187:
scoped_ptr<net::test_server::HttpResponse> HandleRequest(
On 2013/09/23 12:53:29, Mattias Nissler wrote:
> I guess it would be good to factor this out so other tests that need to serve
> update manifests can use the same code. Speaking of which: Are you sure this
> doesn't exist somewhere else already?

Refactored into a reusable TestingUpdateManifestProvider.

AFAICT, there is no code that does something similar. Other tests use a fake
URLFetcherFactory and work with hard-coded manifest files in the test data
directory. I wanted to dynamically construct the manifests so that the embedded
test server can be used for the CRX files instead.

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:190: return
scoped_ptr<net::test_server::HttpResponse>();
On 2013/09/23 12:53:29, Mattias Nissler wrote:
> nit: blank line here for readability

Done.

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:193: while
((id_pos = request.relative_url.find("id%3D", id_pos)) !=
On 2013/09/23 12:53:29, Mattias Nissler wrote:
> Can you use proper URL parsing, for GetValueForKeyInQuery in
> net/base/url_util.h?
> 
> Also not that there is switches::kAppsGalleryURL, which can potentially make
all
> this significantly easier.

Re GetValueForKeyInQuery(): Unfortunately, I cannot use that. It will extract
only one value per key and the key I am looking for repeats several times in the
URL.

Re switches::kAppsGalleryURL: As discussed offline, this is not applicable here.

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:205: content
+= ReplaceStringPlaceholders(
On 2013/09/23 12:53:29, Mattias Nissler wrote:
> base::StringPrintf is probably simpler to use here.

Funny. I specifically chose ReplaceStringPlaceholders because I find
base::StringPrintf clumsy with all the c_str() it requires. But I have no strong
preference for either one. Done.

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:567:
forcelist->add_entries(ReplaceStringPlaceholders("$1;$2", parts, NULL));
On 2013/09/23 12:53:29, Mattias Nissler wrote:
> StringPrintf is simpler

Done.

[asargent_no_longer_on_chrome, 2013-09-24 20:28:59]

lgtm

https://codereview.chromium.org/24261010/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_system.cc (right):

https://codereview.chromium.org/24261010/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/extension_system.cc:140: #endif  //
defined(ENABLE_EXTENSIONS)
It seems a little unfortunate to have this #ifdef'ing going on here, as opposed
to making the concept of a device local management policy more of a first-class
citizen. I guess you aren't doing much work other than initializing a member
variable, so it's probably fine for now, but if we end up needing to put any
more logic inside the #ifdef blocks here it might be time to consider
refactoring.

[Mattias Nissler (ping if slow), 2013-09-25 12:04:08]

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:121: class
ExtensionInstallSuccessObserver
On 2013/09/23 17:03:05, bartfab wrote:
> On 2013/09/23 12:53:29, Mattias Nissler wrote:
> > Is there any good reasons to make these classes if you can just base::Bind
> > CheckNotification?
> 
> CheckNotification() needs to access the NotificationDetails. This is not
> normally possible from a WindowedNotificationObserver callback because the
> callback has no pointer to the WindowedNotificationObserver it belongs to. I
> could instantiate a callback with a pointer to a pointer argument, instantiate
a
> WindowedNotificationObserver that uses the callback and then inject a pointer
to
> the WindowedNotificationObserver via the argument from the first step. But I
> think that this is clumsy and error-prone.

Sounds like a subclass for WindowedNotificationObserver that forwards the
Details pointer to the check callback could at least reduce the number of
subclasses to only one then? 

Arguably, this should be supported by WindowedNotificationObserver directly. I
guess you could generalize it to support callbacks that receive Details and
Source, and then you can have a convenience ctor that adapts the function type.

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:144: const
string16 id_;
On 2013/09/23 17:03:05, bartfab wrote:
> On 2013/09/23 12:53:29, Mattias Nissler wrote:
> > Why is this a string16, while it is a std::string in line 130?
> 
> They listen for different notification types. An installation success provides
> the extension ID as an std::string, an installation failure provides the
> extension ID as a string16.

Doh. Does the extension team have a bug for this? If not, please file one.

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:193: while
((id_pos = request.relative_url.find("id%3D", id_pos)) !=
On 2013/09/23 17:03:05, bartfab wrote:
> On 2013/09/23 12:53:29, Mattias Nissler wrote:
> > Can you use proper URL parsing, for GetValueForKeyInQuery in
> > net/base/url_util.h?
> > 
> > Also not that there is switches::kAppsGalleryURL, which can potentially make
> all
> > this significantly easier.
> 
> Re GetValueForKeyInQuery(): Unfortunately, I cannot use that. It will extract
> only one value per key and the key I am looking for repeats several times in
the
> URL.

Ad hoc URL parsing still isn't a good idea. Note that the underlying url_parse
functionality can handle this case. Maybe we should just make a helper that
decodes a query string to a std::multimap and put that in net/base/url_util.h
for everybody to use?

> 
> Re switches::kAppsGalleryURL: As discussed offline, this is not applicable
here.

https://chromiumcodereview.appspot.com/24261010/diff/6001/chrome/browser/chro...
File
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider_unittest.cc
(right):

https://chromiumcodereview.appspot.com/24261010/diff/6001/chrome/browser/chro...
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider_unittest.cc:52:
typedef testing::Test DeviceLocalAccountManagementPolicyProviderTest;
Don't. Just use the TEST() macro.

https://chromiumcodereview.appspot.com/24261010/diff/6001/chrome/browser/chro...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://chromiumcodereview.appspot.com/24261010/diff/6001/chrome/browser/chro...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:212:
TestingUpdateManifestProvider::TestingUpdateManifestProvider(
Definition order doesn't match declaration order.

https://chromiumcodereview.appspot.com/24261010/diff/6001/chrome/browser/chro...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:215: const
ExtensionMap& extension_map)
Instead of passing in the map, can we just make an AddExtension() call on the
provider? Makes it easier to use.

https://chromiumcodereview.appspot.com/24261010/diff/6001/chrome/browser/chro...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:219:
test_server_->RegisterRequestHandler(
I'd just leave this to the user.

[bartfab (slow), 2013-09-26 12:39:13]

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:121: class
ExtensionInstallSuccessObserver
On 2013/09/25 12:04:08, Mattias Nissler wrote:
> On 2013/09/23 17:03:05, bartfab wrote:
> > On 2013/09/23 12:53:29, Mattias Nissler wrote:
> > > Is there any good reasons to make these classes if you can just base::Bind
> > > CheckNotification?
> > 
> > CheckNotification() needs to access the NotificationDetails. This is not
> > normally possible from a WindowedNotificationObserver callback because the
> > callback has no pointer to the WindowedNotificationObserver it belongs to. I
> > could instantiate a callback with a pointer to a pointer argument,
instantiate
> a
> > WindowedNotificationObserver that uses the callback and then inject a
pointer
> to
> > the WindowedNotificationObserver via the argument from the first step. But I
> > think that this is clumsy and error-prone.
> 
> Sounds like a subclass for WindowedNotificationObserver that forwards the
> Details pointer to the check callback could at least reduce the number of
> subclasses to only one then? 
> 
> Arguably, this should be supported by WindowedNotificationObserver directly. I
> guess you could generalize it to support callbacks that receive Details and
> Source, and then you can have a convenience ctor that adapts the function
type.

Done.

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:144: const
string16 id_;
On 2013/09/25 12:04:08, Mattias Nissler wrote:
> On 2013/09/23 17:03:05, bartfab wrote:
> > On 2013/09/23 12:53:29, Mattias Nissler wrote:
> > > Why is this a string16, while it is a std::string in line 130?
> > 
> > They listen for different notification types. An installation success
provides
> > the extension ID as an std::string, an installation failure provides the
> > extension ID as a string16.
> 
> Doh. Does the extension team have a bug for this? If not, please file one.

Filed crbug.com/298810

https://chromiumcodereview.appspot.com/24261010/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:193: while
((id_pos = request.relative_url.find("id%3D", id_pos)) !=
On 2013/09/25 12:04:08, Mattias Nissler wrote:
> On 2013/09/23 17:03:05, bartfab wrote:
> > On 2013/09/23 12:53:29, Mattias Nissler wrote:
> > > Can you use proper URL parsing, for GetValueForKeyInQuery in
> > > net/base/url_util.h?
> > > 
> > > Also not that there is switches::kAppsGalleryURL, which can potentially
make
> > all
> > > this significantly easier.
> > 
> > Re GetValueForKeyInQuery(): Unfortunately, I cannot use that. It will
extract
> > only one value per key and the key I am looking for repeats several times in
> the
> > URL.
> 
> Ad hoc URL parsing still isn't a good idea. Note that the underlying url_parse
> functionality can handle this case. Maybe we should just make a helper that
> decodes a query string to a std::multimap and put that in net/base/url_util.h
> for everybody to use?
> 
> > 
> > Re switches::kAppsGalleryURL: As discussed offline, this is not applicable
> here.
> 

Done.

https://chromiumcodereview.appspot.com/24261010/diff/6001/chrome/browser/chro...
File
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider_unittest.cc
(right):

https://chromiumcodereview.appspot.com/24261010/diff/6001/chrome/browser/chro...
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider_unittest.cc:52:
typedef testing::Test DeviceLocalAccountManagementPolicyProviderTest;
On 2013/09/25 12:04:08, Mattias Nissler wrote:
> Don't. Just use the TEST() macro.

Done.

https://chromiumcodereview.appspot.com/24261010/diff/6001/chrome/browser/chro...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://chromiumcodereview.appspot.com/24261010/diff/6001/chrome/browser/chro...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:212:
TestingUpdateManifestProvider::TestingUpdateManifestProvider(
On 2013/09/25 12:04:08, Mattias Nissler wrote:
> Definition order doesn't match declaration order.

Done.

https://chromiumcodereview.appspot.com/24261010/diff/6001/chrome/browser/chro...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:215: const
ExtensionMap& extension_map)
On 2013/09/25 12:04:08, Mattias Nissler wrote:
> Instead of passing in the map, can we just make an AddExtension() call on the
> provider? Makes it easier to use.

Done.

https://chromiumcodereview.appspot.com/24261010/diff/6001/chrome/browser/chro...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:219:
test_server_->RegisterRequestHandler(
On 2013/09/25 12:04:08, Mattias Nissler wrote:
> I'd just leave this to the user.

Done.

https://chromiumcodereview.appspot.com/24261010/diff/6001/chrome/browser/exte...
File chrome/browser/extensions/extension_system.cc (right):

https://chromiumcodereview.appspot.com/24261010/diff/6001/chrome/browser/exte...
chrome/browser/extensions/extension_system.cc:140: #endif  //
defined(ENABLE_EXTENSIONS)
On 2013/09/24 20:29:00, Antony Sargent wrote:
> It seems a little unfortunate to have this #ifdef'ing going on here, as
opposed
> to making the concept of a device local management policy more of a
first-class
> citizen. I guess you aren't doing much work other than initializing a member
> variable, so it's probably fine for now, but if we end up needing to put any
> more logic inside the #ifdef blocks here it might be time to consider
> refactoring. 

Just to clarify, because the name is somewhat misleading: This is not
device-local management policy for regular accounts. This is management policy
for *device-local accounts*. A device-local account is a special breed of Chrome
OS account that is distinct from e.g. regular users.

So what I am working on is not a new source of management policy that may apply
more widely in the future. It is a special source of management policy that goes
hand-in-hand with a special type of account.

[bartfab (slow), 2013-09-26 12:46:51]

Hi Scott,

Could you please review content/public/test/test_utils*?

Hi Jim,

Could you please review net/base/url_util*?

[Mattias Nissler (ping if slow), 2013-09-26 14:08:09]

https://chromiumcodereview.appspot.com/24261010/diff/15001/content/public/tes...
File content/public/test/test_utils.h (right):

https://chromiumcodereview.appspot.com/24261010/diff/15001/content/public/tes...
content/public/test/test_utils.h:123: typedef base::Callback<bool(const
NotificationDetails&)>
If we have this, maybe we should include NotificationSource as well?

https://chromiumcodereview.appspot.com/24261010/diff/15001/net/base/url_util.cc
File net/base/url_util.cc (right):

https://chromiumcodereview.appspot.com/24261010/diff/15001/net/base/url_util....
net/base/url_util.cc:80: url_parse::Component query =
url.parsed_for_possibly_invalid_spec().query;
It may make sense to reimplement this function by just doing a full parse and
then returning the first matching entry.

https://chromiumcodereview.appspot.com/24261010/diff/15001/net/base/url_util.h
File net/base/url_util.h (right):

https://chromiumcodereview.appspot.com/24261010/diff/15001/net/base/url_util....
net/base/url_util.h:53: std::string* out_value);
blank line

https://chromiumcodereview.appspot.com/24261010/diff/15001/net/base/url_util....
net/base/url_util.h:59: 
remove extra blank line

[bartfab (slow), 2013-09-26 14:43:04]

https://chromiumcodereview.appspot.com/24261010/diff/15001/content/public/tes...
File content/public/test/test_utils.h (right):

https://chromiumcodereview.appspot.com/24261010/diff/15001/content/public/tes...
content/public/test/test_utils.h:123: typedef base::Callback<bool(const
NotificationDetails&)>
On 2013/09/26 14:08:10, Mattias Nissler wrote:
> If we have this, maybe we should include NotificationSource as well?

Done.

https://chromiumcodereview.appspot.com/24261010/diff/15001/net/base/url_util.cc
File net/base/url_util.cc (right):

https://chromiumcodereview.appspot.com/24261010/diff/15001/net/base/url_util....
net/base/url_util.cc:80: url_parse::Component query =
url.parsed_for_possibly_invalid_spec().query;
On 2013/09/26 14:08:10, Mattias Nissler wrote:
> It may make sense to reimplement this function by just doing a full parse and
> then returning the first matching entry.

This can easily be done but I do not thing it is the right choice. ParseQuery()
is used in testing code only so that the linker will exclude it from the
browser. GetValueForKeyInQuery() is used by actual production code. Basing it on
ParseQuery() will cause the browser binary size to grow and will add the
overhead of a full parsing and multimap construction when really, you just
wanted a single entry.

https://chromiumcodereview.appspot.com/24261010/diff/15001/net/base/url_util.h
File net/base/url_util.h (right):

https://chromiumcodereview.appspot.com/24261010/diff/15001/net/base/url_util....
net/base/url_util.h:53: std::string* out_value);
On 2013/09/26 14:08:10, Mattias Nissler wrote:
> blank line

Done.

https://chromiumcodereview.appspot.com/24261010/diff/15001/net/base/url_util....
net/base/url_util.h:59: 
On 2013/09/26 14:08:10, Mattias Nissler wrote:
> remove extra blank line

Done.

[sky, 2013-09-26 20:22:50]

LGTM

[Mattias Nissler (ping if slow), 2013-09-27 15:01:41]

policy bits LGTM

[bartfab (slow), 2013-09-30 09:56:32]

Hi Jim,

Friendly review ping.

[jar (doing other things), 2013-09-30 16:00:26]

https://chromiumcodereview.appspot.com/24261010/diff/34001/net/base/url_util.h
File net/base/url_util.h (right):

https://chromiumcodereview.appspot.com/24261010/diff/34001/net/base/url_util....
net/base/url_util.h:59: NET_EXPORT bool ParseQuery(const GURL& url, KeyValueMap*
key_values);
Did you consider instead providing a simple iterator for use over the keys?

It always seems like a questionable hammer when a container provides a
translator to a different container.

[bartfab (slow), 2013-09-30 17:00:13]

https://chromiumcodereview.appspot.com/24261010/diff/34001/net/base/url_util.h
File net/base/url_util.h (right):

https://chromiumcodereview.appspot.com/24261010/diff/34001/net/base/url_util....
net/base/url_util.h:59: NET_EXPORT bool ParseQuery(const GURL& url, KeyValueMap*
key_values);
On 2013/09/30 16:00:27, jar wrote:
> Did you consider instead providing a simple iterator for use over the keys?
> 
> It always seems like a questionable hammer when a container provides a
> translator to a different container.

Done. Now that this conversion produces a lightweight iterator instead of an
std::multimap, I also modified getValueForKeyInQuery() to share code with it, as
originally proposed by Mattias.

[jar (doing other things), 2013-09-30 17:25:35]

https://chromiumcodereview.appspot.com/24261010/diff/46001/net/base/url_util.cc
File net/base/url_util.cc (right):

https://chromiumcodereview.appspot.com/24261010/diff/46001/net/base/url_util....
net/base/url_util.cc:74: at_end_(!url.is_valid()) {
nit: when initializer list can't fit on one line, wrap the ":" and indent it 4,
and align all initializers.  See style guide.

https://chromiumcodereview.appspot.com/24261010/diff/46001/net/base/url_util....
net/base/url_util.cc:105: value_ =
UnescapeURLComponent(spec_.substr(value.begin, value.len),
Do you need to do this as you iterate?  Can you be lazy, and only do this if the
caller needs the value (unescaped)?

If you want to be fancy, you can cache the unescaped value... but no reason to
process it unless a caller asks for it.

[bartfab (slow), 2013-09-30 17:50:24]

https://chromiumcodereview.appspot.com/24261010/diff/46001/net/base/url_util.cc
File net/base/url_util.cc (right):

https://chromiumcodereview.appspot.com/24261010/diff/46001/net/base/url_util....
net/base/url_util.cc:74: at_end_(!url.is_valid()) {
On 2013/09/30 17:25:36, jar wrote:
> nit: when initializer list can't fit on one line, wrap the ":" and indent it
4,
> and align all initializers.  See style guide.

Done.

https://chromiumcodereview.appspot.com/24261010/diff/46001/net/base/url_util....
net/base/url_util.cc:105: value_ =
UnescapeURLComponent(spec_.substr(value.begin, value.len),
On 2013/09/30 17:25:36, jar wrote:
> Do you need to do this as you iterate?  Can you be lazy, and only do this if
the
> caller needs the value (unescaped)?
> 
> If you want to be fancy, you can cache the unescaped value... but no reason to
> process it unless a caller asks for it.

Done.

[jar (doing other things), 2013-10-01 02:11:17]

https://chromiumcodereview.appspot.com/24261010/diff/52001/chrome/browser/chr...
File
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc
(right):

https://chromiumcodereview.appspot.com/24261010/diff/52001/chrome/browser/chr...
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:46:
const extensions::Extension* extension,
nit: Curious question: Why isn't this a const ref, if it is not an ouput arg?

https://chromiumcodereview.appspot.com/24261010/diff/52001/chrome/chrome_brow...
File chrome/chrome_browser_chromeos.gypi (right):

https://chromiumcodereview.appspot.com/24261010/diff/52001/chrome/chrome_brow...
chrome/chrome_browser_chromeos.gypi:298:
'browser/chromeos/extensions/device_local_account_management_policy_provider.cc',
nit: FWIW: This sure seems like a long name... perhaps so long as to make
reading it difficult.  Does it really provide value to be sooo long (and cause a
bunch of wraps, etc.)?

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util.cc
File net/base/url_util.cc (right):

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util....
net/base/url_util.cc:85: std::string QueryIterator::GetKey() const {
Should these Getters be defined once we're AtEnd()?

I'd expect at least a DCHECK().

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util.h
File net/base/url_util.h (right):

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util....
net/base/url_util.h:57: const std::string& GetUnescapedValue() const;
It surprised me to see that this was const... and then I saw you made the cached
value mutable.  It is probably clearer to have a non-const method.

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util_...
File net/base/url_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util_...
net/base/url_util_unittest.cc:113: TEST(UrlUtilTest, ParseQuery) {
Please update tests.

[bartfab (slow), 2013-10-01 10:03:40]

https://chromiumcodereview.appspot.com/24261010/diff/52001/chrome/browser/chr...
File
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc
(right):

https://chromiumcodereview.appspot.com/24261010/diff/52001/chrome/browser/chr...
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:46:
const extensions::Extension* extension,
On 2013/10/01 02:11:17, jar wrote:
> nit: Curious question: Why isn't this a const ref, if it is not an ouput arg?

This is the implementation of an existing interface (see the header file), so I
had no choice but to follow the choice made by the interface's original authors.

https://chromiumcodereview.appspot.com/24261010/diff/52001/chrome/chrome_brow...
File chrome/chrome_browser_chromeos.gypi (right):

https://chromiumcodereview.appspot.com/24261010/diff/52001/chrome/chrome_brow...
chrome/chrome_browser_chromeos.gypi:298:
'browser/chromeos/extensions/device_local_account_management_policy_provider.cc',
On 2013/10/01 02:11:17, jar wrote:
> nit: FWIW: This sure seems like a long name... perhaps so long as to make
> reading it difficult.  Does it really provide value to be sooo long (and cause
a
> bunch of wraps, etc.)?

Yes, the name is long but it does provide value. Names like this are common in
enterprise code because enterprise policy ties together different subsystems and
the file/class names need to make sense from the POV of the different
subsystems. In this specific case:

"device_local_account": This code deals with a special type of Chrome OS
account, known as a device-local account. This is the term used throughout all
enterprise code and in all file names dealing with the account type.

"management_policy": The term "policy" has a very specific meaning in the
context of enterprise code. This file here deals with a different type of policy
and the prefix "management" is necessary to disambiguate.

"policy_provider": This states the purpose of this file, from the POV of the
extensions subsystem.

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util.cc
File net/base/url_util.cc (right):

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util....
net/base/url_util.cc:85: std::string QueryIterator::GetKey() const {
On 2013/10/01 02:11:17, jar wrote:
> Should these Getters be defined once we're AtEnd()?
> 
> I'd expect at least a DCHECK().

Done.

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util.h
File net/base/url_util.h (right):

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util....
net/base/url_util.h:57: const std::string& GetUnescapedValue() const;
On 2013/10/01 02:11:17, jar wrote:
> It surprised me to see that this was const... and then I saw you made the
cached
> value mutable.  It is probably clearer to have a non-const method.

Done. I am happy to change the code as requested by its owner but IMHO, this
actually is a prime example of what const/mutable is for: The method is const
because it has no side effects and  returns a value that will not change between
invocations. The caching in a mutable unescaped_value_ is an implementation
detail that has no side effects on any other code.

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util_...
File net/base/url_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util_...
net/base/url_util_unittest.cc:113: TEST(UrlUtilTest, ParseQuery) {
On 2013/10/01 02:11:17, jar wrote:
> Please update tests.

Done.

[jar (doing other things), 2013-10-01 21:58:36]

Patch set 7, net/base/* LGTM

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util.h
File net/base/url_util.h (right):

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util....
net/base/url_util.h:57: const std::string& GetUnescapedValue() const;
On 2013/10/01 10:03:40, bartfab wrote:
> On 2013/10/01 02:11:17, jar wrote:
> > It surprised me to see that this was const... and then I saw you made the
> cached
> > value mutable.  It is probably clearer to have a non-const method.
> 
> Done. I am happy to change the code as requested by its owner but IMHO, this
> actually is a prime example of what const/mutable is for: The method is const
> because it has no side effects and  returns a value that will not change
between
> invocations. The caching in a mutable unescaped_value_ is an implementation
> detail that has no side effects on any other code.

I understand your comment, but the code which does caching is not thread safe...
and I (stupidly?) would have assumed that calling a method on a "mature"
instance (no longer, or not currently volatile or mutating?) would be thread
safe when the method was "const."

To be honest, I don't think such calling applies to this class... but my
paranoid sense of cleanliness suggests it is clearer to not call it const when
it does such thread-unsafe stuff.

I'm more psyched to see mutable used for things like locks, and then for
mutation to protected slots... but maybe I'll get used to your pattern.

Thanks for humoring my paranoia.

[bartfab (slow), 2013-10-02 08:41:19]

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util.h
File net/base/url_util.h (right):

https://chromiumcodereview.appspot.com/24261010/diff/52001/net/base/url_util....
net/base/url_util.h:57: const std::string& GetUnescapedValue() const;
On 2013/10/01 21:58:36, jar wrote:
> On 2013/10/01 10:03:40, bartfab wrote:
> > On 2013/10/01 02:11:17, jar wrote:
> > > It surprised me to see that this was const... and then I saw you made the
> > cached
> > > value mutable.  It is probably clearer to have a non-const method.
> > 
> > Done. I am happy to change the code as requested by its owner but IMHO, this
> > actually is a prime example of what const/mutable is for: The method is
const
> > because it has no side effects and  returns a value that will not change
> between
> > invocations. The caching in a mutable unescaped_value_ is an implementation
> > detail that has no side effects on any other code.
> 
> I understand your comment, but the code which does caching is not thread
safe...
> and I (stupidly?) would have assumed that calling a method on a "mature"
> instance (no longer, or not currently volatile or mutating?) would be thread
> safe when the method was "const."
> 
> To be honest, I don't think such calling applies to this class... but my
> paranoid sense of cleanliness suggests it is clearer to not call it const when
> it does such thread-unsafe stuff.
> 
> I'm more psyched to see mutable used for things like locks, and then for
> mutation to protected slots... but maybe I'll get used to your pattern.
> 
> Thanks for humoring my paranoia.

We actually discussed this locally when I saw your request to not use
const+mutable. We were wondering why you disliked const+mutable and thread
safety was the answer that we came up to as well. True, it should be safe now
while it was not safe before. Even though it would seem unlikely that there is
much reason to share an iterator between two threads... but safer is better.

[commit-bot: I haz the power, 2013-10-02 09:05:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/24261010/73001

[commit-bot: I haz the power, 2013-10-02 12:30:09]

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

[bartfab (slow), 2013-10-02 14:26:45]

Hi Julian,

Could you take a look at the changes since patchset 8 please?

The gist of these changes is that DeviceLocalAccountManagementPolicyProvider
distinguishes between different account types now so that public sessions and
single-app kiosk sessions can allow different sets of apps/extensions. This is
actually something that Mattias had wanted to implement before he went OOO.

[pastarmovj, 2013-10-02 14:44:49]

lgtm with one comment.

https://codereview.chromium.org/24261010/diff/109001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account.cc (right):

https://codereview.chromium.org/24261010/diff/109001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account.cc:81: NOTREACHED();
I think it will be still good to set |type| to something in this case as well. I
am not sure whether |DeviceLocalAccount::TYPE_COUNT| is a good choice though or
make it a CHECK to always crash.

[bartfab (slow), 2013-10-02 14:48:34]

https://codereview.chromium.org/24261010/diff/109001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account.cc (right):

https://codereview.chromium.org/24261010/diff/109001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account.cc:81: NOTREACHED();
On 2013/10/02 14:44:50, pastarmovj wrote:
> I think it will be still good to set |type| to something in this case as well.
I
> am not sure whether |DeviceLocalAccount::TYPE_COUNT| is a good choice though
or
> make it a CHECK to always crash.

Done.

[commit-bot: I haz the power, 2013-10-02 14:48:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/24261010/122002

[commit-bot: I haz the power, 2013-10-02 16:47:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/24261010/122002

[commit-bot: I haz the power, 2013-10-02 17:47:47]

Change committed as 226494

[commit-bot: I haz the power, 2013-10-08 14:19:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/24261010/145001

[commit-bot: I haz the power, 2013-10-09 02:08:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/24261010/145001

[commit-bot: I haz the power, 2013-10-09 02:50:02]

Change committed as 227641