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
_______________________________________________
webkit-dev mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
