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

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

Created:
3 years, 3 months ago by Timothy Loh
Modified:
3 years, 2 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months ago (2016-07-14 07:23:17 UTC) #14
yosin_UTC9
+tkent@, WDYT?
3 years, 3 months 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, 3 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months ago (2016-08-09 06:36:27 UTC) #27
commit-bot: I haz the power
Committed patchset #4 (id:60001)
3 years, 2 months ago (2016-08-09 08:19:46 UTC) #29
commit-bot: I haz the power
3 years, 2 months 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