Allow the user to "repair" a corrupted extension.

Allow the user to "repair" a corrupted extension.

If an extension is detected to be corrupt, introduce an
option to repair it, if it is from the webstore.

Also reposition the "Reload" link (for reload terminated
extensions) to be next to the trash can, rather than above
it.

Screenshots: http://imgur.com/35tL9Xz,CslEoW5

BUG=410896
Committed: https://crrev.com/5f6b69d57487a2b4b43fed6b7334926ee87b9827
Cr-Commit-Position: refs/heads/master@{#295839}

[Devlin, 2014-09-12 18:48:59]

Patchset #1 (id:1) has been deleted

[Devlin, 2014-09-12 18:49:05]

Patchset #1 (id:20001) has been deleted

[Devlin, 2014-09-12 18:49:08]

Patchset #1 (id:30001) has been deleted

[Devlin, 2014-09-12 18:52:59]

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

[Devlin, 2014-09-12 18:52:59]

Dan and Yoyo, please take a look when time allows.
dbeam: c/b/resources, c/b/ui/webui
yoz: c/b/extensions, extensions/

[Devlin, 2014-09-18 18:46:41]

On 2014/09/12 18:52:59, Devlin wrote:
> Dan and Yoyo, please take a look when time allows.
> dbeam: c/b/resources, c/b/ui/webui
> yoz: c/b/extensions, extensions/

Friendly ping.

[Dan Beam, 2014-09-18 21:36:50]

lgtm w/nits

https://codereview.chromium.org/557953005/diff/50001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/557953005/diff/50001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:326:
terminatedReload.addEventListener('click', function(e) {
could this add event listeners more than once (meaning the extension could
reload 2+ times for 1 click)?

  terminatedReload.onclick = function() { ... };

would ensure this doesn't happen...

https://codereview.chromium.org/557953005/diff/50001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:332:
repair.addEventListener('click', function(e) {
same

https://codereview.chromium.org/557953005/diff/50001/chrome/browser/resources...
File chrome/browser/resources/extensions/extensions.css (right):

https://codereview.chromium.org/557953005/diff/50001/chrome/browser/resources...
chrome/browser/resources/extensions/extensions.css:260: link-button rules
without resorting to the Dark Side (!IMPORTANT). */
nit:

  /* Start each comment line with a star,
   * like this. */

https://codereview.chromium.org/557953005/diff/50001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/557953005/diff/50001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:218: //
ExtensionData. Please update it whenever you add or remove any keys here.
it actually worked! :)

[Yoyo Zhou, 2014-09-18 22:47:08]

LGTM

https://chromiumcodereview.appspot.com/557953005/diff/50001/chrome/browser/re...
File chrome/browser/resources/extensions/extensions.html (right):

https://chromiumcodereview.appspot.com/557953005/diff/50001/chrome/browser/re...
chrome/browser/resources/extensions/extensions.html:228: <div
class="corrupted-repair-button link-button"
The "repair" link looks very disconnected from the "corrupted" message in your
screenshot. I wouldn't think to associate one with the other given where they
are placed. Can they be closer?

https://chromiumcodereview.appspot.com/557953005/diff/50001/chrome/browser/ui...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://chromiumcodereview.appspot.com/557953005/diff/50001/chrome/browser/ui...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:1430:
MaybeUpdateAfterNotification();
Why do we need to run this callback if the uninstall doesn't succeed or if the
tab closes? It doesn't seem like there would need to be any updating.

[Devlin, 2014-09-18 23:19:10]

https://chromiumcodereview.appspot.com/557953005/diff/50001/chrome/browser/re...
File chrome/browser/resources/extensions/extensions.html (right):

https://chromiumcodereview.appspot.com/557953005/diff/50001/chrome/browser/re...
chrome/browser/resources/extensions/extensions.html:228: <div
class="corrupted-repair-button link-button"
On 2014/09/18 22:47:08, Yoyo Zhou wrote:
> The "repair" link looks very disconnected from the "corrupted" message in your
> screenshot. I wouldn't think to associate one with the other given where they
> are placed. Can they be closer?

It was designed to be most similar to the "Reload" (when the extension is
terminated), and also to make it clear why there wasn't an "enabled" option for
these "broken" extensions.  I think putting the link closer to the corrupt
message helps the tie between those two, but less of an understanding for "this
is what I have to do to enable it".  I'm happy to run the question by ainslie@
(the designer) though, if you want?

https://chromiumcodereview.appspot.com/557953005/diff/50001/chrome/browser/ui...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://chromiumcodereview.appspot.com/557953005/diff/50001/chrome/browser/ui...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:1430:
MaybeUpdateAfterNotification();
On 2014/09/18 22:47:08, Yoyo Zhou wrote:
> Why do we need to run this callback if the uninstall doesn't succeed or if the
> tab closes? It doesn't seem like there would need to be any updating.

To me, it seems safer and more consistent.  It would be odd (I think) to say
"Run the callback in the case of failures X, Y, and Z, but not failure W."  I'd
prefer to know that the callback will be run, no matter what, and then have the
callback determine if anything needs to be done.

Regarding updating even in these cases, we probably don't have to - unless the
extension got into a wonky state if the install failed or something.  It just
seems better to update.

But if you have a strong opinion on either part, I'm happy to change it :)

[Devlin, 2014-09-19 21:30:09]

https://codereview.chromium.org/557953005/diff/50001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/557953005/diff/50001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:326:
terminatedReload.addEventListener('click', function(e) {
On 2014/09/18 21:36:49, Dan Beam wrote:
> could this add event listeners more than once (meaning the extension could
> reload 2+ times for 1 click)?
> 
>   terminatedReload.onclick = function() { ... };
> 
> would ensure this doesn't happen...

I'm pretty sure it's not a risk - this is all in createNode_(), which clones a
new node, and this is the only place we set the listeners.  But I suppose it
can't hurt to be safe?

https://codereview.chromium.org/557953005/diff/50001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:332:
repair.addEventListener('click', function(e) {
On 2014/09/18 21:36:49, Dan Beam wrote:
> same

Done.

https://codereview.chromium.org/557953005/diff/50001/chrome/browser/resources...
File chrome/browser/resources/extensions/extensions.css (right):

https://codereview.chromium.org/557953005/diff/50001/chrome/browser/resources...
chrome/browser/resources/extensions/extensions.css:260: link-button rules
without resorting to the Dark Side (!IMPORTANT). */
On 2014/09/18 21:36:49, Dan Beam wrote:
> nit:
> 
>   /* Start each comment line with a star,
>    * like this. */

Done.

https://codereview.chromium.org/557953005/diff/50001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/557953005/diff/50001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:218: //
ExtensionData. Please update it whenever you add or remove any keys here.
On 2014/09/18 21:36:50, Dan Beam wrote:
> it actually worked! :)

;)

[Devlin, 2014-09-19 22:40:27]

On 2014/09/18 23:19:10, Devlin wrote:
>
https://chromiumcodereview.appspot.com/557953005/diff/50001/chrome/browser/re...
> File chrome/browser/resources/extensions/extensions.html (right):
> 
>
https://chromiumcodereview.appspot.com/557953005/diff/50001/chrome/browser/re...
> chrome/browser/resources/extensions/extensions.html:228: <div
> class="corrupted-repair-button link-button"
> On 2014/09/18 22:47:08, Yoyo Zhou wrote:
> > The "repair" link looks very disconnected from the "corrupted" message in
your
> > screenshot. I wouldn't think to associate one with the other given where
they
> > are placed. Can they be closer?
> 
> It was designed to be most similar to the "Reload" (when the extension is
> terminated), and also to make it clear why there wasn't an "enabled" option
for
> these "broken" extensions.  I think putting the link closer to the corrupt
> message helps the tie between those two, but less of an understanding for
"this
> is what I have to do to enable it".  I'm happy to run the question by ainslie@
> (the designer) though, if you want?
> 
>
https://chromiumcodereview.appspot.com/557953005/diff/50001/chrome/browser/ui...
> File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):
> 
>
https://chromiumcodereview.appspot.com/557953005/diff/50001/chrome/browser/ui...
> chrome/browser/ui/webui/extensions/extension_settings_handler.cc:1430:
> MaybeUpdateAfterNotification();
> On 2014/09/18 22:47:08, Yoyo Zhou wrote:
> > Why do we need to run this callback if the uninstall doesn't succeed or if
the
> > tab closes? It doesn't seem like there would need to be any updating.
> 
> To me, it seems safer and more consistent.  It would be odd (I think) to say
> "Run the callback in the case of failures X, Y, and Z, but not failure W." 
I'd
> prefer to know that the callback will be run, no matter what, and then have
the
> callback determine if anything needs to be done.
> 
> Regarding updating even in these cases, we probably don't have to - unless the
> extension got into a wonky state if the install failed or something.  It just
> seems better to update.
> 
> But if you have a strong opinion on either part, I'm happy to change it :)

Going ahead and pushing this through since there's some pressure to get this
done, and these are more clarification questions than anything else. But happy
to discuss more on Monday. :)

[Devlin, 2014-09-19 22:40:34]

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

[commit-bot: I haz the power, 2014-09-19 22:41:31]

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

[commit-bot: I haz the power, 2014-09-19 22:49:11]

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

[commit-bot: I haz the power, 2014-09-19 22:49:13]

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

[commit-bot: I haz the power, 2014-09-19 22:49:13]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/58521)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[commit-bot: I haz the power, 2014-09-19 22:49:17]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/58521)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[Devlin, 2014-09-19 23:09:31]

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

[commit-bot: I haz the power, 2014-09-19 23:10:47]

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

[commit-bot: I haz the power, 2014-09-20 01:23:44]

Committed patchset #3 (id:90001) as 3f8094c1f9da0dab4c5e3d13ebd9e6a4a65c4b50

[commit-bot: I haz the power, 2014-09-20 01:24:20]

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