[webkit-reviews] review denied: [Bug 33738] DataTransfer interface does not work on Windows : [Attachment 46711] Patch for the problems described

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied dcheng at google.com's request
for review:
Bug 33738: DataTransfer interface does not work on Windows
https://bugs.webkit.org/show_bug.cgi?id=33738

Attachment 46711: Patch for the problems described
https://bugs.webkit.org/attachment.cgi?id=46711&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +	   Not sure how to add a good test for it.

I'll bet we can come up with a test. If .types and .getData are completely
broken, it should be pretty easy! There are probably existing tests you can
build off of; grep through LayoutTests for uses of event.dataTransfer.

> +	   * platform/win/ClipboardWin.cpp:
> +	   (WebCore::addMimeTypesForFormat):
> +	   (WebCore::ClipboardWin::types):
> +	   (WebCore::ClipboardWin::hasData):

You should add function-level comments about what your patch does and why.

> @@ -568,7 +568,7 @@ HashSet<String> ClipboardWin::types() co
>  
>      FORMATETC data;
>  
> -    while (SUCCEEDED(itr->Next(1, &data, 0))) {
> +    while (itr->Next(1, &data, 0) == S_OK) {
>	   addMimeTypesForFormat(results, data);
>      }
>  
> @@ -789,7 +789,7 @@ bool ClipboardWin::hasData()
>  
>      FORMATETC data;
>  
> -    if (SUCCEEDED(itr->Next(1, &data, 0))) {
> +    if (itr->Next(1, &data, 0) == S_OK) {
>	   // There is at least one item in the IDataObject
>	   return true;
>      }

It might be worth adding a comment about the return value of
IEnumFORMATETC::Next.

The code changes look good, but r- so we can come up with a test and fix up the
ChangeLog.