Close the App Info Dialog if the app or profile becomes invalid

Close the App Info Dialog if the app or profile becomes invalid

Adds the App Info dialog as a profile observer, and closes the dialog
if the current profile is destroyed. Also, if the App Info Dialog is
open for an app and it is uninstalled asynchronously, close the dialog.
This prevents errors where actions can still be taken on the app even
though it's been removed.

BUG=384999, 408473
TEST=While the App Info Dialog is open for an app, uninstall the app.
The dialog and app list should close.
TEST=While the App Info Dialog is open, destroy the profile the app list
is open for. The dialog and app list should close.

Committed: https://crrev.com/58aac678723b1e9c82e2cf5572180b6923d19fc4
Cr-Commit-Position: refs/heads/master@{#297605}

[sashab, 2014-09-18 00:49:13]

sashab@chromium.org changed reviewers:
+ tapted@chromium.org

[sashab, 2014-09-18 00:49:13]

Feel free to pass this on if you don't have time to look at it.

[tapted, 2014-09-18 01:06:32]

can you add a test?

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:105:
StartObservingExtensionRegistry();
adding observers in the constructor/destructor is a pretty common pattern. I'd
just do it here,

  extensions::ExtensionRegistry::Get(profile_)->AddObserver(this);

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:109:
StopObservingExtensionRegistry();
and

  extensions::ExtensionRegistry::Get(profile_)->RemoveObserver(this);

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:143:
StopObservingExtensionRegistry();
can this just GetWidget()->CloseNow()? Pretty sure this only happens when the
Profile* is deleted, which means lots of other things will break.

You can test it out by deleting a profile from chrome://settings while the app
info dialog is displayed

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h
(right):

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:38: virtual
void OnExtensionLoaded(
can just leave this out

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:53: const
extensions::Extension* app_;
while it's not needed for other things - probably safer to make this a string,
just holding the ID

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:54:
extensions::ExtensionRegistry* extension_registry_;
this can probably be dropped

[Matt Giuca, 2014-09-18 01:30:57]

Also, can you add a TEST= line to the CL description so this gets added to the
manual testing checklist.

[Matt Giuca, 2014-09-18 01:33:52]

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

[Matt Giuca, 2014-09-18 01:33:53]

Drive-by.

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:138:
GetWidget()->Close();
Can you write a new method Close() which calls GetWidget()->Close() or whatever
other complicated logic you need to close this thing.

I'm putting this here because you sent me another CL which calls
GetWidget()->Close() in several places and it would be good to abstract it.

[sashab, 2014-09-18 03:00:01]

Unfortunately the async profile stuff is still needed, although I'm happy to
merge it into this CL if you think it's small enough. Added the unit tests as
well.

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:105:
StartObservingExtensionRegistry();
On 2014/09/18 01:06:32, tapted wrote:
> adding observers in the constructor/destructor is a pretty common pattern. I'd
> just do it here,
> 
>   extensions::ExtensionRegistry::Get(profile_)->AddObserver(this);

Done.

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:109:
StopObservingExtensionRegistry();
On 2014/09/18 01:06:32, tapted wrote:
> and
> 
>   extensions::ExtensionRegistry::Get(profile_)->RemoveObserver(this);

Done.

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:138:
GetWidget()->Close();
On 2014/09/18 01:33:52, Matt Giuca wrote:
> Can you write a new method Close() which calls GetWidget()->Close() or
whatever
> other complicated logic you need to close this thing.
> 
> I'm putting this here because you sent me another CL which calls
> GetWidget()->Close() in several places and it would be good to abstract it.

Sure. It's now in two places: AppInfoPanel (which can be used in all the
subviews), and this class just for here. Still makes sense to me to have it as a
separate function even though I only call it once in this class.

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:143:
StopObservingExtensionRegistry();
On 2014/09/18 01:06:32, tapted wrote:
> can this just GetWidget()->CloseNow()? Pretty sure this only happens when the
> Profile* is deleted, which means lots of other things will break.
> 
> You can test it out by deleting a profile from chrome://settings while the app
> info dialog is displayed

This is called too late to make the dialog resilient to async profile changes on
its own. Called Close() for now, but in reality the dialog will fail in this
situation anyway - chrome crashes long before this is called, through the App
List trying to close (from AppListServiceViews::DestroyAppList).

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h
(right):

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:38: virtual
void OnExtensionLoaded(
On 2014/09/18 01:06:32, tapted wrote:
> can just leave this out

Done.

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:53: const
extensions::Extension* app_;
On 2014/09/18 01:06:32, tapted wrote:
> while it's not needed for other things - probably safer to make this a string,
> just holding the ID

Done.

https://codereview.chromium.org/580713002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.h:54:
extensions::ExtensionRegistry* extension_registry_;
On 2014/09/18 01:06:32, tapted wrote:
> this can probably be dropped

Done.

[sashab, 2014-09-18 03:54:16]

Getting all kinds of errors unless I keep track of the extension registry. I
think if a profile is destroyed, OnShutdown() is not always called before
~AppInfoDialog(), so it's impossible to know if the observer should be removed
or not. Even if Get() returns not-NULL, the stack trace caused from
~AppInfoDialog() is:

Received signal 11 <unknown> 000000000000
...
#3 0x000004def67a
extensions::ChromeExtensionsBrowserClient::GetOriginalContext()
#4 0x00000531d0e8 extensions::ExtensionRegistryFactory::GetBrowserContextToUse()
#5 0x7f57ed2d8398
BrowserContextKeyedServiceFactory::GetServiceForBrowserContext()
#6 0x00000531cfb2 extensions::ExtensionRegistryFactory::GetForBrowserContext()
#7 0x000005317325 extensions::ExtensionRegistry::Get()
#8 0x00000430b78f AppInfoDialog::~AppInfoDialog()
#9 0x00000430b929 AppInfoDialog::~AppInfoDialog()

[tapted, 2014-09-18 05:20:19]

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:111:
chrome::NOTIFICATION_PROFILE_DESTROYED,
Let's try it without this: i'm fairly confident about the ExtensionRegistry
calling OnShutdown before its profile is destroyed.

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc
(right):

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:43:
&command_line_,
Can you just pass NULL for the CommandLine? TestExtensionSystem seems to have
code to handle that

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:64:
void DestroyProfile() { profile_.reset(); }
`protected:` data members are allowed in test harnesses - I would just do that
rather than have this and the widget/profile accessors below

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:66:
void UninstallApp(std::string app_id) {
nit: const reference

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:76:
scoped_ptr<base::DictionaryValue> ValidAppManifest() {
It would be nice to consolidate these.. maybe in extension_test_util.h, but that
can be orthogonal.

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:104:
content::TestBrowserThread ui_thread_;
Did you explore alternatives to this? -- it's marked deprecated... but you'd
probably need to convert ViewsTestBase to use it, or give
TestBrowserThreadBundle an option to use MessageLoop::current() instead of
creating its own MessageLoopForUI

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:136:
EXPECT_TRUE(widget()->IsClosed());
I think you need a widget observer - the widget shouldn't just be closed, it
should be destroyed, so you can't use it here, and you can't call CloseNow() on
it in TearDown

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:142:
other_profile.reset(new TestingProfile());
Note this profile doesn't have an extensionsystem, so won't call OnShutdown (for
the other profile)

[sashab, 2014-09-22 05:41:02]

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:111:
chrome::NOTIFICATION_PROFILE_DESTROYED,
On 2014/09/18 05:20:18, tapted wrote:
> Let's try it without this: i'm fairly confident about the ExtensionRegistry
> calling OnShutdown before its profile is destroyed.

Ohhh I see what you're saying. Sorry, I was confusing the two errors (this and
the one from not saving the extension_registry). Yup, this totally works. Done.

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc
(right):

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:43:
&command_line_,
On 2014/09/18 05:20:18, tapted wrote:
> Can you just pass NULL for the CommandLine? TestExtensionSystem seems to have
> code to handle that

TestExtensionSystem, but ExtensionSystem doesn't:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:64:
void DestroyProfile() { profile_.reset(); }
On 2014/09/18 05:20:18, tapted wrote:
> `protected:` data members are allowed in test harnesses - I would just do that
> rather than have this and the widget/profile accessors below

Done.

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:66:
void UninstallApp(std::string app_id) {
On 2014/09/18 05:20:18, tapted wrote:
> nit: const reference

Done.

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:76:
scoped_ptr<base::DictionaryValue> ValidAppManifest() {
On 2014/09/18 05:20:18, tapted wrote:
> It would be nice to consolidate these.. maybe in extension_test_util.h, but
that
> can be orthogonal.

Added a TODO.

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:104:
content::TestBrowserThread ui_thread_;
On 2014/09/18 05:20:18, tapted wrote:
> Did you explore alternatives to this? -- it's marked deprecated... but you'd
> probably need to convert ViewsTestBase to use it, or give
> TestBrowserThreadBundle an option to use MessageLoop::current() instead of
> creating its own MessageLoopForUI

Yup, added a TODO and a comment to crbug.com/272091.

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:136:
EXPECT_TRUE(widget()->IsClosed());
On 2014/09/18 05:20:18, tapted wrote:
> I think you need a widget observer - the widget shouldn't just be closed, it
> should be destroyed, so you can't use it here, and you can't call CloseNow()
on
> it in TearDown

Done.

https://codereview.chromium.org/580713002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:142:
other_profile.reset(new TestingProfile());
On 2014/09/18 05:20:18, tapted wrote:
> Note this profile doesn't have an extensionsystem, so won't call OnShutdown
(for
> the other profile)

Done.

[tapted, 2014-09-23 07:40:36]

lgtm with a few changes - sorry for the lag! This one got inbox-buried..

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:10:
#include "chrome/browser/chrome_notification_types.h"
nit: not needed any more

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc
(right):

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:30:
: widget_destroyed_(false),
nit: initialize widget_(NULL)

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:67:
}
you should do widget_ = NULL here too - it should be deleted as soon as this
function call returns (see e.g. NativeWidgetAura::OnWindowDestroyed)

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:81:
static_cast<extensions::TestExtensionSystem*>(
nit: I don't think you need to static cast here 

extensions::ExtensionSystem::Get(profile)->extension_service()->AddExtension(app);
should work

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:137:
EXPECT_TRUE(widget_->IsClosed());
I think this is a use-after-free, but can be dropped since it's redundant with
the check below

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:166:
EXPECT_TRUE(widget_->IsClosed());
remove

[sashab, 2014-09-24 01:15:37]

The CQ bit was checked by sashab@chromium.org

[sashab, 2014-09-24 01:15:45]

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc
(right):

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views.cc:10:
#include "chrome/browser/chrome_notification_types.h"
On 2014/09/23 07:40:36, tapted wrote:
> nit: not needed any more

Done, tyvm :)

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc
(right):

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:30:
: widget_destroyed_(false),
On 2014/09/23 07:40:36, tapted wrote:
> nit: initialize widget_(NULL)

Done.

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:67:
}
On 2014/09/23 07:40:36, tapted wrote:
> you should do widget_ = NULL here too - it should be deleted as soon as this
> function call returns (see e.g. NativeWidgetAura::OnWindowDestroyed)

Done.

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:81:
static_cast<extensions::TestExtensionSystem*>(
On 2014/09/23 07:40:36, tapted wrote:
> nit: I don't think you need to static cast here 
> 
>
extensions::ExtensionSystem::Get(profile)->extension_service()->AddExtension(app);
> should work

Done.

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:137:
EXPECT_TRUE(widget_->IsClosed());
On 2014/09/23 07:40:36, tapted wrote:
> I think this is a use-after-free, but can be dropped since it's redundant with
> the check below

Hmm, the test was passing, which is strange... But done. Left the IsClosed()
check above in.

https://codereview.chromium.org/580713002/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:166:
EXPECT_TRUE(widget_->IsClosed());
On 2014/09/23 07:40:36, tapted wrote:
> remove

Done.

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

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

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

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

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

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

[sashab, 2014-09-25 00:24:56]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-09-25 00:26:26]

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

[commit-bot: I haz the power, 2014-09-25 01:47:35]

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

[commit-bot: I haz the power, 2014-09-25 01:47:36]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sashab, 2014-09-25 20:52:17]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-09-25 20:53:20]

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

[commit-bot: I haz the power, 2014-09-25 22:45:07]

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

[commit-bot: I haz the power, 2014-09-25 22:45:09]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2014-10-01 01:41:35]

https://codereview.chromium.org/580713002/diff/170001/chrome/browser/ui/views...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc
(right):

https://codereview.chromium.org/580713002/diff/170001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:16:
#include "ui/views/test/widget_test.h"
remove?

https://codereview.chromium.org/580713002/diff/170001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:31:
class TestModalDialogDelegate : public views::DialogDelegateView {
comment?

https://codereview.chromium.org/580713002/diff/170001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:141:
#if defined OS_CHROMEOS
can these be private? (also, a comment. And, do they need to be in a particular
order?)

[sashab, 2014-10-01 03:34:18]

Small fixes :) Should all be OK now. tapted ptal if you like :)

https://codereview.chromium.org/580713002/diff/170001/chrome/browser/ui/views...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc
(right):

https://codereview.chromium.org/580713002/diff/170001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:16:
#include "ui/views/test/widget_test.h"
On 2014/10/01 01:41:35, tapted wrote:
> remove?

Done.

https://codereview.chromium.org/580713002/diff/170001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:31:
class TestModalDialogDelegate : public views::DialogDelegateView {
On 2014/10/01 01:41:35, tapted wrote:
> comment?

This is not actually needed :S Changed to just use views::DialogDelegateView
directly.

https://codereview.chromium.org/580713002/diff/170001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc:141:
#if defined OS_CHROMEOS
On 2014/10/01 01:41:35, tapted wrote:
> can these be private? (also, a comment. And, do they need to be in a
particular
> order?)

Done.

[sashab, 2014-10-01 07:23:45]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-10-01 07:24:43]

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

[commit-bot: I haz the power, 2014-10-01 07:29:06]

Committed patchset #11 (id:190001) as b835ca5671047e120b6c65a20f21c2354a644ef5

[commit-bot: I haz the power, 2014-10-01 07:29:49]

Patchset 11 (id:??) landed as
https://crrev.com/58aac678723b1e9c82e2cf5572180b6923d19fc4
Cr-Commit-Position: refs/heads/master@{#297605}