[Webkit-unassigned] [Bug 25215] Support loading script for nested workers.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=25215





------- Comment #3 from jianli at chromium.org  2009-04-16 11:24 PDT -------
(In reply to comment #2)
> (From update of attachment 29509 [review])
> This means that workers will no longer use CachedResource machinery. I'm not
> sure how bad it is, you may need to consult with someone familiar with its
> design.
Who can I ping to consult with this design? Sending an email to webkit-dev?
> 
> Why is m_loadingAborted necessary (given that we have
> WorkerMessagingProxy::m_askedToTerminate)? Does checking it affect visible
> behavior of the worker? Does this change need a test?
I think it will be better to abort the script loading as early as possible when
Worker::terminate() triggers. This should not cause any visible change to
worker behavior because we will not call WorkerContextProxy::startWorkerContext
when worker is asked to be terminated and then the script loading is finished.
This is same as the check in WorkerMessagingProxy::startWorkerContext.

WorkerMessagingProxy::m_askedToTerminate is not exposed to WorkerContextProxy
interface that can be accessed from Worker. I would prefer use a direct flag
here since all the logic is in Worker scope.
> 
> This patch doesn't have sufficient information in ChangeLog - it doesn't even
> say that it is not a full implementation of the feature, but just a first step.
> Ideally, each modified function should have a comment of its own.
This is the full implementation of script loading for both workers and nested
workers, though it is one of steps to support nested workers. Do you want me to
say something like this in ChangeLog.

I just ran prepareChangeLog to create the change log. Do you want me to add
comments to modified functions in ChangeLog like the following:
   (WebCore::Worker::didFail): Called when the loading fails.
> 

(In reply to comment #2)
> (From update of attachment 29509 [review])
> This means that workers will no longer use CachedResource machinery. I'm not
> sure how bad it is, you may need to consult with someone familiar with its
> design.
> 
> Why is m_loadingAborted necessary (given that we have
> WorkerMessagingProxy::m_askedToTerminate)? Does checking it affect visible
> behavior of the worker? Does this change need a test?
> 
> This patch doesn't have sufficient information in ChangeLog - it doesn't even
> say that it is not a full implementation of the feature, but just a first step.
> Ideally, each modified function should have a comment of its own.
> 


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.