[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