Bug 46937 – [chromium] Minor naming cleanup in WebDragData

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Daniel Cheng]

This is just a minor cleanup change. We need to duplicate the original method, switch Chromium, and then remove the obsolete method in WebKit.

[Daniel Cheng]

Rename hasFileNames to containsFilenames and normalize fileName/FileName to filename/Filename.

[Daniel Cheng]

Created attachment 69430 [details]
Patch

[Ryosuke Niwa]

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.

[Daniel Cheng]

Created attachment 69485 [details]
Patch

[Tony Chang]

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?

[Daniel Cheng]

Created attachment 69490 [details]
Patch

[Ryosuke Niwa]

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.

[WebKit Commit Bot]

Comment on attachment 69490 [details]
Patch

Clearing flags on attachment: 69490

Committed r68915: <http://trac.webkit.org/changeset/68915>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/68915 might have broken GTK Linux 32-bit Debug

[Daniel Cheng]

Created attachment 69836 [details]
Patch

[Tony Chang]

Comment on attachment 69836 [details]
Patch

Actually, do you need to roll WebKit/chromium/DEPS?

[Tony Chang]

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.

[Tony Chang]

Committed r69202: <http://trac.webkit.org/changeset/69202>

[Tony Chang]

Reverted r69202 for reason:

Broke compile of test_shell

Committed r69207: <http://trac.webkit.org/changeset/69207>

[Tony Chang]

Sorry, this should be in https://bugs.webkit.org/show_bug.cgi?id=47227 .