Summary: | [chromium] Minor naming cleanup in WebDragData | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Cheng <dcheng> | ||||||||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, eric, rniwa, tony, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Daniel Cheng
2010-09-30 14:25:05 PDT
Rename hasFileNames to containsFilenames and normalize fileName/FileName to filename/Filename. Created attachment 69430 [details]
Patch
Comment on attachment 69430 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69430&action=review > WebKit/chromium/ChangeLog:10 > + This is a two-part patch--one to add the new methods with normalized > + names, and a followup patch to remove the original methods once > + Chromium is updated. You probably want to mention why we're doing this cleanup. > WebKit/chromium/ChangeLog:19 > + (WebKit::WebDragData::containsFilenames): > + (WebKit::WebDragData::filenames): > + (WebKit::WebDragData::setFilenames): > + (WebKit::WebDragData::appendToFilenames): > + (WebKit::WebDragData::fileContentFilename): > + (WebKit::WebDragData::setFileContentFilename): Could you mention which function replaces what? > WebKit/chromium/src/WebDragData.cpp:163 > + ensureMutable(); > + Vector<String> filenamesCopy; > + filenamesCopy.append(filenames.data(), filenames.size()); > + m_private->setFilenames(filenamesCopy); Instead of duplicating code, can we call setFileNames here? We can move the code here when we're removing setFileNames. > WebKit/chromium/src/WebDragData.cpp:171 > + ensureMutable(); > + Vector<String> filenames = m_private->filenames(); > + filenames.append(filename); > + m_private->setFilenames(filenames); Ditto. Created attachment 69485 [details]
Patch
Comment on attachment 69485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69485&action=review > WebKit/chromium/ChangeLog:14 > + The list of methods that have been renamed: > + * hasFileNames() -> containsFilenames() > + * fileNames() -> filenames() I think Ryosuke was saying that you can put this in the section below. The : after the method names is supposed to be space to write comments about how you modified the method. > WebKit/chromium/ChangeLog:22 > + (WebKit::WebDragData::hasFileNames): For example, here you would write "(WebKit::WebDragData::hasFileNames): renamed to containsFilenames" > WebKit/chromium/public/WebDragData.h:84 > WEBKIT_API void setFileNames(const WebVector<WebString>&); > WEBKIT_API void appendToFileNames(const WebString&); Nit: Can you add a comment that these methods are deprecated/obsolete? Created attachment 69490 [details]
Patch
Comment on attachment 69490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69490&action=review (In reply to comment #5) > (From update of attachment 69485 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69485&action=review > > > WebKit/chromium/ChangeLog:14 > > + The list of methods that have been renamed: > > + * hasFileNames() -> containsFilenames() > > + * fileNames() -> filenames() > > I think Ryosuke was saying that you can put this in the section below. The : after the method names is supposed to be space to write comments about how you modified the method. Yup. That's what I meant. Sorry I should have been more clear. > WebKit/chromium/src/WebDragData.cpp:139 > + return containsFilenames(); > +} > + > +void WebDragData::fileNames(WebVector<WebString>& fileNames) const > +{ > + filenames(fileNames); > +} > + > +void WebDragData::setFileNames(const WebVector<WebString>& fileNames) > +{ > + return setFilenames(fileNames); > +} > + > +void WebDragData::appendToFileNames(const WebString& fileName) > +{ > + return appendToFilenames(fileName); > +} > + > +bool WebDragData::containsFilenames() const > +{ Ah! this is nice. You don't have to move the code anymore! Otherwise LGTM but I'm not a reviewer yet. Comment on attachment 69490 [details] Patch Clearing flags on attachment: 69490 Committed r68915: <http://trac.webkit.org/changeset/68915> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/68915 might have broken GTK Linux 32-bit Debug Created attachment 69836 [details]
Patch
Comment on attachment 69836 [details]
Patch
Actually, do you need to roll WebKit/chromium/DEPS?
Comment on attachment 69836 [details]
Patch
r- because of DEPS roll needed.
Also, I suggest opening a new bug for this since this bug is already closed.
Committed r69202: <http://trac.webkit.org/changeset/69202> Reverted r69202 for reason: Broke compile of test_shell Committed r69207: <http://trac.webkit.org/changeset/69207> Sorry, this should be in https://bugs.webkit.org/show_bug.cgi?id=47227 . |