[Extensions UI] Use developerPrivate API for profile configuration

[Extensions UI] Use developerPrivate API for profile configuration

Transition the chrome://extensions page to use the developerPrivate API for
getting data about a profile's configuration, and for updating that
configuration (e.g. for developer mode).

BUG=461039
Committed: https://crrev.com/b1e5d24b2a3b1bb95b4cbb83b752d775952c91cc
Cr-Commit-Position: refs/heads/master@{#324726}

[Devlin, 2015-04-07 21:02:41]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-04-07 23:17:19]

Patchset #2 (id:40001) has been deleted

[Devlin, 2015-04-07 23:17:23]

Patchset #1 (id:20001) has been deleted

[Devlin, 2015-04-07 23:17:47]

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

[Devlin, 2015-04-07 23:17:49]

Ben, mind taking a look at the C++?

[not at google - send to devlin, 2015-04-08 17:12:02]

lgtm

https://codereview.chromium.org/1060993003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/1060993003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:501:
Profile* profile = Profile::FromBrowserContext(browser_context());
If you inherit from ChromeUIThreadExtensionFunction you will get a profile()
method for free. Looks like there are a lot of instances that nobody noticed
this for (all of these DeveloperPrivate functions should do that).

https://codereview.chromium.org/1060993003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:520:
promote_apps_dev_tools = true;
you could also assign these directly to info.should_show_adt_promo, not have
that redundant-looking promote_apps_dev_tools local variable.

https://codereview.chromium.org/1060993003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:531:
return RespondNow(OneArgument(info.ToValue().release()));
It's more principled to use the generated Result structure,
developer::GetProfileConfiguration::Results::Create(...).

Either that, or add a version of OneArgument that takes a scoped_ptr so you
don't need to release() here.

https://codereview.chromium.org/1060993003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:557:
EXTENSION_FUNCTION_VALIDATE(*update.dismissed_adt_promo);
Unless you do renderer validation, EXTENSION_FUNCTION_VALIDATE is a little
strong here (it's also a little annoying to debug in a release build). Perhaps
return an Error() instead.

[Devlin, 2015-04-09 19:57:24]

https://codereview.chromium.org/1060993003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/1060993003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:501:
Profile* profile = Profile::FromBrowserContext(browser_context());
On 2015/04/08 17:12:02, kalman wrote:
> If you inherit from ChromeUIThreadExtensionFunction you will get a profile()
> method for free. Looks like there are a lot of instances that nobody noticed
> this for (all of these DeveloperPrivate functions should do that).

Done.

https://codereview.chromium.org/1060993003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:520:
promote_apps_dev_tools = true;
On 2015/04/08 17:12:02, kalman wrote:
> you could also assign these directly to info.should_show_adt_promo, not have
> that redundant-looking promote_apps_dev_tools local variable.

Moot - die, ADT promo, die!

https://codereview.chromium.org/1060993003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:531:
return RespondNow(OneArgument(info.ToValue().release()));
On 2015/04/08 17:12:02, kalman wrote:
> It's more principled to use the generated Result structure,
> developer::GetProfileConfiguration::Results::Create(...).
> 
> Either that, or add a version of OneArgument that takes a scoped_ptr so you
> don't need to release() here.

I like the latter.

https://codereview.chromium.org/1060993003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:557:
EXTENSION_FUNCTION_VALIDATE(*update.dismissed_adt_promo);
On 2015/04/08 17:12:02, kalman wrote:
> Unless you do renderer validation, EXTENSION_FUNCTION_VALIDATE is a little
> strong here (it's also a little annoying to debug in a release build). Perhaps
> return an Error() instead.

Moot.

[Devlin, 2015-04-09 19:59:05]

Patchset #2 (id:80001) has been deleted

[Devlin, 2015-04-09 20:00:07]

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

[Devlin, 2015-04-09 20:00:07]

Dan, mind taking a look at *.js?  (Lemme know if I'm overwhelming you and should
have someone else take a few of these.)

[Dan Beam, 2015-04-10 02:48:06]

lgtm

On 2015/04/09 20:00:07, Devlin wrote:
> Dan, mind taking a look at *.js?  (Lemme know if I'm overwhelming you and
should
> have someone else take a few of these.)

i'm never opposed to doing less work

https://codereview.chromium.org/1060993003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extensions.js (right):

https://codereview.chromium.org/1060993003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/extensions/extensions.js:232: var supervised =
profileInfo.isSupervised;
nit: when it fits: /** @const */

https://codereview.chromium.org/1060993003/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js
(right):

https://codereview.chromium.org/1060993003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:127:
getRequiredElement('toggle-dev-on').click();
there's really no downside to using getRequiredElement() here, but this is akin
to assert($(id)).click() where $(id).click() would also fail if $(id) returns
null (so it's just doing unnecessary work).  typically we don't assert() things
that will just be used right after anyways...

[Devlin, 2015-04-10 16:28:22]

Patchset #3 (id:120001) has been deleted

[Devlin, 2015-04-10 16:30:39]

https://codereview.chromium.org/1060993003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/extensions/extensions.js (right):

https://codereview.chromium.org/1060993003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/extensions/extensions.js:232: var supervised =
profileInfo.isSupervised;
On 2015/04/10 02:48:06, Dan Beam wrote:
> nit: when it fits: /** @const */

Done.

https://codereview.chromium.org/1060993003/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js
(right):

https://codereview.chromium.org/1060993003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:127:
getRequiredElement('toggle-dev-on').click();
On 2015/04/10 02:48:06, Dan Beam wrote:
> there's really no downside to using getRequiredElement() here, but this is
akin
> to assert($(id)).click() where $(id).click() would also fail if $(id) returns
> null (so it's just doing unnecessary work).  typically we don't assert()
things
> that will just be used right after anyways...

Fair point.  C++ habit carryover where a failed ASSERT is a lot nicer than a
segfault. :)

Fixed.

[Devlin, 2015-04-10 16:30:55]

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

[Devlin, 2015-04-10 16:30:56]

Mark, mind checking histograms real quick?

[Mark P, 2015-04-10 17:16:51]

histograms.xml lgtm
congratulations, you have the 1000th extension function histogram value!

--mark

[Devlin, 2015-04-10 17:22:31]

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

[Devlin, 2015-04-10 17:22:32]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org,
dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1060993003/#ps140001
(title: "Dan's")

[commit-bot: I haz the power, 2015-04-10 17:22:53]

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

[commit-bot: I haz the power, 2015-04-10 17:28:37]

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

[commit-bot: I haz the power, 2015-04-10 17:28:38]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Devlin, 2015-04-10 17:45:44]

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

[Devlin, 2015-04-10 17:45:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org, dbeam@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1060993003/#ps160001
(title: "Latest master")

[commit-bot: I haz the power, 2015-04-10 17:46:15]

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

[Devlin, 2015-04-10 20:36:37]

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

[Devlin, 2015-04-10 21:13:59]

On 2015/04/10 17:46:15, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1060993003/160001

Dan, mind taking a look at the delta between patch sets 4 and 5?  (Looks like we
were never registering for notifications - something we still need to do until
all events are moved over.)

[Dan Beam, 2015-04-10 21:26:27]

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (left):

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:572:
registered_for_notifications_  = true;
why'd you remove this?

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:481: const
base::ListValue* args) {
if there a way to determine if this code path has ever been run without using a
bool |registered_for_notifications_|?

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:534: }
nit: next time mention you're only aligning with the header's declaration
ordering or do this separately.

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/extension_settings_handler.h (right):

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/extension_settings_handler.h:102: // Callback
for the "register" message.
these are all actually "extensionSettings" prefixed, so these should all say:

  // Callback for the "extensionSettingsRegister" message.

IMO to be consistent with WebUI

[Devlin, 2015-04-10 21:43:05]

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (left):

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:572:
registered_for_notifications_  = true;
On 2015/04/10 21:26:27, Dan Beam wrote:
> why'd you remove this?

See other comment.

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:481: const
base::ListValue* args) {
On 2015/04/10 21:26:27, Dan Beam wrote:
> if there a way to determine if this code path has ever been run without using
a
> bool |registered_for_notifications_|?

I thought since it was moved to be only in ExtensionSettings initialize() (as
opposed to called every time we fetched data), that would serve as a guarantee. 
But I forgot that we keep around the webui even if the page is fully recreated,
as in the case of reloading.  Brought back the bool.

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:534: }
On 2015/04/10 21:26:27, Dan Beam wrote:
> nit: next time mention you're only aligning with the header's declaration
> ordering or do this separately.

Will do, sorry.

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/extension_settings_handler.h (right):

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/extension_settings_handler.h:102: // Callback
for the "register" message.
On 2015/04/10 21:26:27, Dan Beam wrote:
> these are all actually "extensionSettings" prefixed, so these should all say:
> 
>   // Callback for the "extensionSettingsRegister" message.
> 
> IMO to be consistent with WebUI

I would have thought so, too - was just matching those that are there.  Fixed
them all.

[Dan Beam, 2015-04-10 21:48:02]

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:482: Profile*
profile = Profile::FromWebUI(web_ui());
nit: instead of bringing back the bool

  auto* warning_service = WarningService::Get(profile);
  if (extension_management_observer_.IsObserving(warning_service))
    return;

[Dan Beam, 2015-04-10 21:49:03]

lgtm either way

[Devlin, 2015-04-10 21:54:09]

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/1060993003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:482: Profile*
profile = Profile::FromWebUI(web_ui());
On 2015/04/10 21:48:02, Dan Beam wrote:
> nit: instead of bringing back the bool
> 
>   auto* warning_service = WarningService::Get(profile);
>   if (extension_management_observer_.IsObserving(warning_service))
>     return;

Sure, though I prefer registrar_.IsEmpty, since it saves us a keyed service look
up in the case of already being called. :)

[Devlin, 2015-04-10 23:15:57]

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

[Devlin, 2015-04-10 23:15:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org, kalman@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1060993003/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-04-10 23:17:54]

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

[commit-bot: I haz the power, 2015-04-11 00:08:49]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2015-04-11 00:09:40]

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

[Nico, 2015-04-13 04:04:04]

I'll revert this as it could've caused http://crbug.com/476408. The "Revert"
button doesn't seem to work for this issue (filed http://crbug.com/476418), so
I'll do it manually.

[Nico, 2015-04-13 04:26:11]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-04-13 04:26:12]

Manual revert here: https://codereview.chromium.org/1083573002/