[webkit-reviews] review denied: [Bug 32167] Extend ResourceRequest::TargetType to be more specific : [Attachment 44336] First patch.

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Mike Belshe
<mike at belshe.com>'s request for review:
Bug 32167: Extend ResourceRequest::TargetType to be more specific
https://bugs.webkit.org/show_bug.cgi?id=32167

Attachment 44336: First patch.
https://bugs.webkit.org/attachment.cgi?id=44336&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebCore/ChangeLog

> +	   Bug 32167 - Update the ResourceRequest::RequestType.  This
previously
> +	   was specific to Chromium.  Moved into ResourceRequestBase, enabling
> +	   more specificity about the type (which is otherwise only known to
the
> +	   loader), and also making this information available to all
platforms.
> +	   Any platform with a network layer which can utilize this information

> +	   may want to use it for prioritization.

nit: Bug XXX should be a https://bugs.webkit.org/ link so that it is clickable
from trac.webkit.org.


> Index: WebCore/loader/loader.cpp
...
> +ResourceRequestBase::TargetType
CachedResourceTypeToTargetType(CachedResource::Type type)

nit: webkit style has functions starting with a lowercase letter.

nit: i think it should be okay to refer to ResourceRequest::TargetType.
we generally avoid referring to the *Base class since it is actually
just an implementation detail (to allow for code sharing).


> +	   return ResourceRequest::TargetIsStyleSheet;

^^^ here you just use ResourceRequest::, which is nice.


> Index: WebCore/platform/network/ResourceRequestBase.h

> +	   enum TargetType {
> +	       TargetIsMainFrame,     // Resource is a main frame.
> +	       TargetIsSubFrame,      // Resource is a sub frame.
> +	       TargetIsSubResource,   // Resource is a generic subresource. 
(Generally a specific type should be specified)
> +	       TargetIsStyleSheet,    // A stylesheet subresource.
> +	       TargetIsScript,	      // A script subresource.
> +	       TargetIsFontResource,  // A font subresource.
> +	       TargetIsImage,	      // An image subresource.
> +	       TargetIsObject,	      // An object resource.
> +	       TargetIsMedia	      // A media resource.
> +	   };

nit: The comments seems a bit redundant.  WebKit style is typically to
avoid redundant comments.  The parenthetical comment is helpful though.


LGTM with these nits addressed.