[webkit-reviews] review denied: [Bug 28964] [Chromium] ChromiumDataObject should have getter/setter interface : [Attachment 39047] patch: add getter/setter accessor methods

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Roland Steiner
<rolandsteiner at google.com>'s request for review:
Bug 28964: [Chromium] ChromiumDataObject should have getter/setter interface
https://bugs.webkit.org/show_bug.cgi?id=28964

Attachment 39047: patch: add getter/setter accessor methods
https://bugs.webkit.org/attachment.cgi?id=39047&action=review

------- Additional Comments from David Levin <levin at chromium.org>
r- for the PassRefPtr return value


> Index: WebCore/ChangeLog
> ===================================================================

> + Added interface methods.

Really you are adding methods to a class not to an interface. (You have
implementations here.)

> +
> +	   No new tests.

Usually you would explain why there are no new tests.  For example,

  "No functional behavior change so no new tests."


> Index: WebCore/platform/chromium/ChromiumDataObject.h
> +	   inline PassRefPtr<SharedBuffer> content() const { return
fileContent; }

"If a function’s result is an object, but ownership is not being transferred,
the result should be a raw pointer. This includes most getter functions." --
http://webkit.org/coding/RefPtr.html

>	   KURL url;
>	   String urlTitle;

Later: Please consider renaming the variables to m_ when you make them private.