610427 - Remove aNotify in nsHTMLInputElement::SetCheckedChangedInternal

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

See bug 605124 comment 8. Callers setting aNotify=PR_FALSE could be removed.

Comment 1

[Mounir Lamouri (:mounir)]

When Clone() is called, the element has no GetCurrentDoc()?
For AddedToRadioGroup, it can be called during parsing. Is that really safe to remove aNotify here?

Comment 2

[Boris Zbarsky [:bzbarsky]]

> When Clone() is called, the element has no GetCurrentDoc()?

The return value of the clone does not, correct.

> Is that really safe to remove aNotify here?

Right now on m-c we always pass true for aNotify to AddedToRadioGroup, no?

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to comment #2)
> Right now on m-c we always pass true for aNotify to AddedToRadioGroup, no?

But there is:
aNotify = aNotify && !GET_BOOLBIT(mBitField, BF_PARSER_CREATING);

I'm not sure BindToTree can be called before BF_PARSER_CREATING goes PR_FALSE but can't we have AddedToRadioGroup called from AfterSetAttr while parsing? I think the parset is setting mForm so it will depend if that's done before setting the attributes or after, right?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> aNotify = aNotify && !GET_BOOLBIT(mBitField, BF_PARSER_CREATING);

Ah, sure.  But if BF_PARSER_CREATING then GetCurrentDoc is null, no?

> I'm not sure BindToTree can be called before BF_PARSER_CREATING goes PR_FALSE

It can't, iirc.

> but can't we have AddedToRadioGroup called from AfterSetAttr while parsing?

I _think_ so, but at that point GetCurrentDoc is false.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

I've removed aNotify from AddedToRadioGroup given that it sounds not used at all.

Comment 6

[Boris Zbarsky [:bzbarsky]]

That comment about aNotify doesn't make sense, since there's no aNotify around...

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Sorry about that.

Comment 8

[Benjamin Smedberg]

Can you comment on why you nominated this patch? What does it fix that would make us take it in Firefox 4, and what are the risks?

Comment 9

[Mounir Lamouri (:mounir)]

It's actually a follow-up from bug 605124. aNotify has been added in bug 605124 but while reviewing Boris told me it might not be needed so I opened a follow-up for this specific issue.

Why we need that? it's a follow-up for a bug that has been approved.
What are the risks? none given that it's only cleaning something that is going to be pushed. The worst thing that might happen by removing aNotify is notifying when we should not and that would only cost some cycles. But aNotify will only be TRUE, that's why this bug has been opened so, in theory, there are no risks.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 489559 [details] [diff] [review]
Patch v1.1

r=me

Comment 11

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/41e7b8cbd271