177991 2017-10-05 21:12:42 -0700 Split StaticPasteboard::writeString into writeString and writeStringInCustomData 2017-10-06 00:31:03 -0700 1 1 1 Unclassified WebKit HTML Editing WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 rniwa rniwa buildbot cdumez dbates esprehn+autocc kangil.han webkit-bug-importer wenson_hsieh oldest_to_newest 1357446 0 rniwa 2017-10-05 21:12:42 -0700 Split the function to write into platform data versus custom data like we did for readString. 1357452 1 322986 rniwa 2017-10-05 21:53:08 -0700 Created attachment 322986 Cleanup 1357471 2 322986 wenson_hsieh 2017-10-05 23:22:28 -0700 Comment on attachment 322986 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! 1357492 3 rniwa 2017-10-06 00:21:39 -0700 (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); } 1357495 4 rniwa 2017-10-06 00:30:23 -0700 Committed r222964: <http://trac.webkit.org/changeset/222964> 1357497 5 webkit-bug-importer 2017-10-06 00:31:03 -0700 <rdar://problem/34851671> 322986 2017-10-05 21:53:08 -0700 2017-10-05 23:22:28 -0700 Cleanup bug-177991-20171005215309.patch text/plain 5496 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIyMjk1NikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI1IEBACisyMDE3LTEwLTA1ICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIFNwbGl0IFN0YXRpY1Bhc3RlYm9h cmQ6OndyaXRlU3RyaW5nIGludG8gd3JpdGVTdHJpbmcgYW5kIHdyaXRlU3RyaW5nSW5DdXN0b21E YXRhCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNzc5 OTEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBTcGxp dCB3cml0ZVN0cmluZyBpbnRvIHRoZSBvbmUgdGhhdCB3cml0ZXMgaW50byB0aGUgcGxhZm9ybSBw YXN0ZWJvYXJkIG5hdGl2ZWx5IGFuZCB0aGUgY3VzdG9tIHBhc3RlYm9hcmQgZGF0YSBsaWtlIHdl IGRpZAorICAgICAgICBpbiByMjIyNjgwIGZvciByZWFkU3RyaW5nIGFuZCByZWFkU3RyaW5nSW5D dXN0b21EYXRhLgorCisgICAgICAgICogZG9tL0RhdGFUcmFuc2Zlci5jcHA6CisgICAgICAgIChX ZWJDb3JlOjpEYXRhVHJhbnNmZXI6OnNldERhdGEpOgorICAgICAgICAoV2ViQ29yZTo6RGF0YVRy YW5zZmVyOjpzZXREYXRhRnJvbUl0ZW1MaXN0KTogRXh0cmFjdGVkIG91dCBvZiBzZXREYXRhLgor ICAgICAgICAqIGRvbS9EYXRhVHJhbnNmZXIuaDoKKyAgICAgICAgKiBkb20vRGF0YVRyYW5zZmVy SXRlbUxpc3QuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RGF0YVRyYW5zZmVySXRlbUxpc3Q6OmFk ZCk6CisgICAgICAgICogcGxhdGZvcm0vU3RhdGljUGFzdGVib2FyZC5jcHA6CisgICAgICAgIChX ZWJDb3JlOjp1cGRhdGVUeXBlcyk6IEV4dGFjdGVkIG91dCBvZiB3cml0ZVN0cmluZy4KKyAgICAg ICAgKFdlYkNvcmU6OlN0YXRpY1Bhc3RlYm9hcmQ6OndyaXRlU3RyaW5nKToKKyAgICAgICAgKFdl YkNvcmU6OlN0YXRpY1Bhc3RlYm9hcmQ6OndyaXRlU3RyaW5nSW5DdXN0b21EYXRhKToKKyAgICAg ICAgKiBwbGF0Zm9ybS9TdGF0aWNQYXN0ZWJvYXJkLmg6CisKIDIwMTctMTAtMDUgIFJ5b3N1a2Ug Tml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CiAKICAgICAgICAgRGF0YVRyYW5zZmVyIHNob3VsZG4n dCBjb250YWluIHRleHQvaHRtbCB3aGVuIHBlcmZvcm1pbmcgUGFzdGUgYW5kIE1hdGNoIFN0eWxl CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9kb20vRGF0YVRyYW5zZmVyLmNwcAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvV2ViQ29yZS9kb20vRGF0YVRyYW5zZmVyLmNwcAkocmV2aXNpb24gMjIyODk2KQor KysgU291cmNlL1dlYkNvcmUvZG9tL0RhdGFUcmFuc2Zlci5jcHAJKHdvcmtpbmcgY29weSkKQEAg LTE1NiwxMCArMTU2LDIxIEBAIHZvaWQgRGF0YVRyYW5zZmVyOjpzZXREYXRhKGNvbnN0IFN0cmlu ZyYKICAgICBpZiAoKGZvckZpbGVEcmFnKCkgfHwgU2V0dGluZ3M6OmN1c3RvbVBhc3RlYm9hcmRE YXRhRW5hYmxlZCgpKSAmJiBtX3Bhc3RlYm9hcmQtPmNvbnRhaW5zRmlsZXMoKSkKICAgICAgICAg cmV0dXJuOwogCi0gICAgU3RyaW5nIG5vcm1hbGl6ZWRUeXBlID0gbm9ybWFsaXplVHlwZSh0eXBl KTsKLSAgICBtX3Bhc3RlYm9hcmQtPndyaXRlU3RyaW5nKG5vcm1hbGl6ZWRUeXBlLCBkYXRhKTsK KyAgICBzZXREYXRhRnJvbUl0ZW1MaXN0KG5vcm1hbGl6ZVR5cGUodHlwZSksIGRhdGEpOworCiAg ICAgaWYgKG1faXRlbUxpc3QpCi0gICAgICAgIG1faXRlbUxpc3QtPmRpZFNldFN0cmluZ0RhdGEo bm9ybWFsaXplZFR5cGUpOworICAgICAgICBtX2l0ZW1MaXN0LT5kaWRTZXRTdHJpbmdEYXRhKHR5 cGUpOworfQorCit2b2lkIERhdGFUcmFuc2Zlcjo6c2V0RGF0YUZyb21JdGVtTGlzdChjb25zdCBT dHJpbmcmIHR5cGUsIGNvbnN0IFN0cmluZyYgZGF0YSkKK3sKKyAgICBBU1NFUlQoY2FuV3JpdGVE YXRhKCkpOworICAgIFJFTEVBU0VfQVNTRVJUKGlzPFN0YXRpY1Bhc3RlYm9hcmQ+KCptX3Bhc3Rl Ym9hcmQpKTsKKworICAgIGlmIChTZXR0aW5nczo6Y3VzdG9tUGFzdGVib2FyZERhdGFFbmFibGVk KCkgJiYgIVBhc3RlYm9hcmQ6OmlzU2FmZVR5cGVGb3JET01Ub1JlYWRBbmRXcml0ZSh0eXBlKSkK KyAgICAgICAgZG93bmNhc3Q8U3RhdGljUGFzdGVib2FyZD4oKm1fcGFzdGVib2FyZCkud3JpdGVT dHJpbmdJbkN1c3RvbURhdGEodHlwZSwgZGF0YSk7CisgICAgZWxzZQorICAgICAgICBtX3Bhc3Rl Ym9hcmQtPndyaXRlU3RyaW5nKHR5cGUsIGRhdGEpOwogfQogCiB2b2lkIERhdGFUcmFuc2Zlcjo6 dXBkYXRlRmlsZUxpc3QoKQpJbmRleDogU291cmNlL1dlYkNvcmUvZG9tL0RhdGFUcmFuc2Zlci5o Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2RvbS9EYXRhVHJhbnNmZXIuaAkocmV2aXNp b24gMjIyODk2KQorKysgU291cmNlL1dlYkNvcmUvZG9tL0RhdGFUcmFuc2Zlci5oCSh3b3JraW5n IGNvcHkpCkBAIC02NSw2ICs2NSw3IEBAIHB1YmxpYzoKICAgICBTdHJpbmcgZ2V0RGF0YShjb25z dCBTdHJpbmcmIHR5cGUpIGNvbnN0OwogCiAgICAgdm9pZCBzZXREYXRhKGNvbnN0IFN0cmluZyYg dHlwZSwgY29uc3QgU3RyaW5nJiBkYXRhKTsKKyAgICB2b2lkIHNldERhdGFGcm9tSXRlbUxpc3Qo Y29uc3QgU3RyaW5nJiB0eXBlLCBjb25zdCBTdHJpbmcmIGRhdGEpOwogCiAgICAgdm9pZCBzZXRE cmFnSW1hZ2UoRWxlbWVudCosIGludCB4LCBpbnQgeSk7CiAKSW5kZXg6IFNvdXJjZS9XZWJDb3Jl L2RvbS9EYXRhVHJhbnNmZXJJdGVtTGlzdC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNv cmUvZG9tL0RhdGFUcmFuc2Zlckl0ZW1MaXN0LmNwcAkocmV2aXNpb24gMjIyODk2KQorKysgU291 cmNlL1dlYkNvcmUvZG9tL0RhdGFUcmFuc2Zlckl0ZW1MaXN0LmNwcAkod29ya2luZyBjb3B5KQpA QCAtNzgsNyArNzgsNyBAQCBFeGNlcHRpb25PcjxSZWZQdHI8RGF0YVRyYW5zZmVySXRlbT4+IERh CiAgICAgaWYgKCFpc1N1cHBvcnRlZFR5cGUobG93ZXJjYXNlZFR5cGUpKQogICAgICAgICByZXR1 cm4gbnVsbHB0cjsKIAotICAgIG1fZGF0YVRyYW5zZmVyLnBhc3RlYm9hcmQoKS53cml0ZVN0cmlu Zyhsb3dlcmNhc2VkVHlwZSwgZGF0YSk7CisgICAgbV9kYXRhVHJhbnNmZXIuc2V0RGF0YUZyb21J dGVtTGlzdChsb3dlcmNhc2VkVHlwZSwgZGF0YSk7CiAgICAgQVNTRVJUKG1faXRlbXMpOwogICAg IG1faXRlbXMtPmFwcGVuZChEYXRhVHJhbnNmZXJJdGVtOjpjcmVhdGUobV93ZWFrUHRyRmFjdG9y eS5jcmVhdGVXZWFrUHRyKCp0aGlzKSwgbG93ZXJjYXNlZFR5cGUpKTsKICAgICByZXR1cm4gUmVm UHRyPERhdGFUcmFuc2Zlckl0ZW0+IHsgbV9pdGVtcy0+bGFzdCgpLmNvcHlSZWYoKSB9OwpJbmRl eDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vU3RhdGljUGFzdGVib2FyZC5jcHAKPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PQotLS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vU3RhdGljUGFzdGVib2FyZC5jcHAJKHJldmlz aW9uIDIyMjg5NikKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1N0YXRpY1Bhc3RlYm9hcmQu Y3BwCSh3b3JraW5nIGNvcHkpCkBAIC01MCwxNiArNTAsMjIgQEAgU3RyaW5nIFN0YXRpY1Bhc3Rl Ym9hcmQ6OnJlYWRTdHJpbmdJbkN1cwogICAgIHJldHVybiBtX2N1c3RvbURhdGEuZ2V0KHR5cGUp OwogfQogCitzdGF0aWMgdm9pZCB1cGRhdGVUeXBlcyhWZWN0b3I8U3RyaW5nPiYgdHlwZXMsIFN0 cmluZyB0eXBlLCBib29sIHJlcGxhY2UpCit7CisgICAgaWYgKHJlcGxhY2UpCisgICAgICAgIHR5 cGVzLnJlbW92ZUZpcnN0KHR5cGUpOworICAgIEFTU0VSVCghdHlwZXMuY29udGFpbnModHlwZSkp OworICAgIHR5cGVzLmFwcGVuZCh0eXBlKTsKK30KKwogdm9pZCBTdGF0aWNQYXN0ZWJvYXJkOjp3 cml0ZVN0cmluZyhjb25zdCBTdHJpbmcmIHR5cGUsIGNvbnN0IFN0cmluZyYgdmFsdWUpCiB7Ci0g ICAgYXV0byYgcGFzdGVib2FyZERhdGEgPSBQYXN0ZWJvYXJkOjppc1NhZmVUeXBlRm9yRE9NVG9S ZWFkQW5kV3JpdGUodHlwZSkgPyBtX3BsYXRmb3JtRGF0YSA6IG1fY3VzdG9tRGF0YTsKLSAgICBp ZiAocGFzdGVib2FyZERhdGEuc2V0KHR5cGUsIHZhbHVlKS5pc05ld0VudHJ5KQotICAgICAgICBt X3R5cGVzLmFwcGVuZCh0eXBlKTsKLSAgICBlbHNlIHsKLSAgICAgICAgbV90eXBlcy5yZW1vdmVG aXJzdCh0eXBlKTsKLSAgICAgICAgQVNTRVJUKCFtX3R5cGVzLmNvbnRhaW5zKHR5cGUpKTsKLSAg ICAgICAgbV90eXBlcy5hcHBlbmQodHlwZSk7Ci0gICAgfQorICAgIHVwZGF0ZVR5cGVzKG1fdHlw ZXMsIHR5cGUsICFtX3BsYXRmb3JtRGF0YS5zZXQodHlwZSwgdmFsdWUpLmlzTmV3RW50cnkgfHwg bV9jdXN0b21EYXRhLmNvbnRhaW5zKHR5cGUpKTsKK30KKwordm9pZCBTdGF0aWNQYXN0ZWJvYXJk Ojp3cml0ZVN0cmluZ0luQ3VzdG9tRGF0YShjb25zdCBTdHJpbmcmIHR5cGUsIGNvbnN0IFN0cmlu ZyYgdmFsdWUpCit7CisgICAgdXBkYXRlVHlwZXMobV90eXBlcywgdHlwZSwgIW1fY3VzdG9tRGF0 YS5zZXQodHlwZSwgdmFsdWUpLmlzTmV3RW50cnkgfHwgbV9wbGF0Zm9ybURhdGEuY29udGFpbnMo dHlwZSkpOwogfQogCiB2b2lkIFN0YXRpY1Bhc3RlYm9hcmQ6OmNsZWFyKCkKSW5kZXg6IFNvdXJj ZS9XZWJDb3JlL3BsYXRmb3JtL1N0YXRpY1Bhc3RlYm9hcmQuaAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS9TdGF0aWNQYXN0ZWJvYXJkLmgJKHJldmlzaW9uIDIyMjg5NikK KysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1N0YXRpY1Bhc3RlYm9hcmQuaAkod29ya2luZyBj b3B5KQpAQCAtNDcsNiArNDcsNyBAQCBwdWJsaWM6CiAgICAgU3RyaW5nIHJlYWRTdHJpbmdJbkN1 c3RvbURhdGEoY29uc3QgU3RyaW5nJiB0eXBlKSBmaW5hbDsKIAogICAgIHZvaWQgd3JpdGVTdHJp bmcoY29uc3QgU3RyaW5nJiB0eXBlLCBjb25zdCBTdHJpbmcmIGRhdGEpIGZpbmFsOworICAgIHZv aWQgd3JpdGVTdHJpbmdJbkN1c3RvbURhdGEoY29uc3QgU3RyaW5nJiB0eXBlLCBjb25zdCBTdHJp bmcmIGRhdGEpOwogICAgIHZvaWQgY2xlYXIoKSBmaW5hbDsKICAgICB2b2lkIGNsZWFyKGNvbnN0 IFN0cmluZyYgdHlwZSkgZmluYWw7CiAK