Call ShowRootWindow on NativeWindow's RootWindow to display the window.

Call ShowRootWindow on NativeWindow's RootWindow to display the window.

RootWindowHost which gets created by WebContents creation doesn't
get ShowRootWindow call, hence the corresponding X window doesn't
get mapped.

BUG=166914
TEST=Build and run content_shell for chromiumos (chromiumos=1).


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

[Nayan, 2013-01-07 22:42:42]

This issue is still seen in content_shell for chromeos. Please review.

[sky, 2013-01-08 01:20:04]

https://codereview.chromium.org/11614037/diff/1/content/shell/shell_aura.cc
File content/shell/shell_aura.cc (right):

https://codereview.chromium.org/11614037/diff/1/content/shell/shell_aura.cc#n...
content/shell/shell_aura.cc:341:
window_->GetRootWindow()->SetHostSize(gfx::Size(width, height));
Why is this needed?

[Nayan, 2013-01-08 02:25:38]

https://codereview.chromium.org/11614037/diff/1/content/shell/shell_aura.cc
File content/shell/shell_aura.cc (right):

https://codereview.chromium.org/11614037/diff/1/content/shell/shell_aura.cc#n...
content/shell/shell_aura.cc:341:
window_->GetRootWindow()->SetHostSize(gfx::Size(width, height));
On 2013/01/08 01:20:05, sky wrote:
> Why is this needed?

By default, ShellStackingClientAsh creates root window of size (100, 100) in
content/shell/shell_stacking_client_ash.cc
ShellStackingClientAsh::GetDefaultParent function, resulting in root window of
dimension (100,100). Above call will set the root window size to WebView window
size.

[sky, 2013-01-08 18:28:14]

https://codereview.chromium.org/11614037/diff/1/content/shell/shell_aura.cc
File content/shell/shell_aura.cc (right):

https://codereview.chromium.org/11614037/diff/1/content/shell/shell_aura.cc#n...
content/shell/shell_aura.cc:341:
window_->GetRootWindow()->SetHostSize(gfx::Size(width, height));
On 2013/01/08 02:25:38, Nayan wrote:
> On 2013/01/08 01:20:05, sky wrote:
> > Why is this needed?
> 
> By default, ShellStackingClientAsh creates root window of size (100, 100) in
> content/shell/shell_stacking_client_ash.cc
> ShellStackingClientAsh::GetDefaultParent function, resulting in root window of
> dimension (100,100). Above call will set the root window size to WebView
window
> size.

Shouldn't the size passed to CreateWindowWithBounds be used?

[Nayan, 2013-01-08 19:38:36]

> 
> Shouldn't the size passed to CreateWindowWithBounds be used?

Size set to RootWindow is same as the size passed to CreateWindowWithBounds (In
content/shell/shell_aura.cc::PlatformCreateWindow function)

[sky, 2013-01-08 21:26:23]

My point is why isn't the root window getting sized to the widget?

[Nayan, 2013-01-08 23:12:18]

On 2013/01/08 21:26:23, sky wrote:
> My point is why isn't the root window getting sized to the widget?

RootWindow gets created in WebContentsViewAura::CreateView
(content/browser/web_contents/web_contents_view_aura.cc) by calling
window_->SetDefaultParentByRootWindow(target_root, gfx::Rect()). Note that
(0,0,0,0) rect is passed to the above function. One way to fix this issue is to
pass 'initial_size' to 'SetDefaultParentByRootWindow' function.

However, at the beginning of CreateView function, there is a note
// NOTE: we ignore |initial_size| since in some cases it's wrong (such as
// if the bookmark bar is not shown and you create a new tab). The right
// value is set shortly after this, so its safe to ignore.

There are no calls made to set the size of RootWindow after its creation. This
patch sets the size explicitly.

[oshima, 2013-01-08 23:45:56]

On 2013/01/08 23:12:18, Nayan wrote:
> On 2013/01/08 21:26:23, sky wrote:
> > My point is why isn't the root window getting sized to the widget?
> 
> RootWindow gets created in WebContentsViewAura::CreateView
> (content/browser/web_contents/web_contents_view_aura.cc) by calling
> window_->SetDefaultParentByRootWindow(target_root, gfx::Rect()). Note that
> (0,0,0,0) rect is passed to the above function. One way to fix this issue is
to
> pass 'initial_size' to 'SetDefaultParentByRootWindow' function.

I think better solution is to create a root window first. As you can see in the
comment
in SetDefaultParentByRootWindow,

  // TODO(erg): Enable this DCHECK once it is safe.
  //  DCHECK(root_window);

> 
> However, at the beginning of CreateView function, there is a note
> // NOTE: we ignore |initial_size| since in some cases it's wrong (such as
> // if the bookmark bar is not shown and you create a new tab). The right
> // value is set shortly after this, so its safe to ignore.
> 
> There are no calls made to set the size of RootWindow after its creation. This
> patch sets the size explicitly.

[sky, 2013-01-09 00:45:14]

What ever approach you end up going with, make sure you add a comment.

[Nayan, 2013-01-13 03:06:31]

> I think better solution is to create a root window first. As you can see in
the
> comment
> in SetDefaultParentByRootWindow,
> 
>   // TODO(erg): Enable this DCHECK once it is safe.
>   //  DCHECK(root_window);
> 

Uploaded the patch which creates the RootWindow first. As the next step I think
'bounds' and 'context' parameters of GetDefaultParent can be removed. Please
suggest.

[Nayan, 2013-01-13 03:07:18]

https://codereview.chromium.org/11614037/diff/1/content/shell/shell_aura.cc
File content/shell/shell_aura.cc (right):

https://codereview.chromium.org/11614037/diff/1/content/shell/shell_aura.cc#n...
content/shell/shell_aura.cc:341:
window_->GetRootWindow()->SetHostSize(gfx::Size(width, height));
On 2013/01/08 18:28:15, sky wrote:
> On 2013/01/08 02:25:38, Nayan wrote:
> > On 2013/01/08 01:20:05, sky wrote:
> > > Why is this needed?
> > 
> > By default, ShellStackingClientAsh creates root window of size (100, 100) in
> > content/shell/shell_stacking_client_ash.cc
> > ShellStackingClientAsh::GetDefaultParent function, resulting in root window
of
> > dimension (100,100). Above call will set the root window size to WebView
> window
> > size.
> 
> Shouldn't the size passed to CreateWindowWithBounds be used?

Done.

[sky, 2013-01-15 21:53:49]

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell.cc
File content/shell/shell.cc (right):

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell.cc#ne...
content/shell/shell.cc:130: create_params.context =
Shell::stacking_client_->GetDefaultParent(NULL, NULL, gfx::Rect());
> 80

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell.h
File content/shell/shell.h (right):

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell.h#new...
content/shell/shell.h:44: // Content area size for newly created windows.
Why does this need to be here?

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell.h#new...
content/shell/shell.h:250: static aura::RootWindow* CreateRootWindow(int, int);
methods are before members. Additionally name arguments, and names should be the
same in declaration and definition.

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_aura.cc
File content/shell/shell_aura.cc (right):

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_aura....
content/shell/shell_aura.cc:288: aura::RootWindow* root_window =
CreateRootWindow(kTestWindowWidth, kTestWindowHeight);
> 80

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_aura....
content/shell/shell_aura.cc:299: aura::RootWindow* Shell::CreateRootWindow(int
width, int height) {
Convention for static members/methods is to put:
// static
on the previous line.

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_aura....
content/shell/shell_aura.cc:346:
views::Widget::CreateWindowWithContextAndBounds(new
ShellWindowDelegateView(this),
> 80

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_aura....
content/shell/shell_aura.cc:350: // Explicitly call show on the root window.
This just says what the code does. Comment should describe why the code is the
way it is.

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_stack...
File content/shell/shell_stacking_client_ash.cc (right):

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_stack...
content/shell/shell_stacking_client_ash.cc:31: if (!focus_client_.get()) {
Can we instead have an Init() method that does this so that GetDefaultParent()
doesn't have to always check this?

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_stack...
File content/shell/shell_stacking_client_ash.h (right):

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_stack...
content/shell/shell_stacking_client_ash.h:42:
ShellStackingClientAsh(aura::RootWindow* root_window);
explicit. Since this takes ownership it should take a scoped_ptr.

[Nayan, 2013-01-15 23:39:03]

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell.cc
File content/shell/shell.cc (right):

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell.cc#ne...
content/shell/shell.cc:130: create_params.context =
Shell::stacking_client_->GetDefaultParent(NULL, NULL, gfx::Rect());
On 2013/01/15 21:53:49, sky wrote:
> > 80

Done.

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell.h
File content/shell/shell.h (right):

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell.h#new...
content/shell/shell.h:44: // Content area size for newly created windows.
On 2013/01/15 21:53:49, sky wrote:
> Why does this need to be here?

We use kTestWindowWidth and kTestWindowHeight in both shell.cc and
shell_aura.cc. Hence moved this variable to shell.h file.

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell.h#new...
content/shell/shell.h:250: static aura::RootWindow* CreateRootWindow(int, int);
On 2013/01/15 21:53:49, sky wrote:
> methods are before members. Additionally name arguments, and names should be
the
> same in declaration and definition.

Done.

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_aura.cc
File content/shell/shell_aura.cc (right):

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_aura....
content/shell/shell_aura.cc:288: aura::RootWindow* root_window =
CreateRootWindow(kTestWindowWidth, kTestWindowHeight);
On 2013/01/15 21:53:49, sky wrote:
> > 80

Done.

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_aura....
content/shell/shell_aura.cc:299: aura::RootWindow* Shell::CreateRootWindow(int
width, int height) {
On 2013/01/15 21:53:49, sky wrote:
> Convention for static members/methods is to put:
> // static
> on the previous line.

Done.

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_aura....
content/shell/shell_aura.cc:346:
views::Widget::CreateWindowWithContextAndBounds(new
ShellWindowDelegateView(this),
On 2013/01/15 21:53:49, sky wrote:
> > 80

Done.

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_aura....
content/shell/shell_aura.cc:350: // Explicitly call show on the root window.
On 2013/01/15 21:53:49, sky wrote:
> This just says what the code does. Comment should describe why the code is the
> way it is.

Done.

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_stack...
File content/shell/shell_stacking_client_ash.cc (right):

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_stack...
content/shell/shell_stacking_client_ash.cc:31: if (!focus_client_.get()) {
On 2013/01/15 21:53:49, sky wrote:
> Can we instead have an Init() method that does this so that GetDefaultParent()
> doesn't have to always check this?

Done.

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_stack...
File content/shell/shell_stacking_client_ash.h (right):

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell_stack...
content/shell/shell_stacking_client_ash.h:42:
ShellStackingClientAsh(aura::RootWindow* root_window);
On 2013/01/15 21:53:49, sky wrote:
> explicit. Since this takes ownership it should take a scoped_ptr.

Done.

[sky, 2013-01-16 00:55:39]

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell.h
File content/shell/shell.h (right):

https://codereview.chromium.org/11614037/diff/19001/content/shell/shell.h#new...
content/shell/shell.h:44: // Content area size for newly created windows.
On 2013/01/15 23:39:03, Nayan wrote:
> On 2013/01/15 21:53:49, sky wrote:
> > Why does this need to be here?
> 
> We use kTestWindowWidth and kTestWindowHeight in both shell.cc and
> shell_aura.cc. Hence moved this variable to shell.h file.

Ok, make sense.

https://codereview.chromium.org/11614037/diff/29001/content/shell/shell.cc
File content/shell/shell.cc (right):

https://codereview.chromium.org/11614037/diff/29001/content/shell/shell.cc#ne...
content/shell/shell.cc:125: browser_context, site_instance);
Why are you changing this?

https://codereview.chromium.org/11614037/diff/29001/content/shell/shell.cc#ne...
content/shell/shell.cc:131: gfx::Size(kTestWindowWidth, kTestWindowHeight);
Why are you changing this line?

https://codereview.chromium.org/11614037/diff/29001/content/shell/shell.cc#ne...
content/shell/shell.cc:132: create_params.context =
Shell::stacking_client_->GetDefaultParent(
This is aura specific, right?

[Nayan, 2013-01-16 01:07:29]

https://codereview.chromium.org/11614037/diff/29001/content/shell/shell.cc
File content/shell/shell.cc (right):

https://codereview.chromium.org/11614037/diff/29001/content/shell/shell.cc#ne...
content/shell/shell.cc:125: browser_context, site_instance);
On 2013/01/16 00:55:39, sky wrote:
> Why are you changing this?

Done.

https://codereview.chromium.org/11614037/diff/29001/content/shell/shell.cc#ne...
content/shell/shell.cc:131: gfx::Size(kTestWindowWidth, kTestWindowHeight);
On 2013/01/16 00:55:39, sky wrote:
> Why are you changing this line?

Done.

https://codereview.chromium.org/11614037/diff/29001/content/shell/shell.cc#ne...
content/shell/shell.cc:132: create_params.context =
Shell::stacking_client_->GetDefaultParent(
On 2013/01/16 00:55:39, sky wrote:
> This is aura specific, right?

Done.

[oshima, 2013-01-17 08:57:56]

+erg for DCHECK change below.

https://codereview.chromium.org/11614037/diff/31002/content/shell/shell.h
File content/shell/shell.h (right):

https://codereview.chromium.org/11614037/diff/31002/content/shell/shell.h#new...
content/shell/shell.h:47: static const int kTestWindowHeight = 600;
no static in header. just const int is enough.

https://codereview.chromium.org/11614037/diff/31002/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/11614037/diff/31002/ui/aura/window.cc#newcode324
ui/aura/window.cc:324: DCHECK(root_window);
Are you sure this is safe for all cases? (I just started linux_aura,
linux_chromeos_aura bot)
Adding erg in case he has some opinion.

If this is indeed safe, please remove TODO above.

[Nayan, 2013-01-17 19:55:52]

On 2013/01/17 08:57:56, oshima wrote:
> +erg for DCHECK change below.
> 
> https://codereview.chromium.org/11614037/diff/31002/content/shell/shell.h
> File content/shell/shell.h (right):
> 
>
https://codereview.chromium.org/11614037/diff/31002/content/shell/shell.h#new...
> content/shell/shell.h:47: static const int kTestWindowHeight = 600;
> no static in header. just const int is enough.
> 
> https://codereview.chromium.org/11614037/diff/31002/ui/aura/window.cc
> File ui/aura/window.cc (right):
> 
>
https://codereview.chromium.org/11614037/diff/31002/ui/aura/window.cc#newcode324
> ui/aura/window.cc:324: DCHECK(root_window);
> Are you sure this is safe for all cases? (I just started linux_aura,
> linux_chromeos_aura bot)
> Adding erg in case he has some opinion.
Plenty of unit tests are failing and they expect GetDefaultParent to create
RootWindow in case it is not provided already. I think we should keep the DCHECK
in this patch and resolve rest of such issues as separate patch. Opinions?
> 
> If this is indeed safe, please remove TODO above.

[oshima, 2013-01-17 19:58:45]

On 2013/01/17 19:55:52, Nayan wrote:
> On 2013/01/17 08:57:56, oshima wrote:
> > +erg for DCHECK change below.
> > 
> > https://codereview.chromium.org/11614037/diff/31002/content/shell/shell.h
> > File content/shell/shell.h (right):
> > 
> >
>
https://codereview.chromium.org/11614037/diff/31002/content/shell/shell.h#new...
> > content/shell/shell.h:47: static const int kTestWindowHeight = 600;
> > no static in header. just const int is enough.
> > 
> > https://codereview.chromium.org/11614037/diff/31002/ui/aura/window.cc
> > File ui/aura/window.cc (right):
> > 
> >
>
https://codereview.chromium.org/11614037/diff/31002/ui/aura/window.cc#newcode324
> > ui/aura/window.cc:324: DCHECK(root_window);
> > Are you sure this is safe for all cases? (I just started linux_aura,
> > linux_chromeos_aura bot)
> > Adding erg in case he has some opinion.
> Plenty of unit tests are failing and they expect GetDefaultParent to create
> RootWindow in case it is not provided already. I think we should keep the
DCHECK
> in this patch and resolve rest of such issues as separate patch. Opinions?

sgtm

> > 
> > If this is indeed safe, please remove TODO above.

[jochen (gone - plz use gerrit), 2013-01-17 20:02:09]

https://codereview.chromium.org/11614037/diff/31002/content/shell/shell.h
File content/shell/shell.h (right):

https://codereview.chromium.org/11614037/diff/31002/content/shell/shell.h#new...
content/shell/shell.h:47: static const int kTestWindowHeight = 600;
Can you instead add a gfx::Size default_window_size parameter to platform
initialize and pass the size down there?

[Nayan, 2013-01-18 18:59:56]

https://codereview.chromium.org/11614037/diff/31002/content/shell/shell.h
File content/shell/shell.h (right):

https://codereview.chromium.org/11614037/diff/31002/content/shell/shell.h#new...
content/shell/shell.h:47: static const int kTestWindowHeight = 600;
On 2013/01/17 20:02:09, jochen wrote:
> Can you instead add a gfx::Size default_window_size parameter to platform
> initialize and pass the size down there?

Done.

[Nayan, 2013-01-23 21:10:06]

With the recent changes https://codereview.chromium.org/11829040,
ShellStackingClientAsh gets replaced with MinimalAsh and DCHECK in
ui/aura/window.cc is enabled.

Reworked the patch to just include passing of default size argument to
RootWindow creation and to explicitly call ShowRootWindow.

Please review and let me know the comments if any.

[sky, 2013-01-23 22:12:10]

https://codereview.chromium.org/11614037/diff/48001/content/shell/minimal_ash.cc
File content/shell/minimal_ash.cc (right):

https://codereview.chromium.org/11614037/diff/48001/content/shell/minimal_ash...
content/shell/minimal_ash.cc:19: gfx::Rect(default_window_size.width(),
There is a constructor that takes a Size.

https://codereview.chromium.org/11614037/diff/48001/content/shell/minimal_ash.h
File content/shell/minimal_ash.h (right):

https://codereview.chromium.org/11614037/diff/48001/content/shell/minimal_ash...
content/shell/minimal_ash.h:43: MinimalAsh(gfx::Size default_window_size);
explicit const gfx::Size&

https://codereview.chromium.org/11614037/diff/48001/content/shell/shell.cc
File content/shell/shell.cc (right):

https://codereview.chromium.org/11614037/diff/48001/content/shell/shell.cc#ne...
content/shell/shell.cc:131: PlatformInitialize(gfx::Size(kTestWindowWidth,
kTestWindowHeight));
Why the two Initialize variants? Can't the ones that care about kTextWindowWidth
look it up?

[Nayan, 2013-01-23 22:23:10]

https://codereview.chromium.org/11614037/diff/48001/content/shell/minimal_ash.cc
File content/shell/minimal_ash.cc (right):

https://codereview.chromium.org/11614037/diff/48001/content/shell/minimal_ash...
content/shell/minimal_ash.cc:19: gfx::Rect(default_window_size.width(),
On 2013/01/23 22:12:10, sky wrote:
> There is a constructor that takes a Size.

Done.

https://codereview.chromium.org/11614037/diff/48001/content/shell/shell.cc
File content/shell/shell.cc (right):

https://codereview.chromium.org/11614037/diff/48001/content/shell/shell.cc#ne...
content/shell/shell.cc:131: PlatformInitialize(gfx::Size(kTestWindowWidth,
kTestWindowHeight));
On 2013/01/23 22:12:10, sky wrote:
> Why the two Initialize variants? Can't the ones that care about
kTextWindowWidth
> look it up?
This change was made to address jochen's comment -
https://codereview.chromium.org/11614037/diff/31002/content/shell/shell.h#new...

[Nayan, 2013-01-23 22:28:39]

https://codereview.chromium.org/11614037/diff/48001/content/shell/minimal_ash.h
File content/shell/minimal_ash.h (right):

https://codereview.chromium.org/11614037/diff/48001/content/shell/minimal_ash...
content/shell/minimal_ash.h:43: MinimalAsh(gfx::Size default_window_size);
On 2013/01/23 22:12:10, sky wrote:
> explicit const gfx::Size&

Done.

[oshima, 2013-01-24 01:56:11]

https://codereview.chromium.org/11614037/diff/54001/content/shell/shell.h
File content/shell/shell.h (right):

https://codereview.chromium.org/11614037/diff/54001/content/shell/shell.h#new...
content/shell/shell.h:150: static void PlatformInitialize(gfx::Size
default_window_size);
const gfx::Size&

[Nayan, 2013-01-24 03:27:50]

https://codereview.chromium.org/11614037/diff/54001/content/shell/shell.h
File content/shell/shell.h (right):

https://codereview.chromium.org/11614037/diff/54001/content/shell/shell.h#new...
content/shell/shell.h:150: static void PlatformInitialize(gfx::Size
default_window_size);
On 2013/01/24 01:56:11, oshima wrote:
> const gfx::Size&

Done.

[oshima, 2013-01-24 04:01:19]

lgtm (I'm not owner though)

[sky, 2013-01-24 18:39:06]

https://codereview.chromium.org/11614037/diff/56002/content/shell/shell_aura.cc
File content/shell/shell_aura.cc (right):

https://codereview.chromium.org/11614037/diff/56002/content/shell/shell_aura....
content/shell/shell_aura.cc:350: #if defined(OS_CHROMEOS)
Why is this chromeos specific? Don't we need the same call on windows?

[Nayan, 2013-01-24 18:49:11]

https://codereview.chromium.org/11614037/diff/56002/content/shell/shell_aura.cc
File content/shell/shell_aura.cc (right):

https://codereview.chromium.org/11614037/diff/56002/content/shell/shell_aura....
content/shell/shell_aura.cc:350: #if defined(OS_CHROMEOS)
On 2013/01/24 18:39:06, sky wrote:
> Why is this chromeos specific? Don't we need the same call on windows?

I am not sure whether this issue is seen in windows. Moreover we create
'MinimalAsh' and 'CreateWindowWithContextAndBounds' only in case of chromeos
builds.Hence this code made chromeos specific.

[sky, 2013-01-24 19:40:41]

On Thu, Jan 24, 2013 at 10:49 AM,  <qtc746@motorola.com> wrote:
>
>
https://codereview.chromium.org/11614037/diff/56002/content/shell/shell_aura.cc
> File content/shell/shell_aura.cc (right):
>
>
https://codereview.chromium.org/11614037/diff/56002/content/shell/shell_aura....
> content/shell/shell_aura.cc:350: #if defined(OS_CHROMEOS)
> On 2013/01/24 18:39:06, sky wrote:
>>
>> Why is this chromeos specific? Don't we need the same call on windows?
>
>
> I am not sure whether this issue is seen in windows. Moreover we create
> 'MinimalAsh' and 'CreateWindowWithContextAndBounds' only in case of
> chromeos builds.Hence this code made chromeos specific.
>
> https://codereview.chromium.org/11614037/

Then why the ifdef?

[Nayan, 2013-01-24 20:48:17]

> 
> Then why the ifdef?

Sorry for not being descriptive. I haven't tested this issue in windows and I am
assuming that the issue is not seen in windows. There are no such bugs reported
in aura+windows configurations. Hence I restricted the change to just chromeos.

Including this change in windows is harmless and I can modify the patch to
remove ifdef's. Please let me know your opinion.

[sky, 2013-01-24 22:28:47]

On Thu, Jan 24, 2013 at 12:48 PM,  <qtc746@motorola.com> wrote:
>
>> Then why the ifdef?
>
>
> Sorry for not being descriptive. I haven't tested this issue in windows and
> I am
> assuming that the issue is not seen in windows. There are no such bugs
> reported
> in aura+windows configurations. Hence I restricted the change to just
> chromeos.
>
> Including this change in windows is harmless and I can modify the patch to
> remove ifdef's. Please let me know your opinion.

I think the same problem exists for aura-windows. So, nuke the ifdefs.

  -Scott
>
>
>
> https://codereview.chromium.org/11614037/

[Nayan, 2013-01-24 23:47:59]

> I think the same problem exists for aura-windows. So, nuke the ifdefs.

Thanks, I have uploaded re-worked patch for review.

> 
>   -Scott
> >

[sky, 2013-01-25 00:59:04]

https://codereview.chromium.org/11614037/diff/60001/content/shell/shell_aura.cc
File content/shell/shell_aura.cc (right):

https://codereview.chromium.org/11614037/diff/60001/content/shell/shell_aura....
content/shell/shell_aura.cc:285: #if defined(OS_CHROMEOS)
Sorry, since you're here can you merge these two defines, meaning nuke lines 284
and 285.

[Nayan, 2013-01-25 01:06:39]

Removed redundant OS_CHROMEOS ifdef.

[sky, 2013-01-25 16:25:01]

LGTM - thanks

[commit-bot: I haz the power, 2013-01-25 18:06:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qtc746@motorola.com/11614037/54002

[commit-bot: I haz the power, 2013-01-25 18:06:21]

Presubmit check for 11614037-54002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/shell

Presubmit checks took 4.5s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[jochen (gone - plz use gerrit), 2013-01-25 19:20:03]

lgtm

[commit-bot: I haz the power, 2013-01-25 19:21:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qtc746@motorola.com/11614037/54002

[commit-bot: I haz the power, 2013-01-25 20:03:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qtc746@motorola.com/11614037/58004

[commit-bot: I haz the power, 2013-01-25 22:29:25]

Change committed as 178918