Refactor ModelTypePayloadMap and ObjectIdPayloadMap to StateMaps.

Refactor ModelTypePayloadMap and ObjectIdPayloadMap to StateMaps.

This lays the groundwork for allowing us to pass ack handles for object IDs to
SyncNotifierObservers.

BUG=124149
TEST=none, there should be no behavior change.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153158

[dcheng, 2012-08-11 18:35:02]

This is mostly just a renaming change, but I left some TODOs for things that I
think will need a second look when I start implementing support for
InvalidationState.ack_handle.

[akalin, 2012-08-13 19:41:48]

few initial comments

http://codereview.chromium.org/10837214/diff/75/sync/internal_api/public/base...
File sync/internal_api/public/base/model_type_state_map.h (right):

http://codereview.chromium.org/10837214/diff/75/sync/internal_api/public/base...
sync/internal_api/public/base/model_type_state_map.h:25: struct
InvalidationState {
can we move this into its own header?  I think it'll end up being big enough to
warrant its own header

http://codereview.chromium.org/10837214/diff/75/sync/internal_api/public/base...
sync/internal_api/public/base/model_type_state_map.h:27: std::string ack_handle;
can we use invalidation::AckHandle for now, until we figure out how we want to
do our own AckHandles?  I'm a big wary of using plain strings for ack handles...

http://codereview.chromium.org/10837214/diff/75/sync/internal_api/public/base...
sync/internal_api/public/base/model_type_state_map.h:29: bool operator==(const
InvalidationState& rhs) const;
use Equals()

[dcheng, 2012-08-18 00:41:29]

Ready for another pass. I still need to fix a few issues that I've listed below
though.

http://codereview.chromium.org/10837214/diff/75/sync/internal_api/public/base...
File sync/internal_api/public/base/model_type_state_map.h (right):

http://codereview.chromium.org/10837214/diff/75/sync/internal_api/public/base...
sync/internal_api/public/base/model_type_state_map.h:25: struct
InvalidationState {
On 2012/08/13 19:41:48, akalin wrote:
> can we move this into its own header?  I think it'll end up being big enough
to
> warrant its own header

Done.

http://codereview.chromium.org/10837214/diff/75/sync/internal_api/public/base...
sync/internal_api/public/base/model_type_state_map.h:27: std::string ack_handle;
On 2012/08/13 19:41:48, akalin wrote:
> can we use invalidation::AckHandle for now, until we figure out how we want to
> do our own AckHandles?  I'm a big wary of using plain strings for ack
handles...

I've just declared an empty AckHandle class for now in invalidation_state.h.
Reusing invalidation::AckHandle causes a lot of complications with #includes.

http://codereview.chromium.org/10837214/diff/75/sync/internal_api/public/base...
sync/internal_api/public/base/model_type_state_map.h:29: bool operator==(const
InvalidationState& rhs) const;
On 2012/08/13 19:41:48, akalin wrote:
> use Equals()

As discussed, we are now using Gmock matchers.

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
File sync/internal_api/public/base/model_type_state_map.cc (right):

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
sync/internal_api/public/base/model_type_state_map.cc:49: // TODO(dcheng): Do we
need to persist ack_handle here? Is this used to
I need to double check this before I check in.

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
File sync/internal_api/public/base/model_type_state_map_test_util.cc (right):

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
sync/internal_api/public/base/model_type_state_map_test_util.cc:49:
expected_.value_comp());
value_comp() isn't actually sufficient. I'll have to write a small functor for
this.

http://codereview.chromium.org/10837214/diff/3075/sync/notifier/invalidation_...
File sync/notifier/invalidation_util.h (right):

http://codereview.chromium.org/10837214/diff/3075/sync/notifier/invalidation_...
sync/notifier/invalidation_util.h:22: void PrintTo(const invalidation::ObjectId&
id, std::ostream* os);
I ended up placing this here mostly for convenience, since it was so small. If
you feel strongly about it, I'll move it into a separate header.

http://codereview.chromium.org/10837214/diff/3075/sync/notifier/object_id_sta...
File sync/notifier/object_id_state_map_test_util.cc (right):

http://codereview.chromium.org/10837214/diff/3075/sync/notifier/object_id_sta...
sync/notifier/object_id_state_map_test_util.cc:42: const ObjectIdStateMap&
actual, MatchResultListener* listener) const {
I think it would be nice if I pulled this out into a template, as the vast
majority of this is just duplicated.

[akalin, 2012-08-20 21:00:06]

few more nits

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
File sync/internal_api/public/base/invalidation_state.h (right):

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
sync/internal_api/public/base/invalidation_state.h:12: class AckHandle {
add a comment/TODO explaining empty class?

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
File sync/internal_api/public/base/model_type_state_map.cc (right):

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
sync/internal_api/public/base/model_type_state_map.cc:48: // TODO(akalin):
Unpack the value into a protobuf.
i think this todo is obsolete now

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
sync/internal_api/public/base/model_type_state_map.cc:49: // TODO(dcheng): Do we
need to persist ack_handle here? Is this used to
On 2012/08/18 00:41:29, dcheng wrote:
> I need to double check this before I check in.

this is used for about:sync display.  so I don't think you need to persist
ack_handle here.

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
File sync/internal_api/public/base/model_type_state_map_test_util.cc (right):

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
sync/internal_api/public/base/model_type_state_map_test_util.cc:49:
expected_.value_comp());
On 2012/08/18 00:41:29, dcheng wrote:
> value_comp() isn't actually sufficient. I'll have to write a small functor for
> this.

oh?  what's the issue?

http://codereview.chromium.org/10837214/diff/3075/sync/notifier/object_id_sta...
File sync/notifier/object_id_state_map.h (right):

http://codereview.chromium.org/10837214/diff/3075/sync/notifier/object_id_sta...
sync/notifier/object_id_state_map.h:23: ObjectIdStateMap
ObjectIdSetToStateMap(ObjectIdSet ids,
const ObjectIdSet&

[dcheng, 2012-08-21 01:23:39]

I made some minor additions to convince googletest to use the PrintTo() overload
for InvalidationState as well.

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
File sync/internal_api/public/base/invalidation_state.h (right):

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
sync/internal_api/public/base/invalidation_state.h:12: class AckHandle {
On 2012/08/20 21:00:07, akalin wrote:
> add a comment/TODO explaining empty class?

Done.

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
File sync/internal_api/public/base/model_type_state_map.cc (right):

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
sync/internal_api/public/base/model_type_state_map.cc:48: // TODO(akalin):
Unpack the value into a protobuf.
On 2012/08/20 21:00:07, akalin wrote:
> i think this todo is obsolete now

Done.

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
sync/internal_api/public/base/model_type_state_map.cc:49: // TODO(dcheng): Do we
need to persist ack_handle here? Is this used to
On 2012/08/20 21:00:07, akalin wrote:
> On 2012/08/18 00:41:29, dcheng wrote:
> > I need to double check this before I check in.
> 
> this is used for about:sync display.  so I don't think you need to persist
> ack_handle here.

Done.

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
File sync/internal_api/public/base/model_type_state_map_test_util.cc (right):

http://codereview.chromium.org/10837214/diff/3075/sync/internal_api/public/ba...
sync/internal_api/public/base/model_type_state_map_test_util.cc:49:
expected_.value_comp());
On 2012/08/20 21:00:07, akalin wrote:
> On 2012/08/18 00:41:29, dcheng wrote:
> > value_comp() isn't actually sufficient. I'll have to write a small functor
for
> > this.
> 
> oh?  what's the issue?

This doesn't actually compare the values, only the keys.

http://codereview.chromium.org/10837214/diff/3075/sync/notifier/object_id_sta...
File sync/notifier/object_id_state_map.h (right):

http://codereview.chromium.org/10837214/diff/3075/sync/notifier/object_id_sta...
sync/notifier/object_id_state_map.h:23: ObjectIdStateMap
ObjectIdSetToStateMap(ObjectIdSet ids,
On 2012/08/20 21:00:07, akalin wrote:
> const ObjectIdSet&

Done.

[akalin, 2012-08-22 20:19:55]

i think your rebase didn't go as planned?  it seems like this regressed to your
earlier patchset (operator== is back, and InvalidationState is in its old
place).

Also, I notice that your patchsets have blank statuses for some of the files. 
That may cause problems when you try to commit via CQ.  I found that this
happens if I refresh the codereview page while uploading.

[dcheng, 2012-08-22 21:20:49]

On 2012/08/22 20:19:55, akalin wrote:
> i think your rebase didn't go as planned?  it seems like this regressed to
your
> earlier patchset (operator== is back, and InvalidationState is in its old
> place).
> 

Ack, fixed the merge.

> Also, I notice that your patchsets have blank statuses for some of the files. 
> That may cause problems when you try to commit via CQ.  I found that this
> happens if I refresh the codereview page while uploading.

Yeah, I've noticed that refreshing the codereview page seems to make the problem
worse, but making sure I don't refresh it doesn't seem to make it completely go
away =/

[akalin, 2012-08-22 22:24:30]

few more

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation_state_test_util.cc (right):

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state_test_util.cc:9: using
::testing::MakeMatcher;
move using lines inside namespace?

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state_test_util.cc:55: *os << "{
payload: " << state.payload << "}";
space before }

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation_state_test_util.h (right):

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state_test_util.h:13: void
PrintTo(const InvalidationState& state, ::std::ostream* os);
#include <ostream>

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type_state_map.cc (right):

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map.cc:49: bool encoded =
base::Base64Encode(it->second.payload, &base64_marker);
since you're here, change to using JsonDoubleQuote(str, false, ...) from
base/json/string_escape.h

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type_state_map.h (right):

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map.h:34: SYNC_EXPORT
ModelTypeStateMap ModelTypeStateMapFromEnumSet(
since you're here and have to update callers anyway, ModelTypeSetToStateMap

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map.h:37: ModelTypeSet
ModelTypeStateMapToEnumSet(
since you're here, EnumSet -> Set

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type_state_map_test_util.cc (right):

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map_test_util.cc:11: using
::testing::MatchResultListener;
move into namespace?

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map_test_util.cc:31:
ModelTypeStateMap expected_;
const

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type_state_map_test_util.h (right):

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map_test_util.h:8: #include
"sync/internal_api/public/base/invalidation_state_test_util.h"
need this include?

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map_test_util.h:9: #include
"sync/internal_api/public/base/model_type_state_map.h"
can forward-declare?

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
File sync/notifier/object_id_state_map.cc (right):

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
sync/notifier/object_id_state_map.cc:11: for (ObjectIdStateMap::const_iterator
it = id_state_map.begin();
i feel like we do this over and over in chrome (loop over a map to get a set of
keys). :/ Out of scope of this CL, but we may want to add something to
stl_utils.h or something

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
File sync/notifier/object_id_state_map_test_util.cc (right):

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
sync/notifier/object_id_state_map_test_util.cc:7: #include <algorithm>
newline after <algorithm>

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
sync/notifier/object_id_state_map_test_util.cc:10: using ::testing::MakeMatcher;
move into namespace

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
sync/notifier/object_id_state_map_test_util.cc:31: ObjectIdStateMap expected_;
const

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
File sync/notifier/object_id_state_map_test_util.h (right):

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
sync/notifier/object_id_state_map_test_util.h:8: #include
"sync/internal_api/public/base/invalidation_state_test_util.h"
do we need to explicitly include _test_util.h here?

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
sync/notifier/object_id_state_map_test_util.h:9: #include
"sync/notifier/object_id_state_map.h"
can we forward-declare?

[dcheng, 2012-08-22 22:53:18]

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation_state_test_util.cc (right):

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state_test_util.cc:9: using
::testing::MakeMatcher;
On 2012/08/22 22:24:31, akalin wrote:
> move using lines inside namespace?

Done.

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state_test_util.cc:55: *os << "{
payload: " << state.payload << "}";
On 2012/08/22 22:24:31, akalin wrote:
> space before }

Done.

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation_state_test_util.h (right):

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state_test_util.h:13: void
PrintTo(const InvalidationState& state, ::std::ostream* os);
On 2012/08/22 22:24:31, akalin wrote:
> #include <ostream>

Done.

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type_state_map.cc (right):

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map.cc:49: bool encoded =
base::Base64Encode(it->second.payload, &base64_marker);
On 2012/08/22 22:24:31, akalin wrote:
> since you're here, change to using JsonDoubleQuote(str, false, ...) from
> base/json/string_escape.h

Done.

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type_state_map.h (right):

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map.h:34: SYNC_EXPORT
ModelTypeStateMap ModelTypeStateMapFromEnumSet(
On 2012/08/22 22:24:31, akalin wrote:
> since you're here and have to update callers anyway, ModelTypeSetToStateMap

Done.

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map.h:37: ModelTypeSet
ModelTypeStateMapToEnumSet(
On 2012/08/22 22:24:31, akalin wrote:
> since you're here, EnumSet -> Set

Done.

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type_state_map_test_util.cc (right):

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map_test_util.cc:11: using
::testing::MatchResultListener;
On 2012/08/22 22:24:31, akalin wrote:
> move into namespace?

Done.

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map_test_util.cc:31:
ModelTypeStateMap expected_;
On 2012/08/22 22:24:31, akalin wrote:
> const 

Done.

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type_state_map_test_util.h (right):

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map_test_util.h:8: #include
"sync/internal_api/public/base/invalidation_state_test_util.h"
On 2012/08/22 22:24:31, akalin wrote:
> need this include?

See comments in object_id_state_map.h.

http://codereview.chromium.org/10837214/diff/18001/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map_test_util.h:9: #include
"sync/internal_api/public/base/model_type_state_map.h"
On 2012/08/22 22:24:31, akalin wrote:
> can forward-declare?

See comments in object_id_state_map.h.

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
File sync/notifier/object_id_state_map_test_util.cc (right):

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
sync/notifier/object_id_state_map_test_util.cc:7: #include <algorithm>
On 2012/08/22 22:24:31, akalin wrote:
> newline after <algorithm>

Done.

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
sync/notifier/object_id_state_map_test_util.cc:10: using ::testing::MakeMatcher;
On 2012/08/22 22:24:31, akalin wrote:
> move into namespace

Done.

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
sync/notifier/object_id_state_map_test_util.cc:31: ObjectIdStateMap expected_;
On 2012/08/22 22:24:31, akalin wrote:
> const

Done.

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
File sync/notifier/object_id_state_map_test_util.h (right):

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
sync/notifier/object_id_state_map_test_util.h:8: #include
"sync/internal_api/public/base/invalidation_state_test_util.h"
On 2012/08/22 22:24:31, akalin wrote:
> do we need to explicitly include _test_util.h here?

This was the only way I could convince Googletest to uniformly use the PrintTo
overload for InvalidationState; for some reason, including it in the .cc wasn't
always enough.

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
sync/notifier/object_id_state_map_test_util.h:9: #include
"sync/notifier/object_id_state_map.h"
On 2012/08/22 22:24:31, akalin wrote:
> can we forward-declare?

It's a typedef, so I don't think so.

[akalin, 2012-08-22 23:24:27]

lgtm

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
File sync/notifier/object_id_state_map_test_util.h (right):

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
sync/notifier/object_id_state_map_test_util.h:8: #include
"sync/internal_api/public/base/invalidation_state_test_util.h"
On 2012/08/22 22:53:19, dcheng wrote:
> On 2012/08/22 22:24:31, akalin wrote:
> > do we need to explicitly include _test_util.h here?
> 
> This was the only way I could convince Googletest to uniformly use the PrintTo
> overload for InvalidationState; for some reason, including it in the .cc
wasn't
> always enough.

That's weird.  Add a comment?

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
sync/notifier/object_id_state_map_test_util.h:9: #include
"sync/notifier/object_id_state_map.h"
On 2012/08/22 22:53:19, dcheng wrote:
> On 2012/08/22 22:24:31, akalin wrote:
> > can we forward-declare?
> 
> It's a typedef, so I don't think so.

oh, right.

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation_state_test_util.cc (right):

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state_test_util.cc:30:
InvalidationState expected_;
const

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation_state_test_util.h (right):

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state_test_util.h:10: #include
"sync/internal_api/public/base/invalidation_state.h"
possible to forward-declare InvalidationState?

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type_state_map.cc (right):

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map.cc:48: std::string marker;
rename marker to printable_payload

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map.cc:49:
base::JsonDoubleQuote(it->second.payload, false /* add quotes */, &marker);
add quotes -> add_quotes

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type_state_map.h (right):

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map.h:14: #include
"sync/internal_api/public/base/invalidation_state.h"
i wonder if it's possible to forward-declare here, too

http://codereview.chromium.org/10837214/diff/24004/sync/notifier/invalidation...
File sync/notifier/invalidation_util.h (right):

http://codereview.chromium.org/10837214/diff/24004/sync/notifier/invalidation...
sync/notifier/invalidation_util.h:10: #include <iosfwd>
i don't remember whether we bother using iosfwd.  maybe change all the other
places i told you to include <ostream> to include iosfwd instead?

http://codereview.chromium.org/10837214/diff/24004/sync/notifier/object_id_st...
File sync/notifier/object_id_state_map.h (right):

http://codereview.chromium.org/10837214/diff/24004/sync/notifier/object_id_st...
sync/notifier/object_id_state_map.h:18: InvalidationState,
doesn't InvalidationState need to be forward-declared?

[dcheng, 2012-08-22 23:47:11]

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
File sync/notifier/object_id_state_map_test_util.h (right):

http://codereview.chromium.org/10837214/diff/18001/sync/notifier/object_id_st...
sync/notifier/object_id_state_map_test_util.h:8: #include
"sync/internal_api/public/base/invalidation_state_test_util.h"
On 2012/08/22 23:24:27, akalin wrote:
> On 2012/08/22 22:53:19, dcheng wrote:
> > On 2012/08/22 22:24:31, akalin wrote:
> > > do we need to explicitly include _test_util.h here?
> > 
> > This was the only way I could convince Googletest to uniformly use the
PrintTo
> > overload for InvalidationState; for some reason, including it in the .cc
> wasn't
> > always enough.
> 
> That's weird.  Add a comment?

Done.

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation_state_test_util.cc (right):

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state_test_util.cc:30:
InvalidationState expected_;
On 2012/08/22 23:24:27, akalin wrote:
> const

Done.

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
File sync/internal_api/public/base/invalidation_state_test_util.h (right):

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
sync/internal_api/public/base/invalidation_state_test_util.h:10: #include
"sync/internal_api/public/base/invalidation_state.h"
On 2012/08/22 23:24:27, akalin wrote:
> possible to forward-declare InvalidationState?

Done.

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type_state_map.cc (right):

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map.cc:48: std::string marker;
On 2012/08/22 23:24:27, akalin wrote:
> rename marker to printable_payload

Done.

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map.cc:49:
base::JsonDoubleQuote(it->second.payload, false /* add quotes */, &marker);
On 2012/08/22 23:24:27, akalin wrote:
> add quotes -> add_quotes

Done. Since I had to break this up for 80 chars, I just used the actual
parameter name (put_in_quotes)

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
File sync/internal_api/public/base/model_type_state_map.h (right):

http://codereview.chromium.org/10837214/diff/24004/sync/internal_api/public/b...
sync/internal_api/public/base/model_type_state_map.h:14: #include
"sync/internal_api/public/base/invalidation_state.h"
On 2012/08/22 23:24:27, akalin wrote:
> i wonder if it's possible to forward-declare here, too

IIRC, in the past, we chose to #include types that were part of a std::map<>
typedef rather than forward declaring. While we can forward declare, this means
that every file that uses instantiates either StateMap will need to #include
"invalidation_state.h", including a lot of "not obviously related" files.

http://codereview.chromium.org/10837214/diff/24004/sync/notifier/invalidation...
File sync/notifier/invalidation_util.h (right):

http://codereview.chromium.org/10837214/diff/24004/sync/notifier/invalidation...
sync/notifier/invalidation_util.h:10: #include <iosfwd>
On 2012/08/22 23:24:27, akalin wrote:
> i don't remember whether we bother using iosfwd.  maybe change all the other
> places i told you to include <ostream> to include iosfwd instead?

Done.

http://codereview.chromium.org/10837214/diff/24004/sync/notifier/object_id_st...
File sync/notifier/object_id_state_map.h (right):

http://codereview.chromium.org/10837214/diff/24004/sync/notifier/object_id_st...
sync/notifier/object_id_state_map.h:18: InvalidationState,
On 2012/08/22 23:24:27, akalin wrote:
> doesn't InvalidationState need to be forward-declared?

See previous comment for why it's not forward-declared. It wasn't needed
explicitly here since model_type_state_map brings it in, but I've made the
#include explicit now.

[commit-bot: I haz the power, 2012-08-22 23:49:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10837214/18004

[commit-bot: I haz the power, 2012-08-23 00:16:51]

Try job failure for 10837214-18004 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-08-23 01:07:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10837214/19014

[commit-bot: I haz the power, 2012-08-23 01:07:38]

Presubmit check for 10837214-19014 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/extensions

Presubmit checks took 5.6s to calculate.

[dcheng, 2012-08-23 16:38:29]

+sky for chrome/browser stamp
+mihaip for chrome/browser/extensions stamp

[Mihai Parparita -not on Chrome, 2012-08-23 18:54:39]

LGTM for extensions

[sky, 2012-08-23 19:50:39]

LGTM

[commit-bot: I haz the power, 2012-08-23 21:29:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10837214/19014

[commit-bot: I haz the power, 2012-08-24 01:10:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10837214/17087

[commit-bot: I haz the power, 2012-08-24 04:29:46]

Change committed as 153158