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

Issue 315043005: Simplify CSSSelector::isAttributeSelector() by not checking each Match type (Closed)

Created:
6 years, 6 months ago by Inactive
Modified:
6 years, 6 months ago
Reviewers:
esprehn, eseidel
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, dglazkov+blink, ed+blinkwatch_opera.com, rwlbuis, rune+blink
Visibility:
Public.

Description

Simplify CSSSelector::isAttributeSelector() by not checking each Match type Simplify CSSSelector::isAttributeSelector() by not checking each Match type explicitly and reordering the Match enum instead so that the attribute selector match type are contiguous and at the end of the enum. This way, the check can become (m_match >= FirstAttributeSelectorMatch). This optimization is similar to the one we already use in CollectionType.h for collection types. isAttributeSelector() is called from SelectorChecker::checkOne() which is hot code. I see a ~2% progression on Dromaeo's cssquery-jquery: http://dromaeo.com/?id=222514,222518 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175632

Patch Set 1 #

Total comments: 1

Patch Set 2 : Move value to enum #

Total comments: 2

Patch Set 3 : Move new value to end of enum #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -11 lines) Patch
M Source/core/css/CSSSelector.h View 1 2 2 chunks +6 lines, -11 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
Inactive
6 years, 6 months ago (2014-06-04 21:39:05 UTC) #1
esprehn
lgtm, fix nit before landing. https://codereview.chromium.org/315043005/diff/1/Source/core/css/CSSSelector.h File Source/core/css/CSSSelector.h (right): https://codereview.chromium.org/315043005/diff/1/Source/core/css/CSSSelector.h#newcode124 Source/core/css/CSSSelector.h:124: static const Match FirstAttributeSelectorMatch ...
6 years, 6 months ago (2014-06-04 22:38:43 UTC) #2
Inactive
https://codereview.chromium.org/315043005/diff/20001/Source/core/css/CSSSelector.h File Source/core/css/CSSSelector.h (right): https://codereview.chromium.org/315043005/diff/20001/Source/core/css/CSSSelector.h#newcode117 Source/core/css/CSSSelector.h:117: FirstAttributeSelectorMatch = Exact, esprehn@, is this what you had ...
6 years, 6 months ago (2014-06-04 23:49:51 UTC) #3
Inactive
https://codereview.chromium.org/315043005/diff/20001/Source/core/css/CSSSelector.h File Source/core/css/CSSSelector.h (right): https://codereview.chromium.org/315043005/diff/20001/Source/core/css/CSSSelector.h#newcode117 Source/core/css/CSSSelector.h:117: FirstAttributeSelectorMatch = Exact, On 2014/06/04 23:49:52, Chris Dumez wrote: ...
6 years, 6 months ago (2014-06-05 02:00:22 UTC) #4
Inactive
The CQ bit was checked by ch.dumez@samsung.com
6 years, 6 months ago (2014-06-06 01:30:11 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/315043005/30001
6 years, 6 months ago (2014-06-06 01:30:50 UTC) #6
commit-bot: I haz the power
6 years, 6 months ago (2014-06-06 03:30:06 UTC) #7
Message was sent while issue was closed.
Change committed as 175632

Powered by Google App Engine
This is Rietveld 408576698