Bug 36168

Created attachment 50796 [details] patch 0.1

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

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

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.

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

(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

Created attachment 51026 [details] patch 0.1.1 Fixed the style issue. It should also build now that the dependency patch has landed.

Created attachment 51028 [details] patch 0.1.2 Fixed a wrong changeLog entry in patch 0.1.1 (patch 51026)

(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.

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 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.

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 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?

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); (...)

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

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.

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

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.

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.

(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 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 on attachment 51940 [details] (committed in r57061) patch_2 - v4 - code simplification clearing flags, ps: simon, thank you (again^1000)!

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

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 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>

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