Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(408)

Issue 340363003: Let ThreadableLoaderClientWrapper's client handle failures better. (Closed)

Created:
6 years, 6 months ago by sof
Modified:
6 years, 6 months ago
CC:
blink-reviews, eseidel, gavinp+loader_chromium.org, horo, Nate Chapin, kinuko
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Let ThreadableLoaderClientWrapper's client handle failures better. If the ThreadableLoaderClientWrapper is notified of a redirect or CORS access check failure, it forwards that notification to its client. Do that without first moving the wrapper object to a 'done' state, so as to allow the client to fully handle the failure -- the worker threadable loader will for instance signal an error-cancellation failure back at the wrapper. R=abarth@chromium.org,tyoshino@chromium.org BUG=386520 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176945

Patch Set 1 #

Patch Set 2 : Include only directly-relevant tests in this CL #

Patch Set 3 : Add CSP error-on-redirect test + include other CORS tests #

Patch Set 4 : Move all tests to a separate CL; functional change only. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -2 lines) Patch
M Source/core/loader/ThreadableLoaderClientWrapper.h View 1 chunk +5 lines, -2 lines 2 comments Download

Messages

Total messages: 24 (0 generated)
sof
When you have the time, please take a look.
6 years, 6 months ago (2014-06-19 10:46:46 UTC) #1
abarth-chromium
This CL is difficult to review because you're making a large change to the tests ...
6 years, 6 months ago (2014-06-19 16:03:57 UTC) #2
abarth-chromium
One that refactors the tests and one that makes the behavior change?
6 years, 6 months ago (2014-06-19 16:04:12 UTC) #3
sof
On 2014/06/19 16:04:12, abarth wrote: > One that refactors the tests and one that makes ...
6 years, 6 months ago (2014-06-19 16:22:18 UTC) #4
sof
On 2014/06/19 16:22:18, sof wrote: > On 2014/06/19 16:04:12, abarth wrote: > > One that ...
6 years, 6 months ago (2014-06-19 20:25:07 UTC) #5
sof
Changed the redirect test to trigger a failure on CSP-redirect, improving coverage slightly. With https://codereview.chromium.org/347043002/ ...
6 years, 6 months ago (2014-06-20 21:06:56 UTC) #6
sof
So as to (hopefully) make progress, I've shifted the CORS tests to a separate CL ...
6 years, 6 months ago (2014-06-22 13:27:57 UTC) #7
abarth-chromium
https://codereview.chromium.org/340363003/diff/60001/Source/core/loader/ThreadableLoaderClientWrapper.h File Source/core/loader/ThreadableLoaderClientWrapper.h (right): https://codereview.chromium.org/340363003/diff/60001/Source/core/loader/ThreadableLoaderClientWrapper.h#newcode105 Source/core/loader/ThreadableLoaderClientWrapper.h:105: m_done = true; Is the client allowed to destroy ...
6 years, 6 months ago (2014-06-22 14:48:26 UTC) #8
sof
https://codereview.chromium.org/340363003/diff/60001/Source/core/loader/ThreadableLoaderClientWrapper.h File Source/core/loader/ThreadableLoaderClientWrapper.h (right): https://codereview.chromium.org/340363003/diff/60001/Source/core/loader/ThreadableLoaderClientWrapper.h#newcode105 Source/core/loader/ThreadableLoaderClientWrapper.h:105: m_done = true; On 2014/06/22 14:48:26, abarth wrote: > ...
6 years, 6 months ago (2014-06-22 15:49:14 UTC) #9
abarth-chromium
On 2014/06/22 at 15:49:14, sigbjornf wrote: > Does that make sense? Yeah. I need to ...
6 years, 6 months ago (2014-06-22 20:32:27 UTC) #10
sof
On 2014/06/22 20:32:27, abarth wrote: > On 2014/06/22 at 15:49:14, sigbjornf wrote: > > Does ...
6 years, 6 months ago (2014-06-23 07:59:36 UTC) #11
sof
Cc:ed @horo and @kinuko, who may have looked at lifetime details of this wrapper object ...
6 years, 6 months ago (2014-06-23 08:04:50 UTC) #12
tyoshino (SeeGerritForStatus)
Can't we fix EventSource than this change? Calling m_loader->cancel() from didFailAccessControlCheck() looks weird and maybe ...
6 years, 6 months ago (2014-06-24 12:33:39 UTC) #13
sof
On 2014/06/24 12:33:39, tyoshino wrote: > Can't we fix EventSource than this change? Calling m_loader->cancel() ...
6 years, 6 months ago (2014-06-24 12:41:57 UTC) #14
tyoshino (SeeGerritForStatus)
On 2014/06/24 12:41:57, sof wrote: > On 2014/06/24 12:33:39, tyoshino wrote: > > Can't we ...
6 years, 6 months ago (2014-06-24 18:00:45 UTC) #15
tyoshino (SeeGerritForStatus)
On 2014/06/24 18:00:45, tyoshino wrote: > information in the DocumentThreadableLoader the cancel() call is utilizing? ...
6 years, 6 months ago (2014-06-24 18:01:53 UTC) #16
sof
On 2014/06/24 18:00:45, tyoshino wrote: > On 2014/06/24 12:41:57, sof wrote: > > On 2014/06/24 ...
6 years, 6 months ago (2014-06-24 19:55:58 UTC) #17
tyoshino (SeeGerritForStatus)
On 2014/06/24 19:55:58, sof wrote: > On 2014/06/24 18:00:45, tyoshino wrote: > > On 2014/06/24 ...
6 years, 6 months ago (2014-06-25 01:07:15 UTC) #18
sof
abarth: what do you think - had a chance to look into needed details yet?
6 years, 6 months ago (2014-06-25 08:02:47 UTC) #19
abarth-chromium
On 2014/06/25 at 08:02:47, sigbjornf wrote: > abarth: what do you think - had a ...
6 years, 6 months ago (2014-06-25 19:21:59 UTC) #20
sof
On 2014/06/25 19:21:59, abarth wrote: > On 2014/06/25 at 08:02:47, sigbjornf wrote: > > abarth: ...
6 years, 6 months ago (2014-06-25 19:36:16 UTC) #21
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 6 months ago (2014-06-25 19:42:31 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/340363003/60001
6 years, 6 months ago (2014-06-25 19:43:28 UTC) #23
commit-bot: I haz the power
6 years, 6 months ago (2014-06-25 20:28:50 UTC) #24
Message was sent while issue was closed.
Change committed as 176945

Powered by Google App Engine
This is Rietveld 408576698