Show crashed apps on NTP, desaturated. Click to reload.

Show crashed apps on NTP, desaturated. Click to reload.

Also refactored CreateAppInfo a bit.

BUG=90433
TEST=Crash a packaged app with a background page. It appears on the NTP, and clicking its icon relaunches it.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95009

[jstritar, 2011-07-28 15:05:26]

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:117: }
You don't need these { } braces, but you can keep them if you'd like.

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:320: DictionaryValue*
app_info = CreateAppInfo(*it,
Can all these call GetAppInfo?

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:773: }
Maybe you don't need was_terminated?

if (!extension) {
  // Reload the extension because it was terminated.
  extension = extension_service_->GetTerminatedExtension(extension_id);
  extension_service_->ReloadExtension(extension_id);
}

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:54: virtual void
RegisterMessages() OVERRIDE;
thanks for adding all these

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:152: ExtensionService* const
extension_service_;
nice cleanup

[Evan Stade, 2011-07-28 16:33:21]

LGTM, just nits

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:117: }
actually, you can't keep them because the precedent in this file has already
been set to the contrary

however it seems this could/should be combined with the logic above, i.e.

bool enabled = IsEnabled && !GetTerminatedExtension;

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:776: if (was_terminated) {
ditto

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:811: false /*
external_uninstall */, NULL);
indent changed

[Yoyo Zhou, 2011-07-28 17:02:49]

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:117: }
On 2011/07/28 16:33:21, Evan Stade wrote:
> actually, you can't keep them because the precedent in this file has already
> been set to the contrary
> 
> however it seems this could/should be combined with the logic above, i.e.
> 
> bool enabled = IsEnabled && !GetTerminatedExtension;

Done.

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:320: DictionaryValue*
app_info = CreateAppInfo(*it,
On 2011/07/28 15:05:26, jstritar wrote:
> Can all these call GetAppInfo?

I'm not sure -- it was done this way even before GetAppInfo was added. +asargent
since he added the notification parameter.

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:773: }
On 2011/07/28 15:05:26, jstritar wrote:
> Maybe you don't need was_terminated?
> 
> if (!extension) {
>   // Reload the extension because it was terminated.
>   extension = extension_service_->GetTerminatedExtension(extension_id);
>   extension_service_->ReloadExtension(extension_id);
> }

Ah, yes, I changed this and didn't see this afterward. (Though extension isn't
actually used here -- it's more asking if the service has that extension ID.)

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:811: false /*
external_uninstall */, NULL);
On 2011/07/28 16:33:21, Evan Stade wrote:
> indent changed

fixed.

[Yoyo Zhou, 2011-07-28 17:15:00]

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:773: }
On 2011/07/28 17:02:49, Yoyo Zhou wrote:
> On 2011/07/28 15:05:26, jstritar wrote:
> > Maybe you don't need was_terminated?
> > 
> > if (!extension) {
> >   // Reload the extension because it was terminated.
> >   extension = extension_service_->GetTerminatedExtension(extension_id);
> >   extension_service_->ReloadExtension(extension_id);
> > }
> 
> Ah, yes, I changed this and didn't see this afterward. (Though extension isn't
> actually used here -- it's more asking if the service has that extension ID.)

Oh, also, Reloading invalidates the 'extension' pointer here (the Extension gets
freed and a new Extension object is allocated). Is it good practice to
proactively set the pointer to NULL?

[Evan Stade, 2011-07-28 17:35:16]

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7517019/diff/7001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:773: }
On 2011/07/28 17:15:00, Yoyo Zhou wrote:
> On 2011/07/28 17:02:49, Yoyo Zhou wrote:
> > On 2011/07/28 15:05:26, jstritar wrote:
> > > Maybe you don't need was_terminated?
> > > 
> > > if (!extension) {
> > >   // Reload the extension because it was terminated.
> > >   extension = extension_service_->GetTerminatedExtension(extension_id);
> > >   extension_service_->ReloadExtension(extension_id);
> > > }
> > 
> > Ah, yes, I changed this and didn't see this afterward. (Though extension
isn't
> > actually used here -- it's more asking if the service has that extension
ID.)
> 
> Oh, also, Reloading invalidates the 'extension' pointer here (the Extension
gets
> freed and a new Extension object is allocated). Is it good practice to
> proactively set the pointer to NULL?

yes

[jstritar, 2011-07-28 17:47:23]

http://codereview.chromium.org/7517019/diff/2006/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7517019/diff/2006/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:790:
GetExtensionInstallUI()->ConfirmReEnable(this, extension);
what about the extension pointer here, if its invalidated from the
ReloadExtension above?

[Yoyo Zhou, 2011-07-28 18:10:28]

http://codereview.chromium.org/7517019/diff/2006/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7517019/diff/2006/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:790:
GetExtensionInstallUI()->ConfirmReEnable(this, extension);
On 2011/07/28 17:47:24, jstritar wrote:
> what about the extension pointer here, if its invalidated from the
> ReloadExtension above?

I just saw this too -- this is the mythical "terminated and disabled" case, but
just in case it happens, I freshened the extension pointer instead of NULLing
it.