Bug 81875

Summary: Don't parse CSS property names for invalid declarations.
Product: WebKit Reporter: Luke Macpherson <macpherson>
Component: CSSAssignee: Luke Macpherson <macpherson>
Status: RESOLVED WONTFIX    
Severity: Normal CC: dglazkov, eric, gustavo, kling, koivisto, macpherson, menard, pnormand, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Luke Macpherson
Reported 2012-03-21 22:49:40 PDT
Don't parse CSS property names for invalid declarations.
Attachments
Patch (4.10 KB, patch)
2012-03-21 22:51 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
Patch (2.15 KB, patch)
2012-03-22 15:28 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
Patch (2.03 KB, patch)
2012-03-22 15:52 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Luke Macpherson
Comment 1 2012-03-21 22:51:41 PDT
Created attachment 133191 [details] Patch
Alexis Menard (darktears)
Comment 2 2012-03-22 03:59:03 PDT
It looks good to me.
Antti Koivisto
Comment 3 2012-03-22 05:17:05 PDT
Comment on attachment 133191 [details] Patch How common are the invalid declarations? Is this worth making the grammar less readable?
Antti Koivisto
Comment 4 2012-03-22 05:20:58 PDT
Comment on attachment 133191 [details] Patch r- for now, I don't think this special case micro-optimization is worth removing the concept of 'property' from the grammar.
Luke Macpherson
Comment 5 2012-03-22 15:05:41 PDT
Ok Antti, I can re-write it slightly differently and keep property in the grammar but still defer the conversion. Will upload a patch shortly.
Luke Macpherson
Comment 6 2012-03-22 15:28:13 PDT
Created attachment 133366 [details] Patch
Philippe Normand
Comment 7 2012-03-22 15:33:36 PDT
Comment on attachment 133366 [details] Patch Attachment 133366 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12116282
WebKit Review Bot
Comment 8 2012-03-22 15:46:52 PDT
Comment on attachment 133366 [details] Patch Attachment 133366 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12116294
Early Warning System Bot
Comment 9 2012-03-22 15:50:14 PDT
Comment on attachment 133366 [details] Patch Attachment 133366 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12114267
Build Bot
Comment 10 2012-03-22 15:51:42 PDT
Comment on attachment 133366 [details] Patch Attachment 133366 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12123191
Luke Macpherson
Comment 11 2012-03-22 15:52:59 PDT
Created attachment 133372 [details] Patch
Luke Macpherson
Comment 12 2012-03-26 16:09:32 PDT
Ping. Please re-review.
Eric Seidel (no email)
Comment 13 2012-03-26 17:07:53 PDT
So this is a perf improvement? what's the speedup? (On which benchmark?)
Luke Macpherson
Comment 14 2012-03-26 17:22:41 PDT
(In reply to comment #13) > So this is a perf improvement? what's the speedup? (On which benchmark?) It might be a speed improvement, but only when parsing invalid declarations. Otherwise perf should be unchanged. Main reason is that: http://dev.w3.org/csswg/css-variables/ allows the definition of arbitrary property names that no longer map an integer, so having the property string available is useful.
Eric Seidel (no email)
Comment 15 2012-04-19 15:48:53 PDT
Come defend your patch in person at the reviewathon!
Note You need to log in before you can comment on or make changes to this bug.