Bug 67103 - Implement list style properties in CSSStyleApplyProperty.
Summary: Implement list style properties in CSSStyleApplyProperty.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Luke Macpherson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-28 21:38 PDT by Luke Macpherson
Modified: 2011-09-06 19:06 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.45 KB, patch)
2011-08-28 21:45 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (8.48 KB, patch)
2011-08-29 21:18 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (7.63 KB, patch)
2011-09-04 20:22 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2011-08-28 21:38:44 PDT
Implement list style properties in CSSStyleApplyProperty.
Comment 1 Luke Macpherson 2011-08-28 21:45:23 PDT
Created attachment 105457 [details]
Patch
Comment 2 Darin Adler 2011-08-29 10:56:51 PDT
Comment on attachment 105457 [details]
Patch

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

> Source/WebCore/ChangeLog:22
> +        Added version to take a raw pointer. The existing code relies on an implicit cast to PassRefPtr,
> +        but because of the templates used an exactly matching type signature is required.

I think a better fix to that issue would be to use create functions that return PassRefPtr. It seems this code is making too much use of raw pointers for newly-created RefCounted objects that are known to have a reference count of 1. That’s a confusing non-standard coding style in WebKit, more like how we did things in the distant past. Could we use create functions instead? Maybe the create functions would bulk the source code up too much?
Comment 3 Luke Macpherson 2011-08-29 21:18:03 PDT
Created attachment 105573 [details]
Patch
Comment 4 Darin Adler 2011-08-30 10:10:19 PDT
Comment on attachment 105573 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyle.h:607
> -    StyleImage* listStyleImage() const { return inherited->list_style_image.get(); }
> +    PassRefPtr<StyleImage> listStyleImage() const { return inherited->list_style_image; }

This change is not a good idea. While it’s convenient to have a PassRefPtr in CSSStyleApplyProperty.cpp, in other call sites using this type will result in reference count churn.
Comment 5 Luke Macpherson 2011-09-04 20:22:53 PDT
Created attachment 106300 [details]
Patch
Comment 6 Eric Seidel (no email) 2011-09-06 13:53:31 PDT
Comment on attachment 106300 [details]
Patch

Seems reasonable.
Comment 7 WebKit Review Bot 2011-09-06 19:06:10 PDT
Comment on attachment 106300 [details]
Patch

Clearing flags on attachment: 106300

Committed r94625: <http://trac.webkit.org/changeset/94625>
Comment 8 WebKit Review Bot 2011-09-06 19:06:15 PDT
All reviewed patches have been landed.  Closing bug.