Last Comment Bug 585255 - Remove nsTreeWalker usage from nsFocusManager
: Remove nsTreeWalker usage from nsFocusManager
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla6
Assigned To: Craig Topper
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-07 00:07 PDT by Craig Topper
Modified: 2011-05-01 12:02 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (4.65 KB, patch)
2010-08-08 22:20 PDT, Craig Topper
no flags Details | Diff | Splinter Review
Fixes for bz's comments (4.42 KB, patch)
2010-08-09 22:39 PDT, Craig Topper
no flags Details | Diff | Splinter Review
Moved GetPreviousNode to nsINode (4.38 KB, patch)
2010-08-10 22:45 PDT, Craig Topper
jonas: review+
Details | Diff | Splinter Review
Remove nsTreeWalker usage from nsFocusManager (3.96 KB, patch)
2011-04-12 00:19 PDT, Craig Topper
no flags Details | Diff | Splinter Review
Remove nsTreeWalker usage from nsFocusManager (3.97 KB, patch)
2011-04-12 00:41 PDT, Craig Topper
jonas: review+
Details | Diff | Splinter Review

Description Craig Topper 2010-08-07 00:07:38 PDT
nsFocusManager uses nsTreeWalker to locate next and previous nsIContents. This is overkill now that there are fast node traversal methods supported by nsINode. It should use GetNextNode and a new method to get the previous nsIContent.

Patch coming soon.
Comment 1 Craig Topper 2010-08-08 22:20:58 PDT
Created attachment 463996 [details] [diff] [review]
Patch

I've added an GetPreviousContent method to nsIContent. I can move it to nsINode if that is preferred, but since it only knows how to return nsIContent* I think you need to be in an nsIContent to even bother wanting to find a previous content. (My knowledge of nsINode vs nsIContent is very bad.)

Not entirely sure how to test this. Is focus stuff covered by one of the test systems? I did run the built browser and made sure it still seemed to move forwards and backwards correctly.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2010-08-09 04:21:21 PDT
For what it's worth, the behavior of that function wrt aRoot doesn't match the documentation.  Also, there's no problem using nsContentUtils methods in nsGenericElement; they just couldn't be used in nsINode....
Comment 3 Craig Topper 2010-08-09 08:07:13 PDT
Oops I was going to fix that and forgot. I'll post a new patch later today.
Comment 4 Craig Topper 2010-08-09 22:39:47 PDT
Created attachment 464305 [details] [diff] [review]
Fixes for bz's comments
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-08-10 14:34:45 PDT
I think we want

nsIContent* nsIContent::GetPreviousContent

to be changed into

nsIContent* nsINode::GetPreviousContent

i.e. moved to the nsINode interface. We probably also want to keep it inlined such that it can be called from anywhere. This means that you won't be able to call nsContentUtils::ContentIsDescendantOf but unfortunately have to duplicate and inline it :( It's somewhat more ok though since it's DEBUG only.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2010-08-10 14:49:06 PDT
If we want to inline it, the right way to do it is to put the decl in nsINode and the impl at the bottom of nsIContent.h.
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-08-10 14:59:43 PDT
Sounds good to me.
Comment 8 Craig Topper 2010-08-10 22:45:43 PDT
Created attachment 464710 [details] [diff] [review]
Moved GetPreviousNode to nsINode
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-08-16 16:34:36 PDT
Comment on attachment 464710 [details] [diff] [review]
Moved GetPreviousNode to nsINode

Yay! Pretty!
Comment 10 Craig Topper 2010-08-22 21:21:57 PDT
Jonas, should I ask for 2.0 approval or wait til we branch?
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-08-23 02:11:21 PDT
I'd say that we should wait. There's not much value for users here, just value for our continued ease of working with this code, so doesn't give us much to put in FF4.
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-11 23:26:16 PDT
I don't want to land this tonight as the tree is in a crazy state. I'll try to remember to land this tomorrow, otherwise please remind me.
Comment 13 Craig Topper 2011-04-12 00:19:42 PDT
Created attachment 525323 [details] [diff] [review]
Remove nsTreeWalker usage from nsFocusManager
Comment 14 Craig Topper 2011-04-12 00:41:59 PDT
Created attachment 525325 [details] [diff] [review]
Remove nsTreeWalker usage from nsFocusManager
Comment 15 Dão Gottwald [:dao] 2011-05-01 12:02:11 PDT
http://hg.mozilla.org/mozilla-central/rev/af860093a19a

Note You need to log in before you can comment on or make changes to this bug.