Refactor ShellDesktopController and ShellNativeAppWindow to allow for non-aura implementations.

Refactor ShellDesktopController and ShellNativeAppWindow to allow for non-Aura implementations.

Rename ShellDesktopController to ShellDesktopControllerAura; make ShellNativeAppWindow implement only some of the BaseWindow methods, and move aura-specific bits to ShellNativeAppWindowAura. Adds a unit test for ShellNativeAppWindowAura.

This is a precursor to a Cocoa implementation for Mac.

BUG=399363
TBR=danakj@chromium.org,reed@google.com

Committed: https://crrev.com/0e463fafeca7afbda7e4060a7ce591f7eab82c2f
Cr-Commit-Position: refs/heads/master@{#304897}

[Yoyo Zhou, 2014-11-11 01:51:31]

yoz@chromium.org changed reviewers:
+ jamescook@chromium.org

[Yoyo Zhou, 2014-11-11 01:52:15]

Patchset #1 (id:1) has been deleted

[Yoyo Zhou, 2014-11-11 01:55:31]

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/default_shell_browser_main_delegate.cc (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/default_shell_browser_main_delegate.cc:59: #if
defined(USE_AURA)
These look a little funny now, but they'll make sense with an #elif
defined(USE_MACOSX) soon.

[James Cook, 2014-11-11 18:23:53]

Overall approach looks good, questions on some details.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/app/she...
File extensions/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/app/she...
extensions/shell/app/shell_main_delegate.cc:25: #if !defined(DISABLE_NACL) &&
defined(OS_POSIX) && !defined(OS_MACOSX) && \
Are you sure this is right? kNaClLoaderProcess are used below in some code that
is just wrapped with !defined(DISABLE_NACL).

Is your plan to only support NaCl on Posix?  It would be nice if it had a chance
of working elsewhere for tests.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/app/she...
extensions/shell/app/shell_main_delegate.cc:146: process_type ==
switches::kGpuProcess;
In Chrome this is only needed on Mac OS -- is this right?

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/default_shell_browser_main_delegate.cc (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/default_shell_browser_main_delegate.cc:59: #if
defined(USE_AURA)
On 2014/11/11 01:55:31, Yoyo Zhou wrote:
> These look a little funny now, but they'll make sense with an #elif
> defined(USE_MACOSX) soon.

Acknowledged.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/desktop_controller.h (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/desktop_controller.h:55: virtual void
AddAppWindow(gfx::NativeWindow window) = 0;
Do you need to change the athena build for this?  chromeos=1 and build
athena_main to try it.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_app_window_client.cc (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_app_window_client.cc:30: #if defined(USE_AURA)
Are you sure you want to ifdef out the whole function, rather than having it
return nullptr on non-aura?

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_browser_main_delegate.h (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_browser_main_delegate.h:29: // window
rewrap comment

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_browser_main_parts.cc (left):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_browser_main_parts.cc:37: #include
"ui/aura/window_tree_host.h"
Thanks for cleaning this up.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_desktop_controller_aura.h (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller_aura.h:85: virtual void
PowerButtonEventReceived(
nit: no virtual

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller_aura.h:90: virtual void
OnDisplayModeChanged(
nit: no virtual

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_native_app_window_aura.cc (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura.cc:16:
extensions::AppWindow* app_window,
no need for extensions:: here or below

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura.cc:71: }  // namespace
extensions
I know you're just refactoring here, but can you write a trivial unit test for
this class? Just enough to get it into a test harness and verify, for example,
that it gets set up with the right bounds.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_native_app_window_aura.h (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura.h:12: class
ShellNativeAppWindowAura : public ShellNativeAppWindow {
class comment please

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura.h:14:
ShellNativeAppWindowAura(extensions::AppWindow* app_window,
no need for extensions::

[Yoyo Zhou, 2014-11-15 01:05:45]

I added a test, but it ended up being somewhat more involved than I think you
may have hoped for. I still think we may run into issues if we need extensions
unittests that require content::RenderViewHostTestHarness (you can't inherit
from multiple test base classes); luckily, this one turned out not to need it.

I refactored a TestAppWindowContents out of ProcessPowserCollectorTest and
cleaned it/AppWindow up a little.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/app/she...
File extensions/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/app/she...
extensions/shell/app/shell_main_delegate.cc:25: #if !defined(DISABLE_NACL) &&
defined(OS_POSIX) && !defined(OS_MACOSX) && \
On 2014/11/11 18:23:52, James Cook wrote:
> Are you sure this is right? kNaClLoaderProcess are used below in some code
that
> is just wrapped with !defined(DISABLE_NACL).
> 
> Is your plan to only support NaCl on Posix?  It would be nice if it had a
chance
> of working elsewhere for tests.

These are the requirements for ZygoteStarting, but I missed that
kNaClLoaderProcess is used below. I agree it would be nice to have working NaCl,
but it's not my first priority. In this patch I'm just isolating out the parts
with Linux-specific stuff.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/app/she...
extensions/shell/app/shell_main_delegate.cc:146: process_type ==
switches::kGpuProcess;
On 2014/11/11 18:23:52, James Cook wrote:
> In Chrome this is only needed on Mac OS -- is this right?

I think that's right.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/desktop_controller.h (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/desktop_controller.h:55: virtual void
AddAppWindow(gfx::NativeWindow window) = 0;
On 2014/11/11 18:23:52, James Cook wrote:
> Do you need to change the athena build for this?  chromeos=1 and build
> athena_main to try it.

Good catch.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_app_window_client.cc (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_app_window_client.cc:30: #if defined(USE_AURA)
On 2014/11/11 18:23:52, James Cook wrote:
> Are you sure you want to ifdef out the whole function, rather than having it
> return nullptr on non-aura?

Yes, this is confusing: I can't include Obj-C from a .cc file, which
ShellNativeAppWindowMac is going to be. So the mac version of this function has
to be defined in a .mm file. I could make a separate _aura.cc file for this
function, but it seemed like overkill; on the other hand, it'd be more
consistent. Any preference?

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_browser_main_delegate.h (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_browser_main_delegate.h:29: // window
On 2014/11/11 18:23:52, James Cook wrote:
> rewrap comment

Done.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_desktop_controller_aura.h (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller_aura.h:85: virtual void
PowerButtonEventReceived(
On 2014/11/11 18:23:52, James Cook wrote:
> nit: no virtual

Done.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller_aura.h:90: virtual void
OnDisplayModeChanged(
On 2014/11/11 18:23:52, James Cook wrote:
> nit: no virtual

Done.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_native_app_window_aura.cc (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura.cc:16:
extensions::AppWindow* app_window,
On 2014/11/11 18:23:52, James Cook wrote:
> no need for extensions:: here or below

Done.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_native_app_window_aura.h (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura.h:12: class
ShellNativeAppWindowAura : public ShellNativeAppWindow {
On 2014/11/11 18:23:52, James Cook wrote:
> class comment please

Done.

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura.h:14:
ShellNativeAppWindowAura(extensions::AppWindow* app_window,
On 2014/11/11 18:23:52, James Cook wrote:
> no need for extensions::

Done.

[James Cook, 2014-11-17 17:48:36]

Have to run to an interview, I'll finish review when I'm back

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_app_window_client.cc (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_app_window_client.cc:30: #if defined(USE_AURA)
On 2014/11/15 01:05:44, Yoyo Zhou wrote:
> On 2014/11/11 18:23:52, James Cook wrote:
> > Are you sure you want to ifdef out the whole function, rather than having it
> > return nullptr on non-aura?
> 
> Yes, this is confusing: I can't include Obj-C from a .cc file, which
> ShellNativeAppWindowMac is going to be. So the mac version of this function
has
> to be defined in a .mm file. I could make a separate _aura.cc file for this
> function, but it seemed like overkill; on the other hand, it'd be more
> consistent. Any preference?

I don't understand. Are you saying you would create a
shell_app_window_client_aura.cc vs. shell_app_window_client_mac.cc?  Or that the
header for shell_native_app_window_mac.h is going to have stuff in it that you
can't #include here?

If the latter, how do we handle that elsewhere?  Can we do pointer-to-impl and
have the impl in the .mm class?

https://codereview.chromium.org/696063008/diff/60001/extensions/browser/app_w...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/696063008/diff/60001/extensions/browser/app_w...
extensions/browser/app_window/app_window.cc:456: if (modal_dialog_manager)  //
May be NULL in unit tests.
null

https://codereview.chromium.org/696063008/diff/60001/extensions/browser/app_w...
extensions/browser/app_window/app_window.cc:457:
modal_dialog_manager->SetDelegate(NULL);
nullptr

https://codereview.chromium.org/696063008/diff/60001/extensions/browser/app_w...
File extensions/browser/app_window/test_app_window_contents.h (right):

https://codereview.chromium.org/696063008/diff/60001/extensions/browser/app_w...
extensions/browser/app_window/test_app_window_contents.h:13: // Best used with
AppWindow::SetAppWindowContentsForTesting().
Thanks for the helpful class comment!

https://codereview.chromium.org/696063008/diff/60001/extensions/browser/app_w...
extensions/browser/app_window/test_app_window_contents.h:16: explicit
TestAppWindowContents(content::WebContents* web_contents);
nit: While not strictly necessary, forward declare content::WebContents?

https://codereview.chromium.org/696063008/diff/60001/extensions/browser/app_w...
extensions/browser/app_window/test_app_window_contents.h:29: };
DISALLOW_COPY_AND_ASSIGN + macros.h?

[Yoyo Zhou, 2014-11-17 19:35:55]

Just responding to this question first:

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_app_window_client.cc (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_app_window_client.cc:30: #if defined(USE_AURA)
On 2014/11/17 17:48:36, James Cook wrote:
> On 2014/11/15 01:05:44, Yoyo Zhou wrote:
> > On 2014/11/11 18:23:52, James Cook wrote:
> > > Are you sure you want to ifdef out the whole function, rather than having
it
> > > return nullptr on non-aura?
> > 
> > Yes, this is confusing: I can't include Obj-C from a .cc file, which
> > ShellNativeAppWindowMac is going to be. So the mac version of this function
> has
> > to be defined in a .mm file. I could make a separate _aura.cc file for this
> > function, but it seemed like overkill; on the other hand, it'd be more
> > consistent. Any preference?
> 
> I don't understand. Are you saying you would create a
> shell_app_window_client_aura.cc vs. shell_app_window_client_mac.cc?  Or that
the
> header for shell_native_app_window_mac.h is going to have stuff in it that you
> can't #include here?
> 
> If the latter, how do we handle that elsewhere?  Can we do pointer-to-impl and
> have the impl in the .mm class?

In my mac build, I have a shell_app_window_client_mac.mm (no separate .h) which
defines this function. So on Mac, that file gets picked up, and this ifdef hides
the definition here. On Linux/Win with Aura, that file doesn't get picked up and
this ifdef is enabled.

My alternative suggestion is to still keep the common header file, but just
implement this function in shell_app_window_client_aura.cc. I don't know exactly
what the best practice is for this.

[James Cook, 2014-11-17 20:54:22]

Looking good, just nits at this point

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_app_window_client.cc (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_app_window_client.cc:30: #if defined(USE_AURA)
On 2014/11/17 19:35:55, Yoyo Zhou wrote:
> On 2014/11/17 17:48:36, James Cook wrote:
> > On 2014/11/15 01:05:44, Yoyo Zhou wrote:
> > > On 2014/11/11 18:23:52, James Cook wrote:
> > > > Are you sure you want to ifdef out the whole function, rather than
having
> it
> > > > return nullptr on non-aura?
> > > 
> > > Yes, this is confusing: I can't include Obj-C from a .cc file, which
> > > ShellNativeAppWindowMac is going to be. So the mac version of this
function
> > has
> > > to be defined in a .mm file. I could make a separate _aura.cc file for
this
> > > function, but it seemed like overkill; on the other hand, it'd be more
> > > consistent. Any preference?
> > 
> > I don't understand. Are you saying you would create a
> > shell_app_window_client_aura.cc vs. shell_app_window_client_mac.cc?  Or that
> the
> > header for shell_native_app_window_mac.h is going to have stuff in it that
you
> > can't #include here?
> > 
> > If the latter, how do we handle that elsewhere?  Can we do pointer-to-impl
and
> > have the impl in the .mm class?
> 
> In my mac build, I have a shell_app_window_client_mac.mm (no separate .h)
which
> defines this function. So on Mac, that file gets picked up, and this ifdef
hides
> the definition here. On Linux/Win with Aura, that file doesn't get picked up
and
> this ifdef is enabled.
> 
> My alternative suggestion is to still keep the common header file, but just
> implement this function in shell_app_window_client_aura.cc. I don't know
exactly
> what the best practice is for this.

I think separate files is best practice.

You could also have two entirely separate ShellAppWindowClientMac and
ShellAppWindowClientAura classes and a single factory function to create the one
you want. That's also common practice.

https://codereview.chromium.org/696063008/diff/60001/extensions/shell/browser...
File extensions/shell/browser/shell_native_app_window_aura.h (right):

https://codereview.chromium.org/696063008/diff/60001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura.h:30:
DISALLOW_COPY_AND_ASSIGN(ShellNativeAppWindowAura);
#include "base/macros.h"

https://codereview.chromium.org/696063008/diff/60001/extensions/shell/browser...
File extensions/shell/browser/shell_native_app_window_aura_unittest.cc (right):

https://codereview.chromium.org/696063008/diff/60001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura_unittest.cc:28: :
notification_service_(content::NotificationService::Create()) {
Not necessarily for this CL: I think we should create a
content::NotificationService in ExtensionsTest. I get bitten in the ass by this
every time I make a new test and it's pretty confusing.  WDYT?

https://codereview.chromium.org/696063008/diff/60001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura_unittest.cc:34: void
TearDown() override { ExtensionsTest::TearDown(); }
not needed

https://codereview.chromium.org/696063008/diff/60001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura_unittest.cc:63:
window_spec.bounds = gfx::Rect(100, 200, 300, 400);
+1 for varying the data!

https://codereview.chromium.org/696063008/diff/60001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura_unittest.cc:70:
ASSERT_EQ(window_spec.bounds, bounds);
EXPECT_EQ? Do you want the test to crash if this doesn't match?

https://codereview.chromium.org/696063008/diff/60001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura_unittest.cc:76: }  //
namespace extensions
Thanks for writing this test. I know it was a pain, but it helps find places
where the testing system isn't easy enough to use.

[Yoyo Zhou, 2014-11-17 23:55:27]

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_app_window_client.cc (right):

https://codereview.chromium.org/696063008/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_app_window_client.cc:30: #if defined(USE_AURA)
On 2014/11/17 20:54:22, James Cook wrote:
> On 2014/11/17 19:35:55, Yoyo Zhou wrote:
> > On 2014/11/17 17:48:36, James Cook wrote:
> > > On 2014/11/15 01:05:44, Yoyo Zhou wrote:
> > > > On 2014/11/11 18:23:52, James Cook wrote:
> > > > > Are you sure you want to ifdef out the whole function, rather than
> having
> > it
> > > > > return nullptr on non-aura?
> > > > 
> > > > Yes, this is confusing: I can't include Obj-C from a .cc file, which
> > > > ShellNativeAppWindowMac is going to be. So the mac version of this
> function
> > > has
> > > > to be defined in a .mm file. I could make a separate _aura.cc file for
> this
> > > > function, but it seemed like overkill; on the other hand, it'd be more
> > > > consistent. Any preference?
> > > 
> > > I don't understand. Are you saying you would create a
> > > shell_app_window_client_aura.cc vs. shell_app_window_client_mac.cc?  Or
that
> > the
> > > header for shell_native_app_window_mac.h is going to have stuff in it that
> you
> > > can't #include here?
> > > 
> > > If the latter, how do we handle that elsewhere?  Can we do pointer-to-impl
> and
> > > have the impl in the .mm class?
> > 
> > In my mac build, I have a shell_app_window_client_mac.mm (no separate .h)
> which
> > defines this function. So on Mac, that file gets picked up, and this ifdef
> hides
> > the definition here. On Linux/Win with Aura, that file doesn't get picked up
> and
> > this ifdef is enabled.
> > 
> > My alternative suggestion is to still keep the common header file, but just
> > implement this function in shell_app_window_client_aura.cc. I don't know
> exactly
> > what the best practice is for this.
> 
> I think separate files is best practice.
> 
> You could also have two entirely separate ShellAppWindowClientMac and
> ShellAppWindowClientAura classes and a single factory function to create the
one
> you want. That's also common practice.

I moved this function  to a separate file.

https://codereview.chromium.org/696063008/diff/60001/extensions/browser/app_w...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/696063008/diff/60001/extensions/browser/app_w...
extensions/browser/app_window/app_window.cc:456: if (modal_dialog_manager)  //
May be NULL in unit tests.
On 2014/11/17 17:48:36, James Cook wrote:
> null

Done.

https://codereview.chromium.org/696063008/diff/60001/extensions/browser/app_w...
extensions/browser/app_window/app_window.cc:457:
modal_dialog_manager->SetDelegate(NULL);
On 2014/11/17 17:48:36, James Cook wrote:
> nullptr

Done.

https://codereview.chromium.org/696063008/diff/60001/extensions/browser/app_w...
File extensions/browser/app_window/test_app_window_contents.h (right):

https://codereview.chromium.org/696063008/diff/60001/extensions/browser/app_w...
extensions/browser/app_window/test_app_window_contents.h:16: explicit
TestAppWindowContents(content::WebContents* web_contents);
On 2014/11/17 17:48:36, James Cook wrote:
> nit: While not strictly necessary, forward declare content::WebContents?

Done.

https://codereview.chromium.org/696063008/diff/60001/extensions/browser/app_w...
extensions/browser/app_window/test_app_window_contents.h:29: };
On 2014/11/17 17:48:36, James Cook wrote:
> DISALLOW_COPY_AND_ASSIGN + macros.h?

Done.

https://codereview.chromium.org/696063008/diff/60001/extensions/shell/browser...
File extensions/shell/browser/shell_native_app_window_aura.h (right):

https://codereview.chromium.org/696063008/diff/60001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura.h:30:
DISALLOW_COPY_AND_ASSIGN(ShellNativeAppWindowAura);
On 2014/11/17 20:54:22, James Cook wrote:
> #include "base/macros.h"

Done.

https://codereview.chromium.org/696063008/diff/60001/extensions/shell/browser...
File extensions/shell/browser/shell_native_app_window_aura_unittest.cc (right):

https://codereview.chromium.org/696063008/diff/60001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura_unittest.cc:28: :
notification_service_(content::NotificationService::Create()) {
On 2014/11/17 20:54:22, James Cook wrote:
> Not necessarily for this CL: I think we should create a
> content::NotificationService in ExtensionsTest. I get bitten in the ass by
this
> every time I make a new test and it's pretty confusing.  WDYT?

I agree. Added a TODO.

https://codereview.chromium.org/696063008/diff/60001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura_unittest.cc:34: void
TearDown() override { ExtensionsTest::TearDown(); }
On 2014/11/17 20:54:22, James Cook wrote:
> not needed

Done.

https://codereview.chromium.org/696063008/diff/60001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura_unittest.cc:70:
ASSERT_EQ(window_spec.bounds, bounds);
On 2014/11/17 20:54:22, James Cook wrote:
> EXPECT_EQ? Do you want the test to crash if this doesn't match?

Done.

[Yoyo Zhou, 2014-11-17 23:57:05]

derat: please review chrome/browser/power/process_power_collector_unittest.cc
oshima: please review athena/
danakj: please review DEPS addition +ui/gfx in athena/main/DEPS

[Yoyo Zhou, 2014-11-17 23:57:36]

yoz@chromium.org changed reviewers:
+ danakj@chromium.org, derat@chromium.org, oshima@chromium.org

[Yoyo Zhou, 2014-11-17 23:57:37]

Let's try that again...

derat: please review chrome/browser/power/process_power_collector_unittest.cc
oshima: please review athena/
danakj: please review DEPS addition +ui/gfx in athena/main/DEPS

[Daniel Erat, 2014-11-17 23:58:47]

chrome/browser/power/process_power_collector_unittest.cc lgtm

[oshima, 2014-11-18 00:06:18]

athena lgtm

[James Cook, 2014-11-18 00:12:27]

LGTM with nit

https://codereview.chromium.org/696063008/diff/80001/extensions/browser/app_w...
File extensions/browser/app_window/test_app_window_contents.h (right):

https://codereview.chromium.org/696063008/diff/80001/extensions/browser/app_w...
extensions/browser/app_window/test_app_window_contents.h:10: #include
"base/macros.h"
nit: alphabetize, since this is .h file

https://codereview.chromium.org/696063008/diff/80001/extensions/shell/browser...
File extensions/shell/browser/shell_native_app_window_aura.h (right):

https://codereview.chromium.org/696063008/diff/80001/extensions/shell/browser...
extensions/shell/browser/shell_native_app_window_aura.h:10: #include
"base/macros.h"
ditto

[Yoyo Zhou, 2014-11-18 00:14:03]

On 2014/11/18 00:12:27, James Cook wrote:
> LGTM with nit
> 
>
https://codereview.chromium.org/696063008/diff/80001/extensions/browser/app_w...
> File extensions/browser/app_window/test_app_window_contents.h (right):
> 
>
https://codereview.chromium.org/696063008/diff/80001/extensions/browser/app_w...
> extensions/browser/app_window/test_app_window_contents.h:10: #include
> "base/macros.h"
> nit: alphabetize, since this is .h file
> 
>
https://codereview.chromium.org/696063008/diff/80001/extensions/shell/browser...
> File extensions/shell/browser/shell_native_app_window_aura.h (right):
> 
>
https://codereview.chromium.org/696063008/diff/80001/extensions/shell/browser...
> extensions/shell/browser/shell_native_app_window_aura.h:10: #include
> "base/macros.h"
> ditto

Oops, done. (Not sure why the PRESUBMIT didn't catch this.)

[Yoyo Zhou, 2014-11-18 01:46:06]

yoz@chromium.org changed reviewers:
+ reed@google.com

[Yoyo Zhou, 2014-11-18 01:46:07]

I had to fix some build issues - apparently gn is used to build app_shell, but
not any of the test targets. So I only made minimal changes to the BUILD.gn for
now.

reed: can you review the +third_party/skia/include DEPS addition to
extensions/shell/browser?

[Yoyo Zhou, 2014-11-19 02:07:14]

danakj, reed: ping? These are somewhat trivial DEPS additions, so I will TBR
tomorrow if I haven't heard from you.

[Yoyo Zhou, 2014-11-19 21:08:58]

On 2014/11/19 02:07:14, Yoyo Zhou wrote:
> danakj, reed: ping? These are somewhat trivial DEPS additions, so I will TBR
> tomorrow if I haven't heard from you.

TBR danakj
TBR reed

[Yoyo Zhou, 2014-11-19 21:09:06]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-11-19 21:09:52]

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

[commit-bot: I haz the power, 2014-11-19 22:00:57]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2014-11-19 22:02:29]

Patchset 8 (id:??) landed as
https://crrev.com/0e463fafeca7afbda7e4060a7ce591f7eab82c2f
Cr-Commit-Position: refs/heads/master@{#304897}