[webkit-reviews] review denied: [Bug 47394] [chromium] Update ReadableDataObject/WritableDataObject interface for ChromiumDataObject change : [Attachment 70196] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 47394: [chromium] Update ReadableDataObject/WritableDataObject interface
for ChromiumDataObject change
https://bugs.webkit.org/show_bug.cgi?id=47394

Attachment 70196: Patch
https://bugs.webkit.org/attachment.cgi?id=70196&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=70196&action=review

> WebCore/platform/chromium/ReadableDataObject.h:54
> -    virtual String getHTML(String* baseURL) const;
> +    String urlTitle() const;
> +    KURL htmlBaseUrl() const;

I prefer the getHTML method because it makes the caller think about whether
they need the base url or not.	I feel less strongly about the getURL method,
but it's pretty easy to pass in NULL for the title so I'm not sure there's much
benefit to splitting it up.

> WebCore/platform/chromium/WritableDataObject.h:59
> -    virtual void setURL(const String& url, const String& title);
> -    virtual void setHTML(const String& html, const KURL& baseURL);
> +    void setUrlTitle(const String& title);
> +    void setHtmlBaseUrl(const KURL& baseURL);

I prefer the old API where you can set everything in a single function call. 
It makes it less likely that someone forgets to call setUrlTitle or
setHtmlBaseUrl.  Also, in webkit, we tend to capitalize URL (e.g., KURL) and
HTML (e.g., all the files in WebCore/html/).

> WebCore/platform/chromium/WritableDataObject.h:64
>      virtual String urlTitle() const;
>      virtual KURL htmlBaseURL() const;

Are these methods actually called?  Do they need to be virtual?