Bug 133108

Summary:

setData() of DataTransfer has a void return type

Product:

WebKit

Reporter:

Jinwoo Song <jinwoo7.song>

Component:

HTML Editing

Assignee:

Jinwoo Song <jinwoo7.song>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, buildbot, bunhere, cdumez, commit-queue, esprehn+autocc, gyuyoung.kim, kangil.han, kondapallykalyan, krit, rniwa, sergio, stavila

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2	none
Patch	none
Patch	none

Jinwoo Song

Reported 2014-05-19 23:26:36 PDT

According to HTML5 spec, setData() of DataTransfer does not return value. http://www.w3.org/TR/html/editing.html#the-datatransfer-interface

Attachments
Patch (9.70 KB, patch) 2014-05-19 23:34 PDT, Jinwoo Song	no flags	Details Formatted Diff Diff
Patch (11.37 KB, patch) 2014-05-21 03:50 PDT, Jinwoo Song	no flags	Details Formatted Diff Diff
Patch (11.59 KB, patch) 2014-05-22 00:23 PDT, Jinwoo Song	no flags	Details Formatted Diff Diff
Patch (12.17 KB, patch) 2014-05-22 03:25 PDT, Jinwoo Song	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (654.19 KB, application/zip) 2014-05-22 17:45 PDT, Build Bot	no flags	Details
Patch (12.38 KB, patch) 2014-05-22 22:46 PDT, Jinwoo Song	no flags	Details Formatted Diff Diff
Patch (12.35 KB, patch) 2014-05-25 18:21 PDT, Jinwoo Song	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Jinwoo Song

Comment 1 2014-05-19 23:34:59 PDT

Created attachment 231753 [details] Patch

Dirk Schulze

Comment 2 2014-05-20 00:58:46 PDT

Comment on attachment 231753 [details] Patch It lacks a test case but looks good to me in general. Is WebKit the only engine exposing a boolean function? Someone with more experience in the code path should take a look over it.

Jinwoo Song

Comment 3 2014-05-20 02:19:11 PDT

(In reply to comment #2) > (From update of attachment 231753 [details]) > It lacks a test case but looks good to me in general. Is WebKit the only engine exposing a boolean function? Someone with more experience in the code path should take a look over it. Blink also returns a boolean type as WebKit but I could not look into Firefox. I'll prepare a test case in following patch.

Jinwoo Song

Comment 4 2014-05-21 03:50:27 PDT

Created attachment 231822 [details] Patch Add a test case.

Dirk Schulze

Comment 5 2014-05-21 05:55:35 PDT

This build problem on Win is real. Could you investigate please?

Radu Stavila

Comment 6 2014-05-21 07:36:57 PDT

Comment on attachment 231822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231822&action=review > Source/WebCore/platform/win/PasteboardWin.cpp:413 > + return; This return doesn't seem necessary anymore.

Jinwoo Song

Comment 7 2014-05-22 00:23:37 PDT

Created attachment 231864 [details] Patch Fix build for window port.

Grzegorz Czajkowski

Comment 8 2014-05-22 02:35:34 PDT

Comment on attachment 231864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231864&action=review > LayoutTests/editing/pasteboard/set_data_typeof_return.html:13 > + if (typeof setData == "undefined") Nit: You can use shouldBeUndefined from js-test-pre.js and get rid of log() function.

Jinwoo Song

Comment 9 2014-05-22 03:25:44 PDT

Created attachment 231873 [details] Patch Update testcase applying Greg's advice.

Alexey Proskuryakov

Comment 10 2014-05-22 09:22:58 PDT

Comment on attachment 231873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231873&action=review > Source/WebCore/platform/win/PasteboardWin.cpp:400 > - return WebCore::writeURL(m_writableDataObject.get(), URL(URL(), data), String(), false, true); > + WebCore::writeURL(m_writableDataObject.get(), URL(URL(), data), String(), false, true); This loses an early return (which does not affect behavior here, but is a tiny bit inefficient). > Source/WebCore/platform/win/PasteboardWin.cpp:413 > + return; > } > - return true; > + return; These early returns are unnecessary. > LayoutTests/editing/pasteboard/set_data_typeof_return-expected.txt:3 > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". There is no "TEST COMPLETE" here, as promised. To fix that, add js-test-post.js as the last script in <body>. We have a script that creates a correct template for a test, named make-new-script-test.

Build Bot

Comment 11 2014-05-22 17:45:17 PDT

Comment on attachment 231873 [details] Patch Attachment 231873 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5382812232318976 New failing tests: media/media-fragments/TC0001.html

Build Bot

Comment 12 2014-05-22 17:45:24 PDT

Created attachment 231925 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Jinwoo Song

Comment 13 2014-05-22 22:46:59 PDT

Created attachment 231943 [details] Patch Applied AP's comments.

Alexey Proskuryakov

Comment 14 2014-05-23 09:53:05 PDT

Comment on attachment 231943 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231943&action=review > Source/WebCore/platform/efl/PasteboardEfl.cpp:104 > + return; > } More unneeded "return" here.

Jinwoo Song

Comment 15 2014-05-25 18:21:47 PDT

Created attachment 232048 [details] Patch Patch for land.

WebKit Commit Bot

Comment 16 2014-05-25 19:54:12 PDT

Comment on attachment 232048 [details] Patch Clearing flags on attachment: 232048 Committed r169327: <http://trac.webkit.org/changeset/169327>

WebKit Commit Bot

Comment 17 2014-05-25 19:54:19 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.