Bug 36167 - Spatial Navigation: Add isNull and document convenient methods to FocusCandidate
Summary: Spatial Navigation: Add isNull and document convenient methods to FocusCandidate
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on:
Blocks: 36168
  Show dependency treegraph
 
Reported: 2010-03-16 08:08 PDT by Antonio Gomes
Modified: 2010-04-20 13:25 PDT (History)
4 users (show)

See Also:


Attachments
patch 0.1 - adds isNull() and document() to FocusCandidate (3.70 KB, patch)
2010-03-16 08:18 PDT, Antonio Gomes
gns: review-
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
patch 0.2 - addressed kov's suggestions (5.88 KB, patch)
2010-03-17 12:37 PDT, Antonio Gomes
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
patch 0.2.1 - addressed kov's suggestions (4.32 KB, patch)
2010-03-17 12:41 PDT, Antonio Gomes
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
(committed in r56159) patch 0.3 - addressed kov's suggestions (3.63 KB, patch)
2010-03-17 12:54 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-03-16 08:08:13 PDT
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 ...
Comment 1 Antonio Gomes 2010-03-16 08:18:01 PDT
Created attachment 50793 [details]
patch 0.1 - adds isNull() and document() to FocusCandidate
Comment 2 Gustavo Noronha (kov) 2010-03-17 12:07:10 PDT
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.
Comment 3 Antonio Gomes 2010-03-17 12:37:32 PDT
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 :-)
Comment 4 Antonio Gomes 2010-03-17 12:41:36 PDT
Created attachment 50935 [details]
patch 0.2.1 - addressed kov's suggestions

Same as patch 0.2, but w/ the correct ChangeLog entry.
Comment 5 WebKit Review Bot 2010-03-17 12:42:39 PDT
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 6 Gustavo Noronha (kov) 2010-03-17 12:48:18 PDT
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.
Comment 7 Antonio Gomes 2010-03-17 12:54:40 PDT
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.
Comment 8 Antonio Gomes 2010-03-17 20:44:31 PDT
kov, one more time?
Comment 9 Gustavo Noronha (kov) 2010-03-18 06:51:50 PDT
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.
Comment 10 Antonio Gomes 2010-03-18 07:50:41 PDT
thx kov
Comment 11 Antonio Gomes 2010-04-11 20:02:08 PDT
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>