Summary: | Notify the chrome when the focused node has changed. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Evan Stade <estade> | ||||||||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | darin, eric, fishd, levin | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Evan Stade
2009-10-27 13:06:32 PDT
Created attachment 42073 [details]
proposed patch
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? Comment on attachment 42073 [details]
proposed patch
r- for the above mentioned reasons. Thanks for marking it r? though. :)
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) 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.
Comment on attachment 42075 [details]
style fix
A general purpose feature like this should not be added inside PLATFORM(CHROMIUM) ifdefs.
Please add a bug link to the changelog as well (see other changelog entries). Comment on attachment 42075 [details]
style fix
r- for the two comments above (Darin Adler's and mine).
Created attachment 42121 [details]
more fixes
removed PLATFORM defines, added bu number to changelog
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.
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. (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. ah, I stupidly grepped in WebCore/ only. Patch coming up. 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"?
(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 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
Created attachment 42225 [details]
changelog desc changed
done.
Comment on attachment 42225 [details]
changelog desc changed
I'll land it and remove the notImplemented(); from the stub functions.
Committed as http://trac.webkit.org/changeset/50351 |