[webkit-reviews] review denied: [Bug 37981] implement getData('text/html') for webkit win : [Attachment 54041] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Apr 22 12:02:26 PDT 2010


Jian Li <jianli at chromium.org> has denied Tony Chang (Google)
<tony at chromium.org>'s request for review:
Bug 37981: implement getData('text/html') for webkit win
https://bugs.webkit.org/show_bug.cgi?id=37981

Attachment 54041: Patch
https://bugs.webkit.org/attachment.cgi?id=54041&action=review

------- Additional Comments from Jian Li <jianli at chromium.org>
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?

Please also capitalize the comment and end it with period.

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.

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.

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.

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

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

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.


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.

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

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?


More information about the webkit-reviews mailing list