First step at synchronizing client model changes with service.

First step at synchronizing client model changes with service.

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

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

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

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

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

[sky, 2014-04-28 23:04:18]

https://codereview.chromium.org/258623005/diff/60001/mojo/examples/sample_vie...
File mojo/examples/sample_view_manager_app/sample_view_manager_app.cc (right):

https://codereview.chromium.org/258623005/diff/60001/mojo/examples/sample_vie...
mojo/examples/sample_view_manager_app/sample_view_manager_app.cc:47:
scoped_ptr<services::view_manager::ViewTreeNode> node_1_;
Add descriptions for these.

https://codereview.chromium.org/258623005/diff/60001/mojo/examples/sample_vie...
mojo/examples/sample_view_manager_app/sample_view_manager_app.cc:58: // 
mojo::RunLoop loop;
?

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
(right):

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:22:
virtual bool Equals(ViewManagerTransaction* other) {
const ViewManagerTransaction* or const ViewManagerTransaction&

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:29: int
change_id() const { return change_id_; }
uint32_t

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:34:
TYPE_HIERARCHY
Add description for TYPE_HIERARCH as it isn't obvious what is is.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:51: int
type_;
TransactionType?

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:51: int
type_;
Style guide says no protected members.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:52: int
change_id_;
Can this be const? And uint32_t.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:77:
DCHECK(!committed());
Maybe the DCHECK and boolean should be part of the base class and have a pure
virtual DoCommit() or something function.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:91: int
node_id_;
uint16_t I believe.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:127: case
TYPE_ADD:
nit: indentation is off for the switch statement.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:141:
default:
You should be able to remove this so that you get a compile error if a new type
is added without updating this switch statement.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:152:
HierarchyChangeType hierarchy_change_type_;
const

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:153: int
child_id_;
uint16_t for both of these.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:163:
next_change_id_(0),
order doesn't match header.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:219: if
(RemovedFromPendingQueue(change_id))
One problematic area is if a transaction generates multiple notifications. This
is currently possible for OnNodeViewReplaced. There needs to be a better way for
you to identify this on your side. I'll add that to my list.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:271:
Transactions::iterator it = pending_transactions_.begin();
I think you should only have to check the first transaction. To do otherwise
implies you could get notifications out of order, which isn't the case.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h
(right):

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h:23:
explicit ViewManagerSynchronizer(ViewManager* view_manager);
Document who owns view_manager.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h:24:
~ViewManagerSynchronizer();
virtual

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h:68: int
next_id_;
uint16_t.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h:69: int
next_change_id_;
uint32_t.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/view_manager.h (right):

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/view_manager.h:20: explicit
ViewManager(Application* application);
Does this really need Application and not Shell?

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/view_tree_node.h (right):

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/view_tree_node.h:52: int id_;
uint16_t Same for id() above.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/int...
File mojo/services/public/interfaces/view_manager/view_manager.mojom (right):

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/int...
mojo/services/public/interfaces/view_manager/view_manager.mojom:19: interface
IViewManager {
Sadness.

[Ben Goodger (Google), 2014-04-29 00:00:54]

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
(right):

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:219: if
(RemovedFromPendingQueue(change_id))
On 2014/04/28 23:04:19, sky wrote:
> One problematic area is if a transaction generates multiple notifications.
This
> is currently possible for OnNodeViewReplaced. There needs to be a better way
for
> you to identify this on your side. I'll add that to my list.

Hrm and probably for nested hiers.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:271:
Transactions::iterator it = pending_transactions_.begin();
On 2014/04/28 23:04:19, sky wrote:
> I think you should only have to check the first transaction. To do otherwise
> implies you could get notifications out of order, which isn't the case.

Not quite. I currently have transactions for node creations. I have no ack for
those. That's a separate issue I need to talk to you about. But I need to
advance past them in the queue.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/int...
File mojo/services/public/interfaces/view_manager/view_manager.mojom (right):

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/int...
mojo/services/public/interfaces/view_manager/view_manager.mojom:19: interface
IViewManager {
On 2014/04/28 23:04:19, sky wrote:
> Sadness.

:D

[Ben Goodger (Google), 2014-04-29 00:18:42]

updated.


On Mon, Apr 28, 2014 at 5:00 PM, <ben@chromium.org> wrote:

>
> https://codereview.chromium.org/258623005/diff/60001/mojo/
> services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
> File
> mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
> (right):
>
> https://codereview.chromium.org/258623005/diff/60001/mojo/
> services/public/cpp/view_manager/lib/view_manager_
> synchronizer.cc#newcode219
> mojo/services/public/cpp/view_manager/lib/view_manager_
> synchronizer.cc:219:
> if (RemovedFromPendingQueue(change_id))
> On 2014/04/28 23:04:19, sky wrote:
>
>> One problematic area is if a transaction generates multiple
>>
> notifications. This
>
>> is currently possible for OnNodeViewReplaced. There needs to be a
>>
> better way for
>
>> you to identify this on your side. I'll add that to my list.
>>
>
> Hrm and probably for nested hiers.
>
>
> https://codereview.chromium.org/258623005/diff/60001/mojo/
> services/public/cpp/view_manager/lib/view_manager_
> synchronizer.cc#newcode271
> mojo/services/public/cpp/view_manager/lib/view_manager_
> synchronizer.cc:271:
> Transactions::iterator it = pending_transactions_.begin();
> On 2014/04/28 23:04:19, sky wrote:
>
>> I think you should only have to check the first transaction. To do
>>
> otherwise
>
>> implies you could get notifications out of order, which isn't the
>>
> case.
>
> Not quite. I currently have transactions for node creations. I have no
> ack for those. That's a separate issue I need to talk to you about. But
> I need to advance past them in the queue.
>
>
> https://codereview.chromium.org/258623005/diff/60001/mojo/
> services/public/interfaces/view_manager/view_manager.mojom
> File mojo/services/public/interfaces/view_manager/view_manager.mojom
> (right):
>
> https://codereview.chromium.org/258623005/diff/60001/mojo/
> services/public/interfaces/view_manager/view_manager.mojom#newcode19
> mojo/services/public/interfaces/view_manager/view_manager.mojom:19:
> interface IViewManager {
> On 2014/04/28 23:04:19, sky wrote:
>
>> Sadness.
>>
>
> :D
>
> https://codereview.chromium.org/258623005/
>

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

[sky, 2014-04-29 04:14:06]

LGTM with types fixed

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h
(right):

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h:29: void
AddChild(int child_id, int parent_id);
uint16_t for these.

https://codereview.chromium.org/258623005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h:67: uint16
connection_id_;
uint16_t

https://codereview.chromium.org/258623005/diff/120001/mojo/services/public/cp...
File mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
(right):

https://codereview.chromium.org/258623005/diff/120001/mojo/services/public/cp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:68: const
int transaction_type_;
int->TransactionType

https://codereview.chromium.org/258623005/diff/120001/mojo/services/public/cp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:111: int
child_id,
uint16_t for child_id and parent_id, including members.

https://codereview.chromium.org/258623005/diff/120001/mojo/services/public/cp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:124: case
TYPE_ADD:
nit: spacing off, indent case statements 2 more.

https://codereview.chromium.org/258623005/diff/120001/mojo/services/public/cp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:171: int
id = next_id_++;
uint32_t

https://codereview.chromium.org/258623005/diff/120001/mojo/services/public/cp...
File mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h
(right):

https://codereview.chromium.org/258623005/diff/120001/mojo/services/public/cp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.h:59: int
GetNextChangeId();
uint32_t

[Ben Goodger (Google), 2014-04-29 19:57:32]

Committed patchset #10 manually as r266940 (presubmit successful).

[Ben Goodger (Google), 2014-04-29 23:45:07]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2014-04-29 23:46:19]

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

[commit-bot: I haz the power, 2014-04-30 08:59:51]

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

[commit-bot: I haz the power, 2014-04-30 08:59:51]

Failed to apply patch for mojo/mojo_examples.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file mojo/mojo_examples.gypi
  Hunk #1 FAILED at 284.
  1 out of 1 hunk FAILED -- saving rejects to file mojo/mojo_examples.gypi.rej

Patch:       mojo/mojo_examples.gypi
Index: mojo/mojo_examples.gypi
diff --git a/mojo/mojo_examples.gypi b/mojo/mojo_examples.gypi
index
5bece9b149c90cfa8fc397c06963f7be24bf0596..69099939687747782c39149e519a637098eb6c45
100644
--- a/mojo/mojo_examples.gypi
+++ b/mojo/mojo_examples.gypi
@@ -284,11 +284,11 @@
             '../ui/gfx/gfx.gyp:gfx_geometry',
             '../ui/gl/gl.gyp:gl',
             'mojo_bindings',
-            'mojo_environment_standalone',
+            'mojo_environment_chromium',
             'mojo_gles2',
-            'mojo_view_manager_bindings',
+            'mojo_view_manager_lib',
             'mojo_shell_client',
-            'mojo_system',
+            'mojo_system_impl',
             'mojo_utility',
           ],
           'sources': [

[Ben Goodger (Google), 2014-04-30 19:22:50]

Committed patchset #13 manually as r267293 (presubmit successful).

[Ben Goodger (Google), 2014-04-30 19:58:48]

Committed patchset #14 manually as r267311 (presubmit successful).

[Ben Goodger (Google), 2014-05-01 04:24:04]

Committed patchset #16 manually as r267424 (presubmit successful).