Closed Bug 610427 Opened 9 years ago Closed 9 years ago

Remove aNotify in nsHTMLInputElement::SetCheckedChangedInternal

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(1 file, 1 obsolete file)

See bug 605124 comment 8. Callers setting aNotify=PR_FALSE could be removed.
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?
> 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?
(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?
> 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.
Attached patch Patch v1 (obsolete) — Splinter Review
I've removed aNotify from AddedToRadioGroup given that it sounds not used at all.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #489464 - Flags: review?(bzbarsky)
Attachment #489464 - Flags: approval2.0?
That comment about aNotify doesn't make sense, since there's no aNotify around...
Attached patch Patch v1.1Splinter Review
Sorry about that.
Attachment #489464 - Attachment is obsolete: true
Attachment #489559 - Flags: review?(bzbarsky)
Attachment #489559 - Flags: approval2.0?
Attachment #489464 - Flags: review?(bzbarsky)
Attachment #489464 - Flags: approval2.0?
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?
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.
Blocks: 605124
Attachment #489559 - Flags: review?(bzbarsky) → review+
Attachment #489559 - Flags: approval2.0? → approval2.0+
Pushed:
http://hg.mozilla.org/mozilla-central/rev/41e7b8cbd271
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.