[webkit-dev] New loader class to allow workers to do xhr

[Darin Adler]

I have a few thoughts on this. The general approach seems OK.

On Dec 30, 2008, at 11:11 AM, David Levin wrote:

>     class ScriptResourceLoader {

I'm not sure "Script" is the right word here, but I don't have a  
better one. Up until now Script has meant "interface to the JavaScript  
interpreter" rather than objects outside that that are intended for  
use by script. But prefixes like "Programmatic" are uglier, so maybe  
we should stick with Script. Lets see if we can  think of a better  
prefix.

How were you planning on handling synchronous loads? Maybe the  
function for that should be here too as a static member function?

>     public:
>         static PassRefPtr<ScriptResourceLoader>  
> create(ScriptExecutionContext*, ScriptResourceLoaderClient*, const  
> ResourceRequest&, bool skipCanLoadCheck, bool  
> sendResourceLoadCallbacks, bool shouldContentSniff);

Despite their use in existing code I think booleans are a lousy way to  
handle options like these.

I think we can omit skipCanLoadCheck, since it's always false for  
XMLHttpRequest. For the other two I would prefer something easier to  
read at the call site, either named enums or a flags word. The best  
example I could find of the named enum approach is EChildrenOnly in  
markup.h, although the use of an "E" prefix isn't desirable. I  
couldn't find a good example of a flags word.

But this may be in appropriate, since we already use booleans for this  
purpose and you're just refactoring.

>         virtual ~ScriptResourceLoader() {}

ScriptResourceLoader should have a protected destructor instead of a  
public one. There's no legitimate reason for someone who created an  
object using create to call the destructor. Instead only the deref  
function should do that, and since it's virtual it will be a deref  
function specific to a particular derived class.

>         virtual void ref() = 0;
>         virtual void deref() = 0;

I'll need to see more of a design to be sure that the virtual ref/ 
deref design is the best one for this. I understand it can be helpful  
to use ThreadSafeShared rather than RefCounted (or whatever those  
classes are named), but there are other approaches too; it's basically  
an implementation detail, though, not really part of the interface.  
I'll wait to see work that's further along.

>         virtual ~ScriptResourceLoaderClient() {}

This destructor should be protected rather than public. Nobody should  
ever delete a ScriptResourceLoaderClient given a pointer of that  
class. I believe the lifetime rule for these objects is that they are  
responsible for deleting themselves. That's a little hard to program  
with, but if we're sticking with it then the protected destructor  
helps make it clear. But the rules about when it's safe/needed for a  
client to delete itself should be defined as clearly as possible. I  
believe that certain client functions are guaranteed to be the last  
one received, and also if you call cancel() you're guaranteed to not  
get any more client function calls, but I'm not sure. This kind of  
lifetime rule is one of the most important things about an interface  
like this. I presume you'll just keep the current design.

>         virtual void didSendData(unsigned long long bytesSent,  
> unsigned long long totalBytesToBeSent) { }

It's a little strange that each didSendData call repeatedly passes the  
total bytes to be sent; not terrible, but strange. Like other aspects  
of this class it's no doubt just echoing the existing resource loader  
design.

>         virtual void didReceiveData(const char*, int) { }

I think the "int" here needs an argument name to make clear it's the  
size of the data.

>         virtual void didFinishLoading(int resourceIdentifer) { }

Does XMLHttpRequest need to know about the resource identifier? If so,  
how could it possibly know which identifiers to expect? Should we just  
omit that from this interface? Or is this part of the inspector  
interface? It's pretty confusing with the current name, I think.

>         virtual void didFail(bool isCancellation) { }

I think that the cancellation and non-cancellation functions should be  
separate rather than having a single call with a boolean. Like other  
aspects of this class it's no doubt just echoing the existing resource  
loader design.

>         virtual void securityOriginDenyRequest() { }

You're adding a new call here to indicate that the request was denied  
due to the security origin. It's important to use the past tense, so  
it's clear you're not telling the client to deny the request, but  
rather notifying the client that the request was already denied. A  
name like "didFailDueToSecurityOrigin" might be appropriate to fit in  
with the other client calls.

Also, the name needs to express what's different about this failure  
that matters to the client. The practical difference in XMLHttpRequest  
seems to be only the call to internalAbort(), so the fact that this  
failure is due to a security origin check may not be the salient  
difference. It's possible that the important difference is the timing  
of the failure and the name should reflect that. Or maybe all failures  
could call internalAbort(), in which case this can just be didFail()  
and not require a separate function.

>         virtual void receivedCancellation(const ResourceResponse&) { }

This needs "authentication" in its name; the original function  
arguably doesn't need that in its name because its  
AuthenticationChallenge argument makes it clear what kind of  
cancellation we are hearing about.

I don't understand our mix of "did" in some calls and "received" in  
others. We should be more consistent in our naming. If "did" is our  
way to emphasize this is a client function then it should be  
"didReceiveAuthenticationCancellation" or "didCancelAuthentication".

We could dump the "did" prefix if we have a better naming scheme for  
client functions as a group.

     -- Darin