Closed
Bug 585255
Opened 14 years ago
Closed 13 years ago
Remove nsTreeWalker usage from nsFocusManager
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla6
People
(Reporter: craig.topper, Assigned: craig.topper)
Details
Attachments
(1 file, 4 obsolete files)
3.97 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
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.
Attachment #463996 -
Flags: review?(jonas)
Comment 2•14 years ago
|
||
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....
Assignee | ||
Comment 3•14 years ago
|
||
Oops I was going to fix that and forgot. I'll post a new patch later today.
Assignee | ||
Comment 4•14 years ago
|
||
Attachment #463996 -
Attachment is obsolete: true
Attachment #464305 -
Flags: review?(jonas)
Attachment #463996 -
Flags: review?(jonas)
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•14 years ago
|
||
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.
Sounds good to me.
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #464305 -
Attachment is obsolete: true
Attachment #464710 -
Flags: review?(jonas)
Attachment #464305 -
Flags: review?(jonas)
Comment on attachment 464710 [details] [diff] [review] Moved GetPreviousNode to nsINode Yay! Pretty!
Attachment #464710 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 10•14 years ago
|
||
Jonas, should I ask for 2.0 approval or wait til we branch?
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.
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.
Assignee | ||
Comment 13•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #464710 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #525323 -
Attachment is obsolete: true
Assignee | ||
Comment 14•13 years ago
|
||
Attachment #525325 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 15•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/af860093a19a
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•