Deprecate support for xhr.withCredentials for synchronous requests

Deprecate support for xhr.withCredentials for synchronous requests

As per XML Level2 spec, withCredentials flag cannot be set on sync request.
http://xhr.spec.whatwg.org/#the-withcredentials-attribute
However for time being add useCounter and throw console warning to web developers.

Intent to deprecate - https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/BOgLhbdljU4

BUG=383483
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176532

[maheshkk, 2014-06-11 20:34:42]

First proposal is to just throw warning. Still does not match spec,
alternatively we can throw exception and warning.

PTAL

[Inactive, 2014-06-12 13:28:40]

Please update your CL description to contain the behavior of IE11 and Firefox
29.

https://codereview.chromium.org/329323002/diff/1/Source/core/xml/XMLHttpReque...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/329323002/diff/1/Source/core/xml/XMLHttpReque...
Source/core/xml/XMLHttpRequest.cpp:492: String consoleMessage("Support for
withCredentials deprecated for synchronous requests.");
If you do this, you will like need to add a UseCounter as well so that we can
measure usage out there and know if and when it is safe to drop support for
this.

[maheshkk, 2014-06-12 15:01:06]

Thanks for the review Chris. 

On 2014/06/12 13:28:40, Chris Dumez wrote:
> Please update your CL description to contain the behavior of IE11 and Firefox
> 29.
> 

Firefox already throws exception for this attribute and IE11 ignores as Chrome
does. 


>
https://codereview.chromium.org/329323002/diff/1/Source/core/xml/XMLHttpReque...
> File Source/core/xml/XMLHttpRequest.cpp (right):
> 
>
https://codereview.chromium.org/329323002/diff/1/Source/core/xml/XMLHttpReque...
> Source/core/xml/XMLHttpRequest.cpp:492: String consoleMessage("Support for
> withCredentials deprecated for synchronous requests.");
> If you do this, you will like need to add a UseCounter as well so that we can
> measure usage out there and know if and when it is safe to drop support for
> this.

Agreed. I'm not sure if we should deprecate this attribute or just remove
support from our implementation as we did for timeout attribute. 

Adding dglazkov for comment.

[maheshkk, 2014-06-13 20:13:34]

dglazkov, PTAL, and do you think we should deprecate withCredentials or just
throw exception as per spec.

[dglazkov, 2014-06-13 20:42:06]

On 2014/06/13 at 20:13:34, mahesh.kk wrote:
> dglazkov, PTAL, and do you think we should deprecate withCredentials or just
throw exception as per spec.

How long have we shipped this property? What's our confidence that nobody on the
web is using this?

[maheshkk, 2014-06-13 20:43:24]

On 2014/06/13 20:42:06, dglazkov wrote:
> On 2014/06/13 at 20:13:34, mahesh.kk wrote:
> > dglazkov, PTAL, and do you think we should deprecate withCredentials or just
> throw exception as per spec.
> 
> How long have we shipped this property? What's our confidence that nobody on
the
> web is using this?

I'm not sure about the usage. I will add useCount and through deprecated message
instead. Thanks!

[maheshkk, 2014-06-13 21:48:26]

Added UseCounter and deprecate message for withCredentials. PTAL.

[sof, 2014-06-16 14:21:34]

https://codereview.chromium.org/329323002/diff/20001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/329323002/diff/20001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:494: return;
If I understand your intent correctly, isn't this doing something a bit
stronger? i.e., it's issuing a deprecation message and ignoring the value being
set.

[maheshkk, 2014-06-16 15:53:21]

On 2014/06/16 14:21:34, sof wrote:
>
https://codereview.chromium.org/329323002/diff/20001/Source/core/xml/XMLHttpR...
> File Source/core/xml/XMLHttpRequest.cpp (right):
> 
>
https://codereview.chromium.org/329323002/diff/20001/Source/core/xml/XMLHttpR...
> Source/core/xml/XMLHttpRequest.cpp:494: return;
> If I understand your intent correctly, isn't this doing something a bit
> stronger? i.e., it's issuing a deprecation message and ignoring the value
being
> set.

Right, on other hand, IMO, if we do continue to set the value, developers will
less likely notice the deprecation message until the feature is working fine. 
This change will only warn developers about the deprecated feature and not throw
exception as per the specification.

[Inactive, 2014-06-16 17:07:33]

https://codereview.chromium.org/329323002/diff/20001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/329323002/diff/20001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:494: return;
On 2014/06/16 14:21:34, sof wrote:
> If I understand your intent correctly, isn't this doing something a bit
> stronger? i.e., it's issuing a deprecation message and ignoring the value
being
> set.

sof@ is right, IMHO. We should probably not ignore the input value until we get
usage numbers. It is too early to do this IMO, we should just add a usecounter
(and print the deprecation warning if API Owners are OK with it).

[maheshkk, 2014-06-16 17:57:42]

On 2014/06/16 17:07:33, Chris Dumez wrote:
>
https://codereview.chromium.org/329323002/diff/20001/Source/core/xml/XMLHttpR...
> File Source/core/xml/XMLHttpRequest.cpp (right):
> 
>
https://codereview.chromium.org/329323002/diff/20001/Source/core/xml/XMLHttpR...
> Source/core/xml/XMLHttpRequest.cpp:494: return;
> On 2014/06/16 14:21:34, sof wrote:
> > If I understand your intent correctly, isn't this doing something a bit
> > stronger? i.e., it's issuing a deprecation message and ignoring the value
> being
> > set.
> 
> sof@ is right, IMHO. We should probably not ignore the input value until we
get
> usage numbers. It is too early to do this IMO, we should just add a usecounter
> (and print the deprecation warning if API Owners are OK with it).

Thanks Chris and sof! I have updated patch to only display deprecated message
without change in existing behavior.

[Inactive, 2014-06-16 18:05:47]

https://codereview.chromium.org/329323002/diff/40001/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/329323002/diff/40001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:737: return "Support for withCredentials
deprecated for synchronous requests.";
Setting 'XMLHttpRequest.withCredentials' for synchronous requests is deprecated.

https://codereview.chromium.org/329323002/diff/40001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/329323002/diff/40001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:491: // However for time being only throw
warning message to console for web developers.
s/throw/print a/

[sof, 2014-06-16 18:11:12]

Great, thanks for updating the patch.

If not already, have you looked at

 http://www.chromium.org/blink#TOC-Launch-Process:-Deprecation

and considered sending an intent-to-deprecate to blink-dev about this one? We
want to make sure that there are no surprises, and XHR is quite widely used
after all.

https://codereview.chromium.org/329323002/diff/40001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/329323002/diff/40001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:492: if (!m_async &&
executionContext()->isDocument()) {
Remove the braces?

[maheshkk, 2014-06-16 21:38:42]

Thanks again Chris, sof for your reviews! I have updated patch per your
comments.

[Inactive, 2014-06-16 21:40:18]

On 2014/06/16 21:38:42, maheshkk wrote:
> Thanks again Chris, sof for your reviews! I have updated patch per your
> comments.

Please add the link to your "intent to deprecate"
(https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/BOgLhbdljU4)
to your CL description.

[eseidel, 2014-06-16 23:23:51]

https://codereview.chromium.org/329323002/diff/80001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/329323002/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:490: // FIXME: According to XMLHttpRequest
Level 2 we should throw InvalidAccessError exception here.
Seems like we should only bother to print this deprecation if sites are actually
changing the value from its default?

[Inactive, 2014-06-17 00:02:22]

https://codereview.chromium.org/329323002/diff/80001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/329323002/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:490: // FIXME: According to XMLHttpRequest
Level 2 we should throw InvalidAccessError exception here.
On 2014/06/16 23:23:51, eseidel (OOO until June 16) wrote:
> Seems like we should only bother to print this deprecation if sites are
actually
> changing the value from its default?

Eric, the idea is to eventually match the spec and start throwing if m_async is
false (no matter what value is being assigned). As a result, I think it makes
sense to display the deprecation message, no matter what value is being set as
we would break those pages if we started throwing. Do you agree?

P.S: You status still says "OOO" :)

[tyoshino (SeeGerritForStatus), 2014-06-17 02:19:49]

https://codereview.chromium.org/329323002/diff/80001/LayoutTests/http/tests/i...
File LayoutTests/http/tests/inspector/network-test.js (right):

https://codereview.chromium.org/329323002/diff/80001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/network-test.js:94: if (async)
I think we should catch bad combination of arguments from a buggy test case. How
about throwing when !async && withCredentials?

https://codereview.chromium.org/329323002/diff/80001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/329323002/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:490: // FIXME: According to XMLHttpRequest
Level 2 we should throw InvalidAccessError exception here.
On 2014/06/17 00:02:22, Chris Dumez wrote:
> On 2014/06/16 23:23:51, eseidel (OOO until June 16) wrote:
> > Seems like we should only bother to print this deprecation if sites are
> actually
> > changing the value from its default?
> 
> Eric, the idea is to eventually match the spec and start throwing if m_async
is
> false (no matter what value is being assigned). As a result, I think it makes
> sense to display the deprecation message, no matter what value is being set as
> we would break those pages if we started throwing. Do you agree?
> 
> P.S: You status still says "OOO" :)

Sounds good. Otherwise, we should fix the spec first.

[sof, 2014-06-18 08:16:06]

https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.cpp:492: if (!m_async &&
executionContext()->isDocument())
No need to check isDocument(), just pass in executionContext() to count() (or
countDeprecation().)

That has the added benefit of also making the non-Document (i.e., Workers) case
eventually work.

https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.cpp:493: UseCounter::countDeprecation(document(),
UseCounter::SyncXHRWithCredentials);
With https://codereview.chromium.org/337283003 adding a global syncXHR use
counter, should we turn this into a count() for SyncXHRWithCredentials initially
& return once data for that has settled?

[maheshkk, 2014-06-18 15:36:34]

On 2014/06/18 08:16:06, sof wrote:
>
https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
> File Source/core/xml/XMLHttpRequest.cpp (right):
> 
>
https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
> Source/core/xml/XMLHttpRequest.cpp:492: if (!m_async &&
> executionContext()->isDocument())
> No need to check isDocument(), just pass in executionContext() to count() (or
> countDeprecation().)
> 
> That has the added benefit of also making the non-Document (i.e., Workers)
case
> eventually work.
> 

I thought about doing that. However UseCounter::count doesn't count for
non-Document ExecutionContext.

>
https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
> Source/core/xml/XMLHttpRequest.cpp:493:
UseCounter::countDeprecation(document(),
> UseCounter::SyncXHRWithCredentials);
> With https://codereview.chromium.org/337283003 adding a global syncXHR use
> counter, should we turn this into a count() for SyncXHRWithCredentials
initially
> & return once data for that has settled?

I din't quite understand, do you mean for a given request we only count
SyncXHRWithCredentials (if its being set) and not XMLHttpRequestSynchronous?
And only do count() and not countDeprecate()?

[sof, 2014-06-18 15:44:07]

(Please comment on the issues raised rather than inline; it just gets messy
without.)

On 2014/06/18 15:36:34, maheshkk wrote:
> On 2014/06/18 08:16:06, sof wrote:
> >
>
https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
> > File Source/core/xml/XMLHttpRequest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
> > Source/core/xml/XMLHttpRequest.cpp:492: if (!m_async &&
> > executionContext()->isDocument())
> > No need to check isDocument(), just pass in executionContext() to count()
(or
> > countDeprecation().)
> > 
> > That has the added benefit of also making the non-Document (i.e., Workers)
> case
> > eventually work.
> > 
> 
> I thought about doing that. However UseCounter::count doesn't count for
> non-Document ExecutionContext.
> 

Yes, but once http://code.google.com/p/chromium/issues/detail?id=376039 is in
place, you'll automatically get it. And I think you want to include sync XHR
uses from within Workers, where it perhaps is more common (a guess.)

> >
>
https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
> > Source/core/xml/XMLHttpRequest.cpp:493:
> UseCounter::countDeprecation(document(),
> > UseCounter::SyncXHRWithCredentials);
> > With https://codereview.chromium.org/337283003 adding a global syncXHR use
> > counter, should we turn this into a count() for SyncXHRWithCredentials
> initially
> > & return once data for that has settled?
> 
> I din't quite understand, do you mean for a given request we only count
> SyncXHRWithCredentials (if its being set) and not XMLHttpRequestSynchronous?
> And only do count() and not countDeprecate()?

We count both. But before we can consider deprecating, usage counts for
"xhr.withCredentials = value;" would be useful IMHO.

[maheshkk, 2014-06-18 15:55:08]

https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.cpp:492: if (!m_async &&
executionContext()->isDocument())
On 2014/06/18 08:16:06, sof wrote:
> No need to check isDocument(), just pass in executionContext() to count() (or
> countDeprecation().)
> 
> That has the added benefit of also making the non-Document (i.e., Workers)
case
> eventually work.

You are right. Once http://code.google.com/p/chromium/issues/detail?id=376039 is
done it will make much more sense.

https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.cpp:493: UseCounter::countDeprecation(document(),
UseCounter::SyncXHRWithCredentials);
On 2014/06/18 08:16:06, sof wrote:
> With https://codereview.chromium.org/337283003 adding a global syncXHR use
> counter, should we turn this into a count() for SyncXHRWithCredentials
initially
> & return once data for that has settled?

IMO, printing deprecated message will serve as early warning without waiting for
useCount. Because blink doesn't match other browsers and spec. And remove the
support once useCount is less after significant time provided to developers with
deprecated warning message. Thoughts?

[sof, 2014-06-18 16:13:36]

https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.cpp:493: UseCounter::countDeprecation(document(),
UseCounter::SyncXHRWithCredentials);
On 2014/06/18 15:55:08, maheshkk wrote:
> On 2014/06/18 08:16:06, sof wrote:
> > With https://codereview.chromium.org/337283003 adding a global syncXHR use
> > counter, should we turn this into a count() for SyncXHRWithCredentials
> initially
> > & return once data for that has settled?
> 
> IMO, printing deprecated message will serve as early warning without waiting
for
> useCount. Because blink doesn't match other browsers and spec. And remove the
> support once useCount is less after significant time provided to developers
with
> deprecated warning message. Thoughts? 

I understand what you hope is the side-effect of also emitting the deprecation
message. I could be interpreting this wrongly, but I don't think we're quite
there yet here wrt deprecation and removal -- what if the usage data (and other
considerations) show that we cannot go ahead with this, wouldn't it be
counterproductive to have emitted a deprecation message for a while?

[maheshkk, 2014-06-18 23:53:07]

https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/329323002/diff/100001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.cpp:493: UseCounter::countDeprecation(document(),
UseCounter::SyncXHRWithCredentials);
On 2014/06/18 16:13:36, sof wrote:
> On 2014/06/18 15:55:08, maheshkk wrote:
> > On 2014/06/18 08:16:06, sof wrote:
> > > With https://codereview.chromium.org/337283003 adding a global syncXHR use
> > > counter, should we turn this into a count() for SyncXHRWithCredentials
> > initially
> > > & return once data for that has settled?
> > 
> > IMO, printing deprecated message will serve as early warning without waiting
> for
> > useCount. Because blink doesn't match other browsers and spec. And remove
the
> > support once useCount is less after significant time provided to developers
> with
> > deprecated warning message. Thoughts? 
> 
> I understand what you hope is the side-effect of also emitting the deprecation
> message. I could be interpreting this wrongly, but I don't think we're quite
> there yet here wrt deprecation and removal -- what if the usage data (and
other
> considerations) show that we cannot go ahead with this, wouldn't it be
> counterproductive to have emitted a deprecation message for a while?

Agreed. We could go by only adding UseCounter now and iterate again after
obtaining the use count to deprecate. 
Mozilla, however throws exception. So web developers who might be using this
already have split in browsers behavior. I understand that it would be much
safer to deprecate after getting use count.

[eseidel, 2014-06-19 00:07:58]

I think it's fine to go ahead and deprecate directly.  No need to use count just
to deprecate.  Deprecate now, if the count is too high we can remove the
"deprecate" bits since we'll have decided it's too high to remove if its' too
high to deprecate.

[maheshkk, 2014-06-19 00:26:45]

On 2014/06/19 00:07:58, eseidel wrote:
> I think it's fine to go ahead and deprecate directly.  No need to use count
just
> to deprecate.  Deprecate now, if the count is too high we can remove the
> "deprecate" bits since we'll have decided it's too high to remove if its' too
> high to deprecate.

Thanks for the review Eric. I have updated patch as per sof suggestion to use
executionContext to count even xhr sync requests from workers. 

PTAL.

[sof, 2014-06-19 06:37:20]

Good - with that clarification made, lgtm (patchset 6.)

[maheshkk, 2014-06-19 08:26:11]

The CQ bit was checked by mahesh.kk@samsung.com

[commit-bot: I haz the power, 2014-06-19 08:26:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mahesh.kk@samsung.com/329323002/120001

[commit-bot: I haz the power, 2014-06-19 08:26:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-19 08:26:47]

Failed to apply patch for Source/core/frame/UseCounter.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/frame/UseCounter.h
  Hunk #1 FAILED at 463.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/frame/UseCounter.h.rej

Patch:       Source/core/frame/UseCounter.h
Index: Source/core/frame/UseCounter.h
diff --git a/Source/core/frame/UseCounter.h b/Source/core/frame/UseCounter.h
index
e76d3cfdfe6e09982a6d3ac7e7ced033d13e75b3..7f6d44fbd14d3f37a909ef11191fcdd8f0c7c78c
100644
--- a/Source/core/frame/UseCounter.h
+++ b/Source/core/frame/UseCounter.h
@@ -463,6 +463,7 @@ public:
         HTMLImportsAsyncAttribute = 463,
         FontFaceSetReady = 464,
         XMLHttpRequestSynchronous = 465,
+        SyncXHRWithCredentials = 466,
         // Add new features immediately above this line. Don't change assigned
         // numbers of any item, and don't reuse removed slots.
         // Also, run update_use_counter_feature_enum.py in
chromium/src/tools/metrics/histograms/

[tyoshino (SeeGerritForStatus), 2014-06-19 08:31:13]

https://codereview.chromium.org/329323002/diff/80001/LayoutTests/http/tests/i...
File LayoutTests/http/tests/inspector/network-test.js (right):

https://codereview.chromium.org/329323002/diff/80001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/network-test.js:94: if (async)
On 2014/06/17 02:19:49, tyoshino wrote:
> I think we should catch bad combination of arguments from a buggy test case.
How
> about throwing when !async && withCredentials?

Reminder. (it's ok to skip it as it was drive-by comment, but want to hear some
answer)

[maheshkk, 2014-06-19 08:45:43]

On 2014/06/19 08:31:13, tyoshino wrote:
>
https://codereview.chromium.org/329323002/diff/80001/LayoutTests/http/tests/i...
> File LayoutTests/http/tests/inspector/network-test.js (right):
> 
>
https://codereview.chromium.org/329323002/diff/80001/LayoutTests/http/tests/i...
> LayoutTests/http/tests/inspector/network-test.js:94: if (async)
> On 2014/06/17 02:19:49, tyoshino wrote:
> > I think we should catch bad combination of arguments from a buggy test case.
> How
> > about throwing when !async && withCredentials?
> 
> Reminder. (it's ok to skip it as it was drive-by comment, but want to hear
some
> answer)

@tyoshino sorry you had to remind, I thought comment #17 from Chris answered
your question. 
What I understood from mailing list and chat on irc is we cant change existing
behavior and only show a deprecation message. Throwing an exception would break
existing use cases and that isnt an option until we know usecount. Did I
misinterpret your comment?

[tyoshino (SeeGerritForStatus), 2014-06-19 08:58:16]

On 2014/06/19 08:45:43, maheshkk wrote:
> @tyoshino sorry you had to remind, I thought comment #17 from Chris answered
> your question. 
> What I understood from mailing list and chat on irc is we cant change existing
> behavior and only show a deprecation message. Throwing an exception would
break
> existing use cases and that isnt an option until we know usecount. Did I
> misinterpret your comment?

Thanks and sorry if my comment was unclear.

I suggested that your change to the makeXHR() method in network-test.js should
also validate combination of async and withCredentials.

With your change, makeXHR() will silently ignores withCredentials parameter set
to true when async is set. That means the author of a layout test which uses
makeXHR() won't notice he/she's using wrong combination of async argument and
withCredentials argument. If we put

else
  throw new TypeError('withCredentials must be false when async is false');

then, the layout test fails and the author realizes he/she used wrong
parameters.

[tyoshino (SeeGerritForStatus), 2014-06-19 08:59:09]

Sorry. Typo

On 2014/06/19 08:58:16, tyoshino wrote:
> With your change, makeXHR() will silently ignores withCredentials parameter
set
> to true when async is set. That means the author of a layout test which uses

s/async is set/async is unset/

[maheshkk, 2014-06-19 09:14:43]

On 2014/06/19 08:58:16, tyoshino wrote:
> On 2014/06/19 08:45:43, maheshkk wrote:
> > @tyoshino sorry you had to remind, I thought comment #17 from Chris answered
> > your question. 
> > What I understood from mailing list and chat on irc is we cant change
existing
> > behavior and only show a deprecation message. Throwing an exception would
> break
> > existing use cases and that isnt an option until we know usecount. Did I
> > misinterpret your comment?
> 
> Thanks and sorry if my comment was unclear.
> 
> I suggested that your change to the makeXHR() method in network-test.js should
> also validate combination of async and withCredentials.
> 
> With your change, makeXHR() will silently ignores withCredentials parameter
set
> to true when async is set. That means the author of a layout test which uses
> makeXHR() won't notice he/she's using wrong combination of async argument and
> withCredentials argument. If we put
> 
> else
>   throw new TypeError('withCredentials must be false when async is false');
> 
> then, the layout test fails and the author realizes he/she used wrong
> parameters.

Ah my bad. now that I read your comments wrt network-test.js, that's a good
catch. 
Now that we are not throwing any exception, (original patch did) I had to skip
setting withCredetials for sync requests.
Now that we only have deprecation message ideally I think I will remove that
change and let all cases go through deprecation message, might have more layout
expected files to update however :)

[tyoshino (SeeGerritForStatus), 2014-06-19 15:17:35]

On 2014/06/19 09:14:43, maheshkk wrote:
> Ah my bad. now that I read your comments wrt network-test.js, that's a good
> catch. 
> Now that we are not throwing any exception, (original patch did) I had to skip
> setting withCredetials for sync requests.
> Now that we only have deprecation message ideally I think I will remove that
> change and let all cases go through deprecation message, might have more
layout
> expected files to update however :)

Oh, I was also misunderstanding the effect of the change in network-test.js. I
thought everyone using the makeXHR function is not passing (async,
withCredentials) = (false, true) combination.

Reviewed new expectations.

lgtm

[maheshkk, 2014-06-19 16:49:35]

The CQ bit was checked by mahesh.kk@samsung.com

[commit-bot: I haz the power, 2014-06-19 16:50:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mahesh.kk@samsung.com/329323002/140001

[commit-bot: I haz the power, 2014-06-19 17:58:50]

Change committed as 176532