Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(137)

Issue 2103043004: Fix EditingStyle::mergeStyle()'s handling of custom properties (Closed)

Created:
3 years, 1 month ago by Timothy Loh
Modified:
3 years ago
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix EditingStyle::mergeStyle()'s handling of custom properties This patch fixes the logic of EditingStyle::mergeStyle() to correctly handle custom properties. Currently it serializes the CSSValue and then reparses it, which, aside from being inefficient, doesn't work for custom properties as the custom property name is lost (since we only have the enum value CSSPropertyVariable). BUG=622420 Committed: https://crrev.com/aadb63893e4c1358d1e5139aa29552eb190682c8 Cr-Commit-Position: refs/heads/master@{#410614}

Patch Set 1 #

Total comments: 6

Patch Set 2 : test tweak #

Total comments: 8

Patch Set 3 : test tweak #

Total comments: 3

Patch Set 4 : gtest instead of layout test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -2 lines) Patch
M third_party/WebKit/Source/core/core.gypi View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/StylePropertySet.cpp View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/editing/EditingStyle.h View 1 2 3 2 chunks +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/EditingStyle.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
A third_party/WebKit/Source/core/editing/EditingStyleTest.cpp View 1 2 3 1 chunk +41 lines, -0 lines 0 comments Download

Messages

Total messages: 31 (11 generated)
Timothy Loh
I don't really know the surrounding context to this code, so the test is just ...
3 years, 1 month ago (2016-06-29 04:30:06 UTC) #5
yosin_UTC9
I can't access crbug.com/622420. Could you add me CC in that bug? Thanks in advance. ...
3 years, 1 month ago (2016-06-29 05:11:42 UTC) #6
Timothy Loh
https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode15 third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:15: document.execCommand('RemoveFormat', 'true', 'true') On 2016/06/29 05:11:41, Yosi_UTC9 wrote: > ...
3 years, 1 month ago (2016-06-29 05:22:04 UTC) #7
alancutter (OOO until 2018)
lgtm with test nit. https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode16 third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:16: window.testRunner && testRunner.dumpAsText(); On 2016/06/29 ...
3 years, 1 month ago (2016-06-29 07:26:10 UTC) #8
yosin_UTC9
https://codereview.chromium.org/2103043004/diff/20001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/20001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode11 third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:11: newElem.style.cssText = '--AAAA: var(--BBBB)'; I think this line should ...
3 years, 1 month ago (2016-06-30 04:07:38 UTC) #9
Timothy Loh
https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/1/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode16 third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:16: window.testRunner && testRunner.dumpAsText(); On 2016/06/29 07:26:10, alancutter wrote: > ...
3 years, 1 month ago (2016-06-30 04:16:42 UTC) #10
yosin_UTC9
https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode1 third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:1: <!doctype html> How about writing gTest for this case, ...
3 years, 1 month ago (2016-07-01 09:28:49 UTC) #11
Timothy Loh
https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode1 third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:1: <!doctype html> On 2016/07/01 09:28:49, Yosi_UTC9 wrote: > How ...
3 years, 1 month ago (2016-07-07 02:54:33 UTC) #12
yosin_UTC9
https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode1 third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html:1: <!doctype html> On 2016/07/07 at 02:54:32, Timothy Loh wrote: ...
3 years, 1 month ago (2016-07-07 05:47:22 UTC) #13
alancutter (OOO until 2018)
On 2016/07/07 at 05:47:22, yosin wrote: > https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html > File third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html (right): > > https://codereview.chromium.org/2103043004/diff/40001/third_party/WebKit/LayoutTests/editing/execCommand/remove-format-variable-crash.html#newcode1 ...
3 years, 1 month ago (2016-07-14 07:23:17 UTC) #14
yosin_UTC9
+tkent@, WDYT?
3 years, 1 month ago (2016-07-14 07:49:55 UTC) #16
tkent
On 2016/07/14 at 07:49:55, yosin wrote: > +tkent@, WDYT? IMO, we should make C++ unit ...
3 years, 1 month ago (2016-07-14 08:14:13 UTC) #17
Timothy Loh
On 2016/07/07 05:47:22, Yosi_UTC9 wrote: > We would like to check result of the regressed ...
3 years ago (2016-08-09 03:27:13 UTC) #18
yosin_UTC9
lgtm since it has a test. On 2016/08/09 at 03:27:13, timloh wrote: > On 2016/07/07 ...
3 years ago (2016-08-09 05:15:59 UTC) #19
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/2103043004/60001
3 years ago (2016-08-09 05:17:35 UTC) #22
Timothy Loh
On 2016/08/09 05:15:59, Yosi_UTC9 wrote: > On 2016/08/09 at 03:27:13, timloh wrote: > > In ...
3 years ago (2016-08-09 05:21:43 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/269625)
3 years ago (2016-08-09 06:34:20 UTC) #25
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/2103043004/60001
3 years ago (2016-08-09 06:36:27 UTC) #27
commit-bot: I haz the power
Committed patchset #4 (id:60001)
3 years ago (2016-08-09 08:19:46 UTC) #29
commit-bot: I haz the power
3 years ago (2016-08-09 08:21:55 UTC) #31
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/aadb63893e4c1358d1e5139aa29552eb190682c8
Cr-Commit-Position: refs/heads/master@{#410614}

Powered by Google App Engine
This is Rietveld 408576698