Bug 37981 – implement getData('text/html') for webkit win

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Chang]

implement getData('text/html') for webkit win

[Tony Chang]

Created attachment 54041 [details]
Patch

[WebKit Review Bot]

Attachment 54041 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/win/ClipboardUtilitiesWin.cpp:158:  CF_HTMLToMarkup is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/win/ClipboardUtilitiesWin.cpp:158:  cf_html is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/win/ClipboardUtilitiesWin.cpp:410:  getCF_HTML is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/win/ClipboardUtilitiesWin.cpp:419:  cf_html is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/win/ClipboardUtilitiesWin.h:68:  getCF_HTML is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tony Chang]

This is the same as bug 19893, but only an implementation for Safari Win.

A couple notes:
- Neither mozilla nor IE implement this
- It first provides text/html data, then grabs cf_html data, which is more common on Windows.  Not sure if the order should change or if it should only return text/html data.

[Jian Li]

Comment on attachment 54041 [details]
Patch

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?

[Tony Chang]

Created attachment 54123 [details]
v2

[WebKit Review Bot]

Attachment 54123 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Traceback (most recent call last):
  File "WebKitTools/Scripts/check-webkit-style", line 129, in <module>
    main()
  File "WebKitTools/Scripts/check-webkit-style", line 116, in main
    patch = checkout.create_patch()
  File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style_references.py", line 79, in create_patch
    return self._scm.create_patch()
  File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/common/checkout/scm.py", line 508, in create_patch
    return run_command(['git', 'diff', '--binary', 'HEAD'])
  File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/common/system/executive.py", line 85, in run_command
    return Executive().run_command(*args, **kwargs)
  File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/common/system/executive.py", line 215, in run_command
    output = output.decode("utf-8")
  File "/usr/lib/python2.6/encodings/utf_8.py", line 16, in decode
    return codecs.utf_8_decode(input, errors, True)
UnicodeDecodeError: 'utf8' codec can't decode byte 0xa0 in position 2496: unexpected code byte


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tony Chang]

(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.

[Jian Li]

Comment on attachment 54123 [details]
v2

LayoutTests/platform/win/editing/pasteboard/onpaste-text-html-expected.txt:1
 +  CONSOLE MESSAGE: line 21: text/plain: This test verifies that we can get text/html from the clipboard during an onpaste event. 
Why do we have a different expected result on Win? It seems that the 2nd and the 3d line are a little bit different. Can you investigate why causes the difference?

LayoutTests/platform/win/editing/pasteboard/onpaste-text-html-expected.txt:4
 +  Paste content in this div.This test verifies that we can get text/html from the clipboard during an onpaste event. 
Why do we have a garbage character 'Â' at this line?

LayoutTests/platform/win/fast/events/ondrop-text-html-expected.txt:1
 +  CONSOLE MESSAGE: line 21: text/plain: This test verifies that we can get text/html from the drag object during an ondrop event. 
Why do we have a different expected result on Win?

[Eric Seidel (no email)]

Comment on attachment 54123 [details]
v2

platform/win/editing/pasteboard/onpaste-text-html-expected.txt appears to contain a latin1 space instead of a utf-8 space.  According to my research (see webkit-dev) expected.txt files should always be utf-8.  How did you generate this output?

[Tony Chang]

(In reply to comment #9)
> (From update of attachment 54123 [details])
> platform/win/editing/pasteboard/onpaste-text-html-expected.txt appears to
> contain a latin1 space instead of a utf-8 space.  According to my research (see
> webkit-dev) expected.txt files should always be utf-8.  How did you generate
> this output?

I ran run-webkit-tests.  There are a few existing layout tests that have this, which means most platforms seem to generate this in some cases (see attached list, although some are probably valid utf8).

[Tony Chang]

Created attachment 54250 [details]
list of tests with \xa0

[Tony Chang]

Comment on attachment 54123 [details]
v2

Resetting for review since plenty of existing results have \xa0 and this is needed for tests to pass.

To answer Jian's question, it's just how windows handles nbsp.

[Jian Li]

Comment on attachment 54123 [details]
v2

LayoutTests/ChangeLog:6
 +          https://bugs.webkit.org/show_bug.cgi?id=37981
Please also mention that 2 layout tests are rebaselined due to the reason you spoke of.

WebCore/platform/win/ClipboardUtilitiesWin.cpp:394
 +  String getTextHTML(IDataObject* data, bool& success)
It might be better to define the function as:
  bool getTextHTML(IDataObject* data, String& result)

WebCore/platform/win/ClipboardUtilitiesWin.cpp:409
 +  String getCFHTML(IDataObject* data, bool& success)
ditto.

WebCore/platform/win/ClipboardUtilitiesWin.cpp:466
 +      if (SUCCEEDED(data->GetData(htmlFormat(), &store))) {
This and the following line seem to be identical to getCFHTML. Can you change to call the helper function?

WebCore/platform/win/PasteboardWin.cpp:295
 +              PassRefPtr<DocumentFragment> fragment = fragmentFromCFHTML(frame->document(), cf_html);
Please also update the name "cf_html".

[Tony Chang]

Created attachment 62115 [details]
Patch

[Tony Chang]

Created attachment 62124 [details]
Patch

[Tony Chang]

Ok, finally circling back to this patch.  I fixed the style in a separate change so this patch only has the changes necessary for enabling getData("text/html") on windows (Safari win).



(In reply to comment #13)
> (From update of attachment 54123 [details])
> LayoutTests/ChangeLog:6
>  +          https://bugs.webkit.org/show_bug.cgi?id=37981
> Please also mention that 2 layout tests are rebaselined due to the reason you spoke of.

This magically seems to work now, so no rebaselines.

> WebCore/platform/win/ClipboardUtilitiesWin.cpp:394
>  +  String getTextHTML(IDataObject* data, bool& success)
> It might be better to define the function as:
>   bool getTextHTML(IDataObject* data, String& result)

I kept it this way to match the existing methods (getURL and getPlainText).

> WebCore/platform/win/ClipboardUtilitiesWin.cpp:466
>  +      if (SUCCEEDED(data->GetData(htmlFormat(), &store))) {
> This and the following line seem to be identical to getCFHTML. Can you change to call the helper function?

They're the same up until the call to extactMarkupFromCFHTML.  I could refactor the 5 lines before that into a function, but I'm not sure there's much benefit.

[Ojan Vafai]

Comment on attachment 62124 [details]
Patch

> +String urlToMarkup(const KURL& url, const String& title);

Was this accidental? I don't see it used anywhere.

> > WebCore/platform/win/ClipboardUtilitiesWin.cpp:466
> >  +      if (SUCCEEDED(data->GetData(htmlFormat(), &store))) {
> > This and the following line seem to be identical to getCFHTML. Can you change to call the helper function?
> 
> They're the same up until the call to extactMarkupFromCFHTML.  I could refactor the 5 lines before that into a function, but I'm not sure there's much benefit.

I almost always think it's worth pulling these sorts of things into a static helper function. But your call. If there's a bug in this in the future, it only needs to be fixed in one place.

[Tony Chang]

Created attachment 62844 [details]
Patch for landing

[Tony Chang]

Committed r64242: <http://trac.webkit.org/changeset/64242>