Bug 177991 – Split StaticPasteboard::writeString into writeString and writeStringInCustomData

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Split the function to write into platform data versus custom data like we did for readString.

[Ryosuke Niwa]

Created attachment 322986 [details]
Cleanup

[Wenson Hsieh]

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!

[Ryosuke Niwa]

(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);
}

[Ryosuke Niwa]

Committed r222964: <http://trac.webkit.org/changeset/222964>

[Radar WebKit Bug Importer]

<rdar://problem/34851671>