[Webkit-unassigned] [Bug 62954] add/removeSpellcheckRange use RefPtr incorrectly for arguments

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62954


Hironori Bono <hbono at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #97900|0                           |1
        is obsolete|                            |
  Attachment #97944|                            |review?, commit-queue?
               Flag|                            |




--- Comment #5 from Hironori Bono <hbono at chromium.org>  2011-06-21 00:31:39 PST ---
Created an attachment (id=97944)
 --> (https://bugs.webkit.org/attachment.cgi?id=97944&action=review)
A quick fix v2

Greetings Darin,

Thank you for your review and comments.

(In reply to comment #4)
> (From update of attachment 97900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97900&action=review
> 
> I’d prefer to see a function that makes removeSpellcheckRange use a raw pointer.

Thank you for noticing it. I have replaced its parameter from PassRefPtr to a raw pointer.

> > Source/WebCore/html/HTMLDivElement.cpp:94
> > -void HTMLDivElement::addSpellcheckRange(unsigned long start, unsigned long length, RefPtr<DOMStringList> suggestions, unsigned short options)
> > +void HTMLDivElement::addSpellcheckRange(unsigned long start, unsigned long length, PassRefPtr<DOMStringList> prpSuggestions, unsigned short options)
> >  {
> > -    document()->markers()->addUserSpellingMarker(this, start, length, suggestions, options);
> > +    document()->markers()->addUserSpellingMarker(this, start, length, prpSuggestions, options);
> >  }
> 
> Here there is no reason to change the name of the argument. Just “suggestions” would be fine. Same for the other overrides of this function.

Oops, it seems a script used for creating this change replaced this parameter as well. I have removed them.

> > Source/WebCore/html/HTMLDivElement.h:45
> > +    void removeSpellcheckRange(PassRefPtr<SpellcheckRange>);
> 
> I don’t understand why the remove function takes a PassRefPtr. The caller is not passing ownership in, so I’d think we could just use a raw pointer. Is there some problem with DOM bindings that means we have to use PassRefPtr here?

Sorry for my confusing change. If I recall correctly, I used RefPtr here just because I saw a valgrind error when implementing r88332. But, I cannot see any valgrind errors when I run this change on valgrind now. (Probably I misread another valgrind error.) I have replaced PassRefPtr with a raw pointer since it does not cause any valgrind errors any longer.

Regards,

Hironori Bono

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.