Bug 49085 – Spatial Navigation: Add a helper function isSpatialNavigationEnabled()

• Component: HTML Editing
• Status: RESOLVED FIXED

[Chang Shu]

Add a helper function isSpatialNavigationEnabled() in spatialnavigation.h to avoid duplicate code.

[Chang Shu]

Created attachment 73109 [details]
fix patch

[Antonio Gomes]

Comment on attachment 73109 [details]
fix patch

Looks good! I still think we could pass Page* or Settings* as a parameter.

Any specific reason for Frame?

[Chang Shu]

(In reply to comment #2)
> (From update of attachment 73109 [details])
> Looks good! I still think we could pass Page* or Settings* as a parameter.
> 
> Any specific reason for Frame?

The reason I prefer using Frame over Settings is that the caller doesn't have to do an additional check against pointer frame, which is taken care of inside the helper function. Design-wise, a user is asking if spatial navigation is enabled in a frame. The user shouldn't know about the internal implementation of Frame.
In terms of Page, it's pretty much the same as Frame. Frame::settings() returns the page->settings(). In case its implementation changes for any reason, say, each frame has its own settings, this helper function needs no change.

[Darin Adler]

Comment on attachment 73109 [details]
fix patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73109&action=review

> WebCore/page/SpatialNavigation.cpp:58
> +    if (frame && frame->settings() && frame->settings()->isSpatialNavigationEnabled())
> +        return true;
> +
> +    return false;

This:

    if (x)
        return true;
    return false;

Is the same as this:

    return x;

You should remove the if.

[Chang Shu]

sure!
> This:
> 
>     if (x)
>         return true;
>     return false;
> 
> Is the same as this:
> 
>     return x;
> 
> You should remove the if.

[Chang Shu]

Created attachment 73116 [details]
fix patch 2

[Antonio Gomes]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 73109 [details] [details])
> > Looks good! I still think we could pass Page* or Settings* as a parameter.
> > 
> > Any specific reason for Frame?
> 
> The reason I prefer using Frame over Settings is that the caller doesn't have to do an additional check against pointer frame, which is taken care of inside the helper function. Design-wise, a user is asking if spatial navigation is enabled in a frame. The user shouldn't know about the internal implementation of Frame.
> In terms of Page, it's pretty much the same as Frame. Frame::settings() returns the page->settings(). In case its implementation changes for any reason, say, each frame has its own settings, this helper function needs no change.

I do not think there is any plan to make settings per Frame. It is essentially a Page concept.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 73116 [details]:

http/tests/security/cross-origin-css.html

Please file bugs against the tests.  These tests were authored by [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 73116 [details]
fix patch 2

Clearing flags on attachment: 73116

Committed r71479: <http://trac.webkit.org/changeset/71479>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.