[chromium] Combine URL, HTML and file content setter/getters in WebDragData
Created attachment 124159 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
I'm not 100% sure about changing WebURL->WebString for URL data. Advantages: - We can support text/uri-list correctly. - Internally, that's how we're going to store the data anyway. Disadvantages: - We need a duplicate 'convert to URL' implementation on platforms that don't support text/uri-list (Win, Mac). There will be an implementation in ChromiumDataObject and one in webkit/glue. Other potential issues to note: Eventually, I'd like the drag data glue to transfer items back and forth as lists of data as well which means these new methods will be obsoleted quickly. This makes the glue code a little more complicated but possibly more efficient, as it doesn't require us to iterate through the data store multiple times when converting from WebDragData->WebDropData. With the current interface, we'll have to iterate through the data store when we read plain text, URL, HTML, DownloadURL (these can early return as soon as we find them though...), file content, and custom data (these require iterating through the full list every time).
Associated Chromium code review at http://codereview.chromium.org/9290052/
Hmm, this actually results in some really ugly looking temporary glue code, e.g. WebString WebDragData::url(WebString& title) const { String title2; WebString url = m_privateData->url(title2); title = title2; return url; } Maybe it's OK since it's temporary?
Comment on attachment 124159 [details] Patch I'm not sure I fully understand why you want to make this change. Have there been bugs where people have forgotten to set all properties in a group? Does this simplify the implementation somehow? Couldn't we also just more explicitly group the related properties by using a common prefix? (Well, I guess we already mostly do that.) View in context: https://bugs.webkit.org/attachment.cgi?id=124159&action=review > Source/WebKit/chromium/public/platform/WebDragData.h:70 > + WEBKIT_EXPORT WebString url(WebString& title) const; it is generally bad to have a method named after only one of the properties that it returns. if you could come up with good names for these sets of properties, then you could define structs by those names, and then use the name of the struct for the property names. perhaps you could use the format names that these correspond to as a guide? hmm... I also see that WebClipboard has write{URL,HTML} and so on. it has a similar problem.
(In reply to comment #6) > (From update of attachment 124159 [details]) > I'm not sure I fully understand why you want to make this change. Have there been > bugs where people have forgotten to set all properties in a group? Does this simplify > the implementation somehow? Couldn't we also just more explicitly group the related > properties by using a common prefix? (Well, I guess we already mostly do that.) > > View in context: https://bugs.webkit.org/attachment.cgi?id=124159&action=review > > > Source/WebKit/chromium/public/platform/WebDragData.h:70 > > + WEBKIT_EXPORT WebString url(WebString& title) const; > > it is generally bad to have a method named after only one of the properties that it returns. > if you could come up with good names for these sets of properties, then you could define > structs by those names, and then use the name of the struct for the property names. > > perhaps you could use the format names that these correspond to as a guide? hmm... I also > see that WebClipboard has write{URL,HTML} and so on. it has a similar problem. Some background: Today, we have a pretty big mess in ClipboardChromium and DataTransferItemListChromium. We try to keep a dirty bit to figure out when we need to update DataTransferItemListChromium, but ultimately, it's difficult to keep the two in sync. There are things that are supported in one interface don't work in the other. For example, you can set your own drag out data by using dataTransfer.setData('x', 'y') but dataTransfer.items.add('y', 'x') is completely unimplemented and does nothing. To fix that, I'd like to unify the backing store for ChromiumDataObject and DataTransferItemListChromium. There were two primary approaches I considered: 1. Keep the current approach, using a struct with various named fields to store "special" types of data. This keeps the glue simple, but significantly complicates the generation of dataTransfer.items. We'd need to establish a consistent order of iterating through the special elements, custom MIME types, and file types in order to make sure that DataTransferItemList::item(index) is deterministic when the items of the collection haven't changed. For example, in a hypothetical situation where we want to get the 5th element of DataTransferItemList, we might have to do the following: - Check text/plain. The field is set, so add 1 to the counter. - Check text/url-list. This field is not set, so don't change the counter. - Check text/html. This field is set, so increment counter to 2. - Check custom MIME types. It's set, so we start iterating through it, incrementing the counter once for each iteration. - As it turns out, the counter hits 4 before we reach the end of the custom MIME types, so return a DataTransferItem corresponding to the current custom MIME type. 2. Convert ChromiumDataObject to use a similar mechanism to DataTransferItemListChromium internally. We would basically keep a list of DataTransferItems, with an interface that might look something like this: DataTransferItem: String type(); // MIME type String data(); // String data, if it's a string item String title(); // URL title if type() == 'text/uri-list' KURL baseURL(); // Base URL if type() == 'text/html' String filename(); // Name of a file if this represents a file a user is dragging into a WebView // Next 3 members used to handle image drag out PassRefPtr<SharedBuffer> bufferData(); // Data for image String bufferName(); // Name for image, including extension String bufferExtension(); // Just the extension This leads to a very straightforward ChromiumDataObject/DataTransferItemList implementation, but results in more complicated glue code. In the original uber patch I posted in https://bugs.webkit.org/show_bug.cgi?id=76598, I opted to take the second approach. If you take a look at the WebDragData.h changes in that patch, I hadn't completely figured out the story for the glue though--I was trying to make a generic data list node that would be flexible enough to store the types that need to go between the browser and renderer. This patch is an intermediate step between the current state and the final implementation of #2. In the intermediate stage, it simplifies the glue to be able to set URL/title together in one function call. since we can just add a DataTransferItem to the ChromiumDataObject with the MIME type, URL data, and URL title all set. Otherwise, the setUrlTitle() implementation might look like this: - If text/uri-list node doesn't exist in CDO, create it. - Otherwise, find the existing text/uri-list node. - Set the title on that text/uri-list node. Eventually, I'd like to end up with an approach similar to the uber patch though. WebDragData would only expose items() and setItems(), and DataTransferItem would be a concept that the glue layer would understand. The hard work of converting between the list-based approach and the field-based approach would be done in webdropdata.cc.
Created attachment 124637 [details] Patch
Created attachment 124893 [details] Patch
Comment on attachment 124893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124893&action=review > Source/WebKit/chromium/public/platform/WebDragData.h:53 > + // Internally, we represent data in a drag as a list of I can't tell if this sentence stops intentionally or if 'struct Item' is the end of the sentence. Please complete the sentence and end with a period. > Source/WebKit/chromium/public/platform/WebDragData.h:63 > + // Only valid when storageType == STRING Nit: End the sentence with a period, here and below. > Source/WebKit/chromium/src/WebDragData.cpp:72 > + HashSet<String> types = m_private->types(); Can |types| be a const reference? > Source/WebKit/chromium/src/WebDragData.cpp:128 > +void WebDragData::setItemList(const WebVector<Item>& itemList) > +{ > + for (size_t i = 0; i < itemList.size(); ++i) > + addItem(itemList[i]); Do we need to clear out any old items?
Created attachment 125439 [details] Patch
Created attachment 125440 [details] Patch Remove merge marker
Comment on attachment 124893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124893&action=review >> Source/WebKit/chromium/public/platform/WebDragData.h:53 >> + // Internally, we represent data in a drag as a list of > > I can't tell if this sentence stops intentionally or if 'struct Item' is the end of the sentence. Please complete the sentence and end with a period. I removed this stale comment and added more useful comments on storageType. >> Source/WebKit/chromium/public/platform/WebDragData.h:63 >> + // Only valid when storageType == STRING > > Nit: End the sentence with a period, here and below. Done. >> Source/WebKit/chromium/src/WebDragData.cpp:72 >> + HashSet<String> types = m_private->types(); > > Can |types| be a const reference? Yes, we're binding to a temporary but it doesn't really matter in this case.
Comment on attachment 125440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125440&action=review Code seems fine. Please let fishd also review. > Source/WebKit/chromium/ChangeLog:10 > + simplifythe serialization/deserialization between platform-specific data and WebDragData. Nit: missing space between 'simplifythe'
Created attachment 125458 [details] Patch Fix typo
Comment on attachment 125458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125458&action=review > Source/WebKit/chromium/public/platform/WebDragData.h:53 > + struct Item { Do you think there will be files (.h or .cc) where we will only need to deal with the Item struct, possibly requiring a forward declaration? Perhaps it should be declared at the top-level in its own header file? WebDragDataItem? > Source/WebKit/chromium/public/platform/WebDragData.h:54 > + enum StorageType { nit: enums should be named like so: enum Foo { FooBar, FooBaz, ... }; These should be StorageType{String,Filename,SharedBuffer} NOTE: The term "shared buffer" is really more of a WebCore thing. Maybe instead of "sharedBufferData" you should call it "binaryData"? > Source/WebKit/chromium/public/platform/WebDragData.h:103 > + WebVector<Item> itemList() const; nit: we usually just pluralize the name instead. itemList -> items
Created attachment 125678 [details] Patch
Comment on attachment 125458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125458&action=review >> Source/WebKit/chromium/public/platform/WebDragData.h:53 >> + struct Item { > > Do you think there will be files (.h or .cc) where we will only need to deal > with the Item struct, possibly requiring a forward declaration? Perhaps it > should be declared at the top-level in its own header file? > > WebDragDataItem? I haven't encountered any instances of needing a forward declaration during the process of this conversion. Do you think it's still necessary? >> Source/WebKit/chromium/public/platform/WebDragData.h:54 >> + enum StorageType { > > nit: enums should be named like so: > > enum Foo { > FooBar, > FooBaz, > ... > }; > > These should be StorageType{String,Filename,SharedBuffer} > > NOTE: The term "shared buffer" is really more of a WebCore thing. Maybe > instead of "sharedBufferData" you should call it "binaryData"? Done. >> Source/WebKit/chromium/public/platform/WebDragData.h:103 >> + WebVector<Item> itemList() const; > > nit: we usually just pluralize the name instead. itemList -> items I used item and itemList for consistency because the DataTransferItem spec in HTML5 uses the same naming scheme.
(In reply to comment #18) > (From update of attachment 125458 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125458&action=review > > >> Source/WebKit/chromium/public/platform/WebDragData.h:53 > >> + struct Item { > > > > Do you think there will be files (.h or .cc) where we will only need to deal > > with the Item struct, possibly requiring a forward declaration? Perhaps it > > should be declared at the top-level in its own header file? > > > > WebDragDataItem? > > I haven't encountered any instances of needing a forward declaration during the process of this conversion. Do you think it's still necessary? OK > >> Source/WebKit/chromium/public/platform/WebDragData.h:103 > >> + WebVector<Item> itemList() const; > > > > nit: we usually just pluralize the name instead. itemList -> items > > I used item and itemList for consistency because the DataTransferItem spec in HTML5 uses the same naming scheme. I think it is better to be consistent with the WebKit API, which you are modifying, than with HTML5 :-)
Comment on attachment 125678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125678&action=review > Source/WebKit/chromium/public/platform/WebDragData.h:60 > + // There are three different types of data we store in Item nodes: nit: I'd probably just make these comments above each enum value. enum StorageType { // Blah, blah, blah StorageTypeString, // Blah, blah, blah StorageTypeFilename, ... }; This way it is a bit less verbose as you don't need to repeat the enum value name.
Committed r107846: <http://trac.webkit.org/changeset/107846>