Bug 47394 – [chromium] Update ReadableDataObject/WritableDataObject interface for ChromiumDataObject change

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Daniel Cheng]

1. Condensing getURL()/setURL()/getHTML()/setHTML()/urlTitle()/htmlBaseURL() into urlTitle()/htmlBaseURL() and the corresponding setters.
2. Use ClipboardType instead of bools.

[Daniel Cheng]

Created attachment 70196 [details]
Patch

[Tony Chang]

Comment on attachment 70196 [details]
Patch

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?

[Daniel Cheng]

Created attachment 70302 [details]
Patch

(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.

[WebKit Commit Bot]

Comment on attachment 70302 [details]
Patch

Clearing flags on attachment: 70302

Committed r69431: <http://trac.webkit.org/changeset/69431>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.