Discussion:
Issue 518855 in chromium: Split the constructor of ThreadableLoader into ctor and start() method
(too old to reply)
c***@googlecode.com
2015-08-10 14:55:38 UTC
Permalink
Raw Message
Status: Started
Owner: ***@chromium.org
CC: ***@chromium.org
Labels: Type-Bug Pri-2 Cr-Blink-Loader OS-All

New issue 518855 by ***@chromium.org: Split the constructor of
ThreadableLoader into ctor and start() method
https://code.google.com/p/chromium/issues/detail?id=518855

Currently, ThreadableLoader::create() can return null or call didFail()
synchronously when something wrong (e.g. mixed content blocking).
However, this might be problematic because:
- Some call sites of create() lacks null checks.
- We usually do like:
m_loader = ThreadableLoader::create(executionContext, this, ...);
but if this->didFail() is called inside create(), m_loader is not yet set
at that point.
- Also it's not good to do much work in a constructor.

This issue resolves this problem by splitting the constructor into a new
trivial constructor and start():
- The constructor and create() do not call ThreadableLoaderClient methods.
- create() always return non-null.

CL by tyoshino@ is under review: https://codereview.chromium.org/1264453002/

Moving ThreadableLoaderClient* argument from the constructor/create() to
start() will be clearer, i.e. it explicitly states that
ThreadableLoaderClient methods are never called from create(), but it
requires code restructure, thus will be not included in the CL.
--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings
--
--
Automated mail from issue updates at http://crbug.com/
Subscription options: http://groups.google.com/a/chromium.org/group/chromium-bugs

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-bugs+***@chromium.org.
c***@googlecode.com
2015-08-10 15:33:44 UTC
Permalink
Raw Message
Issue 518855: Split the constructor of ThreadableLoader into ctor and
start() method
https://code.google.com/p/chromium/issues/detail?id=518855

This issue is now blocking issue chromium:514547.
See https://code.google.com/p/chromium/issues/detail?id=514547

--
You received this message because you are listed in the owner
or CC fields of this issue, or because you starred this issue.
You may adjust your issue notification preferences at:
http://code.google.com/hosting/settings
--
--
Automated mail from issue updates at http://crbug.com/
Subscription options: http://groups.google.com/a/chromium.org/group/chromium-bugs

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-bugs+***@chromium.org.
c***@googlecode.com
2016-02-16 09:09:27 UTC
Permalink
Raw Message
Comment #2 on issue 518855 by ***@chromium.org: Split the constructor
of ThreadableLoader into ctor and start() method
https://code.google.com/p/chromium/issues/detail?id=518855#c2

The following revision refers to this bug:

https://chromium.googlesource.com/chromium/src.git/+/f2a0d7de0ca23cb14148af488c18f8638a3976a8

commit f2a0d7de0ca23cb14148af488c18f8638a3976a8
Author: tyoshino <***@chromium.org>
Date: Tue Feb 16 09:04:34 2016

Split the constructor of ThreadableLoader into two methods (ctor and
start())

Reasons:
- Basically, it's not good to do much work in a constructor.
- DocumentThreadableLoader may fail synchronously, and then
m_client->didFail() will be called. The design that m_client is passed
to the constructor means that the client should be ready for
callbacks (didFail(), etc.) when calling the constructor. It's better
if both didFail() call by network thread and the sync one are handled
by single logic. However, in the sync callback case, the loader
instance is not yet created. This difference gives us some unexpected
complexity in the logic to be implemented in didFail().
- Splitting the constructor into a new trivial constructor and start()
method allows finishing association between the loader and the client
without worry about failure.

This CL simplifies EventSource code as m_requestInFlight is unnecessary.

start() takes a ResourceRequest as it's good to keep the current design
that we don't hold ResourceRequest in a member variable.

BUG=518855,514547,389326

Review URL: https://codereview.chromium.org/1264453002

Cr-Commit-Position: refs/heads/master@{#375537}

[add]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-async-post-xhr-blocked.html
[add]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-fetch-blocked.html
[add]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-async-xhr-post.html
[add]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-fetch.html
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/core/loader/MockThreadableLoader.h
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/core/loader/ThreadableLoader.cpp
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/core/loader/ThreadableLoader.h
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/core/page/EventSource.cpp
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/core/page/EventSource.h
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/core/page/EventSourceTest.cpp
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/core/workers/WorkerScriptLoader.cpp
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/modules/fetch/FetchBlobDataConsumerHandle.cpp
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/modules/fetch/FetchBlobDataConsumerHandle.h
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/modules/fetch/FetchManager.cpp
[modify]
http://crrev.com/f2a0d7de0ca23cb14148af488c18f8638a3976a8/third_party/WebKit/Source/web/AssociatedURLLoader.cpp
--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings
--
--
Automated mail from issue updates at http://crbug.com/
Subscription options: http://groups.google.com/a/chromium.org/group/chromium-bugs

---
You received this message because you are subscribed to the Google Groups "Chromium-bugs" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-bugs+***@chromium.org.
c***@googlecode.com
2016-02-18 09:21:59 UTC
Permalink
Raw Message
Updates:
Status: Fixed
Labels: Cr-Blink-Network-XHR

Comment #3 on issue 518855 by ***@chromium.org: Split the constructor
of ThreadableLoader into ctor and start() method
https://code.google.com/p/chromium/issues/detail?id=518855

Done!
--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings
--
--
Automated mail from issue updates at http://crbug.com/
Subscription options: http://groups.google.com/a/chromium.org/group/chromium-bugs

---
You received this message because you are subscribed to the Google Groups "Chromium-bugs" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-bugs+***@chromium.org.
Loading...