Bug 30832 - Notify the chrome when the focused node has changed.
: Notify the chrome when the focused node has changed.
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-10-27 13:06 PST by
Modified: 2009-10-30 14:01 PST (History)


Attachments
proposed patch (3.96 KB, patch)
2009-10-28 17:54 PST, Evan Stade
eric: review-
Review Patch | Details | Formatted Diff | Diff
style fix (3.96 KB, patch)
2009-10-28 18:20 PST, Evan Stade
levin: review-
Review Patch | Details | Formatted Diff | Diff
more fixes (3.88 KB, patch)
2009-10-29 11:47 PST, Evan Stade
levin: review-
Review Patch | Details | Formatted Diff | Diff
stubs (15.77 KB, patch)
2009-10-30 11:58 PST, Evan Stade
darin: review+
Review Patch | Details | Formatted Diff | Diff
changelog desc changed (14.75 KB, patch)
2009-10-30 12:13 PST, Evan Stade
levin: review+
levin: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-10-27 13:06:32 PST
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.
------- Comment #1 From 2009-10-28 17:54:46 PST -------
Created an attachment (id=42073) [details]
proposed patch
------- Comment #2 From 2009-10-28 17:58:30 PST -------
(From update of attachment 42073 [details])
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?
------- Comment #3 From 2009-10-28 18:03:03 PST -------
(From update of attachment 42073 [details])
r- for the above mentioned reasons.  Thanks for marking it r? though. :)
------- Comment #4 From 2009-10-28 18:05:01 PST -------
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)
------- Comment #5 From 2009-10-28 18:20:55 PST -------
Created an attachment (id=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.
------- Comment #6 From 2009-10-28 20:22:49 PST -------
(From update of attachment 42075 [details])
A general purpose feature like this should not be added inside PLATFORM(CHROMIUM) ifdefs.
------- Comment #7 From 2009-10-28 21:21:46 PST -------
Please add a bug link to the changelog as well (see other changelog entries).
------- Comment #8 From 2009-10-28 21:23:39 PST -------
(From update of attachment 42075 [details])
r- for the two comments above (Darin Adler's and mine).
------- Comment #9 From 2009-10-29 11:47:30 PST -------
Created an attachment (id=42121) [details]
more fixes

removed PLATFORM defines, added bu number to changelog
------- Comment #10 From 2009-10-29 22:20:03 PST -------
(From update of attachment 42121 [details])
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.
------- Comment #11 From 2009-10-30 11:02:52 PST -------
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.
------- Comment #12 From 2009-10-30 11:25:20 PST -------
(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.
------- Comment #13 From 2009-10-30 11:29:05 PST -------
ah, I stupidly grepped in WebCore/ only. Patch coming up.
------- Comment #14 From 2009-10-30 11:58:24 PST -------
Created an attachment (id=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"?
------- Comment #15 From 2009-10-30 12:11:29 PST -------
(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.
------- Comment #16 From 2009-10-30 12:13:02 PST -------
(From update of attachment 42224 [details])
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
------- Comment #17 From 2009-10-30 12:13:41 PST -------
Created an attachment (id=42225) [details]
changelog desc changed

done.
------- Comment #18 From 2009-10-30 12:15:28 PST -------
(From update of attachment 42225 [details])
I'll land it and remove the notImplemented(); from the stub functions.
------- Comment #19 From 2009-10-30 14:01:06 PST -------
Committed as http://trac.webkit.org/changeset/50351