Bug 36167

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

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.

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

Created attachment 50935 [details] patch 0.2.1 - addressed kov's suggestions Same as patch 0.2, but w/ the correct ChangeLog entry.

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.

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.

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.

kov, one more time?

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.

thx kov

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>