Bug 59028 – Move complexity from DataTransferItemsChromium and DataTransferItemChromium for base class

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Igor Trindade Oliveira]

Many code used by DataTransferItemsChromium and DataTransferItemChromium can be reused by others platforms. Moving the complexity for base class we can reduce the necessary effort to support DataTransferItems element in different plataforms.

[Igor Trindade Oliveira]

Created attachment 90509 [details]
Patch

Initial patch

[Tony Chang]

Daniel, can you take an initial pass at this?

[Daniel Cheng]

Comment on attachment 90509 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90509&action=review

> Source/WebCore/dom/DataTransferItem.h:36
> +#include "Clipboard.h"

I think forward declarations are preferred in header files where possible (same comment in DataTransferItems.h)

> Source/WebCore/dom/DataTransferItem.h:49
> +    static PassRefPtr<DataTransferItem> create(PassRefPtr<Clipboard> owner, ScriptExecutionContext*, const String& data, const String& type);

I'm not sure how useful it is to hoist this method up into the shared implementation. Is there somewhere in the shared implementation that would call this? If not, I think it would be better to leave this method on the platform implementations. That way, we don't have to expose internal enums publicly.

> Source/WebCore/dom/DataTransferItem.h:62
> +    void setKind(const String&);

Who uses these setters?

[Igor Trindade Oliveira]

(In reply to comment #3)
> (From update of attachment 90509 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90509&action=review
> 
> > Source/WebCore/dom/DataTransferItem.h:36
> > +#include "Clipboard.h"
> 
> I think forward declarations are preferred in header files where possible (same comment in DataTransferItems.h)

Ok. Fixing that.

> 
> > Source/WebCore/dom/DataTransferItem.h:49
> > +    static PassRefPtr<DataTransferItem> create(PassRefPtr<Clipboard> owner, ScriptExecutionContext*, const String& data, const String& type);
> 
> I'm not sure how useful it is to hoist this method up into the shared implementation. Is there somewhere in the shared implementation that would call this? If not, I think it would be better to leave this method on the platform implementations. That way, we don't have to expose internal enums publicly.

The DataTransferItems.cpp is using this method, take a look in DataTransferItems::add (  m_items.append(DataTransferItem::create(m_owner, m_context, data, type)); )
. I can try to find a better solution.
> 
> > Source/WebCore/dom/DataTransferItem.h:62
> > +    void setKind(const String&);
> 
> Who uses these setters?

Right now, nobody :) , i just added it because in future some platform can use this setter. But i do not have objections to remove these setters.

[Igor Trindade Oliveira]

Created attachment 90594 [details]
Patch

The forward declaration is not possible in DataTransferItem.h, the template needs to know about Clipboard, if i do not include it, it would be an incomplete type.
The setters was removed(my fault) and the enumerator and constructor from DataTransferItemChromium are private again.

[Igor Trindade Oliveira]

Created attachment 90600 [details]
Patch

The forward declaration is not possible in DataTransferItem.h, the template needs to know about Clipboard, if i do not include it, it would be an incomplete type.
The setters was removed(my fault) and the enumerator and constructor from DataTransferItemChromium are private again.
Constify attributes in DataTransferItem.h

[Tony Chang]

Comment on attachment 90600 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90600&action=review

The change looks fine, but I think we need to update the other build systems.

> Source/WebCore/WebCore.gypi:2428
> +            'dom/DataTransferItems.cpp',

Should we add this file to the other build systems (xcode, vcproj, etc)?

> Source/WebCore/dom/DataTransferItems.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

I suspect this is the wrong copyright.

> Source/WebCore/dom/DataTransferItems.cpp:47
> +unsigned long DataTransferItems::length()

It would be nice in a follow up patch to change these to size_t since that's what wtf::Vector uses.

[Igor Trindade Oliveira]

Created attachment 90939 [details]
Patch

Updates for others build systems, change copyright and unsigned long to size_t.

[Tony Chang]

Comment on attachment 90939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90939&action=review

> Source/WebCore/dom/DataTransferItems.cpp:14
> + *     * Neither the name of Google Inc. nor the names of its

Nit: This should probably be Nokia.

[Igor Trindade Oliveira]

(In reply to comment #9)
> (From update of attachment 90939 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90939&action=review
> 
> > Source/WebCore/dom/DataTransferItems.cpp:14
> > + *     * Neither the name of Google Inc. nor the names of its
> 
> Nit: This should probably be Nokia.

I am from Nokia :). I am reusing Google code so i am putting Google and Nokia copyrights, ok for you?

[Tony Chang]

Comment on attachment 90939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90939&action=review

>>> Source/WebCore/dom/DataTransferItems.cpp:14
>>> + *     * Neither the name of Google Inc. nor the names of its
>> 
>> Nit: This should probably be Nokia.
> 
> I am from Nokia :). I am reusing Google code so i am putting Google and Nokia copyrights, ok for you?

Yes, that sounds fine to me :)

[Igor Trindade Oliveira]

Created attachment 90946 [details]
Patch

Adding Nokia in copyright. So the phrase: "Neither the name of Google Inc. nor the names of its
contributors may be used to endorse or promote products derived ..." does not need to be changed because Nokia is a contributor.

[WebKit Commit Bot]

Comment on attachment 90946 [details]
Patch

Rejecting attachment 90946 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2

Last 500 characters of output:
nsferItems.cpp
patching file Source/WebCore/dom/DataTransferItems.h
patching file Source/WebCore/platform/chromium/DataTransferItemChromium.cpp
patching file Source/WebCore/platform/chromium/DataTransferItemChromium.h
patching file Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp
patching file Source/WebCore/platform/chromium/DataTransferItemsChromium.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Tony Chang', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8507372

[Igor Trindade Oliveira]

Created attachment 90954 [details]
Patch

Trying again. I updated my local branch.

[WebKit Commit Bot]

Comment on attachment 90954 [details]
Patch

Clearing flags on attachment: 90954

Committed r84857: <http://trac.webkit.org/changeset/84857>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.