Make ThreadableLoader class to use references

Make ThreadableLoader class to use references

ExecutionContext of ThreadableLoader class can not be null.
Move to use references to make caller code safer and remove
extra null checks. Keeping the existing interfaces as is to
create a new CL for changing remaining clients of ThreadableLoader.
Makes this patch smaller and reviewable.

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

[maheshkk, 2014-03-21 21:20:18]

There are three more clients implements ThreaderLoader and I will move the code
to reference and delete the old interfaces in a new CL.

PTAL

[Inactive, 2014-03-22 15:09:54]

https://codereview.chromium.org/204983007/diff/20001/Source/core/loader/Threa...
File Source/core/loader/ThreadableLoader.h (right):

https://codereview.chromium.org/204983007/diff/20001/Source/core/loader/Threa...
Source/core/loader/ThreadableLoader.h:86: static void
loadResourceSynchronously(ExecutionContext* context, const ResourceRequest&
request, ThreadableLoaderClient& client, const ThreadableLoaderOptions& options)
Do we really need to add these? Can't we simply fix the callers? I don't like
these, especially without an assertion making sure context is non-null.

https://codereview.chromium.org/204983007/diff/20001/Source/core/loader/Worke...
File Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/204983007/diff/20001/Source/core/loader/Worke...
Source/core/loader/WorkerThreadableLoader.cpp:122: Document& document =
toDocument(*context);
Dereferencing context without a assertion to make sure it is non-null looks
unsafe. Why can't we update this method to make the ExecutionContext by
reference?

https://codereview.chromium.org/204983007/diff/20001/Source/core/loader/Worke...
File Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/204983007/diff/20001/Source/core/loader/Worke...
Source/core/loader/WorkerThreadableLoader.h:137: WorkerGlobalScope&
m_workerGlobalScope;
This changes behavior. We no longer hold a strong reference to the
workerGlobalScope and this is likely not OK. This needs to remain a RefPtr

[Inactive, 2014-03-22 15:13:05]

https://codereview.chromium.org/204983007/diff/20001/Source/web/AssociatedURL...
File Source/web/AssociatedURLLoader.cpp (right):

https://codereview.chromium.org/204983007/diff/20001/Source/web/AssociatedURL...
Source/web/AssociatedURLLoader.cpp:350: // FIXME: We can not create
DocumentThreadableLoader if document does not exist for the frame.
Why the FIXME? The assertion should suffice. The code would have already hit the
assertion in the DocumentThreadableLoader constructor before your patch if the
document was null.

[maheshkk, 2014-03-24 18:17:48]

PTAL

https://codereview.chromium.org/204983007/diff/20001/Source/core/loader/Threa...
File Source/core/loader/ThreadableLoader.h (right):

https://codereview.chromium.org/204983007/diff/20001/Source/core/loader/Threa...
Source/core/loader/ThreadableLoader.h:86: static void
loadResourceSynchronously(ExecutionContext* context, const ResourceRequest&
request, ThreadableLoaderClient& client, const ThreadableLoaderOptions& options)
On 2014/03/22 15:09:54, Chris Dumez wrote:
> Do we really need to add these? Can't we simply fix the callers? I don't like
> these, especially without an assertion making sure context is non-null.

We can fix other callers, I was trying to minimize the patch for review by
splitting it in to two patches. But I realized later my second patch changed
about 4 files and I think its not too bad to include the changes in one. I will
do that.

https://codereview.chromium.org/204983007/diff/20001/Source/core/loader/Worke...
File Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/204983007/diff/20001/Source/core/loader/Worke...
Source/core/loader/WorkerThreadableLoader.cpp:122: Document& document =
toDocument(*context);
On 2014/03/22 15:09:54, Chris Dumez wrote:
> Dereferencing context without a assertion to make sure it is non-null looks
> unsafe. Why can't we update this method to make the ExecutionContext by
> reference?

WorkerThreadableLoader::MainThreadBridge implements callbacks CrossThreadTask
callbacks. CrossThreadTask signatures takes pointers and so MainThreadBridge
needs to be this way for now. I will look at CrossThreadTask later.

https://codereview.chromium.org/204983007/diff/20001/Source/core/loader/Worke...
File Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/204983007/diff/20001/Source/core/loader/Worke...
Source/core/loader/WorkerThreadableLoader.h:137: WorkerGlobalScope&
m_workerGlobalScope;
On 2014/03/22 15:09:54, Chris Dumez wrote:
> This changes behavior. We no longer hold a strong reference to the
> workerGlobalScope and this is likely not OK. This needs to remain a RefPtr

Good point. I will leave this a RefPtr.

https://codereview.chromium.org/204983007/diff/20001/Source/web/AssociatedURL...
File Source/web/AssociatedURLLoader.cpp (right):

https://codereview.chromium.org/204983007/diff/20001/Source/web/AssociatedURL...
Source/web/AssociatedURLLoader.cpp:350: // FIXME: We can not create
DocumentThreadableLoader if document does not exist for the frame.
On 2014/03/22 15:13:06, Chris Dumez wrote:
> Why the FIXME? The assertion should suffice. The code would have already hit
the
> assertion in the DocumentThreadableLoader constructor before your patch if the
> document was null.
 Done.

[Inactive, 2014-03-31 17:43:25]

https://codereview.chromium.org/204983007/diff/60001/Source/core/fileapi/File...
File Source/core/fileapi/FileReaderLoader.cpp (right):

https://codereview.chromium.org/204983007/diff/60001/Source/core/fileapi/File...
Source/core/fileapi/FileReaderLoader.cpp:125: ASSERT(executionContext);
Is it a lot of work to update the callers?

https://codereview.chromium.org/204983007/diff/60001/Source/core/fileapi/File...
Source/core/fileapi/FileReaderLoader.cpp:132: ASSERT(executionContext);
Ditto.

https://codereview.chromium.org/204983007/diff/60001/Source/core/page/EventSo...
File Source/core/page/EventSource.cpp (right):

https://codereview.chromium.org/204983007/diff/60001/Source/core/page/EventSo...
Source/core/page/EventSource.cpp:128: ExecutionContext* executionContextLocal =
executionContext();
I don't really like the "*Local" naming. I guess you could do:
ExecutionContext* executionContext = this->executionContext();

But why do we even introduce a local variable here?

https://codereview.chromium.org/204983007/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerScriptLoader.cpp (right):

https://codereview.chromium.org/204983007/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerScriptLoader.cpp:58: m_url = url;
We should probably add an ASSERT(executionContext) here.

https://codereview.chromium.org/204983007/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerScriptLoader.cpp:77: ASSERT(client);
We should probably add an ASSERT(executionContext) here.

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

https://codereview.chromium.org/204983007/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:795: ExecutionContext* executionContextLocal
= executionContext();
Same comment about naming / local variable.

[maheshkk, 2014-03-31 20:46:48]

PTAL

https://codereview.chromium.org/204983007/diff/60001/Source/core/fileapi/File...
File Source/core/fileapi/FileReaderLoader.cpp (right):

https://codereview.chromium.org/204983007/diff/60001/Source/core/fileapi/File...
Source/core/fileapi/FileReaderLoader.cpp:125: ASSERT(executionContext);
On 2014/03/31 17:43:26, Chris Dumez wrote:
> Is it a lot of work to update the callers?

I have changed the startInternal to use reference. Most of this class code now
uses reference for executeContext. 

There are 8 different callers to start() and will upload a different patch to
limit the number of files for this patch to review.

https://codereview.chromium.org/204983007/diff/60001/Source/core/page/EventSo...
File Source/core/page/EventSource.cpp (right):

https://codereview.chromium.org/204983007/diff/60001/Source/core/page/EventSo...
Source/core/page/EventSource.cpp:128: ExecutionContext* executionContextLocal =
executionContext();
On 2014/03/31 17:43:26, Chris Dumez wrote:
> I don't really like the "*Local" naming. I guess you could do:
> ExecutionContext* executionContext = this->executionContext();
> 

Thanks! changed it as per your suggestion.  

> But why do we even introduce a local variable here?

Local variable is to limit calling the getter multiple times.

https://codereview.chromium.org/204983007/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerScriptLoader.cpp (right):

https://codereview.chromium.org/204983007/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerScriptLoader.cpp:58: m_url = url;
On 2014/03/31 17:43:26, Chris Dumez wrote:
> We should probably add an ASSERT(executionContext) here.

I have moved this method and all the callers to use reference.

https://codereview.chromium.org/204983007/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerScriptLoader.cpp:77: ASSERT(client);
On 2014/03/31 17:43:26, Chris Dumez wrote:
> We should probably add an ASSERT(executionContext) here.

I have moved this method and all the callers to use reference.

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

https://codereview.chromium.org/204983007/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:795: ExecutionContext* executionContextLocal
= executionContext();
On 2014/03/31 17:43:26, Chris Dumez wrote:
> Same comment about naming / local variable.

Done.

[Inactive, 2014-04-01 13:25:04]

lgtm for core/ with 1 nit.

Someone else needs to look at web/.

https://codereview.chromium.org/204983007/diff/80001/Source/core/page/EventSo...
File Source/core/page/EventSource.cpp (right):

https://codereview.chromium.org/204983007/diff/80001/Source/core/page/EventSo...
Source/core/page/EventSource.cpp:128: ExecutionContext* executionContext =
this->executionContext();
nit: We should probably add an assert after this assignment to make sure it is
non-null.

[maheshkk, 2014-04-02 16:50:39]

AdamK, could you please review web/ changes?

[adamk, 2014-04-02 17:16:41]

Just a few nits, lgtm once those are taken care of.

https://codereview.chromium.org/204983007/diff/80001/Source/core/page/EventSo...
File Source/core/page/EventSource.cpp (right):

https://codereview.chromium.org/204983007/diff/80001/Source/core/page/EventSo...
Source/core/page/EventSource.cpp:128: ExecutionContext* executionContext =
this->executionContext();
On 2014/04/01 13:25:04, Chris Dumez wrote:
> nit: We should probably add an assert after this assignment to make sure it is
> non-null.

Seems like the nicest-looking change is:

ASSERT(executionContext());
ExecutionContext& executionContext = *this->executionContext();

https://codereview.chromium.org/204983007/diff/80001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/204983007/diff/80001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.cpp:216: ASSERT(executionContext);
Same here as other comment, might as well do these in the reverse order (ASSERT,
then assign to a reference).

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

https://codereview.chromium.org/204983007/diff/80001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:796: ASSERT(executionContext);
Same here.

[maheshkk, 2014-04-04 21:11:02]

Thanks Chris and Adam for review!

[maheshkk, 2014-04-04 21:11:09]

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

[commit-bot: I haz the power, 2014-04-04 21:11:14]

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

[commit-bot: I haz the power, 2014-04-04 21:20:41]

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

[commit-bot: I haz the power, 2014-04-04 21:20:42]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[abarth-chromium, 2014-04-04 21:37:32]

The CQ bit was checked by abarth@chromium.org

[maheshkk, 2014-04-04 21:38:01]

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

[commit-bot: I haz the power, 2014-04-04 21:38:18]

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

[commit-bot: I haz the power, 2014-04-04 22:22:47]

Change committed as 170906