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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Apr 17 13:25:55 PDT 2009


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





------- Comment #6 from jianli at chromium.org  2009-04-17 13:25 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.


-- 
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.



More information about the webkit-unassigned mailing list