This is just a minor cleanup change. We need to duplicate the original method, switch Chromium, and then remove the obsolete method in WebKit.
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 .