[webkit-reviews] review denied: [Bug 36167] Spatial Navigation: Add isNull and document convenient methods to FocusCandidate : [Attachment 50793] patch 0.1 - adds isNull() and document() to FocusCandidate

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 17 12:07:10 PDT 2010


Gustavo Noronha (kov) <gns at gnome.org> has denied Antonio Gomes (tonikitoo)
<tonikitoo at webkit.org>'s request for review:
Bug 36167: Spatial Navigation: Add isNull and document convenient methods to
FocusCandidate
https://bugs.webkit.org/show_bug.cgi?id=36167

Attachment 50793: patch 0.1 - adds isNull() and document() to FocusCandidate
https://bugs.webkit.org/attachment.cgi?id=50793&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
> 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.


More information about the webkit-reviews mailing list