[webkit-reviews] review denied: [Bug 35407] [chromium] add ability to activate the focused node in a WebView : [Attachment 49972] activate->simulateClick

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Evan Stade
<estade at chromium.org>'s request for review:
Bug 35407: [chromium] add ability to activate the focused node in a WebView
https://bugs.webkit.org/show_bug.cgi?id=35407

Attachment 49972: activate->simulateClick
https://bugs.webkit.org/attachment.cgi?id=49972&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebKit/chromium/ChangeLog
...
> +	   [chromium] add functionality to activate the focused node in a
WebView
> +	   https://bugs.webkit.org/show_bug.cgi?id=35407
> +
> +	   * public/WebNode.h:
> +	   * public/WebView.h:
> +	   * src/WebNode.cpp:
> +	   (WebKit::WebNode::activate): Added.

It looks like this ChangeLog should be re-generated since the newly
added method is named simulateClick.  It'd be good to make the comment
in the ChangeLog also say that you are adding an access for the focused
node, and that you are providing a method to simulate a click on a node.


> Index: WebKit/chromium/public/WebView.h
...
> +    // Get the currently focused node. If no node is focused, returns a Null


nit: s/Null/null/ for consistency with other comments.


> Index: WebKit/chromium/src/WebNode.cpp
...
> +void WebNode::simulateClick()
> +{

insert an ASSERT(m_private) here please.

> +    RefPtr<Event> noEvent;
> +    m_private->dispatchSimulatedClick(noEvent);
> +}


> Index: WebKit/chromium/src/WebViewImpl.cpp
...
> +WebNode WebViewImpl::focusedNode()
> +{
> +    if (!m_page.get())
> +	   return WebNode();
> +
> +    RefPtr<Frame> frame = m_page->mainFrame();
> +    if (!frame.get())
> +	   return WebNode();
> +
> +    RefPtr<Document> document = frame->document();
> +    if (!document.get())
> +	   return WebNode();
> +
> +    RefPtr<Node> focusedNode = document->focusedNode();

What if the focused node is actually contained in a subframe?
Perhaps you should use FocusController::focusedFrame to access
the document containing the focused node.

This also suggests that it might be best if we instead added
focusedNode to WebDocument.  So, your code in Chromium would
then need to access the focusedNode like so:

  WebFrame* frame = view->focusedFrame();
  if (frame) {
    WebDocument doc = frame->document();
    if (!doc.isNull()) {
      WebNode node = doc.focusedNode(); 
      ...
    }
  }

Sorry for giving bad advice about putting focusedNode on WebView.