[Extensions] Make chrome://extensions use developerPrivate for unpacked loading

[Extensions] Make chrome://extensions use developerPrivate for unpacked loading

Make the chrome://extensions page use chrome.developerPrivate API to
select a file path and load an unpacked extension.
Also add tests for the api function, and convert it to a
UIThreadExtensionFunction.

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

[Devlin, 2015-03-03 20:09:49]

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

[not at google - send to devlin, 2015-03-03 22:22:11]

lgtm with 2 comments - however, it looks like you have a leark.

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:894: //
If this call came from the web ui, we record it.
Why only from the web UI?
- if it's because we're specifically looking for some kind of UI metric, then
it'd be better to expose content::RecordComputedAction to WebUI and call it from
there.
- if it's to track how many times users try to load an unpacked extension, we
should always record it.

That said I've heard that Actions are expensive, and we don't appear to use it
(it doesn't appear in actions.xml), so let's just delete it entirely.

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
(right):

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:32:
// Creates a new ExtensionToolbarModel for the given |context|.
Uh what does this comment have to do with the code?

[Devlin, 2015-03-03 22:52:09]

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

[Devlin, 2015-03-03 22:52:10]

Dan, mind checking closure compiler/extension_loader.js?

[Dan Beam, 2015-03-03 23:37:03]

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_loader.js (right):

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_loader.js:187: * @private
nit: @private {boolean}

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_loader.js:194:
ExtensionLoader.prototype = {
nit: put simple default in the prototype, e.g.

  ExtensionLoader.prototype = {
    /**
     * Whether or not we are currently loading an unpacked extension.
     * @private {boolean}
     */
    isLoading_: false,
    ...
  };

(don't do this with arrays/objects)

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_loader.js:206: var lastError =
chrome.runtime.lastError;
:(

[Dan Beam, 2015-03-03 23:37:07]

lgtm

[Devlin, 2015-03-04 00:37:06]

Patchset #3 (id:40001) has been deleted

[Devlin, 2015-03-04 16:51:14]

Patchset #3 (id:60001) has been deleted

[Devlin, 2015-03-04 17:29:13]

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:894: //
If this call came from the web ui, we record it.
On 2015/03/03 22:22:10, kalman wrote:
> Why only from the web UI?
> - if it's because we're specifically looking for some kind of UI metric, then
> it'd be better to expose content::RecordComputedAction to WebUI and call it
from
> there.
> - if it's to track how many times users try to load an unpacked extension, we
> should always record it.
> 
> That said I've heard that Actions are expensive, and we don't appear to use it
> (it doesn't appear in actions.xml), so let's just delete it entirely.

Was gonna ask about this. Happy to remove.

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
(right):

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:32:
// Creates a new ExtensionToolbarModel for the given |context|.
On 2015/03/03 22:22:10, kalman wrote:
> Uh what does this comment have to do with the code?

Wait, you mean this *isn't* an extension toolbar model? ;)

Removed.

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_loader.js (right):

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_loader.js:187: * @private
On 2015/03/03 23:37:03, Dan Beam wrote:
> nit: @private {boolean}

Done.

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_loader.js:194:
ExtensionLoader.prototype = {
On 2015/03/03 23:37:03, Dan Beam wrote:
> nit: put simple default in the prototype, e.g.
> 
>   ExtensionLoader.prototype = {
>     /**
>      * Whether or not we are currently loading an unpacked extension.
>      * @private {boolean}
>      */
>     isLoading_: false,
>     ...
>   };
> 
> (don't do this with arrays/objects)

Done.

https://codereview.chromium.org/979453002/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_loader.js:206: var lastError =
chrome.runtime.lastError;
On 2015/03/03 23:37:03, Dan Beam wrote:
> :(

Yeah :/

[Devlin, 2015-03-04 17:29:20]

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

[Devlin, 2015-03-04 17:29:21]

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

[commit-bot: I haz the power, 2015-03-04 17:30:58]

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

[commit-bot: I haz the power, 2015-03-04 17:51:22]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-03-04 17:52:07]

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