Closed Bug 610427 Opened 9 years ago Closed 9 years ago
Notify in ns HTMLInput Element::Set Checked Changed Internal
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.
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)
That comment about aNotify doesn't make sense, since there's no aNotify around...
Sorry about that.
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.
Attachment #489559 - Flags: review?(bzbarsky) → review+
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.