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

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Daniel Cheng <dcheng at chromium.org>
(In reply to comment #2)
> (From update of attachment 70196 [details])
> 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?

They are called, but I forgot to devirtualize them. They are used when copying
data from WebKit to the browser host.
I also went ahead and inlined all the simple setters/getters into the header.