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