Bug 72131 – REGRESSION (r99924): broke 2 pasteboard tests on GTK

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

editing/pasteboard/copy-backslash-with-euc.html and editing/pasteboard/paste-text-events.html now fail. I locally checked that reverting r99924 and r99925 works. Diffs:

--- /var/lib/buildbot/build/gtk-linux-32-release/build/layout-test-results/editing/pasteboard/copy-backslash-with-euc-expected.txt 
+++ /var/lib/buildbot/build/gtk-linux-32-release/build/layout-test-results/editing/pasteboard/copy-backslash-with-euc-actual.txt 
@@ -14,7 +14,7 @@
 from EUC secure to UTF8 text-control: 
 from EUC secure to UTF8 content-editable: ••••••••••••••••••••••••
 from EUC control to UTF8 text-control: 
-from EUC control to UTF8 content-editable: \ from EUC text control
+from EUC control to UTF8 content-editable: ••••••••••••••••••••••••
 
 Results
 
@@ -25,7 +25,7 @@
 from EUC secure to UTF8 text-control: PASS
 from EUC secure to UTF8 content-editable: PASS
 from EUC control to UTF8 text-control: PASS
-from EUC control to UTF8 content-editable: PASS
+from EUC control to UTF8 content-editable: FAIL: the actual text was '••••••••••••••••••••••••' (char code of the first character was 8226)
 from UTF8 div to EUC text-control: PASS
 from UTF8 div to EUC content-editable: PASS
 from UTF8 transform to EUC text-control: PASS
@@ -33,5 +33,5 @@
 from UTF8 secure to EUC text-control: PASS
 from UTF8 secure to EUC content-editable: PASS
 from UTF8 control to EUC text-control: PASS
-from UTF8 control to EUC content-editable: PASS
+from UTF8 control to EUC content-editable: FAIL: the actual text was '•••••••••••••••••••••••••' (char code of the first character was 8226)
 
--- /var/lib/buildbot/build/gtk-linux-32-release/build/layout-test-results/editing/pasteboard/paste-text-events-expected.txt 
+++ /var/lib/buildbot/build/gtk-linux-32-release/build/layout-test-results/editing/pasteboard/paste-text-events-actual.txt 
@@ -8,7 +8,7 @@
 PASS event.data is 'PlainHello'
 PASS testTargetInput.value is 'PlainHello'
 PASS event.data is ''
-PASS testTargetEditable.innerHTML is 'PlainHello'
+FAIL testTargetEditable.innerHTML should be PlainHello. Was •••••••••••••••••••••••••.
 PASS event.data is 'RichHello'
 PASS testTargetTextarea.value is 'RichHello'
 PASS event.data is 'RichHello'

[Philippe Normand]

Skipped in GTK: http://trac.webkit.org/changeset/99973
Any idea what might be wrong Daniel?

[Daniel Cheng]

Sorry. I'll investigate and fix ASAP.

[Daniel Cheng]

One of the bugs seems to be a race when running layout tests. I'm guessing that manipulating the clipboard in the layout tests is changing the contents of the system clipboard--when I run paste-text-events.html by itself, it seems to pass.

I'm still trying to understand the copy-backslash-with-euc.html failure though.

[Daniel Cheng]

Actually it appears the problem is GTK does not reset the clipboard to a clean state before writing to it in Pasteboard::writeSelection, Pasteboard::writePlaintext, and probably other locations as well. If you don't reset the clipboard to a clean state, then you might have the plain text copied from one page with the URL/html previously copied from somewhere else. I've verified locally that fixing this fixes the problem.

[Martin Robinson]

(In reply to comment #4)
> Actually it appears the problem is GTK does not reset the clipboard to a clean state before writing to it in Pasteboard::writeSelection, Pasteboard::writePlaintext, and probably other locations as well. If you don't reset the clipboard to a clean state, then you might have the plain text copied from one page with the URL/html previously copied from somewhere else. I've verified locally that fixing this fixes the problem.

Interesting. I'll fix this today then, assuming that your patch isn't complete. Thank you for looking at this!

[Martin Robinson]

Created attachment 115763 [details]
Patch

[Daniel Cheng]

Comment on attachment 115763 [details]
Patch

Informal review--the changes look right to me.

[Tony Chang]

Comment on attachment 115763 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115763&action=review

> Source/WebCore/platform/gtk/ClipboardGtk.cpp:120
>      default:

Nit: Can we convert this default: into ClipboardDataTypeImage:?  Then the compiler will be able to warn us in the future if we add a new type.

[Martin Robinson]

Committed r100817: <http://trac.webkit.org/changeset/100817>

[Martin Robinson]

(In reply to comment #8)
> (From update of attachment 115763 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115763&action=review
> 
> > Source/WebCore/platform/gtk/ClipboardGtk.cpp:120
> >      default:
> 
> Nit: Can we convert this default: into ClipboardDataTypeImage:?  Then the compiler will be able to warn us in the future if we add a new type.

Thanks for the review. Landed with this change.