Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(96)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 7904021: [Sync] Rework SharedValue<T> into Immutable<T> (Closed)

Created:
9 years, 3 months ago by akalin
Modified:
9 years, 3 months ago
Reviewers:
Nicolas Zea
CC:
chromium-reviews, Raghu Simha, ncarter (slow), Paweł Hajdan Jr., tim (not reviewing), idana
Visibility:
Public.

Description

[Sync] Rework SharedValue<T> into Immutable<T> Rework SharedValue<T> into Immutable<T>, which behaves similarly but is easier to use and is better documented. Make everything that used SharedValue<T> use Immutable<T> instead. Make SyncData use Immutable<T>. BUG= TEST= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101455

Patch Set 1 #

Patch Set 2 : Rename vars #

Total comments: 6

Patch Set 3 : Fix typo #

Patch Set 4 : Address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+598 lines, -168 lines) Patch
M chrome/browser/sync/api/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/sync/api/sync_change_unittest.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/sync/api/sync_data.h View 1 2 3 2 chunks +24 lines, -18 lines 0 comments Download
M chrome/browser/sync/api/sync_data.cc View 1 2 3 4 chunks +31 lines, -28 lines 0 comments Download
M chrome/browser/sync/js/js_arg_list.h View 2 chunks +6 lines, -7 lines 0 comments Download
M chrome/browser/sync/js/js_arg_list.cc View 1 chunk +3 lines, -4 lines 0 comments Download
M chrome/browser/sync/js/js_event_details.h View 2 chunks +6 lines, -7 lines 0 comments Download
M chrome/browser/sync/js/js_event_details.cc View 1 chunk +3 lines, -5 lines 0 comments Download
M chrome/browser/sync/js/js_transaction_observer.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/sync/js/js_transaction_observer.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/sync/syncable/syncable.h View 3 chunks +6 lines, -27 lines 0 comments Download
M chrome/browser/sync/syncable/syncable.cc View 5 chunks +5 lines, -23 lines 0 comments Download
M chrome/browser/sync/syncable/transaction_observer.h View 1 chunk +1 line, -1 line 0 comments Download
A chrome/browser/sync/util/immutable.h View 1 2 3 1 chunk +262 lines, -0 lines 0 comments Download
A chrome/browser/sync/util/immutable_unittest.cc View 1 2 3 1 chunk +244 lines, -0 lines 0 comments Download
D chrome/browser/sync/util/shared_value.h View 1 chunk +0 lines, -44 lines 0 comments Download
M chrome/chrome.gyp View 1 chunk +1 line, -1 line 0 comments Download
M chrome/chrome_tests.gypi View 1 2 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
akalin
+zea for review
9 years, 3 months ago (2011-09-15 18:44:49 UTC) #1
Nicolas Zea
Really slick! Some nits, else lgtm with green linux_valgrind. http://codereview.chromium.org/7904021/diff/1019/chrome/browser/sync/api/sync_data.h File chrome/browser/sync/api/sync_data.h (right): http://codereview.chromium.org/7904021/diff/1019/chrome/browser/sync/api/sync_data.h#newcode12 chrome/browser/sync/api/sync_data.h:12: ...
9 years, 3 months ago (2011-09-15 23:39:45 UTC) #2
akalin
Addressed all comments, will land after linux_valgrind goes green http://codereview.chromium.org/7904021/diff/1019/chrome/browser/sync/api/sync_data.h File chrome/browser/sync/api/sync_data.h (right): http://codereview.chromium.org/7904021/diff/1019/chrome/browser/sync/api/sync_data.h#newcode12 chrome/browser/sync/api/sync_data.h:12: ...
9 years, 3 months ago (2011-09-16 00:59:59 UTC) #3
Nicolas Zea
9 years, 3 months ago (2011-09-16 01:24:48 UTC) #4 
Ah, as in making something be immutable, not as in making the immutable
something. Fair enough, LGTM

On Thu, Sep 15, 2011 at 5:59 PM, <akalin@chromium.org> wrote:

> Addressed all comments, will land after linux_valgrind goes green
>
>
>
> http://codereview.chromium.**org/7904021/diff/1019/chrome/**
>
browser/sync/api/sync_data.h<http://codereview.chromium.org/7904021/diff/1019/chrome/browser/sync/api/sync_data.h>
> File chrome/browser/sync/api/sync_**data.h (right):
>
> http://codereview.chromium.**org/7904021/diff/1019/chrome/**
>
browser/sync/api/sync_data.h#**newcode12<http://codereview.chromium.org/7904021/diff/1019/chrome/browser/sync/api/sync_data.h#newcode12>
> chrome/browser/sync/api/sync_**data.h:12: #include
> "base/memory/ref_counted.h"
> On 2011/09/15 23:39:46, nzea wrote:
>
>> can we get rid of this now?
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7904021/diff/1019/chrome/**
>
browser/sync/util/immutable.h<http://codereview.chromium.org/7904021/diff/1019/chrome/browser/sync/util/immutable.h>
> File chrome/browser/sync/util/**immutable.h (right):
>
> http://codereview.chromium.**org/7904021/diff/1019/chrome/**
>
browser/sync/util/immutable.h#**newcode128<http://codereview.chromium.org/7904021/diff/1019/chrome/browser/sync/util/immutable.h#newcode128>
> chrome/browser/sync/util/**immutable.h:128: // headers).  For example, if
> you want to do this:
> On 2011/09/15 23:39:46, nzea wrote:
>
>> I find the distinction between the immutable type itself (T above) and
>> Traits::Container to be a fairly subtle one, and from what I can tell
>>
> mainly
>
>> useful for forward declaring types. Perhaps a comment here emphasizing
>>
> this
>
>> point?
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7904021/diff/1019/chrome/**
>
browser/sync/util/immutable_**unittest.cc<http://codereview.chromium.org/7904021/diff/1019/chrome/browser/sync/util/immutable_unittest.cc>
> File chrome/browser/sync/util/**immutable_unittest.cc (right):
>
> http://codereview.chromium.**org/7904021/diff/1019/chrome/**
>
browser/sync/util/immutable_**unittest.cc#newcode203<http://codereview.chromium.org/7904021/diff/1019/chrome/browser/sync/util/immutable_unittest.cc#newcode203>
> chrome/browser/sync/util/**immutable_unittest.cc:203: // Make sure making
> the container immutable doesn't incur any copies
> On 2011/09/15 23:39:46, nzea wrote:
>
>> immutable container?
>>
>
> "making the container immutable" sounds fine to me...
>
>
>
http://codereview.chromium.**org/7904021/<http://codereview.chromium.org/7904...
>

Powered by Google App Engine
This is Rietveld 408576698