[webkit-reviews] review requested: [Bug 62954] add/removeSpellcheckRange use RefPtr incorrectly for arguments : [Attachment 97944] A quick fix v2

[bugzilla-daemon at webkit.org]

Hironori Bono <hbono at chromium.org> has asked  for review:
Bug 62954: add/removeSpellcheckRange use RefPtr incorrectly for arguments
https://bugs.webkit.org/show_bug.cgi?id=62954

Attachment 97944: A quick fix v2
https://bugs.webkit.org/attachment.cgi?id=97944&action=review

------- Additional Comments from Hironori Bono <hbono at chromium.org>
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