Bug 44719 - Fix RenderStyle::addCursor to use a StyleImage, not a CachedImage
Summary: Fix RenderStyle::addCursor to use a StyleImage, not a CachedImage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-26 13:47 PDT by Simon Fraser (smfr)
Modified: 2010-08-30 09:01 PDT (History)
1 user (show)

See Also:


Attachments
Patch (11.14 KB, patch)
2010-08-27 16:47 PDT, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-08-26 13:47:10 PDT
Per comment in CSSStyleSelector:
                     // FIXME: Temporary clumsiness to pass off a CachedImage to an API that will eventually convert to using
                    // StyleImage.
Comment 1 Simon Fraser (smfr) 2010-08-27 16:47:34 PDT
Created attachment 65786 [details]
Patch
Comment 2 Darin Adler 2010-08-29 12:54:19 PDT
Comment on attachment 65786 [details]
Patch

>          case CSSPropertyBackgroundImage:
> +            // FIXME: broken for multiple backgrounds.

It would be better if this was capitalized sentence-style like our comments usually are, and if there was a corresponding bug umber.

> +            case CSSPropertyCursor: {
> +                if (CursorList* cursorList = m_style->cursors()) {
> +                    for (size_t i = 0; i < cursorList->size(); ++i) {
> +                        CursorData& currentCursor = (*cursorList)[i];
> +                        if (currentCursor.image()->isPendingImage()) {
> +                            CSSImageValue* imageValue = static_cast<StylePendingImage*>(currentCursor.image())->cssImageValue();
> +                            currentCursor.setImage(imageValue->cachedImage(docLoader));
> +                        }
> +                    }
> +                }
>                  break;
> +            }

Is it correct to do nothing if there are not cursors?

> +    void setImage(StyleImage* image) { m_image = image; }    

This function should take a PassRefPtr, not a raw pointer.

> +    void addCursor(StyleImage*, const IntPoint& = IntPoint());

Should this take a PassRefPtr instead of a raw pointer? The IntPoint argument should have a name, hotSpot, instead of being unnamed. I don't think it's obvious what it is without a name.
Comment 3 Simon Fraser (smfr) 2010-08-30 09:01:05 PDT
http://trac.webkit.org/changeset/66391