Simplify nsEventStateManager::SetContentState

RESOLVED FIXED in mozilla5

Status

()

defect
P1
normal
RESOLVED FIXED
9 years ago
3 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

Trunk
mozilla5
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

It's far more complicated than it needs to be.
Assignee: nobody → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
Attachment #511453 - Attachment is obsolete: true
Attachment #511453 - Flags: review?(dbaron)

Updated

8 years ago
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?

r=dbaron
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]
Pushed:
  http://hg.mozilla.org/projects/cedar/rev/728077e8679d
  http://hg.mozilla.org/projects/cedar/rev/c8c2edd1f1ee
Flags: in-testsuite-
Whiteboard: [need landing] → fixed-in-cedar
Target Milestone: --- → mozilla2.2

Updated

8 years ago
Status: NEW → RESOLVED
Closed: 8 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.