Bug 143925 - CSSParser::parseValue() copies the m_parsedProperties vector at addParsedProperties()
Summary: CSSParser::parseValue() copies the m_parsedProperties vector at addParsedProp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-18 14:58 PDT by Simon Fraser (smfr)
Modified: 2015-04-20 19:27 PDT (History)
6 users (show)

See Also:

Attachments
Patch (4.12 KB, patch)
2015-04-20 11:34 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (4.16 KB, patch)
2015-04-20 11:57 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2015-04-18 14:58:26 PDT 
MutableStyleProperties::addParsedProperties() takes a const Vector<CSSProperty>& but m_parsedProperties is a Vector<CSSProperty, 256>, and apparently this forces a copy.
Comment 1 Chris Dumez 2015-04-20 10:34:36 PDT 
Very nice find :) We really supposed to use ParsedPropertyVector type here.
Comment 2 Chris Dumez 2015-04-20 11:34:04 PDT 
Created attachment 251171 [details]
Patch
Comment 3 Simon Fraser (smfr) 2015-04-20 11:46:49 PDT 
Comment on attachment 251171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251171&action=review

> Source/WebCore/css/CSSParser.h:84
> +    using ParsedPropertyVector = Vector<CSSProperty, 256>;

Does the compiler enforce a match between this and the typedef?

> Source/WebCore/css/StyleProperties.cpp:770
> +    for (auto& property : properties)

const auto&?
Comment 4 Chris Dumez 2015-04-20 11:50:07 PDT 
Comment on attachment 251171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251171&action=review

>> Source/WebCore/css/CSSParser.h:84
>> +    using ParsedPropertyVector = Vector<CSSProperty, 256>;
> 
> Does the compiler enforce a match between this and the typedef?

Which typedef? I just made the typedef public so it could be used in StyleProperties. I used a 'using' statement but it is equivalent to a typedef in this case.

>> Source/WebCore/css/StyleProperties.cpp:770
>> +    for (auto& property : properties)
> 
> const auto&?

auto will be a const CSSProperty so it is equivalent but OK.
Comment 5 Chris Dumez 2015-04-20 11:57:05 PDT 
Created attachment 251179 [details]
Patch
Comment 6 Chris Dumez 2015-04-20 12:07:22 PDT 
To avoid mistakes like this in the future, I think we should make the following constructor explicit:
template<size_t otherCapacity, typename otherOverflowBehaviour>
Vector(const Vector<T, otherCapacity, otherOverflowBehaviour>&);

I can look into this in a follow-up patch as it likely is a larger change.
Comment 7 Chris Dumez 2015-04-20 12:41:07 PDT 
Comment on attachment 251179 [details]
Patch

Clearing flags on attachment: 251179

Committed r183024: <http://trac.webkit.org/changeset/183024>
Comment 8 Chris Dumez 2015-04-20 12:41:13 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 9 Chris Dumez 2015-04-20 19:27:17 PDT 
(In reply to comment #6)
> To avoid mistakes like this in the future, I think we should make the
> following constructor explicit:
> template<size_t otherCapacity, typename otherOverflowBehaviour>
> Vector(const Vector<T, otherCapacity, otherOverflowBehaviour>&);
> 
> I can look into this in a follow-up patch as it likely is a larger change.

-> https://bugs.webkit.org/show_bug.cgi?id=143970