633271 - Simplify nsEventStateManager::SetContentState

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

It's far more complicated than it needs to be.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Stop shoehorning focus state change notifications into the ESM.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Simplify nsEventStateManager::SetContentState.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Simplify nsEventStateManager::SetContentState.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Pushed:
  http://hg.mozilla.org/projects/cedar/rev/728077e8679d
  http://hg.mozilla.org/projects/cedar/rev/c8c2edd1f1ee

Comment 7

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/728077e8679d
http://hg.mozilla.org/mozilla-central/rev/c8c2edd1f1ee

Comment 8

[[e-mail]]

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.)

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[[e-mail]]

(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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.