| 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: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Jinwoo Song
2014-05-19 23:26:36 PDT
Created attachment 231753 [details]
Patch
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.
(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. Created attachment 231822 [details]
Patch
Add a test case.
This build problem on Win is real. Could you investigate please? 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. Created attachment 231864 [details]
Patch
Fix build for window port.
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. Created attachment 231873 [details]
Patch
Update testcase applying Greg's advice.
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. 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 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
Created attachment 231943 [details]
Patch
Applied AP's comments.
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. Created attachment 232048 [details]
Patch
Patch for land.
Comment on attachment 232048 [details] Patch Clearing flags on attachment: 232048 Committed r169327: <http://trac.webkit.org/changeset/169327> All reviewed patches have been landed. Closing bug. |