[webkit-reviews] review denied: [Bug 33364] [Chromium] Should not select a word on right-click in editable text : [Attachment 46552] Proposed patch (rev.3)

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied TAMURA, Kent
<tkent at chromium.org>'s request for review:
Bug 33364: [Chromium] Should not select a word on right-click in editable text
https://bugs.webkit.org/show_bug.cgi?id=33364

Attachment 46552: Proposed patch (rev.3)
https://bugs.webkit.org/attachment.cgi?id=46552&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>

> +++ b/WebKit/chromium/public/WebFrame.h
> @@ -327,6 +327,9 @@ public:
>  
>      // Replaces the selection with the given text.
>      virtual void replaceSelection(const WebString& text) = 0;
> +    // Replaces the selection with the given text or replaces a word around
the

^^^ please insert a new line above the comment so that there is a new line
between functions.


> +    virtual void replaceWord(const WebString& text) = 0;

I think you should give this function a more descriptive name.	I'm also
concerned that it has the side-effect of altering the selection.  Perhaps
for the WebKit API we should really break this up.  We already have a
hasSelection() method, so it seems like you could just add a method
that causes selection to be set at the word around the caret.  Like this:

  virtual void selectWordAroundCaret() = 0;


> +++ b/WebKit/chromium/src/WebFrameImpl.cpp
...
> +void WebFrameImpl::replaceWord(const WebString& text)
> +{
> +    SelectionController* controller = frame()->selection();
> +    // Should have a caret or a ranged selection because the corrected word
was
> +    // based on them. See selectMisspelledWord() in
ContextMenuClientImpl.cpp.
> +    ASSERT(!controller->isNone());

It seems like there could be a race here.  Do we allow events to be processed
between the time selectMisspelledWord is called and the time that replaceWord
is called?