[Extensions] Fix chrome url override settings

[Extensions] Fix chrome url override settings

Previously, chrome url overrides were added and removed at extension load and
unload. They should instead be added at install time, updated when the extension
is unloaded, and removed when the extension is uninstalled. Change the pref from
a string (of the url) to a dictionary with { entry: <url>, active: <bool> } to
allow for inactive, but retained, entries.

Also add regression tests.

BUG=571429
Committed: https://crrev.com/5fafa5aa3c2bde50f4f50511f134192147f86b55
Cr-Commit-Position: refs/heads/master@{#367137}

[Devlin, 2015-12-22 23:27:31]

Description was changed from

==========
Fix overrides
BUG=
==========

to

==========
[Extensions] Fix chrome url override settings

Previously, chrome url overrides were added and removed at extension load and
unload. They should instead be added at install time, updated when the extension
is unloaded, and removed when the extension is uninstalled. Change the pref from
a string (of the url) to a dictionary with { entry: <url>, active: <bool> } to
allow for inactive, but retained, entries.

BUG=571429
==========

[Devlin, 2015-12-22 23:27:35]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-12-22 23:27:44]

Patchset #1 (id:20001) has been deleted

[Devlin, 2015-12-22 23:28:13]

Description was changed from

==========
[Extensions] Fix chrome url override settings

Previously, chrome url overrides were added and removed at extension load and
unload. They should instead be added at install time, updated when the extension
is unloaded, and removed when the extension is uninstalled. Change the pref from
a string (of the url) to a dictionary with { entry: <url>, active: <bool> } to
allow for inactive, but retained, entries.

BUG=571429
==========

to

==========
[Extensions] Fix chrome url override settings

Previously, chrome url overrides were added and removed at extension load and
unload. They should instead be added at install time, updated when the extension
is unloaded, and removed when the extension is uninstalled. Change the pref from
a string (of the url) to a dictionary with { entry: <url>, active: <bool> } to
allow for inactive, but retained, entries.

Also add regression tests.

BUG=571429
==========

[Devlin, 2015-12-22 23:31:22]

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

[Devlin, 2015-12-22 23:31:23]

Finnur, mind taking a look?

This turned out a little more complicated than I wanted - it was broken on a few
different fronts. :/

After I fixed it this way, I thought of another alternative, which would be to
migrate these to extension controlled prefs (which I think is what, e.g.,
search, homepage, and proxy overrides do).  That might be the better solution in
the long run, but would also be a multi-stage process, so I think this might be
good in the meantime.

https://codereview.chromium.org/1543923002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_web_ui.cc (right):

https://codereview.chromium.org/1543923002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_web_ui.cc:63: void
InitializeOverridesList(base::ListValue* list) {
Transforming prefs. Yuck.  But I can't think of a better way to indicate active
vs. inactive.

[Devlin, 2015-12-22 23:31:55]

(Also, trybots are refusing to patch for some reason - I just synced and it
builds/passes tests locally.)

[Finnur, 2015-12-23 13:08:03]

LGTM, with nits.

https://codereview.chromium.org/1543923002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_override_apitest.cc (right):

https://codereview.chromium.org/1543923002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_override_apitest.cc:62: return
testing::AssertionFailure() << gurl;
Does this do what you expect, or do you need gurl.spec().c_str() or something?

https://codereview.chromium.org/1543923002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_web_ui.cc (right):

https://codereview.chromium.org/1543923002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_web_ui.cc:184:
CHECK((*iter)->GetAsDictionary(&dict));
I know this isn't a DCHECK, but including production code inside these macros
makes me feel a bit uncomfortable. Perhaps convert to:

bool success = (*iter)->GetAsDictionary(&dict);
CHECK(success);

Same below... (a few cases)

https://codereview.chromium.org/1543923002/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/override/newtab/manifest.json (left):

https://codereview.chromium.org/1543923002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/override/newtab/manifest.json:7: "scripts":
["background.js"]
Does this file need to be deleted now?

[Devlin, 2015-12-30 02:10:52]

Patchset #3 (id:80001) has been deleted

[Devlin, 2015-12-30 02:11:05]

Patchset #3 (id:100001) has been deleted

[Devlin, 2015-12-30 02:12:04]

Patchset #3 (id:120001) has been deleted

[Devlin, 2015-12-30 02:12:10]

Patchset #3 (id:140001) has been deleted

[Devlin, 2015-12-30 02:12:19]

Patchset #3 (id:160001) has been deleted

[Devlin, 2015-12-30 02:54:52]

Patchset #3 (id:180001) has been deleted

[Devlin, 2015-12-30 02:55:21]

Patchset #3 (id:200001) has been deleted

[Devlin, 2015-12-30 02:55:32]

Patchset #3 (id:220001) has been deleted

[Devlin, 2015-12-30 03:10:40]

https://codereview.chromium.org/1543923002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_override_apitest.cc (right):

https://codereview.chromium.org/1543923002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_override_apitest.cc:62: return
testing::AssertionFailure() << gurl;
On 2015/12/23 13:08:03, Finnur wrote:
> Does this do what you expect, or do you need gurl.spec().c_str() or something?

This works; GURL has an operator <<.

https://codereview.chromium.org/1543923002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_web_ui.cc (right):

https://codereview.chromium.org/1543923002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_web_ui.cc:184:
CHECK((*iter)->GetAsDictionary(&dict));
On 2015/12/23 13:08:03, Finnur wrote:
> I know this isn't a DCHECK, but including production code inside these macros
> makes me feel a bit uncomfortable. Perhaps convert to:
> 
> bool success = (*iter)->GetAsDictionary(&dict);
> CHECK(success);
> 
> Same below... (a few cases)

Done.

https://codereview.chromium.org/1543923002/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/override/newtab/manifest.json (left):

https://codereview.chromium.org/1543923002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/override/newtab/manifest.json:7: "scripts":
["background.js"]
On 2015/12/23 13:08:03, Finnur wrote:
> Does this file need to be deleted now?

Good call, done.

[Devlin, 2015-12-30 03:10:59]

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

[Devlin, 2015-12-30 03:10:59]

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

[Devlin, 2015-12-30 03:10:59]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/1543923002/#ps240001
(title: "Latest master")

[Devlin, 2015-12-30 03:11:00]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/1543923002/#ps240001
(title: "Latest master")

[commit-bot: I haz the power, 2015-12-30 03:11:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1543923002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1543923002/240001

[commit-bot: I haz the power, 2015-12-30 03:42:15]

Description was changed from

==========
[Extensions] Fix chrome url override settings

Previously, chrome url overrides were added and removed at extension load and
unload. They should instead be added at install time, updated when the extension
is unloaded, and removed when the extension is uninstalled. Change the pref from
a string (of the url) to a dictionary with { entry: <url>, active: <bool> } to
allow for inactive, but retained, entries.

Also add regression tests.

BUG=571429
==========

to

==========
[Extensions] Fix chrome url override settings

Previously, chrome url overrides were added and removed at extension load and
unload. They should instead be added at install time, updated when the extension
is unloaded, and removed when the extension is uninstalled. Change the pref from
a string (of the url) to a dictionary with { entry: <url>, active: <bool> } to
allow for inactive, but retained, entries.

Also add regression tests.

BUG=571429
==========

[commit-bot: I haz the power, 2015-12-30 03:42:16]

Committed patchset #3 (id:240001)

[commit-bot: I haz the power, 2015-12-30 03:43:22]

Description was changed from

==========
[Extensions] Fix chrome url override settings

Previously, chrome url overrides were added and removed at extension load and
unload. They should instead be added at install time, updated when the extension
is unloaded, and removed when the extension is uninstalled. Change the pref from
a string (of the url) to a dictionary with { entry: <url>, active: <bool> } to
allow for inactive, but retained, entries.

Also add regression tests.

BUG=571429
==========

to

==========
[Extensions] Fix chrome url override settings

Previously, chrome url overrides were added and removed at extension load and
unload. They should instead be added at install time, updated when the extension
is unloaded, and removed when the extension is uninstalled. Change the pref from
a string (of the url) to a dictionary with { entry: <url>, active: <bool> } to
allow for inactive, but retained, entries.

Also add regression tests.

BUG=571429

Committed: https://crrev.com/5fafa5aa3c2bde50f4f50511f134192147f86b55
Cr-Commit-Position: refs/heads/master@{#367137}
==========

[commit-bot: I haz the power, 2015-12-30 03:43:22]

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