Snap widgets to pixel boundary on ash

Snap widgets to pixel boundary on ash

Introduced SnapToPixelLayoutManager and used where it makes sense.
WorkspaceLayoutManager uses WindowState, so it's the property is manually set there.


This depends on the following CL:
https://codereview.chromium.org/375693006/

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

[Jun Mukai, 2014-07-16 22:59:08]

https://codereview.chromium.org/357063002/diff/310001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/357063002/diff/310001/ash/root_window_control...
ash/root_window_controller.cc:862: 
Not sure why this is necessary.  Instead of setting the property below, why not
just setting the SnapToPixelLayoutManager there?

https://codereview.chromium.org/357063002/diff/310001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/357063002/diff/310001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:40:
WorkspaceLayoutManager::WorkspaceLayoutManager(aura::Window* window)
Why isn't WorkspaceLayoutManager a subclass of SnapToPixelLayoutManager?

[oshima, 2014-07-17 22:07:56]

https://codereview.chromium.org/357063002/diff/310001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/357063002/diff/310001/ash/root_window_control...
ash/root_window_controller.cc:862: 
On 2014/07/16 22:59:08, Jun Mukai wrote:
> Not sure why this is necessary.  Instead of setting the property below, why
not
> just setting the SnapToPixelLayoutManager there?

Because some containers may install its own layout managers, and I wanted to
keep it more declarative. (I will change CreateContainer to
take bitmap flags that specifies, this , screen coordinates, animation and so
forth, but in separate CL)

I can be convinced the other way though.

https://codereview.chromium.org/357063002/diff/310001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/357063002/diff/310001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:40:
WorkspaceLayoutManager::WorkspaceLayoutManager(aura::Window* window)
On 2014/07/16 22:59:08, Jun Mukai wrote:
> Why isn't WorkspaceLayoutManager a subclass of SnapToPixelLayoutManager?

because this class has its own logic to adjust offset and won't use its
SetChildBounds

[Jun Mukai, 2014-07-17 22:26:31]

https://codereview.chromium.org/357063002/diff/310001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/357063002/diff/310001/ash/root_window_control...
ash/root_window_controller.cc:862: 
On 2014/07/17 22:07:56, oshima wrote:
> On 2014/07/16 22:59:08, Jun Mukai wrote:
> > Not sure why this is necessary.  Instead of setting the property below, why
> not
> > just setting the SnapToPixelLayoutManager there?
> 
> Because some containers may install its own layout managers, and I wanted to
> keep it more declarative. (I will change CreateContainer to
> take bitmap flags that specifies, this , screen coordinates, animation and so
> forth, but in separate CL)
> 
> I can be convinced the other way though.
> 

Hmm, I don't get the point.

If a container with the snap-to-pixel property installs its own layout manager,
the layout manager implementation should respect the property, or inherit
SnapToPixelLayoutManager. Otherwise, the property is just ignored. Right?

Then, the declaration doesn't help the situation so much. The implementers need
to care about SnapToPixelLayoutManager anyways.

How about this:
- keep this class and InstallSnapLayoutManagerToContainers().
  This class is solely responsible for the SnapToPixel property setups.
- in ctor of SnapToPixelLayoutManager, don't set property. Instead, add a DCHECK
to verify if the declarations (i.e. this class) is correct.

https://codereview.chromium.org/357063002/diff/310001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/357063002/diff/310001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:40:
WorkspaceLayoutManager::WorkspaceLayoutManager(aura::Window* window)
On 2014/07/17 22:07:56, oshima wrote:
> On 2014/07/16 22:59:08, Jun Mukai wrote:
> > Why isn't WorkspaceLayoutManager a subclass of SnapToPixelLayoutManager?
> 
> because this class has its own logic to adjust offset and won't use its
> SetChildBounds

Acknowledged.

[Jun Mukai, 2014-07-17 22:27:16]

lgtm

https://codereview.chromium.org/357063002/diff/310001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/357063002/diff/310001/ash/root_window_control...
ash/root_window_controller.cc:862: 
On 2014/07/17 22:26:31, Jun Mukai wrote:
> On 2014/07/17 22:07:56, oshima wrote:
> > On 2014/07/16 22:59:08, Jun Mukai wrote:
> > > Not sure why this is necessary.  Instead of setting the property below,
why
> > not
> > > just setting the SnapToPixelLayoutManager there?
> > 
> > Because some containers may install its own layout managers, and I wanted to
> > keep it more declarative. (I will change CreateContainer to
> > take bitmap flags that specifies, this , screen coordinates, animation and
so
> > forth, but in separate CL)
> > 
> > I can be convinced the other way though.
> > 
> 
> Hmm, I don't get the point.
> 
> If a container with the snap-to-pixel property installs its own layout
manager,
> the layout manager implementation should respect the property, or inherit
> SnapToPixelLayoutManager. Otherwise, the property is just ignored. Right?
> 
> Then, the declaration doesn't help the situation so much. The implementers
need
> to care about SnapToPixelLayoutManager anyways.
> 
> How about this:
> - keep this class and InstallSnapLayoutManagerToContainers().
>   This class is solely responsible for the SnapToPixel property setups.
> - in ctor of SnapToPixelLayoutManager, don't set property. Instead, add a
DCHECK
> to verify if the declarations (i.e. this class) is correct.

ah, your new patchset is in this way. Thanks for quick work.

[oshima, 2014-07-17 22:28:40]

https://codereview.chromium.org/357063002/diff/310001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/357063002/diff/310001/ash/root_window_control...
ash/root_window_controller.cc:862: 
On 2014/07/17 22:27:16, Jun Mukai wrote:
> On 2014/07/17 22:26:31, Jun Mukai wrote:
> > On 2014/07/17 22:07:56, oshima wrote:
> > > On 2014/07/16 22:59:08, Jun Mukai wrote:
> > > > Not sure why this is necessary.  Instead of setting the property below,
> why
> > > not
> > > > just setting the SnapToPixelLayoutManager there?
> > > 
> > > Because some containers may install its own layout managers, and I wanted
to
> > > keep it more declarative. (I will change CreateContainer to
> > > take bitmap flags that specifies, this , screen coordinates, animation and
> so
> > > forth, but in separate CL)
> > > 
> > > I can be convinced the other way though.
> > > 
> > 
> > Hmm, I don't get the point.
> > 
> > If a container with the snap-to-pixel property installs its own layout
> manager,
> > the layout manager implementation should respect the property, or inherit
> > SnapToPixelLayoutManager. Otherwise, the property is just ignored. Right?
> > 
> > Then, the declaration doesn't help the situation so much. The implementers
> need
> > to care about SnapToPixelLayoutManager anyways.
> > 
> > How about this:
> > - keep this class and InstallSnapLayoutManagerToContainers().
> >   This class is solely responsible for the SnapToPixel property setups.
> > - in ctor of SnapToPixelLayoutManager, don't set property. Instead, add a
> DCHECK
> > to verify if the declarations (i.e. this class) is correct.
> 
> ah, your new patchset is in this way. Thanks for quick work.

Sorry, I should have mentioned that.

[oshima, 2014-07-17 22:29:10]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-07-17 22:30:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/357063002/370001

[commit-bot: I haz the power, 2014-07-18 03:38:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[oshima, 2014-07-18 04:16:22]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-07-18 04:17:04]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-07-18 04:17:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/357063002/390001

[commit-bot: I haz the power, 2014-07-18 08:04:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-18 08:54:31]

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

[commit-bot: I haz the power, 2014-07-18 08:54:33]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[oshima, 2014-07-18 14:17:19]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-07-18 14:17:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/357063002/410001

[commit-bot: I haz the power, 2014-07-18 16:26:20]

Change committed as 284119