Bug 36168 – Spatial Navigation: Code simplification in FocusController.cpp and SpatialNavigation.cpp

• Status: CLOSED
• Resolution: FIXED
• Component: Accessibility

[Antonio Gomes]

WebCore::distanceInDirection method was handling much of the logic not strictly 
related only to the distance acquisition between nodes. This method will simplified and renamed to 'WebCore::distanceDataForNode'. It now is responsible for not only getting the distance, but also parent document distance, alignment and parent document alignment to the
current focused node. All assignement logic previously there was moved to updateFocusCandidateIfCloser.

patch coming

[Antonio Gomes]

Created attachment 50796 [details]
patch 0.1

[Eric Seidel (no email)]

Attachment 50796 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/860024

[WebKit Review Bot]

Attachment 50796 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/928007

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/page/FocusController.cpp:410:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 1 in 5 files


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

[WebKit Review Bot]

Attachment 50796 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/919009

[Antonio Gomes]

(In reply to comment #5)
> Attachment 50796 [details] did not build on gtk:
> Build output: http://webkit-commit-queue.appspot.com/results/919009

please, the build problems are to be ignored, because it depends on patch in
bug 36167

[Antonio Gomes]

Created attachment 51026 [details]
patch 0.1.1

Fixed the style issue. It should also build now that the dependency patch has landed.

[Antonio Gomes]

Created attachment 51028 [details]
patch 0.1.2

Fixed a wrong changeLog entry in patch 0.1.1 (patch 51026)

[Antonio Gomes]

(In reply to comment #8)
> Created an attachment (id=51028) [details]
> patch 0.1.2
> 
> Fixed a wrong changeLog entry in patch 0.1.1 (patch 51026)

Simon, could you please have a look at that one? I am on my way to make the code more easily understandable and extensible, and this is the second step. 

What patch basically do is move much of the logic in |distanceForNode| method to |updateFocusCandidateIfCloser| . The former also gets renambed to |distanceDataForNode|, and gets responsible now for getting distance, parentDistance, alignment and parentAlignment all at once.

I just finished a follow up patch that will make updateFocusCandidateIfCloser method much clearer than it is now.

[Antonio Gomes]

Created attachment 51190 [details]
patch 0.2

Code simplification and improvements (part I)

see comment #9 (https://bugs.webkit.org/show_bug.cgi?id=36168#c9) for summary.

[Kenneth Rohde Christiansen]

Comment on attachment 51190 [details]
patch 0.2

You are doing many changes at once, so this makes it hard to review. Please split this up in smaller patches, like one making use of the new isNull, etc.

The helper is confusing to me, and doesn't make it easier for me to understand the code.

Function definitions should be on one line.

Please do not add FIXME_antonio, as told by others before.

[Antonio Gomes]

Created attachment 51232 [details]
(committed in r56311) patch_1 v0.1 - making use of isNull and document methods.

As per kenneth's request, lets split things up to help reviewing.

[Kenneth Rohde Christiansen]

Comment on attachment 51232 [details]
(committed in r56311) patch_1 v0.1 - making use of isNull and document methods.

Why is this patch called v0.1? Is this not the final one that you are submitting for review?

>      if (alignment != None
> -        || (candidate.node && candidate.parentAlignment >= alignment
> -        && (candidate.node->document() == dest->document())
> -        && alignment > None)) {
> +        || (!candidate.isNull() && candidate.parentAlignment >= alignment
> +        && candidate.document() == dest->document())) {
>  

Is it possible to refactor code to bail out earlier if the alignment or the candidate is null?

[Antonio Gomes]

Created attachment 51285 [details]
patch_2 - v1 - code simplification

I will try to help reviewers to understand the refactory going on here, by pointing out that is just code moving around, so one does not need to understand the code completely and in all its details to review it.

--> In the first chunk of the patch: it is code moving from distanceForNode to updateFocusCandidateIfCloser
+    bool sameDocument = candidate.document() == closest.document();
(...)

--> In the 2nd chunk, code is not being removed but moving down to the same method body, 
    with different variable names.
-    // If |focusedNode| and |candidate| are in the same document AND
(...)


--> In the 3rd chunk: it is also about code moving from distanceForNode to updateFocusCandidateIfCloser
+    if (candidate.alignment != None
(...)


--> the last big chunck in distanceInDirection: is not a sonely code removal. It was moved to updateFocusCandidateIfCloser
-    RectsAlignment alignment = alignmentForRects(direction, curRect, targetRect);
(...)

[Eric Seidel (no email)]

Attachment 51285 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/978125

[Antonio Gomes]

Comment on attachment 51285 [details]
patch_2 - v1 - code simplification

(In reply to comment #15)
> Attachment 51285 [details] did not build on mac:
> Build output: http://webkit-commit-queue.appspot.com/results/978125

I talked to Eric and he clarified the Mac build problem:

"The problem here is that you've made FocusController (a header file
which is exposed privately from WebCore to WebKit) depend on
SpatialNavigation.h w/o also exposing SpatialNavigation.h.

Do you have to make it depend on the header?  You can definitely mark
SpatialNavigation as a "private" header, instead of "project" as it
defaults to, and then the build will work, but itmight be easier to
just avoid the header spawl all together."


clearing review as I will address it now.

[Antonio Gomes]

Created attachment 51362 [details]
patch_2 - v2 - code simplification

Same as "patch_2 - v1" but updated to not need to expose SpatialNavigation.h to private headers.

Please see comment #14 https://bugs.webkit.org/show_bug.cgi?id=36168#c14 for easy reviewing

[Antonio Gomes]

Created attachment 51417 [details]
patch_2 - v3 - code simplification

Same as "patch_2 - v2" , but including SpatialNavigation.h as a Private Header to Mac.

please see comment #14 for easy reviewing.

[Antonio Gomes]

Created attachment 51940 [details]
(committed in r57061) patch_2 - v4 - code simplification

Same as "patch_2 - v3", rebased to Tot.

please see comment #14 for easy reviewing.

[Antonio Gomes]

(In reply to comment #19)
> Created an attachment (id=51940) [details]
> patch_2 - v4 - code simplification
> 
> Same as "patch_2 - v3", rebased to Tot.
> 
> please see comment #14 for easy reviewing.

@smfr , could you please help me w/ that one? it blocks bug 36773 and bug 36463 , plus 5 not filed.

thank you

[Simon Fraser (smfr)]

Comment on attachment 51940 [details]
(committed in r57061) patch_2 - v4 - code simplification

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

> +        data, while all assignement logic previously distanceInDirection method

previously _in_

> +        Parent document distance and alignment acquisitions, on their turns, have

s/on their turns/in turn

> +        also changed location: they are both got from deepFindFocusableNodeInDirection,

s/got from/obtained from

> +        and passed in a recursive call to findFocusableNodeInDirection via the candidateParent
> +        variable (options parameter). Yet on deepFindFocusableNodeInDirection method, the need for the

s/Yet/In addition,


> diff --git a/WebCore/page/FocusController.h b/WebCore/page/FocusController.h

> +    void findFocusableNodeInDirection(Document*, Node*, FocusDirection, KeyboardEvent*, FocusCandidate&, const FocusCandidate& c = FocusCandidate());

I think you should provide names for the two FocusCandidate arguments so it's possible to tell what they are for.

r=me

[Antonio Gomes]

Comment on attachment 51940 [details]
(committed in r57061) patch_2 - v4 - code simplification

clearing flags, 

ps: simon, thank you (again^1000)!

[Antonio Gomes]

I filed bug 37092 explicitily about clean up / simplify updateFocusCandidateIfCloser, so we can close this one for now.

[Antonio Gomes]

Comment on attachment 51940 [details]
(committed in r57061) patch_2 - v4 - code simplification

Clearing flags on attachment: 51940

Committed r57061: <http://trac.webkit.org/changeset/57061>

[Antonio Gomes]

Comment on attachment 51232 [details]
(committed in r56311) patch_1 v0.1 - making use of isNull and document methods.

Clearing flags on attachment: 51232

Committed r56311: <http://trac.webkit.org/changeset/56311>

[Simon Hausmann]

Revision r56311 cherry-picked into qtwebkit-2.0 with commit 85ff8ee031398d33480f47e9c9126774e146349dRevision r57061 cherry-picked into qtwebkit-2.0 with commit bed27b1b0729fc488d61b78e1ce57d695cb2d920