Bug 112279 – Small ResourceLoader cleanups

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Brady Eidson]

Two small ResourceLoader cleanups I noticed were worthwhile while working on a larger patch:

1 - Classes that derive from ResourceLoader should use OVERRIDE on the virtual methods they override.  Important for catching mistakes in future refactoring.
2 - the "bool allAtOnce" flag is mysterious when reading code that has just "true" or "false", so change it to an enum that encapsulates what it means.

[Brady Eidson]

Created attachment 192972 [details]
Patch v1

[Geoffrey Garen]

Comment on attachment 192972 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=192972&action=review

r=me

> Source/WebCore/loader/ResourceLoaderTypes.h:27
> +#ifndef ResourceLoaderTypes_h
> +#define ResourceLoaderTypes_h

Are more types going in this header? If not, it should be named "DataPayloadType.h".

> Source/WebCore/loader/ResourceLoaderTypes.h:32
> +//  - DataPayloadWholeResource is expected when the buffer points to a whole resource. There will only be one such didReceiveData callback for the load.

"is expected" is a little strong in this context. DataPayloadWholeResource is an optimization that happens sometimes. We don't have a specific expectation about how often. I would say, "DataPayloadWholeResource indicates that the buffer points...".

> Source/WebCore/loader/ResourceLoaderTypes.h:33
> +//  - DataPayloadBytes is expected when the buffer points to a range of bytes, which may or may not be a whole resource.

I would use "indicates" here as well.

[Brady Eidson]

(In reply to comment #2)
> (From update of attachment 192972 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192972&action=review
> 
> r=me
> 
> > Source/WebCore/loader/ResourceLoaderTypes.h:27
> > +#ifndef ResourceLoaderTypes_h
> > +#define ResourceLoaderTypes_h
> 
> Are more types going in this header? If not, it should be named "DataPayloadType.h".

At least one other will be in the extremely near future.

> 
> > Source/WebCore/loader/ResourceLoaderTypes.h:32
> > +//  - DataPayloadWholeResource is expected when the buffer points to a whole resource. There will only be one such didReceiveData callback for the load.
> 
> "is expected" is a little strong in this context. DataPayloadWholeResource is an optimization that happens sometimes. We don't have a specific expectation about how often. I would say, "DataPayloadWholeResource indicates that the buffer points...".
> 
> > Source/WebCore/loader/ResourceLoaderTypes.h:33
> > +//  - DataPayloadBytes is expected when the buffer points to a range of bytes, which may or may not be a whole resource.
> 
> I would use "indicates" here as well.

Done and done.

Thanks!

[Brady Eidson]

http://trac.webkit.org/changeset/145753, but I accidentally hadn't git commited the comment change.  Will follow up.

[Brady Eidson]

http://trac.webkit.org/changeset/145755 to fix the comment.