1317367 - Get rid of IsCallerChrome* calls in HTMLInputElement

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This is involved enough to spin out from bug 1316661.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Get rid of the XPCOM MozGet/SetFileNameArray methods on HTMLInputElement

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make HTMLInputElement::GetValue infallible again; just return empty string on OOM. To a first approximation no one checks the return value anyway

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Pass an explicit CallerType to HTMLInputElement::SetValue at all callsites

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Pass an explicit CallerType to HTMLInputElement::GetValueInternal

I'm not 100% sure that I'm being very consistent in my handling of
mFocusedValue, but since that's not used for file inputs, I don't think it
matters much...

A bigger problem is if people start using this caller type for things other than
file inputs.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8810701 [details] [diff] [review]
part 2.  Make HTMLInputElement::GetValue infallible again; just return empty string on OOM.  To a first approximation no one checks the return value anyway

> HTMLInputElement::GetValueInternal(nsAString& aValue) const
> {
>   switch (GetValueMode()) {
>     case VALUE_MODE_VALUE:
>       if (IsSingleLineTextControl(false)) {
>         mInputData.mState->GetValue(aValue, true);
>       } else if (!aValue.Assign(mInputData.mValue, fallible)) {
>-        return NS_ERROR_OUT_OF_MEMORY;
>-      }
>-      return NS_OK;
>+        return;
>+      }
>+      return;
Why two 'return's?. The latter should be enough.
Hmm, is aValue truncated if Assign fails? If not, .Truncate() explicitly?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8810702 [details] [diff] [review]
part 3.  Pass an explicit CallerType to HTMLInputElement::SetValue at all callsites

>+  void GetValue(nsAString& aValue, CallerType aCallerType) {
{ goes to its own line


>+++ b/layout/forms/nsNumberControlFrame.cpp
>@@ -686,17 +686,20 @@ nsNumberControlFrame::SetValueOfAnonText
>     ICUUtils::LocalizeNumber(val.toDouble(), langTagIter, localizedValue);
>   }
> #endif
> 
>   // We need to update the value of our anonymous text control here. Note that
>   // this must be its value, and not its 'value' attribute (the default value),
>   // since the default value is ignored once a user types into the text
>   // control.
>-  HTMLInputElement::FromContent(mTextField)->SetValue(localizedValue);
>+  IgnoredErrorResult rv;
>+  HTMLInputElement::FromContent(mTextField)->SetValue(localizedValue,
>+                                                      CallerType::System,
>+                                                      rv);
Why ::System here? We're in number control frame, wouldn't non system work

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8810703 [details] [diff] [review]
part 4.  Pass an explicit CallerType to HTMLInputElement::GetValueInternal

ok, this is tiny bit trickier. Need to ensure the type of the input can't somehow change just before GetValue(aValue, CallerType::System);

Actually, need to re-check also setter.
...after lunch.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Hmm, is aValue truncated if Assign fails? If not, .Truncate() explicitly?

Good point, will truncate.  And remove the extra return.

> { goes to its own line

Done.

> Why ::System here? We're in number control frame, wouldn't non system work

Done.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8810703 [details] [diff] [review]
part 4.  Pass an explicit CallerType to HTMLInputElement::GetValueInternal

>diff --git a/accessible/html/HTMLFormControlAccessible.cpp b/accessible/html/HTMLFormControlAccessible.cpp
>--- a/accessible/html/HTMLFormControlAccessible.cpp
>+++ b/accessible/html/HTMLFormControlAccessible.cpp
>@@ -347,17 +347,17 @@ HTMLTextFieldAccessible::Value(nsString&
>   nsCOMPtr<nsIDOMHTMLTextAreaElement> textArea(do_QueryInterface(mContent));
>   if (textArea) {
>     textArea->GetValue(aValue);
>     return;
>   }
> 
>   HTMLInputElement* input = HTMLInputElement::FromContent(mContent);
>   if (input)
>-    input->GetValue(aValue);
>+    input->GetValue(aValue, CallerType::System);
> }
> 
> void
> HTMLTextFieldAccessible::ApplyARIAState(uint64_t* aState) const
> {
>   HyperTextAccessibleWrap::ApplyARIAState(aState);
>   aria::MapToState(aria::eARIAAutoComplete, mContent->AsElement(), aState);
> 
>@@ -547,17 +547,17 @@ HTMLSpinnerAccessible::NativeRole()
> 
> void
> HTMLSpinnerAccessible::Value(nsString& aValue)
> {
>   AccessibleWrap::Value(aValue);
>   if (!aValue.IsEmpty())
>     return;
> 
>-  HTMLInputElement::FromContent(mContent)->GetValue(aValue);
>+  HTMLInputElement::FromContent(mContent)->GetValue(aValue, CallerType::System);
> }
> 
> double
> HTMLSpinnerAccessible::MaxValue() const
> {
>   double value = AccessibleWrap::MaxValue();
>   if (!IsNaN(value))
>     return value;
>@@ -623,17 +623,17 @@ HTMLRangeAccessible::IsWidget() const
> 
> void
> HTMLRangeAccessible::Value(nsString& aValue)
> {
>   LeafAccessible::Value(aValue);
>   if (!aValue.IsEmpty())
>     return;
> 
>-  HTMLInputElement::FromContent(mContent)->GetValue(aValue);
>+  HTMLInputElement::FromContent(mContent)->GetValue(aValue, CallerType::System);
> }
Per IRC a11y code could use NonSystem.

>     case VALUE_MODE_FILENAME:
>-      if (nsContentUtils::LegacyIsCallerChromeOrNativeCode()) {
>-        aValue.Assign(mFirstFilePath);
>-      } else {
>-        // Just return the leaf name
>-        if (mFilesOrDirectories.IsEmpty()) {
>-          aValue.Truncate();
>-        } else {
>-          GetDOMFileOrDirectoryName(mFilesOrDirectories[0], aValue);
>-        }
>-      }
>-
>+      NS_NOTREACHED("Someone screwed up here");
>+      // We'll just return empty string if someone does screw up.
>       return;
Want to explicitly Truncate the string

>@@ -3854,27 +3859,27 @@ HTMLInputElement::PreHandleEvent(EventCh
>   aVisitor.mItemFlags |= mType;
> 
>   if (aVisitor.mEvent->mMessage == eFocus &&
>       aVisitor.mEvent->IsTrusted() &&
>       MayFireChangeOnBlur() &&
>       // StartRangeThumbDrag already set mFocusedValue on 'mousedown' before
>       // we get the 'focus' event.
>       !mIsDraggingRange) {
>-    GetValue(mFocusedValue);
>+    GetValue(mFocusedValue, CallerType::System);
I guess this doesn't make the current setup any worse, where the value of mFocusedValue may depend on whether there is content JS running

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I guess this doesn't make the current setup any worse

Yeah.  It's all a bit weird.

Fixed the other issues.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/600c55ee185b
part 1.  Get rid of the XPCOM MozGet/SetFileNameArray methods on HTMLInputElement.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/071649612731
part 2.  Make HTMLInputElement::GetValue infallible again; just return empty string on OOM.  To a first approximation no one checks the return value anyway.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/85d87f11fc09
part 3.  Pass an explicit CallerType to HTMLInputElement::SetValue at all callsites.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/948aee86bbf4
part 4.  Pass an explicit CallerType to HTMLInputElement::GetValueInternal.  r=smaug

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/600c55ee185b
https://hg.mozilla.org/mozilla-central/rev/071649612731
https://hg.mozilla.org/mozilla-central/rev/85d87f11fc09
https://hg.mozilla.org/mozilla-central/rev/948aee86bbf4