Bug 62954

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

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

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

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?

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

Greetings, Sorry, I have uploaded a wrong patch. I will upload the correct soon. Regards, Hironori Bono

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.

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

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

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

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