WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
25215
Support loading script for nested workers.
https://bugs.webkit.org/show_bug.cgi?id=25215
Summary
Support loading script for nested workers.
Jian Li
Reported
2009-04-15 11:34:50 PDT
Support loading script for nested workers.
Attachments
Proposed Patch
(7.28 KB, patch)
2009-04-15 11:48 PDT
,
Jian Li
no flags
Details
Formatted Diff
Diff
Proposed Patch
(7.67 KB, patch)
2009-04-17 13:20 PDT
,
Jian Li
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Jian Li
Comment 1
2009-04-15 11:48:28 PDT
Created
attachment 29509
[details]
Proposed Patch
Alexey Proskuryakov
Comment 2
2009-04-16 04:01:32 PDT
Comment on
attachment 29509
[details]
Proposed Patch 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.
Jian Li
Comment 3
2009-04-16 11:24:27 PDT
(In reply to
comment #2
)
> (From update of
attachment 29509
[details]
[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
[details]
[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. >
Alexey Proskuryakov
Comment 4
2009-04-17 08:44:31 PDT
> Who can I ping to consult with this design? Sending an email to webkit-dev?
Yes, an e-mail to webkit-dev sounds fine.
> This is the full implementation of script loading for both workers and nested > workers, though it is one of steps to support nested workers.
How does it fix a FIXME in WorkerThreadableLoader::MainThreadBridge::mainThreadCreateLoader? The current implementation of ThreadableLoader doesn't work in nested workers.
> 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.
As you imply, this comment would not be helpful. The purpose of comments is two-fold. First, they let people not directly involved with implementation of a feature to follow the progress easily, and learn new concepts. Second, they help when investigating bugs introduced by a patch - if a particular change has bad side effects, the person fixing the code needs to know its primary purpose in order to not defeat it when fixing side effects. This patch is not trivial, and it definitely needs a detailed ChangeLog. You should mention at least the following: - Worker loading now uses ThreadableLoader instead of CachedScript, because the latter is not thread safe. Thus, it implements a different set of callbacks; - terminate() now has a different effect on loading; - there may be little to say about didFail(), but a comment for didFailRedirectCheck() can explain how worker loading is supposed to handle redirects; - a comment for abortLoading() should explain when it is supposed to be called - e.g. it's very surprising that it is called from didFail() in your patch; - anything else that required your special attention, or that could confuse others reading your patch.
Jian Li
Comment 5
2009-04-17 13:20:10 PDT
Created
attachment 29588
[details]
Proposed Patch
Jian Li
Comment 6
2009-04-17 13:25:55 PDT
(In reply to
comment #4
)
> > Who can I ping to consult with this design? Sending an email to webkit-dev? > > Yes, an e-mail to webkit-dev sounds fine.
I've sent an email to webkit-dev.
> > > This is the full implementation of script loading for both workers and nested > > workers, though it is one of steps to support nested workers. > > How does it fix a FIXME in > WorkerThreadableLoader::MainThreadBridge::mainThreadCreateLoader? The current > implementation of ThreadableLoader doesn't work in nested workers. >
It seems that the inappropriate asserts prevent WorkerThreadableLoader from working in nested workers. I changed the asserts and update the FIXME. As pointed out in FIXME, the current implementation of WorkerThreadableLoader is not the final elegant solution, but it just works for script loading in nested workers. I think levin is working on improving it.
> > 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. > > As you imply, this comment would not be helpful. > > The purpose of comments is two-fold. First, they let people not directly > involved with implementation of a feature to follow the progress easily, and > learn new concepts. Second, they help when investigating bugs introduced by a > patch - if a particular change has bad side effects, the person fixing the code > needs to know its primary purpose in order to not defeat it when fixing side > effects. > > This patch is not trivial, and it definitely needs a detailed ChangeLog. You > should mention at least the following: > - Worker loading now uses ThreadableLoader instead of CachedScript, because the > latter is not thread safe. Thus, it implements a different set of callbacks; > - terminate() now has a different effect on loading; > - there may be little to say about didFail(), but a comment for > didFailRedirectCheck() can explain how worker loading is supposed to handle > redirects; > - a comment for abortLoading() should explain when it is supposed to be called > - e.g. it's very surprising that it is called from didFail() in your patch; > - anything else that required your special attention, or that could confuse > others reading your patch. >
I updated the ChangeLog. Thank you very much for helping me on this. The reason for didFail() calling abortLoading() is to cleanup the loading resource since I use abortLoading to server dual purposes: abort loading and/or cleanup resources.
Chris Dumez
Comment 7
2022-09-17 19:39:39 PDT
https://commits.webkit.org/254597@main
Radar WebKit Bug Importer
Comment 8
2022-09-17 19:41:46 PDT
<
rdar://problem/100079583
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug