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.
Created attachment 192972 [details] Patch v1
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.
(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!
http://trac.webkit.org/changeset/145753, but I accidentally hadn't git commited the comment change. Will follow up.
http://trac.webkit.org/changeset/145755 to fix the comment.