Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
490083002:
[XHR] Move the code to clear variables to internalAbort() (Closed)
|
Created:
6 years, 4 months ago by tyoshino (SeeGerritForStatus) Modified:
6 years, 3 months ago CC:
blink-reviews Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
Description[XHR] Move the code to clear variables to internalAbort()
This CL adds internalAbort() call to handleDidCancel(). This is
not harmful. It's rather good that we clear resources.
R=yhirano,sof,kouhei
BUG=406229
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181791
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Reverted removal of cancelLoader() in handleDidTimeout #
Total comments: 6
Patch Set 4 : Final picture #Patch Set 5 : #Patch Set 6 : #
Total comments: 5
Patch Set 7 : Removed assertion #
Total comments: 2
Patch Set 8 : Addressed #16 #Patch Set 9 : Rebase #Messages
Total messages: 24 (2 generated)
Sorry please stop reviewing. This breaks some layout tests.
Fixed
https://codereview.chromium.org/490083002/diff/40001/Source/core/xml/XMLHttpR... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/490083002/diff/40001/Source/core/xml/XMLHttpR... Source/core/xml/XMLHttpRequest.cpp:942: { ASSERT(m_error) here? https://codereview.chromium.org/490083002/diff/40001/Source/core/xml/XMLHttpR... Source/core/xml/XMLHttpRequest.cpp:957: // If abort() called cancelLoader() and a nested open() ended up I'm not sure if this split is good. The latter half of |cancelLoader| is about a recursion problem of |abort|, not about canceling loader. https://codereview.chromium.org/490083002/diff/40001/Source/core/xml/XMLHttpR... Source/core/xml/XMLHttpRequest.cpp:1075: // Note: The below event dispatch may be called while |hasPendingActivity() == false|, Please wrap the comment in 80 columns.
https://codereview.chromium.org/490083002/diff/40001/Source/core/xml/XMLHttpR... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/490083002/diff/40001/Source/core/xml/XMLHttpR... Source/core/xml/XMLHttpRequest.cpp:957: // If abort() called cancelLoader() and a nested open() ended up On 2014/08/21 06:49:20, yhirano wrote: > I'm not sure if this split is good. The latter half of |cancelLoader| is about a > recursion problem of |abort|, not about canceling loader. Call of abort() is just one of possible scenarios where reentry may happen. Also, internalAbort() is used not only by abort(). internalAbort() is not good name. I plan to name it to something different without abort. After this change, I want to reorganize calls of methods for clearing variables such as clearResponse() and clearVariableForLoading().
https://codereview.chromium.org/490083002/diff/40001/Source/core/xml/XMLHttpR... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/490083002/diff/40001/Source/core/xml/XMLHttpR... Source/core/xml/XMLHttpRequest.cpp:957: // If abort() called cancelLoader() and a nested open() ended up On 2014/08/21 07:06:31, tyoshino wrote: > On 2014/08/21 06:49:20, yhirano wrote: > > I'm not sure if this split is good. The latter half of |cancelLoader| is about > a > > recursion problem of |abort|, not about canceling loader. > > Call of abort() is just one of possible scenarios where reentry may happen. > > Also, internalAbort() is used not only by abort(). internalAbort() is not good > name. I plan to name it to something different without abort. > > After this change, I want to reorganize calls of methods for clearing variables > such as clearResponse() and clearVariableForLoading(). I'm a bit confused. Are you going to call |cancelLoader| without preceding |internalAbort|, or call |internalAbort| without following |cancelLoader|?
https://codereview.chromium.org/490083002/diff/40001/Source/core/xml/XMLHttpR... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/490083002/diff/40001/Source/core/xml/XMLHttpR... Source/core/xml/XMLHttpRequest.cpp:957: // If abort() called cancelLoader() and a nested open() ended up On 2014/08/21 08:01:36, yhirano wrote: > On 2014/08/21 07:06:31, tyoshino wrote: > > On 2014/08/21 06:49:20, yhirano wrote: > > > I'm not sure if this split is good. The latter half of |cancelLoader| is > about > > a > > > recursion problem of |abort|, not about canceling loader. > > > > Call of abort() is just one of possible scenarios where reentry may happen. > > > > Also, internalAbort() is used not only by abort(). internalAbort() is not good > > name. I plan to name it to something different without abort. > > > > After this change, I want to reorganize calls of methods for clearing > variables > > such as clearResponse() and clearVariableForLoading(). > > I'm a bit confused. > Are you going to call |cancelLoader| without preceding |internalAbort|, or call > |internalAbort| without following |cancelLoader|? The latter. See the newly added FIXME below. I think cancelLoader() is the code most hard to understand. I'd like to split code in the new internalAbort() from ones in cancelLoader() first and then reorganize code currently placed in internalAbort() and other variable clearing code.
OK. My main motivation is not elimination of cancel() call but reorganizing variable clearing code. I'll revert the split and upload revised version. Please suspend review for now.
PTAL
https://codereview.chromium.org/490083002/diff/100001/Source/core/xml/XMLHttp... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/490083002/diff/100001/Source/core/xml/XMLHttp... Source/core/xml/XMLHttpRequest.cpp:548: ASSERT(m_state == UNSENT); We don't need this assertion: it's too obvious. https://codereview.chromium.org/490083002/diff/100001/Source/core/xml/XMLHttp... Source/core/xml/XMLHttpRequest.cpp:930: clearRequest(); Doesn't Moving clearing code beyond loader->cancel() cause a behavior change?
https://codereview.chromium.org/490083002/diff/100001/Source/core/xml/XMLHttp... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/490083002/diff/100001/Source/core/xml/XMLHttp... Source/core/xml/XMLHttpRequest.cpp:930: clearRequest(); On 2014/08/21 11:23:09, yhirano wrote: > Doesn't Moving clearing code beyond loader->cancel() cause a behavior change? If internalAbort() returned true, it means send() didn't happen, but open() may still have happened. So, m_requestHeaders may contain after cancel(), but we still have m_error set. So, there shouldn't be any problem.
lgtm. Please remove the assertion.
https://codereview.chromium.org/490083002/diff/100001/Source/core/xml/XMLHttp... File Source/core/xml/XMLHttpRequest.cpp (right): https://codereview.chromium.org/490083002/diff/100001/Source/core/xml/XMLHttp... Source/core/xml/XMLHttpRequest.cpp:548: ASSERT(m_state == UNSENT); On 2014/08/21 11:23:09, yhirano wrote: > We don't need this assertion: it's too obvious. Done. https://codereview.chromium.org/490083002/diff/100001/Source/core/xml/XMLHttp... Source/core/xml/XMLHttpRequest.cpp:930: clearRequest(); On 2014/08/21 12:19:48, tyoshino wrote: > On 2014/08/21 11:23:09, yhirano wrote: > > Doesn't Moving clearing code beyond loader->cancel() cause a behavior change? > > If internalAbort() returned true, it means send() didn't happen, but open() may > still have happened. So, m_requestHeaders may contain after cancel(), but we > still have m_error set. So, there shouldn't be any problem. And, as we're clearing the variables before cancel(), in window.onload (see below): BEFORE THIS CL: Some properties e.g. responseURL was working AFTER THIS CL: Everything are cleared I think we don't have to care this.
+sof
https://codereview.chromium.org/490083002/diff/120001/Source/core/xml/XMLHttp... File Source/core/xml/XMLHttpRequest.cpp (left): https://codereview.chromium.org/490083002/diff/120001/Source/core/xml/XMLHttp... Source/core/xml/XMLHttpRequest.cpp:993: void XMLHttpRequest::handleDidFailGeneric() Remove the method from the class declaration also?
https://codereview.chromium.org/490083002/diff/120001/Source/core/xml/XMLHttp... File Source/core/xml/XMLHttpRequest.cpp (left): https://codereview.chromium.org/490083002/diff/120001/Source/core/xml/XMLHttp... Source/core/xml/XMLHttpRequest.cpp:993: void XMLHttpRequest::handleDidFailGeneric() On 2014/08/21 21:12:44, sof wrote: > Remove the method from the class declaration also? Done.
lgtm
kouhei@chromium.org changed reviewers: + kouhei@chromium.org
lgtm
Rebased. Updated the CL description to say that this adds internalAbort() to handleDidCancel().
The CQ bit was checked by tyoshino@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/490083002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as 181791 |
