Bug 62954 – add/removeSpellcheckRange use RefPtr incorrectly for arguments

• Status: RESOLVED
• Resolution: INVALID
• Component: HTML Editing

[Darin Adler]

Arguments should be PassRefPtr or raw pointers, never RefPtr.

[Hironori Bono]

Created attachment 97783 [details]
A quick patch v0

Greetings,

(In reply to comment #0)
> Arguments should be PassRefPtr or raw pointers, never RefPtr.

Thank you for noticing it. I have replaced all the RefPtr arguments in my r88332: <http://trac.webkit.org/changeset/88332> with PassRefPtr. (Even though I have not tried this change on valgrind, I think this change works without memory leaks.) It would be definitely helpful to give me feedback.

Regards,

Hironori Bono

[Darin Adler]

Comment on attachment 97783 [details]
A quick patch v0

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

> Source/WebCore/dom/DocumentMarkerController.cpp:686
> +bool DocumentMarkerController::addUserSpellingMarker(Node* node, unsigned start, unsigned length, PassRefPtr<DOMStringList> srcSuggestions, unsigned options)

The prefix we use here is “prp”, not “src”.

[Hironori Bono]

Created attachment 97900 [details]
A quick patch v1

Greetings Darin,

(In reply to comment #2)
> (From update of attachment 97783 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97783&action=review
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:686
> > +bool DocumentMarkerController::addUserSpellingMarker(Node* node, unsigned start, unsigned length, PassRefPtr<DOMStringList> srcSuggestions, unsigned options)
> 
> The prefix we use here is “prp”, not “src”.

Thank you for noticing it. (I'm a little wondering why check-webkit-style did not notice it, though. It may be a good idea to write a change for it?) I have updated this change to use prp prefixes for PassRefPtr arguments. Is it possible to review the updated one?

Regards,

Hironori Bono

[Darin Adler]

Comment on attachment 97900 [details]
A quick patch v1

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.

> 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.

> 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?

[Hironori Bono]

Created attachment 97944 [details]
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

[Hironori Bono]

Greetings,

Sorry, I have uploaded a wrong patch. I will upload the correct soon.

Regards,

Hironori Bono

[Simon Fraser (smfr)]

Comment on attachment 97944 [details]
A quick fix v2

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

> Source/WebCore/dom/DocumentMarkerController.cpp:687
>  {

drop the 'prp'. We don't use Hungarian notation.

[Darin Adler]

Simon, we do use prp for PassRefPtr, even though we make no other uses of Hungarian notation. It’s mentioned here <http://www.webkit.org/coding/RefPtr.html> in the section labeled “Function arguments”.

[Eric Seidel (no email)]

Comment on attachment 97944 [details]
A quick fix v2

I believe Darin answered simon's concern.  marking cq+ (Since the author is not a committer).

[WebKit Review Bot]

Comment on attachment 97944 [details]
A quick fix v2

Rejecting attachment 97944 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
nputElement.h.rej
patching file Source/WebCore/html/HTMLTextAreaElement.cpp
Hunk #1 FAILED at 455.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/html/HTMLTextAreaElement.cpp.rej
patching file Source/WebCore/html/HTMLTextAreaElement.h
Hunk #1 FAILED at 69.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/html/HTMLTextAreaElement.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Simon Fraser', u'--for..." exit_code: 1

Full output: http://queues.webkit.org/results/10998131

[Hironori Bono]

Greetings,

Sorry for the lack of updates. My change that added add/removeSpellcheckRange has been rolled out, i.e. this issue is obsolete.

Regards,

Hironori Bono