Bug 36168

Summary: Spatial Navigation: Code simplification in FocusController.cpp and SpatialNavigation.cpp
Product: WebKit Reporter: Antonio Gomes <tonikitoo>
Component: AccessibilityAssignee: Antonio Gomes <tonikitoo>
Status: CLOSED FIXED    
Severity: Normal CC: dglazkov, eric, gns, hausmann, simon.fraser, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on: 18662, 36167    
Bug Blocks: 35784, 36463, 36773    
Attachments:
Description Flags
patch 0.1
tonikitoo: commit-queue-
patch 0.1.1
tonikitoo: commit-queue-
patch 0.1.2
tonikitoo: commit-queue-
patch 0.2
kenneth: review-, tonikitoo: commit-queue-
(committed in r56311) patch_1 v0.1 - making use of isNull and document methods.
none
patch_2 - v1 - code simplification
none
patch_2 - v2 - code simplification
tonikitoo: commit-queue-
patch_2 - v3 - code simplification
tonikitoo: commit-queue-
(committed in r57061) patch_2 - v4 - code simplification none

Description Antonio Gomes 2010-03-16 08:27:51 PDT
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
Comment 1 Antonio Gomes 2010-03-16 08:32:54 PDT
Created attachment 50796 [details]
patch 0.1
Comment 2 Eric Seidel (no email) 2010-03-16 08:35:13 PDT
Attachment 50796 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/860024
Comment 3 WebKit Review Bot 2010-03-16 08:35:36 PDT
Attachment 50796 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/928007
Comment 4 WebKit Review Bot 2010-03-16 08:37:31 PDT
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.
Comment 5 WebKit Review Bot 2010-03-16 08:39:27 PDT
Attachment 50796 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/919009
Comment 6 Antonio Gomes 2010-03-16 08:42:01 PDT
(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
Comment 7 Antonio Gomes 2010-03-18 08:33:21 PDT
Created attachment 51026 [details]
patch 0.1.1

Fixed the style issue. It should also build now that the dependency patch has landed.
Comment 8 Antonio Gomes 2010-03-18 08:45:17 PDT
Created attachment 51028 [details]
patch 0.1.2

Fixed a wrong changeLog entry in patch 0.1.1 (patch 51026)
Comment 9 Antonio Gomes 2010-03-18 12:59:51 PDT
(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.
Comment 10 Antonio Gomes 2010-03-19 15:13:12 PDT
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.
Comment 11 Kenneth Rohde Christiansen 2010-03-20 08:00:48 PDT
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.
Comment 12 Antonio Gomes 2010-03-20 14:15:16 PDT
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.
Comment 13 Kenneth Rohde Christiansen 2010-03-20 14:39:10 PDT
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?
Comment 14 Antonio Gomes 2010-03-22 07:48:51 PDT
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);
(...)
Comment 15 Eric Seidel (no email) 2010-03-22 08:06:04 PDT
Attachment 51285 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/978125
Comment 16 Antonio Gomes 2010-03-22 12:23:24 PDT
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.
Comment 17 Antonio Gomes 2010-03-22 15:01:25 PDT
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
Comment 18 Antonio Gomes 2010-03-23 06:42:46 PDT
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.
Comment 19 Antonio Gomes 2010-03-29 11:35:40 PDT
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.
Comment 20 Antonio Gomes 2010-04-02 15:11:07 PDT
(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
Comment 21 Simon Fraser (smfr) 2010-04-02 15:21:50 PDT
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
Comment 22 Antonio Gomes 2010-04-04 19:58:39 PDT
Comment on attachment 51940 [details]
(committed in r57061) patch_2 - v4 - code simplification

clearing flags, 

ps: simon, thank you (again^1000)!
Comment 23 Antonio Gomes 2010-04-05 09:01:07 PDT
I filed bug 37092 explicitily about clean up / simplify updateFocusCandidateIfCloser, so we can close this one for now.
Comment 24 Antonio Gomes 2010-04-11 20:03:40 PDT
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>
Comment 25 Antonio Gomes 2010-04-11 20:04:40 PDT
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>
Comment 26 Simon Hausmann 2010-04-20 13:15:27 PDT
Revision r56311 cherry-picked into qtwebkit-2.0 with commit 85ff8ee031398d33480f47e9c9126774e146349dRevision r57061 cherry-picked into qtwebkit-2.0 with commit bed27b1b0729fc488d61b78e1ce57d695cb2d920