See the code here: https://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/dom/html/HTMLInputElement.cpp#6549 The third argument should be false, I think, since this call isn't changing the value. Note that passing true here causes a DoSetCheckedChanged(true, false) here , but this code wants DoSetCheckedChanged(false, false) . Now all of this bites us because of the underlying UpdateState() call here which can be expensive and shows up in Speedometer profiles: http://bit.ly/2t1DjC8. This is a focused view as an example: http://bit.ly/2t1gPBg The simple fix is as green as try can be these days: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e555fa8b7e8af5845bdc3129157d445380c4078  https://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/dom/html/HTMLInputElement.cpp#3231  https://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/dom/html/HTMLInputElement.cpp#6550
Also this is a practical example on why boolean arguments are a bad idea. :-( We should really fix this code, but it's late, and I'm too tired to write the bigger patch...
Comment on attachment 8880693 [details] [diff] [review] Pass the correct argument to DoSetChecked() in HTMLInputElement::DoneCreatingElement() to avoid doing excessive work So... We have two relevant codepaths here. There's DoneCreatingElement, which does: DoSetChecked(DefaultChecked(), false, true); DoSetCheckedChanged(false, false); (the arg ordering is a bit annoying here; DoSetChecked takes "checked", "notify", "setvaluechanged", while DoSetCheckedChanged takes "checkedchanged", "notify). DoSetChecked in this case calls DoSetCheckedChanged(true, false). This is the call you're proposing we optimize out. The other relevant codepath is AfterSetAttr for the "checked" attribute, if it happens after DoneCreatingElement. That does: DoSetChecked(DefaultChecked(), true, true); SetCheckedChanged(false); which is the same exact thing, but with aNotify == true (SetCheckedChanged(arg) just calls DoSetCheckedChanged(arg, true)). We should certainly keep these codepaths in sync with each other, at the very least. OK, so why are we doing this bizarre "DoSetCheckedChanged(true, aNotify)" followed by "DoSetCheckedChanged(false, aNotify)"? For most controls that's an expensive no-op. For the case of radio controls.... I guess it's still a no-op, but even more expensive? Anyway, given that I think we should: 1) Pass false for the third arg in both places (DoneCreatingElement and AfterSetAttr) so attr setting is consistent whether it happens before or after DoneCreatingElement. 2) Remove the DoSetCheckedChanged(false, aNotify) calls, since they will now be (not too expensive) no-ops, right?
Attachment #8880693 - Flags: review?(bzbarsky) → review-
Comment on attachment 8881016 [details] [diff] [review] Pass the correct argument to DoSetChecked() in HTMLInputElement::DoneCreatingElement() and AfterSetAttr() and delete the no-op DoSetCheckedChanged() call there to avoid doing excessive work r=me
Attachment #8881016 - Flags: review?(bzbarsky) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c215e7162d Pass the correct argument to DoSetChecked() in HTMLInputElement::DoneCreatingElement() and AfterSetAttr() and delete the no-op DoSetCheckedChanged() call there to avoid doing excessive work; r=bzbarsky
You need to log in before you can comment on or make changes to this bug.