Make sideloaded (externally installed) extensions display webstore info

Make sideloaded (externally installed) extensions display webstore info
Have sideloaded extensions pull data from the webstore (a la inline install) in
order to give the users a better idea of what extension they are installing.

Images worth 1000 words: http://imgur.com/zlexZeb,VljPXLz,WzT2ZOc#0

XIB changes:
* rename 'app/nibs/ExtensionInstallPromptInline.xib' to 'app/nibs/ExtensionInstallPromptWebstoreData', since the same prompt is now used for inline prompts and for sideloaded extensions when webstore data is available.

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

[Finnur, 2014-01-24 11:38:30]

Awesome! This is exactly what I was hoping to see.

Just a few comments, I think we are close here.

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/external_install_ui.cc (left):

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/external_install_ui.cc:233: return;
Tread carefully here.
https://code.google.com/p/chromium/issues/detail?id=256640

I think there's a reason why we use the ID and not the extension pointer so I
don't think it is safe to simplify this. Yoyo can probably say for sure.

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/external_install_ui.cc (right):

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/external_install_ui.cc:228: // If we don't have a
browser, we can't go to the webstore to fetch data.
Does this ever happen?

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/ui/gtk/e...
File chrome/browser/ui/gtk/extensions/extension_install_dialog_gtk.cc (right):

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/ui/gtk/e...
chrome/browser/ui/gtk/extensions/extension_install_dialog_gtk.cc:202: if
(has_webstore_data) {
I only see a unit test change on Mac. Is there an additional file for Mac
missing with a change similar to this?

[Finnur, 2014-01-24 11:39:18]

Oops, meant to include Yoyo for the discussion (Yoyo: see my first comment).

[Devlin, 2014-01-24 18:10:23]

Added the extra mac files, but I confess myself quite ignorant in objective-c. 
The trybots seem happy, but a little extra attention would probably be good. :)

Cheers,
- Devlin

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/external_install_ui.cc (right):

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/external_install_ui.cc:228: // If we don't have a
browser, we can't go to the webstore to fetch data.
On 2014/01/24 11:38:31, Finnur wrote:
> Does this ever happen?

TL;DR: Yes, but usually only in tests.

It can, though rarely in practice.  We create this at line 502, and |browser|
can be NULL if we're on android (I don't know that we can ever actually get this
far on android, though - that might just be for compilation), or if
FindTabbedBrowser() returns NULL.  This happens in unittests, most notably those
for ExtensionService.  But I figured that, rather than crash, we should handle
it gracefully, on the off-chance it does happen in normal use.

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/ui/gtk/e...
File chrome/browser/ui/gtk/extensions/extension_install_dialog_gtk.cc (right):

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/ui/gtk/e...
chrome/browser/ui/gtk/extensions/extension_install_dialog_gtk.cc:202: if
(has_webstore_data) {
On 2014/01/24 11:38:31, Finnur wrote:
> I only see a unit test change on Mac. Is there an additional file for Mac
> missing with a change similar to this?

Whoops, yes there is.  Thanks! :)

[Yoyo Zhou, 2014-01-24 23:12:57]

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/external_install_ui.cc (left):

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/external_install_ui.cc:233: return;
On 2014/01/24 11:38:31, Finnur wrote:
> Tread carefully here.
> https://code.google.com/p/chromium/issues/detail?id=256640
> 
> I think there's a reason why we use the ID and not the extension pointer so I
> don't think it is safe to simplify this. Yoyo can probably say for sure.

Yeah, I don't remember the exact scenario, but it's risky to assume the
Extension doesn't get reloaded, uninstalled, etc. particularly because there's
an arbitrary amount of time between the delegate getting created and the user
acting on it (Proceed or Abort).

[Finnur, 2014-01-27 13:41:03]

Go back to looking up the extension pointer in the ExternalInstallDialogDelegate
and have someone verify the Mac changes and then we are good to go.

From my perspective I'm happy with the Mac changes I've seen, but I can't say if
the NIB changes are warranted/correct. I suggest sail should have the final say.

One nit:

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/external_install_ui.cc (right):

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/external_install_ui.cc:228: // If we don't have a
browser, we can't go to the webstore to fetch data.
It is certainly not unheard of to have these things return NULL in tests. We
don't have extension support on Android, so we won't get as far as showing the
install dialog. I would therefore document that this only happens in tests.

[Devlin, 2014-01-27 18:15:20]

Sail - mind taking a look?

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/external_install_ui.cc (left):

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/external_install_ui.cc:233: return;
On 2014/01/24 23:12:57, Yoyo Zhou wrote:
> On 2014/01/24 11:38:31, Finnur wrote:
> > Tread carefully here.
> > https://code.google.com/p/chromium/issues/detail?id=256640
> > 
> > I think there's a reason why we use the ID and not the extension pointer so
I
> > don't think it is safe to simplify this. Yoyo can probably say for sure.
> 
> Yeah, I don't remember the exact scenario, but it's risky to assume the
> Extension doesn't get reloaded, uninstalled, etc. particularly because there's
> an arbitrary amount of time between the delegate getting created and the user
> acting on it (Proceed or Abort).

Done.

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/external_install_ui.cc (right):

https://codereview.chromium.org/145153002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/external_install_ui.cc:228: // If we don't have a
browser, we can't go to the webstore to fetch data.
On 2014/01/27 13:41:04, Finnur wrote:
> It is certainly not unheard of to have these things return NULL in tests. We
> don't have extension support on Android, so we won't get as far as showing the
> install dialog. I would therefore document that this only happens in tests.

Done.

[Finnur, 2014-01-28 09:42:09]

LGTM.

And now, with a fresh mind, I understand how trivial the nibs change is and
asking for a second opinion from a Mac person is probably overkill. This is in
code I have changed myself and I think the change is correct.

I leave it up to you whether you want to wait for sail.

[Devlin, 2014-01-28 19:29:35]

Nico, would you mind taking a look at c/b/ui/cocoa?

[Nico, 2014-01-28 19:32:16]

When changing xib files, always include a section like
http://svnsearch.org/svnsearch/repos/CHROMIUM/search?logMessage=%22xib%20chan...
as the diff is so difficult to understand.

Other than that, cocoa/ lgtm

[Devlin, 2014-01-28 19:37:33]

On 2014/01/28 19:32:16, Nico wrote:
> When changing xib files, always include a section like
>
http://svnsearch.org/svnsearch/repos/CHROMIUM/search?logMessage=%2522xib%2520...
> as the diff is so difficult to understand.
> 
> Other than that, cocoa/ lgtm

Good point. Done. :)

[commit-bot: I haz the power, 2014-01-28 19:40:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/145153002/...

[commit-bot: I haz the power, 2014-01-28 22:21:45]

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

[commit-bot: I haz the power, 2014-01-28 23:09:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/145153002/...

[commit-bot: I haz the power, 2014-01-29 02:48:17]

Change committed as 247604