Adding SpatialNavigation API to webkit2
Created attachment 197155 [details] Patch
*** Bug 114297 has been marked as a duplicate of this bug. ***
Created attachment 197156 [details] Patch
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
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.
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.
Shouldn't the bug title have [WK2][GTK] ?
(In reply to comment #7) > Shouldn't the bug title have [WK2][GTK] ? It should. Thanks.
Created attachment 197265 [details] Patch
(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
(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.
(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.
Created attachment 197306 [details] Patch
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.
Comment on attachment 197306 [details] Patch adding cq request
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
(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
Created attachment 198124 [details] Patch
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.
Created attachment 198126 [details] Patch
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.
(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.
Comment on attachment 198126 [details] Patch asking for cq+ and r+ Still waiting for a WK2 owner review.
(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.
Created attachment 199509 [details] Patch
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.
Created attachment 200077 [details] Patch
adding wk2 owners.
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
Comment on attachment 200077 [details] Patch up?
API looks good to me too, we just need a WebKit2 owner to approve the cross platform changes.
Any progress? The patch must be rebased in order to apply on HEAD because other settings were added in the meantime.
(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)
(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.
Created attachment 208003 [details] Patch
Comment on attachment 208003 [details] Patch Attachment 208003 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1525459
(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.)
Created attachment 217761 [details] Patch
Created attachment 217762 [details] Patch
(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.
Comment on attachment 217762 [details] Patch Clearing flags on attachment: 217762 Committed r159734: <http://trac.webkit.org/changeset/159734>
Can we close this bug ?