Bug 87675 - Give StylePropertySet a proper mutability flag.
Summary: Give StylePropertySet a proper mutability flag.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on: 87750
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-28 12:36 PDT by Andreas Kling
Modified: 2012-05-29 08:55 PDT (History)
5 users (show)

See Also:


Attachments
Patch (17.52 KB, patch)
2012-05-28 14:29 PDT, Andreas Kling
darin: review+
Details | Formatted Diff | Diff
Also patch (33.46 KB, patch)
2012-05-29 04:56 PDT, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2012-05-28 12:36:36 PDT
Hax.
Comment 1 Andreas Kling 2012-05-28 14:29:25 PDT
Created attachment 144398 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-28 14:32:11 PDT
Attachment 144398 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/css/CSSStyleRule.cpp:64:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2012-05-28 14:57:02 PDT
Comment on attachment 144398 [details]
Patch

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

> Source/WebCore/css/StylePropertySet.h:56
> +    static PassRefPtr<StylePropertySet> createMutable(StylePropertyVector& properties)

I think we want adopt in this function name for the same reason we have it in the other function name, because the function adopts a property vector.
Comment 4 Antti Koivisto 2012-05-28 15:22:18 PDT
Comment on attachment 144398 [details]
Patch

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

> Source/WebCore/css/StyleRule.h:123
>      StylePropertySet* properties() const { return m_properties.get(); }
> +    StylePropertySet* mutableProperties();

Could properties() made return a const? We already do that for the immutable element properties.
Comment 5 Antti Koivisto 2012-05-29 03:27:22 PDT
Comment on attachment 144398 [details]
Patch

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

> Source/WebCore/css/StyleRule.cpp:188
> +StylePropertySet* StyleRule::mutableProperties()
> +{
> +    if (m_properties && !m_properties->isMutable())
> +        m_properties = m_properties->copy();
> +    return m_properties.get();
> +}

At least the copy constructor assumes that m_properties can't be null and looking at the parser I don't think it ever can be.
Comment 6 Andreas Kling 2012-05-29 04:56:53 PDT
Created attachment 144526 [details]
Also patch
Comment 7 WebKit Review Bot 2012-05-29 05:00:23 PDT
Attachment 144526 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/css/CSSStyleRule.cpp:64:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Antti Koivisto 2012-05-29 05:01:52 PDT
Comment on attachment 144526 [details]
Also patch

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

> Source/WebCore/css/StyleRule.cpp:240
> +    if (m_properties && !m_properties->isMutable())

Still don't need null check here.
Comment 9 Andreas Kling 2012-05-29 05:25:56 PDT
Committed r118761: <http://trac.webkit.org/changeset/118761>