Split the function to write into platform data versus custom data like we did for readString.
Created attachment 322986 [details] Cleanup
Comment on attachment 322986 [details] Cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=322986&action=review > Source/WebCore/dom/DataTransfer.cpp:162 > + m_itemList->didSetStringData(type); Is the change from normalizedType => type intended? This would make it so that if you setData('text', 'foo') after the item list has been ensured, the DataTransferItemList will contain an item of type 'text', rather than 'text/plain'. > Source/WebCore/platform/StaticPasteboard.cpp:63 > + updateTypes(m_types, type, !m_platformData.set(type, value).isNewEntry || m_customData.contains(type)); I think this code is a little hard to follow — I imagined this would look something like writeString: moveToEndOfTypes(type); m_platformData.set(type, value); ...and writeStringInCustomData: moveToEndOfTypes(type); m_customData.set(type, value); ...and the private helper method moveToEndOfTypes would just do: if (m_types.contains(types)) m_types.removeFirst(type); m_types.append(type); (or if we want to avoid the linear search through all types) if (m_customData.contains(type) || m_platformData.contains(type)) m_types.removeFirst(type); m_types.append(type); ...but I don't think it's that big a deal. Up to you!
(In reply to Wenson Hsieh from comment #2) > Comment on attachment 322986 [details] > Cleanup > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322986&action=review > > > Source/WebCore/dom/DataTransfer.cpp:162 > > + m_itemList->didSetStringData(type); > > Is the change from normalizedType => type intended? This would make it so > that if you setData('text', 'foo') after the item list has been ensured, the > DataTransferItemList will contain an item of type 'text', rather than > 'text/plain'. Oh oops, unintended. Fixed. > > Source/WebCore/platform/StaticPasteboard.cpp:63 > > + updateTypes(m_types, type, !m_platformData.set(type, value).isNewEntry || m_customData.contains(type)); > > I think this code is a little hard to follow — I imagined this would look > something like > > writeString: > > moveToEndOfTypes(type); > m_platformData.set(type, value); > > ...and writeStringInCustomData: > > moveToEndOfTypes(type); > m_customData.set(type, value); We don't want to do that because that'd result in two hash lookups. > ...and the private helper method moveToEndOfTypes would just do: > > if (m_types.contains(types)) > m_types.removeFirst(type); > m_types.append(type); > > (or if we want to avoid the linear search through all types) > > if (m_customData.contains(type) || m_platformData.contains(type)) > m_types.removeFirst(type); > m_types.append(type); > > ...but I don't think it's that big a deal. Up to you! That's what I'm doing now but I guess it's not that clear. Revised it as: bool typeWasAlreadyPresent = !m_platformData.set(type, value).isNewEntry || m_customData.contains(type); updateTypes(m_types, type, typeWasAlreadyPresent); and renamed the last argument from replace to moveToEnd: static void updateTypes(Vector<String>& types, String type, bool moveToEnd) { if (moveToEnd) types.removeFirst(type); ASSERT(!types.contains(type)); types.append(type); }
Committed r222964: <http://trac.webkit.org/changeset/222964>
<rdar://problem/34851671>