Closed
Bug 1375741
Opened 7 years ago
Closed 7 years ago
HTMLInputElement::DoneCreatingElement() passes the wrong third argument to DoSetChecked() resulting in some unneeded work
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
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 [1], but this code wants DoSetCheckedChanged(false, false) [2]. 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 [1] https://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/dom/html/HTMLInputElement.cpp#3231 [2] https://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/dom/html/HTMLInputElement.cpp#6550
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8880693 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•7 years ago
|
||
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 3•7 years ago
|
||
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-
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8881016 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•7 years ago
|
Attachment #8880693 -
Attachment is obsolete: true
Comment 5•7 years ago
|
||
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 eakhgari@mozilla.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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c3c215e7162d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•