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

Issue 2558023002: Inline FundamentalValue into base::Value (Closed)

Created:
4 years ago by vabr (Chromium)
Modified:
3 years, 10 months ago
Reviewers:
jdoerrie, brettw
CC:
chromium-reviews, vmpstr+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Inline FundamentalValue into base::Value This is forked from patch from issue 2475603002 at patchset 20001 (http://crrev.com/2475603002#ps20001). The first patchset here is the original + git cl format. BUG=646113

Patch Set 1 #

Patch Set 2 : Rebased #

Patch Set 3 : Copy and move constructor added #

Patch Set 4 : tests #

Patch Set 5 : Comment fixed #

Total comments: 4

Patch Set 6 : EXPECT_EQ(boolean literal...) -> EXPECT_{TRUE|FALSE}(...) #

Patch Set 7 : explicit #

Total comments: 4

Patch Set 8 : Remove {} from if #

Total comments: 2

Patch Set 9 : Just rebased #

Patch Set 10 : Value(Type) initializes with the default value #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+328 lines, -98 lines) Patch
M base/values.h View 1 2 3 4 5 6 7 8 3 chunks +32 lines, -17 lines 0 comments Download
M base/values.cc View 1 2 3 4 5 6 7 8 9 5 chunks +161 lines, -81 lines 1 comment Download
M base/values_unittest.cc View 1 2 3 4 5 1 chunk +135 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 51 (39 generated)
vabr (Chromium)
Brett, Jan, Feel free to leave comments here. Cheers, Vaclav
4 years ago (2016-12-08 09:04:51 UTC) #18
jdoerrie
Left a few comments. thanks Vaclav! https://codereview.chromium.org/2558023002/diff/80001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2558023002/diff/80001/base/values.cc#newcode275 base/values.cc:275: InternalCopyFrom(that); Just out ...
4 years ago (2016-12-08 10:13:08 UTC) #23
vabr (Chromium)
Thanks, Jan! Please rebase your patch on the newest patch set here, otherwise Android compilation ...
4 years ago (2016-12-08 10:49:48 UTC) #24
brettw
lgtm https://codereview.chromium.org/2558023002/diff/120001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2558023002/diff/120001/base/values.cc#newcode269 base/values.cc:269: InternalCopyFrom(that); Thanks for finding this was missing! Seems ...
4 years ago (2016-12-08 21:57:47 UTC) #29
brettw
Be sure to remove "[Copy of]" from the description before checkin.
4 years ago (2016-12-08 22:15:50 UTC) #30
vabr (Chromium)
Thanks, Brett! Should I land this now, or would you prefer to have the cascade ...
4 years ago (2016-12-09 07:57:22 UTC) #32
brettw
As I told Jan, we don't need to wait until everything is done before landing ...
4 years ago (2016-12-09 20:42:24 UTC) #40
vabr (Chromium)
On 2016/12/09 20:42:24, brettw (ping after 24h) wrote: > As I told Jan, we don't ...
4 years ago (2016-12-09 20:48:21 UTC) #41
jdoerrie
I just reread the proposal and found another point worth discussing. https://codereview.chromium.org/2558023002/diff/140001/base/values.cc File base/values.cc (right): ...
3 years, 12 months ago (2016-12-23 14:18:26 UTC) #42
vabr (Chromium)
https://codereview.chromium.org/2558023002/diff/140001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2558023002/diff/140001/base/values.cc#newcode266 base/values.cc:266: Value::Value(Type type) : type_(type) {} On 2016/12/23 14:18:26, jdoerrie ...
3 years, 12 months ago (2016-12-23 21:34:22 UTC) #47
jdoerrie
Thanks Vaclav! I left a comment on the usage of NOTREACHED. https://codereview.chromium.org/2558023002/diff/180001/base/values.cc File base/values.cc (right): ...
3 years, 11 months ago (2017-01-02 09:55:10 UTC) #50
vabr (Chromium)
3 years, 10 months ago (2017-02-07 23:41:46 UTC) #51
I am closing this, because it was replaced by
https://codereview.chromium.org/2645073002/.

Powered by Google App Engine
This is Rietveld 408576698