[Webkit-unassigned] [Bug 37981] implement getData('text/html') for webkit win

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37981





--- Comment #7 from Tony Chang (Google) <tony at chromium.org>  2010-04-22 20:12:36 PST ---
(In reply to comment #4)
> (From update of attachment 54041 [details])
> WebCore/platform/win/ClipboardUtilitiesWin.cpp:158
>  +  static String CF_HTMLToMarkup(const String& cf_html)
> The function name violates WebKit coding style. Please remove "_". Also, the
> comment says "find" while the function name suggests "convert". Could you
> please change either the comment or the function name to make them consistent?

Renamed the function to extractMarkupFromCFHTML.

> LayoutTests/ChangeLog:5
>  +          implement getData('text/html') for webkit win
> Please capitalize the sentence.
> 
> LayoutTests/platform/win/Skipped: 
>  +  # Need to handle getting text/html in ClipboardWin::getData.
> Are you going to add this soon? If not, please file a bug to keep track of it.

That's what this patch fixes and what this bug is about. :)


> WebCore/ChangeLog:5
>  +          implement getData('text/html') for webkit win
> Please capitalize the sentence. You may also need to update the bug title for
> this.

Done.

> WebCore/platform/win/ClipboardUtilitiesWin.cpp:158
>  +  static String CF_HTMLToMarkup(const String& cf_html)
> Please change the parameter name to match the WebKit coding style.

Done.

> WebCore/platform/win/ClipboardUtilitiesWin.cpp:400
>  +          // raw html
> This comment is too simple.

Removed.

> WebCore/platform/win/ClipboardUtilitiesWin.cpp:401
>  +          UChar* data = (UChar*)GlobalLock(store.hGlobal);
> Please change to use C++ style cast.

Done.

> WebCore/platform/win/ClipboardUtilitiesWin.cpp:404
>  +          ReleaseStgMedium(&store);
> Per MSDN, we need to check for pUnkForRelease
>     if pUnkForRelease is NULL, the receiver of the medium is responsible for
> releasing it; otherwise, pUnkForRelease points to the IUnknown on the
> appropriate object so its Release method can be called.

Do you have a link to the documentation where we need to do this?  I was just
moving the existing code, so it wasn't doing this before.  I also don't see us
doing this anywhere in Chromium's clipboard code.

> WebCore/platform/win/ClipboardUtilitiesWin.cpp:441
>  +  PassRefPtr<DocumentFragment> fragmentFromCF_HTML(Document* doc, const
> String& cf_html)
> Please also update the declaration in the old code to match the WebKit coding
> style.

Done.  This caused the extra churn in PasteboardWin and ClipboardWin.

> WebCore/platform/win/ClipboardUtilitiesWin.cpp:417
>  +          char* data = (char*)GlobalLock(store.hGlobal);
> Please change to use C++ style cast.

Done.

> WebCore/platform/win/ClipboardUtilitiesWin.cpp:410
>  +  String getCF_HTML(IDataObject* data, bool& success)
> This function is very similar to getTextHTML. Is it possible to merge them into
> one?

It's tricky because one uses UChar* and the other uses char*.  Maybe you could
use a template, but then there's the UTF8 conversion in the char* one.  I'm not
sure they're that similar.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.