Bug 157415 - Refactor FocusController::findFocusableElementRecursively
Summary: Refactor FocusController::findFocusableElementRecursively
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 151379
  Show dependency treegraph
 
Reported: 2016-05-05 23:01 PDT by Ryosuke Niwa
Modified: 2016-05-09 09:19 PDT (History)
6 users (show)

See Also:


Attachments
Cleanup (13.42 KB, patch)
2016-05-06 00:04 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Renamed one more function (14.69 KB, patch)
2016-05-06 00:18 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-05-05 23:01:12 PDT
FocusController::findFocusableElementRecursively and FocusController::nextFocusableElement/previousFocusableElement are named backwards.
The former finds a focusable element and the latter two finds a focusable element or a shadow host.

Fix this mess.
Comment 1 Ryosuke Niwa 2016-05-06 00:04:38 PDT
Created attachment 278240 [details]
Cleanup
Comment 2 Ryosuke Niwa 2016-05-06 00:18:29 PDT
Created attachment 278243 [details]
Renamed one more function
Comment 3 Darin Adler 2016-05-07 12:50:14 PDT
Comment on attachment 278243 [details]
Renamed one more function

View in context: https://bugs.webkit.org/attachment.cgi?id=278243&action=review

I am getting lost in all these super-long function names. The new names do seem more accurate and precise.

> Source/WebCore/page/FocusController.cpp:435
> +    while (owner) {

This would be more readable as a for loop, but I guess it’s hard to get outerScope into something visible in the loop condition.
Comment 4 Ryosuke Niwa 2016-05-09 08:42:47 PDT
Thanks for the review!

(In reply to comment #3)
> Comment on attachment 278243 [details]
> Renamed one more function
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278243&action=review
> 
> I am getting lost in all these super-long function names. The new names do
> seem more accurate and precise.

Hopefully next round of cleanup will help.

> > Source/WebCore/page/FocusController.cpp:435
> > +    while (owner) {
> 
> This would be more readable as a for loop, but I guess it’s hard to get
> outerScope into something visible in the loop condition.

Yeah, I thought about the same thing but I didn't want to move FocusNavigationScope out of the loop.
Comment 5 WebKit Commit Bot 2016-05-09 09:19:34 PDT
Comment on attachment 278243 [details]
Renamed one more function

Clearing flags on attachment: 278243

Committed r200576: <http://trac.webkit.org/changeset/200576>
Comment 6 WebKit Commit Bot 2016-05-09 09:19:39 PDT
All reviewed patches have been landed.  Closing bug.