Closed
Bug 1397904
Opened 7 years ago
Closed 7 years ago
nsTextControlFrame is too QI heavy
Categories
(Core :: Layout: Form Controls, enhancement)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: smaug, Assigned: smaug)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
(deleted),
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
It QIs to nsITextControlElement all the time. Could we not do that somehow?
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8905890 -
Flags: review?(amarchesini)
Assignee | ||
Comment 2•7 years ago
|
||
That is a quick and simple approach to help with bug 1346723.
Other code paths using nsITextControlElement aren't as hot.
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
And I couldn't prove that we don't need the strong reference, and the WeakFrame there hints that objects may die, so I decided to stick with strong references.
Comment 5•7 years ago
|
||
Comment on attachment 8905890 [details] [diff] [review]
less_qiing_when_setting_textformcontrol_value.diff
Review of attachment 8905890 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/html/HTMLInputElement.h
@@ +342,5 @@
> {
> return mHasPatternAttribute;
> }
>
> + virtual already_AddRefed<nsITextControlElement> GetAsTextControlElement() override
no virtual for final classes.
::: dom/html/HTMLTextAreaElement.h
@@ +167,5 @@
>
> NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(HTMLTextAreaElement,
> nsGenericHTMLFormElementWithState)
>
> + virtual already_AddRefed<nsITextControlElement> GetAsTextControlElement() override
here as well.
::: layout/forms/nsTextControlFrame.cpp
@@ +1137,1 @@
> NS_ASSERTION(txtCtrl, "Content not a text control element");
MOZ_ASSERT here?
Attachment #8905890 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 6•7 years ago
|
||
The existing coding style uses virtual and override, and IMO virtual is a great annotation to tell that the method call is slow.
And yes, I guess I could change that to MOZ_ASSERT
Assignee | ||
Comment 7•7 years ago
|
||
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/560c9ee4d584
nsTextControlFrame::SetValueChanged is too QI heavy, r=baku
Comment 9•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•