Bug 30832 – Notify the chrome when the focused node has changed.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Evan Stade]

This is similar to AX code that is already in place, except that this also informs the chrome when there stops being a focused node. This is needed for Chromium to show the anchor for links that have keyboard focus.

chromium side is here: http://codereview.chromium.org/337032

If there is some sort of test I should add for this, I'll be glad to.

[Evan Stade]

Created attachment 42073 [details]
proposed patch

[Eric Seidel (no email)]

Comment on attachment 42073 [details]
proposed patch

You'll need to mark patches r=? for them to be reviewed.
See: http://webkit.org/coding/contributing.html

It's not clear to me why this is not already covered by existing AX apis.

This style is incorrect:
+    if (!focusChangeBlocked) {
+        page()->chrome()->focusedNodeChanged(m_focusedNode.get());
+    }
no {} around one-line ifs.

Please explain why no other port needs this/why this can't be implemented on top of existing AX infrastructure in WebCore/WebKit?

[Eric Seidel (no email)]

Comment on attachment 42073 [details]
proposed patch

r- for the above mentioned reasons.  Thanks for marking it r? though. :)

[Evan Stade]

other ports could use it if they wanted to, I suppose. I wasn't sure they'd want to, since it is similar to the AX code, and I didn't want to change the way AX worked because of the possibility of ill consequences for those other platforms.

The difference from the AX infrastructure is that AX doesn't tell us when a node loses focus. (notice difference in the conditional)

[Evan Stade]

Created attachment 42075 [details]
style fix

also, I don't view this as an accessibility feature any more than supporting tab to navigate a page is an accessibility feature.

[Darin Adler]

Comment on attachment 42075 [details]
style fix

A general purpose feature like this should not be added inside PLATFORM(CHROMIUM) ifdefs.

[David Levin]

Please add a bug link to the changelog as well (see other changelog entries).

[David Levin]

Comment on attachment 42075 [details]
style fix

r- for the two comments above (Darin Adler's and mine).

[Evan Stade]

Created attachment 42121 [details]
more fixes

removed PLATFORM defines, added bu number to changelog

[David Levin]

Comment on attachment 42121 [details]
more fixes

My only concern is that this will break the builds for non-chromium clients.

I think the easiest way to handle this is to not make this function pure  
   "virtual void focusedNodeChanged(Node*) = 0" 
but I would recommend against this.

The very nice thing about pure methods is that they help ports to quick realize when they got broken by a parameter being added/changed/etc.

So instead it would be *much better* to find each class that derives from ChromeClient and given them a default implementation that does nothing.

With this last change, I think this will be ready to go in.

[Evan Stade]

That the reason I initially ifdefed it to chromium.

Anyway, I think I covered all the ChromeClient implementations that are in the WebKit tree (correct me if I'm wrong). There are only two, EmptyChromeClient and SVGChromeClient (which inherits from the former). All the others are presumably in other repositories (for example, chrome's is in the chromium tree).

So whoever rolls this change into the chromium tree will need to simultaneously land my chrome-side change (actually I think it would work to land the chrome side change first). I don't know what the procedure is for other projects. Presumably this will break them and they will promptly realize they need a stub implementation.

[David Levin]

(In reply to comment #11)
> Anyway, I think I covered all the ChromeClient implementations that are in the
> WebKit tree (correct me if I'm wrong).

git grep "public WebCore::ChromeClient"
   WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:    class ChromeClient : public WebCore::ChromeClient { 
   WebKit/mac/WebCoreSupport/WebChromeClient.h:class WebChromeClient : public WebCore::ChromeClient {
   WebKit/win/WebCoreSupport/WebChromeClient.h:class WebChromeClient : public WebCore::ChromeClient {

git grep "public ChromeClient"
   WebCore/loader/EmptyClients.h:class EmptyChromeClient : public ChromeClient {
   WebCore/page/chromium/ChromeClientChromium.h:    class ChromeClientChromium : public ChromeClient {
   WebKit/haiku/WebCoreSupport/ChromeClientHaiku.h:    class ChromeClientHaiku : public ChromeClient {
   WebKit/qt/WebCoreSupport/ChromeClientQt.h:    class ChromeClientQt : public ChromeClient
   WebKit/wx/WebKitSupport/ChromeClientWx.h:class ChromeClientWx : public ChromeClient {

It looks like there are at 8, so your patch is missing 6 of them.

[Evan Stade]

ah, I stupidly grepped in WebCore/ only. Patch coming up.

[Evan Stade]

Created attachment 42224 [details]
stubs

I'm not sure what the convention is for all the various ChangeLog entries. I copied the same message into all of them. Would it be better to put "add a stub implementation of a ChromeClient function"?

[Darin Adler]

(In reply to comment #14)
> I'm not sure what the convention is for all the various ChangeLog entries. I
> copied the same message into all of them. Would it be better to put "add a stub
> implementation of a ChromeClient function"?

Yes, I think so.

[Darin Adler]

Comment on attachment 42224 [details]
stubs

I don't think we need notImplemented() inside any of these functions. That's only there when we know we need to add code, but this is a case where an empty function is probably OK for most ports at least at first. There's not necessarily anything to implement.

Patch seems fine, r=me

[Evan Stade]

Created attachment 42225 [details]
changelog desc changed

done.

[David Levin]

Comment on attachment 42225 [details]
changelog desc changed

I'll land it and remove the notImplemented(); from the stub functions.

[David Levin]

Committed as http://trac.webkit.org/changeset/50351