Bug 35407 – [chromium] add ability to activate the focused node in a WebView

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Evan Stade]

related:
http://code.google.com/p/chromium/issues/detail?id=29500
and
http://codereview.chromium.org/660137

[Evan Stade]

Created attachment 49537 [details]
patch

[WebKit Review Bot]

Attachment 49537 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:1291:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebKit/chromium/src/WebViewImpl.cpp:1307:  no_event is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Evan Stade]

Created attachment 49538 [details]
style fix

[WebKit Review Bot]

Attachment 49538 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:1291:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebKit/chromium/src/WebViewImpl.cpp:1307:  no_event is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Evan Stade]

Created attachment 49539 [details]
style fix, try2

[WebKit Review Bot]

Attachment 49539 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/313027

[Evan Stade]

Created attachment 49540 [details]
style fix, try3

one of these days I'll upload the right file.

[WebKit Review Bot]

Attachment 49540 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:1291:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Evan Stade]

Created attachment 49543 [details]
style again

By the way, to whoever reviews this: this code is in large part copy-pasted from clearFocusedNode(). I'm not greatly confident in its correctness, but it seems to work.

[Darin Fisher (:fishd, Google)]

Comment on attachment 49543 [details]
style again

> Index: WebKit/chromium/public/WebView.h
...
> +    // Activate the currently focused node, if any. If there is no focused node,
> +    // do nothing.
> +    virtual void activateFocusedNode() = 0;

I think it would be better to provide an accessor for the focusedNode.
Then add a method on WebNode called simulateClick.  It is better to
provide the basic building blocks in the API, so that we can more easily
do other things in the future.

My only question is whether or not we need to pass a WebInputEvent to
that simulateClick method.  This could matter for the popup blocker
since it will need to know if there was a user gesture.  Have you
tested what happens if the 'click' event handler tries to open up a
popup window?

[Evan Stade]

Created attachment 49849 [details]
expose WebNode

suggestion implemented. Tested + working on popup links.

[Evan Stade]

Created attachment 49972 [details]
activate->simulateClick

changed name of function from activate() to simulateClick() as I suppose "activate" only makes sense for certain nodes (e.g., links).

[Adam Barth]

I tried to review your patch, but fishd wants to sign off on all patches that touch the WebKit API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 49972 [details]
activate->simulateClick

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

[Evan Stade]

Created attachment 50141 [details]
review comments

I've done as you said, but along the way I noticed that WebViewImpl has a function called focusedWebCoreNode() which is not part of the API. It's implementation is exactly as you suggested (it acts on the document of the focused framE). It would thus be easy (and possibly more convenient for users of the API) to expose this function as part of the API.

[Darin Fisher (:fishd, Google)]

> I've done as you said, but along the way I noticed that WebViewImpl has a
> function called focusedWebCoreNode() which is not part of the API. It's
> implementation is exactly as you suggested (it acts on the document of the
> focused framE). It would thus be easy (and possibly more convenient for users
> of the API) to expose this function as part of the API.

Yeah, there's a balance between convenience and minimalism.  I tend to favor having the API provide the more basic building blocks, and then leave it up to the embedder to write helper functions to make things more convenient.  That way the interfaces are simpler and easier to maintain.  This case is pretty borderline.

[WebKit Commit Bot]

Comment on attachment 50141 [details]
review comments

Rejecting patch 50141 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Darin Fisher', '--force']" exit_code: 1
Last 500 characters of output:
king copy)
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: src/WebNode.cpp
|===================================================================
|--- src/WebNode.cpp	(revision 55462)
|+++ src/WebNode.cpp	(working copy)
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored

Full output: http://webkit-commit-queue.appspot.com/results/342183

[Evan Stade]

Created attachment 50235 [details]
fix paths in .diff

no code change, just fix patch so it applies

[WebKit Commit Bot]

Comment on attachment 50235 [details]
fix paths in .diff

Clearing flags on attachment: 50235

Committed r55808: <http://trac.webkit.org/changeset/55808>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.