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

Issue 2913663002: Retain is_current_color information in BorderValue constructor (Closed)

Created:
3 years, 6 months ago by nainar
Modified:
3 years, 6 months ago
Reviewers:
meade_UTC10, shend
CC:
blink-reviews, blink-reviews-style_chromium.org, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Retain is_current_color information in BorderValue constructor This CL fixes the BorderValue constructor to retain the is_current_color information which it was losing due to us resolving the StyleColor to Color but still using it as a StyleColor. This meant we didn't have the is_current_color information. This fixes a bug in the code where we are losing information when constructing BorderValue. A unit test is also added to the effect. BUG=628043 Review-Url: https://codereview.chromium.org/2913663002 Cr-Commit-Position: refs/heads/master@{#475433} Committed: https://chromium.googlesource.com/chromium/src/+/de1b3604290f911cf4fc76988ee4d5b11a4f07f9

Patch Set 1 #

Total comments: 2

Patch Set 2 : add one more test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -1 line) Patch
M third_party/WebKit/Source/core/style/BorderValue.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/style/BorderValueTest.cpp View 1 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (12 generated)
nainar
shend@, PTAL? Thanks!
3 years, 6 months ago (2017-05-30 01:17:50 UTC) #3
shend
lgtm with nits https://codereview.chromium.org/2913663002/diff/1/third_party/WebKit/Source/core/style/BorderValueTest.cpp File third_party/WebKit/Source/core/style/BorderValueTest.cpp (right): https://codereview.chromium.org/2913663002/diff/1/third_party/WebKit/Source/core/style/BorderValueTest.cpp#newcode60 third_party/WebKit/Source/core/style/BorderValueTest.cpp:60: EXPECT_EQ(border.ColorIsCurrentColor(), true); nit: I would test ...
3 years, 6 months ago (2017-05-30 01:23:03 UTC) #4
nainar
meade@, PTAL? Thanks! https://codereview.chromium.org/2913663002/diff/1/third_party/WebKit/Source/core/style/BorderValueTest.cpp File third_party/WebKit/Source/core/style/BorderValueTest.cpp (right): https://codereview.chromium.org/2913663002/diff/1/third_party/WebKit/Source/core/style/BorderValueTest.cpp#newcode60 third_party/WebKit/Source/core/style/BorderValueTest.cpp:60: EXPECT_EQ(border.ColorIsCurrentColor(), true); Done.
3 years, 6 months ago (2017-05-30 02:51:32 UTC) #8
meade_UTC10
lgtm with nit It's not clear to me how this benefits generating computed style (the ...
3 years, 6 months ago (2017-05-30 03:53:53 UTC) #9
nainar
On 2017/05/30 at 03:53:53, meade wrote: > lgtm with nit > > It's not clear ...
3 years, 6 months ago (2017-05-30 03:56:58 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2913663002/20001
3 years, 6 months ago (2017-05-30 05:45:34 UTC) #16
commit-bot: I haz the power
3 years, 6 months ago (2017-05-30 05:50:58 UTC) #19
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/de1b3604290f911cf4fc76988ee4...

Powered by Google App Engine
This is Rietveld 408576698