Closed Bug 1341376 Opened 3 years ago Closed 3 years ago

HTMLInputElement::ValueAsDateEnabled and HTMLInputElement::SetFilesOrDirectories should use BoolVarCache

Categories

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

36 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: smaug, Assigned: jessica)

References

Details

Attachments

(1 file, 1 obsolete file)

GetBool shows up in some profiles.
Hsin-Yi, anyone on your team have time to take this?
Flags: needinfo?(htsai)
Priority: -- → P1
Same with Exception::Initialize().
Sure, Jessica has been investigating this!
Flags: needinfo?(htsai)
Assignee: nobody → jjong
Attached patch patch, v1. (obsolete) — Splinter Review
(In reply to :Ehsan Akhgari from comment #2)
> Same with Exception::Initialize().

At a first glance, didn't find calls to Preferences::GetBool() in Exception::Initializa() [1] though.

[1] https://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/dom/base/DOMException.cpp#397-421
Comment on attachment 8839832 [details] [diff] [review]
patch, v1.

Will file another bug to do some clean-up/merging on datetime and experimental_forms preferences.
Attachment #8839832 - Flags: review?(bugs)
yeah, I can't see any Get* pref method calls in Exception either.
Ehsan, perhaps you meant some other class? But please file another bug for that.
Flags: needinfo?(ehsan)
(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #7)
> yeah, I can't see any Get* pref method calls in Exception either.
> Ehsan, perhaps you meant some other class? But please file another bug for
> that.

The profile here clearly shows the call: https://perfht.ml/2l8Rq1f.  There is probably some inlining involved.  Bug 1341716.
Flags: needinfo?(ehsan)
Comment on attachment 8839832 [details] [diff] [review]
patch, v1.

>+HTMLInputElement::IsExperimentalFormsEnabled() {
Nit, { to its own line


>       if (success) {
>         newType = aResult.GetEnumValue();
>         if ((IsExperimentalMobileType(newType) &&
>-             !Preferences::GetBool("dom.experimental_forms", false)) ||
>+             !IsExperimentalFormsEnabled()) ||
>             (newType == NS_FORM_INPUT_NUMBER &&
>              !Preferences::GetBool("dom.forms.number", false)) ||
>             (newType == NS_FORM_INPUT_COLOR &&
>              !Preferences::GetBool("dom.forms.color", false)) ||
Ah, want to file a separate bug for number and color?
Attachment #8839832 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #9)
> Comment on attachment 8839832 [details] [diff] [review]
> patch, v1.
> 
> >+HTMLInputElement::IsExperimentalFormsEnabled() {
> Nit, { to its own line
> 
> 
> >       if (success) {
> >         newType = aResult.GetEnumValue();
> >         if ((IsExperimentalMobileType(newType) &&
> >-             !Preferences::GetBool("dom.experimental_forms", false)) ||
> >+             !IsExperimentalFormsEnabled()) ||
> >             (newType == NS_FORM_INPUT_NUMBER &&
> >              !Preferences::GetBool("dom.forms.number", false)) ||
> >             (newType == NS_FORM_INPUT_COLOR &&
> >              !Preferences::GetBool("dom.forms.color", false)) ||
> Ah, want to file a separate bug for number and color?

Hmm, I think I'll do it in this bug, will upload a new patch for this.
Attached patch patch, v2.Splinter Review
Use BoolVarCache to get dom.forms.color and dom.forms.number prefs as well, carrying r+.
Attachment #8839832 - Attachment is obsolete: true
Attachment #8840323 - Flags: review+
Thanks
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/212db4739778
Use BoolVarCache to cache preferences in HTMLInputElement. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/212db4739778
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.