[Extensions] Sync the 'allowed scripting on all urls' preference

[Extensions] Sync the 'allowed scripting on all urls' preference

Sync the flag for whether or not an extension is allowed to script on all urls,
independent of the flag to enable the feature. This (eventually) allows people
to install extensions on a machine with the flag without the scary warning
about what will happen if it's synced.

BUG=452756
Committed: https://crrev.com/d1aa852e6df52eaca4da775a3c40b3c1c8431008
Cr-Commit-Position: refs/heads/master@{#316100}

[Devlin, 2015-02-10 19:12:49]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-02-10 19:12:54]

Patchset #1 (id:20001) has been deleted

[Devlin, 2015-02-10 20:43:18]

Patchset #1 (id:40001) has been deleted

[Devlin, 2015-02-10 22:20:48]

rdevlin.cronin@chromium.org changed reviewers:
+ kalman@chromium.org

[not at google - send to devlin, 2015-02-11 01:34:51]

Little comments, but I want to see the outcome.

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

https://codereview.chromium.org/907533004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_service_unittest.cc:6354: bool
default_allowed_scripting =
Would make me more comfortable if this were const.

https://codereview.chromium.org/907533004/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_sync_data.h (right):

https://codereview.chromium.org/907533004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_sync_data.h:29: enum PrefValue {
This "PrefValue" name is freaking me out. Can you call it OptionalBoolean,
literally what it is.

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

https://codereview.chromium.org/907533004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_sync_data_unittest.cc:57:
extension_sync_data.all_urls_enabled() == kPrefTrue);
This looks convoluted.

We know that extension_specifics->all_urls_enabled() is true.

Therefore this condition is equivalent to
EXPECT_EQ(true, extension_sync_data.all_urls_enabled() == kPrefTrue)

which is only true when extension_sync_data.all_urls_enabled() == kPrefTrue

Therefore this condition is equivalent to
EXPECT_EQ(kPrefTrue, extension_sync_data.all_urls_enabled())

which is a lot simpler.

However, I see that it's a symptom of a larger problem. These tests seem to be
basically testing that if you serialize ExtensionSyncData into
ExtensionSpecifics, then back again, you get the same result. They just
overcomplicate it. Nevertheless it was a good idea, and now would be a good time
to implement it properly.

Proposal: have 2 methods. One asserts that serializing a SyncData and then
deserializing it equals the old value. The other, the opposite.

Then call that 3 times each, mutating some data if you really want, but most
importantly test all 3 configurations of all urls enabled (not set, true,
false).

Note that the protobuf objects implement an operator= method. That might come in
handy.

https://codereview.chromium.org/907533004/diff/60001/sync/protocol/extension_...
File sync/protocol/extension_specifics.proto (right):

https://codereview.chromium.org/907533004/diff/60001/sync/protocol/extension_...
sync/protocol/extension_specifics.proto:47: // may not be present if the user
hasn't given an explicit preference!
I would own up to what this is, a tri-state boolean - i.e. idiomatically
optional, whereas most fields are pragmatically optional (e.g. |name| isn't
*really* optional). And, why.

[Devlin, 2015-02-11 17:52:21]

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

https://codereview.chromium.org/907533004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_service_unittest.cc:6354: bool
default_allowed_scripting =
On 2015/02/11 01:34:51, kalman wrote:
> Would make me more comfortable if this were const.

Done.

https://codereview.chromium.org/907533004/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_sync_data.h (right):

https://codereview.chromium.org/907533004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_sync_data.h:29: enum PrefValue {
On 2015/02/11 01:34:51, kalman wrote:
> This "PrefValue" name is freaking me out. Can you call it OptionalBoolean,
> literally what it is.

Fair. Done.

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

https://codereview.chromium.org/907533004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_sync_data_unittest.cc:57:
extension_sync_data.all_urls_enabled() == kPrefTrue);
On 2015/02/11 01:34:51, kalman wrote:
> This looks convoluted.
> 
> We know that extension_specifics->all_urls_enabled() is true.
> 
> Therefore this condition is equivalent to
> EXPECT_EQ(true, extension_sync_data.all_urls_enabled() == kPrefTrue)
> 
> which is only true when extension_sync_data.all_urls_enabled() == kPrefTrue
> 
> Therefore this condition is equivalent to
> EXPECT_EQ(kPrefTrue, extension_sync_data.all_urls_enabled())
> 
> which is a lot simpler.
> 
> However, I see that it's a symptom of a larger problem. These tests seem to be
> basically testing that if you serialize ExtensionSyncData into
> ExtensionSpecifics, then back again, you get the same result. They just
> overcomplicate it. Nevertheless it was a good idea, and now would be a good
time
> to implement it properly.
> 
> Proposal: have 2 methods. One asserts that serializing a SyncData and then
> deserializing it equals the old value. The other, the opposite.
> 
> Then call that 3 times each, mutating some data if you really want, but most
> importantly test all 3 configurations of all urls enabled (not set, true,
> false).
> 
> Note that the protobuf objects implement an operator= method. That might come
in
> handy.

Yeah, the current test leaves a little to be wanted.  New test: check
serialization/deserialization in both directions, as well as correctness.  A few
more lines of code, but probably worth it.

https://codereview.chromium.org/907533004/diff/60001/sync/protocol/extension_...
File sync/protocol/extension_specifics.proto (right):

https://codereview.chromium.org/907533004/diff/60001/sync/protocol/extension_...
sync/protocol/extension_specifics.proto:47: // may not be present if the user
hasn't given an explicit preference!
On 2015/02/11 01:34:51, kalman wrote:
> I would own up to what this is, a tri-state boolean - i.e. idiomatically
> optional, whereas most fields are pragmatically optional (e.g. |name| isn't
> *really* optional). And, why.

Done.

[not at google - send to devlin, 2015-02-11 20:05:54]

lgtm

[Devlin, 2015-02-11 20:06:46]

rdevlin.cronin@chromium.org changed reviewers:
+ zea@chromium.org

[Devlin, 2015-02-11 20:06:47]

+zea@ for sync stuff.

[Nicolas Zea, 2015-02-12 18:50:26]

sync lgtm

[Nicolas Zea, 2015-02-12 18:51:11]

Oops, missed one thing.

https://codereview.chromium.org/907533004/diff/80001/sync/protocol/extension_...
File sync/protocol/extension_specifics.proto (right):

https://codereview.chromium.org/907533004/diff/80001/sync/protocol/extension_...
sync/protocol/extension_specifics.proto:50: optional bool all_urls_enabled = 9;
please also update proto_value_conversions.cc to include this field

[Devlin, 2015-02-12 20:24:22]

New patchsets have been uploaded after l-g-t-m from
kalman@chromium.org,zea@chromium.org

[Devlin, 2015-02-12 20:24:56]

https://codereview.chromium.org/907533004/diff/80001/sync/protocol/extension_...
File sync/protocol/extension_specifics.proto (right):

https://codereview.chromium.org/907533004/diff/80001/sync/protocol/extension_...
sync/protocol/extension_specifics.proto:50: optional bool all_urls_enabled = 9;
On 2015/02/12 18:51:10, Nicolas Zea wrote:
> please also update proto_value_conversions.cc to include this field

Done.

[Nicolas Zea, 2015-02-12 21:38:55]

LGTM

[Devlin, 2015-02-12 22:40:28]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2015-02-12 22:42:16]

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

[commit-bot: I haz the power, 2015-02-13 00:26:06]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2015-02-13 00:26:53]

Patchset 3 (id:??) landed as
https://crrev.com/d1aa852e6df52eaca4da775a3c40b3c1c8431008
Cr-Commit-Position: refs/heads/master@{#316100}