Bug 114298 – [WK2][GTK] Adding SpatialNavigation setting to webkit2

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Danilo de Paula]

Adding SpatialNavigation API to webkit2

[Danilo de Paula]

Created attachment 197155 [details]
Patch

[Danilo de Paula]

*** Bug 114297 has been marked as a duplicate of this bug. ***

[Danilo de Paula]

Created attachment 197156 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Review Bot]

Attachment 197156 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/WebPreferencesStore.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h"
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1095:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1097:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1098:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1099:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1100:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 197156 [details]
Patch

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

Patch looks good to me, thanks! r- because it lacks unit tests. It requires the approval of another GTK+ reviewer and one WebKit2 owner.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1093
> +     */

Add Since: 2.2

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2695
> + */

Since: 2.2 here too

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2717
> + * Returns: %TRUE If HTML5 spatial navigation support is enabled or %FALSE otherwise.

And here too.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h:388
> +webkit_settings_set_enable_spatial_navigation                  (WebKitSettings *settings,
> +                                                                gboolean enabled);

Nit: Please line up argument names like all other functions.

[Mikhail Pozdnyakov]

Shouldn't the bug title have [WK2][GTK] ?

[Danilo de Paula]

(In reply to comment #7)
> Shouldn't the bug title have [WK2][GTK] ?

It should. Thanks.

[Danilo de Paula]

Created attachment 197265 [details]
Patch

[Danilo de Paula]

(In reply to comment #6)
> (From update of attachment 197156 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197156&action=review
> 
> Patch looks good to me, thanks! r- because it lacks unit tests. It requires the approval of another GTK+ reviewer and one WebKit2 owner.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1093
> > +     */
> 
> Add Since: 2.2
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2695
> > + */
> 
> Since: 2.2 here too
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2717
> > + * Returns: %TRUE If HTML5 spatial navigation support is enabled or %FALSE otherwise.
> 
> And here too.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h:388
> > +webkit_settings_set_enable_spatial_navigation                  (WebKitSettings *settings,
> > +                                                                gboolean enabled);

Fixed the things added above


> 
> Nit: Please line up argument names like all other functions.
I'm following the same rule as the rest of the file

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #6)
> > (From update of attachment 197156 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=197156&action=review
> > 
> > Patch looks good to me, thanks! r- because it lacks unit tests. It requires the approval of another GTK+ reviewer and one WebKit2 owner.
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1093
> > > +     */
> > 
> > Add Since: 2.2
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2695
> > > + */
> > 
> > Since: 2.2 here too
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2717
> > > + * Returns: %TRUE If HTML5 spatial navigation support is enabled or %FALSE otherwise.
> > 
> > And here too.
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h:388
> > > +webkit_settings_set_enable_spatial_navigation                  (WebKitSettings *settings,
> > > +                                                                gboolean enabled);
> 
> Fixed the things added above
> 
> 
> > 
> > Nit: Please line up argument names like all other functions.
> I'm following the same rule as the rest of the file

No, you are not. There's a single space between gboolean and enabled that should be at the same column that settings, like all other functions.

[Danilo de Paula]

(In reply to comment #11)
> No, you are not. There's a single space between gboolean and enabled that should be at the same column that settings, like all other functions.

Sorry, you're right. I thought you were talking about the 58 spaces between gboolean and the beginning of the line. My mistake. fixed.

[Danilo de Paula]

Created attachment 197306 [details]
Patch

[WebKit Commit Bot]

Attachment 197306 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/WebPreferencesStore.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h"
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1097:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1099:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1100:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1101:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Danilo de Paula]

Comment on attachment 197306 [details]
Patch

adding cq request

[Gustavo Noronha (kov)]

Comment on attachment 197306 [details]
Patch

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

Straight-forward API, looks good to me, cq- because of the wording in the doc

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1087
> +     * Whether to enable Spatial navigation. This feature consists in the ability

I'd use lower case for spatial here

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1091
> +     * there is an element he might be trying to reach towards the right, and if

s/he/they/

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1092
> +     * there are multiple elements, which element he probably wants.

ditto

[Danilo de Paula]

(In reply to comment #16)
> (From update of attachment 197306 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197306&action=review
> 
> Straight-forward API, looks good to me, cq- because of the wording in the doc
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1087
> > +     * Whether to enable Spatial navigation. This feature consists in the ability
> 
> I'd use lower case for spatial here

Spatial Navigation is a feature described with a capital leading letter on webkitgtk+ API, shouldn't it be "Spatial Navigation" instead of "spatial navigation"?
http://webkitgtk.org/reference/webkitgtk/stable/WebKitWebSettings.html#WebKitWebSettings--enable-spatial-navigation

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1091
> > +     * there is an element he might be trying to reach towards the right, and if
> 
> s/he/they/

He, in that case, is the user. Also, it's the same text used on webkitgtk+ docs: http://webkitgtk.org/reference/webkitgtk/stable/WebKitWebSettings.html#WebKitWebSettings--enable-spatial-navigation

[Danilo de Paula]

Created attachment 198124 [details]
Patch

[WebKit Commit Bot]

Attachment 198124 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/WebPreferencesStore.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h"
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1097:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1099:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1100:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1101:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Danilo de Paula]

Created attachment 198126 [details]
Patch

[WebKit Commit Bot]

Attachment 198126 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/WebPreferencesStore.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h"
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1097:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1099:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1100:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1101:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Danilo de Paula]

(In reply to comment #20)
> Created an attachment (id=198126) [details]
> Patch

http://en.wikipedia.org/wiki/Singular_they
^ mentioned by Kov on a private discussion.

[Danilo de Paula]

Comment on attachment 198126 [details]
Patch

asking for cq+ and r+

Still waiting for a WK2 owner review.

[Carlos Garcia Campos]

(In reply to comment #23)
> (From update of attachment 198126 [details])
> asking for cq+ and r+
> 
> Still waiting for a WK2 owner review.

Please don't commit new api patches into trunk until we get trunk in sync with the stable branch.

[Danilo de Paula]

Created attachment 199509 [details]
Patch

[WebKit Commit Bot]

Attachment 199509 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/WebPreferencesStore.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h"
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1097:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1099:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1100:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1101:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Danilo de Paula]

Created attachment 200077 [details]
Patch

[Danilo de Paula]

adding wk2 owners.

[Danilo de Paula]

Explaining this API: Enable spatialNavigation is pretty important for vendors who want to ship webkit2 (in this case, webkit2gtk) in their products when there's a lack of a pointer-input-method (such as a mouse or a touch screen).

Ie: TVs that can only be controlled by the directional keys of a remote control

[Danilo de Paula]

Comment on attachment 200077 [details]
Patch

up?

[Carlos Garcia Campos]

API looks good to me too, we just need a WebKit2 owner to approve the cross platform changes.

[Kwang Yul Seo]

Any progress? The patch must be rebased in order to apply on HEAD because other settings were added in the meantime.

[Danilo de Paula]

(In reply to comment #32)
> Any progress? The patch must be rebased in order to apply on HEAD because other settings were added in the meantime.

Although I got a positive feedback from others GTK developers, I didn't get any feedback from a wk2 owner. I asked for it on IRC a few times...

With all the discussion about the ENABLE_CSS_DIRECTIONAL_FOCUS flag, it might be a good opportunity to review this one now (since this patch was created to be able to use the old DIRECTIONAL_FOCUS patch on a wk2 browser for a specific client)

[Danilo de Paula]

(In reply to comment #33)
> (In reply to comment #32)
> > Any progress? The patch must be rebased in order to apply on HEAD because other settings were added in the meantime.
> 
> Although I got a positive feedback from others GTK developers, I didn't get any feedback from a wk2 owner. I asked for it on IRC a few times...
> 
> With all the discussion about the ENABLE_CSS_DIRECTIONAL_FOCUS flag, it might be a good opportunity to review this one now (since this patch was created to be able to use the old DIRECTIONAL_FOCUS patch on a wk2 browser for a specific client)

I will rebase this anyway.

[Danilo de Paula]

Created attachment 208003 [details]
Patch

[Build Bot]

Comment on attachment 208003 [details]
Patch

Attachment 208003 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1525459

[Arunprasad Rajkumar]

(In reply to comment #35)
> Created an attachment (id=208003) [details]
> Patch

Danilo, Thanks for your effort. Let me take this activity if you are busy :)

(I would like to add a WK2 C WKPreference API as well to control spatial navigation.)

[Arunprasad Rajkumar]

Created attachment 217761 [details]
Patch

[Arunprasad Rajkumar]

Created attachment 217762 [details]
Patch

[Arunprasad Rajkumar]

(In reply to comment #39)
> Created an attachment (id=217762) [details]
> Patch

Added WK2 CAPI in WKPreferences.h additional to Danilo's patch. WKPreferences.h API is directly used by Nix port.

[WebKit Commit Bot]

Comment on attachment 217762 [details]
Patch

Clearing flags on attachment: 217762

Committed r159734: <http://trac.webkit.org/changeset/159734>

[Sergio Villar Senin]

Can we close this bug ?