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

Issue 27033011: Fix readonly and type attribute selector incorrect style sharing (Closed)

Created:
7 years, 2 months ago by esprehn
Modified:
7 years, 2 months ago
CC:
blink-reviews, dglazkov+blink, apavlov+blink_chromium.org, darktears
Visibility:
Public.

Description

Fix readonly and type attribute selector incorrect style sharing RuleSet has a special case in isCommonAttributeSelectorAttribute for allowing style sharing on elements that match "type" or "readonly" attribute selectors as long as they have the same value. This was added long ago in r96393 and worked based on the assumption that there was logic in canShareStyleWithElement that actually checked the value of the type and readonly attributes on the style sharing candidates. Later in r123730 the type attribute handling was regressed when it was moved to only check the value for <input>. The readonly attribute was later regressed in the same way in r134984. This patch restores the checks those two patches moved but retains the switch from getAttribute to fastGetAttribute for HTML which was likely most of the performance improvement and fixes the bug where elements would incorrectly share style even if one of them matched an attribute selector. Unfortunately this does mean that we need to call the slow geAttribute in SVG documents which may be a minor regression. We can work to improve the performance of SVG in the future once we get the correctness back. BUG=307170 R=leviw@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159699

Patch Set 1 #

Patch Set 2 : fix for svg #

Unified diffs Side-by-side diffs Delta from patch set Stats (+53 lines, -6 lines) Patch
A LayoutTests/fast/css/style-sharing-type-and-readonly.html View 1 chunk +26 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/style-sharing-type-and-readonly-expected.txt View 1 chunk +14 lines, -0 lines 0 comments Download
M Source/core/css/resolver/SharedStyleFinder.cpp View 1 3 chunks +13 lines, -6 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
esprehn
7 years, 2 months ago (2013-10-15 06:58:12 UTC) #1
esprehn
7 years, 2 months ago (2013-10-15 06:58:27 UTC) #2
leviw_travelin_and_unemployed
Great catch! lgtm.
7 years, 2 months ago (2013-10-15 07:20:56 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/esprehn@chromium.org/27033011/1
7 years, 2 months ago (2013-10-15 07:45:14 UTC) #4
commit-bot: I haz the power
Change committed as 159662
7 years, 2 months ago (2013-10-15 10:19:52 UTC) #5
leviw_travelin_and_unemployed
Thanks for the explanation on irc. LGTM.
7 years, 2 months ago (2013-10-15 20:17:13 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/esprehn@chromium.org/27033011/13001
7 years, 2 months ago (2013-10-15 20:19:23 UTC) #7
esprehn
7 years, 2 months ago (2013-10-15 23:59:22 UTC) #8
Message was sent while issue was closed.
Committed patchset #2 manually as r159699 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698