Closed Bug 633271 Opened 10 years ago Closed 10 years ago

Simplify nsEventStateManager::SetContentState


(Core :: DOM: Core & HTML, defect, P1)






(Reporter: bzbarsky, Assigned: bzbarsky)


(Blocks 1 open bug)



(2 files, 1 obsolete file)

It's far more complicated than it needs to be.
Attachment #511453 - Flags: review?(dbaron)
Assignee: nobody → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
Attachment #511453 - Attachment is obsolete: true
Attachment #511453 - Flags: review?(dbaron)
Attachment #511452 - Flags: review?(enndeakin) → review+
Comment on attachment 511644 [details] [diff] [review]
part 2.  Simplify nsEventStateManager::SetContentState.

Making notifyContent1, notifyContent2, and commonAncestor into weak pointers scares me a little -- maybe they should be nsCOMPtrs?

Attachment #511644 - Flags: review?(dbaron) → review+
> maybe they should be nsCOMPtrs?

Yeah, I suppose that would be safer.  In theory ContentStatesChanged observers should't do anything evil and I think we have code that depends on that, but for now this is the safe thing to do.  Made that change.
Whiteboard: [need review] → [need landing]
Flags: in-testsuite-
Whiteboard: [need landing] → fixed-in-cedar
Target Milestone: --- → mozilla2.2
Closed: 10 years ago
Resolution: --- → FIXED
This broke DOM Inspector which expects to be able to set the focus state. (I won't file a bug because I don't know which way around to file it.)
How could setting the focus state have possibly worked before this patch?  The GetContentState() call would not set the focus flag, since the focus manager would not know anything about the SetContentState call that DOM Inspector made.

So I can believe that setting focus state in DOMI is broken, but I really doubt that it was this change (as opposed to the change that added the focus manager) broke it.
(In reply to Boris Zbarsky from comment #9)
> How could setting the focus state have possibly worked before this patch?
You're right, it didn't, but you added an assertion that I didn't get before, and I hadn't realised that this was because getting the state already failed.
Ah, I added the assertion to catch any callers who might think their code is doing something when it actually didn't.... ;)  Sounds like it worked.

Do we need an API for inpsector to set focus state?  It should be pretty doable.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.