Reorder

Implement reordering of nodes.

Note that this doesn't apply to roots. If an app is embedded and its root node is reordered, there is no notification to the app. I figure this is up to the window manager to expose an API for this type of thing.

R=sky@chromium.org
http://crbug.com/365012

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

[sky, 2014-06-11 19:36:45]

https://codereview.chromium.org/327073003/diff/100001/mojo/services/public/cp...
File mojo/services/public/cpp/view_manager/lib/view_tree_node.cc (right):

https://codereview.chromium.org/327073003/diff/100001/mojo/services/public/cp...
mojo/services/public/cpp/view_manager/lib/view_tree_node.cc:133: (direction ==
ORDER_BELOW && child_i +1 == target_i)) {
nit: '+1' -> '+ 1'

https://codereview.chromium.org/327073003/diff/100001/mojo/services/view_mana...
File mojo/services/view_manager/view_manager_connection.cc (right):

https://codereview.chromium.org/327073003/diff/100001/mojo/services/view_mana...
mojo/services/view_manager/view_manager_connection.cc:288: const Node*
relatve_node) const {
relative_node.

https://codereview.chromium.org/327073003/diff/100001/mojo/services/view_mana...
mojo/services/view_manager/view_manager_connection.cc:289: return
node->id().connection_id == id_ &&
You should also check: both nodes are non-null, and the parent is in known nodes
and that node isn't already in the target position.

[Ben Goodger (Google), 2014-06-11 20:31:40]

Updated & added some more tests.


On Wed, Jun 11, 2014 at 12:36 PM, <sky@chromium.org> wrote:

>
> https://codereview.chromium.org/327073003/diff/100001/
> mojo/services/public/cpp/view_manager/lib/view_tree_node.cc
> File mojo/services/public/cpp/view_manager/lib/view_tree_node.cc
> (right):
>
> https://codereview.chromium.org/327073003/diff/100001/
> mojo/services/public/cpp/view_manager/lib/view_tree_node.cc#newcode133
> mojo/services/public/cpp/view_manager/lib/view_tree_node.cc:133:
> (direction == ORDER_BELOW && child_i +1 == target_i)) {
> nit: '+1' -> '+ 1'
>
> https://codereview.chromium.org/327073003/diff/100001/
> mojo/services/view_manager/view_manager_connection.cc
> File mojo/services/view_manager/view_manager_connection.cc (right):
>
> https://codereview.chromium.org/327073003/diff/100001/
> mojo/services/view_manager/view_manager_connection.cc#newcode288
> mojo/services/view_manager/view_manager_connection.cc:288: const Node*
> relatve_node) const {
> relative_node.
>
> https://codereview.chromium.org/327073003/diff/100001/
> mojo/services/view_manager/view_manager_connection.cc#newcode289
> mojo/services/view_manager/view_manager_connection.cc:289: return
> node->id().connection_id == id_ &&
> You should also check: both nodes are non-null, and the parent is in
> known nodes and that node isn't already in the target position.
>
> https://codereview.chromium.org/327073003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-06-11 20:39:06]

LGTM

[Ben Goodger (Google), 2014-06-11 20:47:55]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2014-06-11 20:53:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ben@chromium.org/327073003/190001

[commit-bot: I haz the power, 2014-06-11 22:13:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  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/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15633)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18769)

[commit-bot: I haz the power, 2014-06-11 22:20:44]

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

[commit-bot: I haz the power, 2014-06-11 22:20:45]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)

[Ben Goodger (Google), 2014-06-12 16:15:09]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2014-06-12 16:17:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ben@chromium.org/327073003/230001

[commit-bot: I haz the power, 2014-06-12 19:29:43]

Change committed as 276768