1. Condensing getURL()/setURL()/getHTML()/setHTML()/urlTitle()/htmlBaseURL() into urlTitle()/htmlBaseURL() and the corresponding setters. 2. Use ClipboardType instead of bools.
Created attachment 70196 [details] Patch
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?
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.
Comment on attachment 70302 [details] Patch Clearing flags on attachment: 70302 Committed r69431: <http://trac.webkit.org/changeset/69431>
All reviewed patches have been landed. Closing bug.