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.
Created attachment 90509 [details] Patch Initial patch
Daniel, can you take an initial pass at this?
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?
(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.
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.
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
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.
Created attachment 90939 [details] Patch Updates for others build systems, change copyright and unsigned long to size_t.
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.
(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?
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 :)
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.
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
Created attachment 90954 [details] Patch Trying again. I updated my local branch.
Comment on attachment 90954 [details] Patch Clearing flags on attachment: 90954 Committed r84857: <http://trac.webkit.org/changeset/84857>
All reviewed patches have been landed. Closing bug.