[webkit-reviews] review granted: [Bug 55633] REGRESSION (WebKit2): Tab keys no longer observe Full Keyboard Access : [Attachment 84593] proposed fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 55633: REGRESSION (WebKit2): Tab keys no longer observe Full Keyboard
Access
https://bugs.webkit.org/show_bug.cgi?id=55633

Attachment 84593: proposed fix
https://bugs.webkit.org/attachment.cgi?id=84593&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=84593&action=review

> Source/WebCore/ChangeLog:13
> +	   compared to the former.

I would say “superset of information returned by” rather than “superset of
information compared to”.

> Source/WebKit2/ChangeLog:15
> +	   Get the current state of full keybaord access, listening for change
notifications.

Typo: keybaord.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:344
> +#if PLATFORM(MAC)
> +KeyboardUIMode WebChromeClient::keyboardUIMode()

Isn’t there a Mac-specific file to put this in?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1195
> +    bool fullKeyboardAccessEnabled =
WebProcess::shared().fullKeyboardAccessEnabled();
> +    return static_cast<KeyboardUIMode>((fullKeyboardAccessEnabled ?
KeyboardAccessFull : KeyboardAccessDefault) | (m_tabToLinks ?
KeyboardAccessTabsToLinks : 0));

I think there’s a way to write this that avoids the typecasts:

    KeyboardUIMode mode = WebProcess::shared().fullKeyboardAccessEnabled() ?
KeyboardAccessFull : KeyboardAccessDefault;
    if (m_tabToLinks)
	mode |= KeyboardAccessTabsToLinks;
    return mode;

Not sure you’ll like my version better, but if you do, please use it.

> Source/WebKit2/WebProcess/mac/FullKeyboardAccessWatcher.mm:45
> +	   // The keyboard access mode is reported by two bits:
> +	   // Bit 0 is set if feature is on
> +	   // Bit 1 is set if full keyboard access works for any control, not
just text boxes and lists.
> +	   fullKeyboardAccessEnabled = (mode & 0x2);

This comment doesn’t make it clear why we look at only bit 1 here. The why part
of the comment is the most important part!

> Source/WebKit2/WebProcess/mac/FullKeyboardAccessWatcher.mm:51
> +    self = [super init];

I think it would be slightly better if this class made it clear it was
incorrect to create an instance. For example, in some classes I have overridden
init to make it return nil or throw an exception or something like that, and
then put the actual initialization in a method of a different name, say _init,
with the call to [super init] there.

Nothing in the header tells you not to create an instance of this class.

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:360
> +    return
m_webPage->settings()->testAttribute(QWebSettings::LinksIncludedInFocusChain) ?

> +	   KeyboardAccessTabsToLinks : KeyboardAccessDefault;

Normally we put the ? on the start of the second line, not the end of the
first.