Bug 36167 – Spatial Navigation: Add isNull and document convenient methods to FocusCandidate

• Component: Accessibility
• Status: CLOSED FIXED

[Antonio Gomes]

It turns out that Spatial Navigation related code (in FocusController.cpp for
instance) can be simplified and look better if FocusCandidate class offers some
convinient method. This patch introduces a couple of them (isNull and a Document*
getter).


patch comming ...

[Antonio Gomes]

Created attachment 50793 [details]
patch 0.1 - adds isNull() and document() to FocusCandidate

[Gustavo Noronha (kov)]

Comment on attachment 50793 [details]
patch 0.1 - adds isNull() and document() to FocusCandidate

> It turns out that Spatial Navigation related code (in FocusController.cpp for
> instance) can be simplified and look better if FocusCandidate class offer some
> convinient method. This patch introduces a couple of them (isNull and a Document
> getter).

convenience methods sounds better.
 
> A followup refactory patch will be making use of these helper methods.

refactoring

93      FocusCandidate()
94          : node(0)
 93     FocusCandidate(Node* n = 0)
 94         : node(n)

Having a default value in a constructor doesn't look very idiomatic to me. I may be wrong, given my small experience with C++, but then again, most code I've touched in WebKit have two separate constructors for this kind of case - one for when you don't want to give it a node, one for when you have to give it a node.

Otherwise, the patch is fine, but I think it would be preferable if we have at least some code refactored to use the new methods in the same patch. r- for the constructor problem.

[Antonio Gomes]

Created attachment 50933 [details]
patch 0.2 - addressed kov's suggestions

thx for reviewing Gustavo.

> convenience methods sounds better.

Changed.

>> A followup refactory patch will be making use of these helper methods.
>refactoring

Changed.

> Having a default value in a constructor doesn't
> look very idiomatic to me. I may be wrong, given my
> small experience with C++, but then again, most code
> I've touched in WebKit have two separate constructors
> for this kind of case - one for when you don't want to
> give it a node, one for when you have to give it a node.

Done. I added an additional constructor as suggested, as well
as a private |init| method that assigns default values to all
other class member data commonly initiated by both class construtors.

> Otherwise, the patch is fine, but I think it would be
> preferable if we have at least some code refactored to
> use the new methods in the same patch. r- for the
> constructor problem.

bug 36168 is all about this new refactored code :-)

[Antonio Gomes]

Created attachment 50935 [details]
patch 0.2.1 - addressed kov's suggestions

Same as patch 0.2, but w/ the correct ChangeLog entry.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/page/SpatialNavigation.h:115:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 2 files


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

[Gustavo Noronha (kov)]

Comment on attachment 50935 [details]
patch 0.2.1 - addressed kov's suggestions

I think you should be good repeating the initialization in both constructors, I think that's preferable. Having an additional function seems unnecessary here. Also, I just noticed you're using inline for the small functions. This is usually frowned upon unless you have a reason to do it. The compiler should be smart enough to do the optimization otherwise.

[Antonio Gomes]

Created attachment 50940 [details]
(committed in r56159) patch 0.3 - addressed kov's suggestions

> I think you should be good repeating the initialization in both constructors, I
> think that's preferable. Having an additional function seems unnecessary here.

Done

> Also, I just noticed you're using inline for the small functions. This is
> usually frowned upon unless you have a reason to do it. The compiler should be
> smart enough to do the optimization otherwise.

Done.

[Antonio Gomes]

kov, one more time?

[Gustavo Noronha (kov)]

Comment on attachment 50940 [details]
(committed in r56159) patch 0.3 - addressed kov's suggestions

You could also make the methods const (bool isNull() const { return !node; }), but otherwise looks good to me.

[Antonio Gomes]

thx kov

[Antonio Gomes]

Comment on attachment 50940 [details]
(committed in r56159) patch 0.3 - addressed kov's suggestions

Clearing flags on attachment: 50940

Committed r56159: <http://trac.webkit.org/changeset/56159>