Thomas Wisniewski Assignee
	Description • 8 years ago

In bug 933759, the XHR Open() methods were changed to match the WebIDL in the spec, but there is still follow-up work to do:

1) Fold these two methods into one, removing their use of Optionals:
>void Open(const nsACString& aMethod, const nsAString& aUrl, bool aAsync, const nsAString& aUsername, const nsAString& aPassword, ErrorResult& aRv);
>nsresult Open(const nsACString& aMethod, const nsACString& aUrl, const Optional<bool>& aAsync, const Optional<nsAString>& aUsername, const Optional<nsAString>& aPassword);

2) Correct the handling of the username and password parameters to better-match steps 7 and 8 in the spec.

Here is a patch which does these things. A try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b74d9cc3a3b4

Unfortunately, I can't think of a way to add web platform tests to ensure that #2 doesn't break, because Firefox and Chrome both disallow URL authentication even in XHRs (and I'd be surprised if WebKit or Edge allow it).

I'm also not sure if it's even worth changing the spec at this point to use header-based authentication instead, even if the feature is technically broken anyway in modern browsers (at least without preference-flipping).

	Thomas Wisniewski Assignee
	Comment 1 • 8 years ago

Odd, the patch didn't upload. Here it is.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 8 years ago

Comment on attachment 8791057 [details] [diff] [review]
refactor-xhr-open-methods-to-not-use-optionals-and-correct-auth-handling.diff

> XMLHttpRequestMainThread::Open(const nsACString& aMethod, const nsAString& aUrl,
>                                ErrorResult& aRv)
> {
>-  aRv = Open(aMethod, NS_ConvertUTF16toUTF8(aUrl), Optional<bool>(true),
>-             Optional<nsAString>(), Optional<nsAString>());
>+  nsString voidString;
>+  voidString.SetIsVoid(true);
>+  Open(aMethod, aUrl, true, voidString, voidString, aRv);
Just pass NullString()

(sorry, you can blame me it is NullString and not VoidString, but I think setIsVoid should be renamed to setItNull.)

>+++ b/dom/xhr/XMLHttpRequestWorker.cpp
>@@ -1421,25 +1421,31 @@ OpenRunnable::MainThreadRunInternal()
>   if (mTimeout) {
>     rv = mProxy->mXHR->SetTimeout(mTimeout);
>     NS_ENSURE_SUCCESS(rv, rv);
>   }
> 
>   MOZ_ASSERT(!mProxy->mInOpen);
>   mProxy->mInOpen = true;
> 
>-  rv = mProxy->mXHR->Open(mMethod, NS_ConvertUTF16toUTF8(mURL),
>-                          Optional<bool>(true), mUser, mPassword);
>+  ErrorResult rv2;
>+  nsString voidString;
>+  voidString.SetIsVoid(true);
>+  mProxy->mXHR->Open(mMethod, mURL, true,
>+                     mUser.WasPassed() ? mUser.Value() : voidString,
>+                     mPassword.WasPassed() ? mPassword.Value() : voidString,
>+                     rv2);
Just pass NullString()

	Thomas Wisniewski Assignee
	Comment 3 • 8 years ago

Thanks! I'm just glad there *is* a NullString()... I only thought to look for VoidString() :S

Is that something we'd like to change for consistency's sake? I don't mind doing the legwork, if it's just a matter of renaming some variables (or possibly leaving the "void" versions around for a deprecation period, for the sake of Thunderbird and such).

	Thomas Wisniewski Assignee
	Comment 4 • 8 years ago

New patch with review comments addressed.

Carrying over r+ and requesting check-in.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 8 years ago

Going through the code to see where SetIsVoid could be replaced with NullString() would be great.
I don't think SetIsVoid is used that often in such cases where the string is supposed to be null all the time.

	Pulsebot
	Comment 7 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66b76839f96a
Refactor XHR.open() methods to remove use of Optionals and correct auth handling. r=smaug

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 8 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/66b76839f96a