Bug 177700 – Share more pasteboard code between iOS and macOS and remove dependency on Settings

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Now that we've added PasteboardCocoa.mm in https://trac.webkit.org/r222656,
we can share more code between iOS and macOS.

Also, we remove the layering violation inadvertently introduced in https://trac.webkit.org/r222595
whereby which Pasteboard code depends on Settings.

[Ryosuke Niwa]

Created attachment 322277 [details]
Cleanup

[Ryosuke Niwa]

Created attachment 322279 [details]
Cleanup

[Wenson Hsieh]

Comment on attachment 322279 [details]
Cleanup

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

> Source/WebCore/dom/DataTransfer.cpp:146
> +    if (Settings::customPasteboardDataEnabled() && !Pasteboard::isSafeTypeForDOMToReadAndWrite(type))

Shouldn't this be !Pasteboard::isSafeTypeForDOMToReadAndWrite(normalizedType) here? (instead of the non-normalized type)

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #3)
> Comment on attachment 322279 [details]
> Cleanup
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322279&action=review
> 
> > Source/WebCore/dom/DataTransfer.cpp:146
> > +    if (Settings::customPasteboardDataEnabled() && !Pasteboard::isSafeTypeForDOMToReadAndWrite(type))
> 
> Shouldn't this be
> !Pasteboard::isSafeTypeForDOMToReadAndWrite(normalizedType) here? (instead
> of the non-normalized type)

Let's augment editing/pasteboard/data-transfer-get-data-on-drop-custom.html so that instead of writing "text/plain", we try to write "Text" so that we have test coverage for this codepath where type normalization matters.

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #4)
> (In reply to Wenson Hsieh from comment #3)
> > Comment on attachment 322279 [details]
> > Cleanup
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=322279&action=review
> > 
> > > Source/WebCore/dom/DataTransfer.cpp:146
> > > +    if (Settings::customPasteboardDataEnabled() && !Pasteboard::isSafeTypeForDOMToReadAndWrite(type))
> > 
> > Shouldn't this be
> > !Pasteboard::isSafeTypeForDOMToReadAndWrite(normalizedType) here? (instead
> > of the non-normalized type)
> 
> Let's augment editing/pasteboard/data-transfer-get-data-on-drop-custom.html
> so that instead of writing "text/plain", we try to write "Text" so that we
> have test coverage for this codepath where type normalization matters.

That's not going to quite work because this is a bug in getData, not setData. We need to write a new test for this.

[Ryosuke Niwa]

Created attachment 322288 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 322288 [details]
Patch for landing

Wait for EWS.

[Wenson Hsieh]

(In reply to Ryosuke Niwa from comment #5)
> (In reply to Wenson Hsieh from comment #4)
> > (In reply to Wenson Hsieh from comment #3)
> > > Comment on attachment 322279 [details]
> > > Cleanup
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=322279&action=review
> > > 
> > > > Source/WebCore/dom/DataTransfer.cpp:146
> > > > +    if (Settings::customPasteboardDataEnabled() && !Pasteboard::isSafeTypeForDOMToReadAndWrite(type))
> > > 
> > > Shouldn't this be
> > > !Pasteboard::isSafeTypeForDOMToReadAndWrite(normalizedType) here? (instead
> > > of the non-normalized type)
> > 
> > Let's augment editing/pasteboard/data-transfer-get-data-on-drop-custom.html
> > so that instead of writing "text/plain", we try to write "Text" so that we
> > have test coverage for this codepath where type normalization matters.
> 
> That's not going to quite work because this is a bug in getData, not
> setData. We need to write a new test for this.

Oh, true.

I suppose the new test could exercise both setData/getData with non-normalized versions of text/plain, text/html and text/uri-list and make sure that we can write and read these types from DOM (and also use internals.settings.setCustomPasteboardDataEnabled(true)). I can add this in a followup.

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #8)
>
> I suppose the new test could exercise both setData/getData with
> non-normalized versions of text/plain, text/html and text/uri-list and make
> sure that we can write and read these types from DOM (and also use
> internals.settings.setCustomPasteboardDataEnabled(true)). I can add this in
> a followup.

Sure, feel free to write a test if you'd like. If not, I can write one as well.

[Ryosuke Niwa]

Created attachment 322289 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 322289 [details]
Patch for landing

Wait for EWS (after WPE builds).

[Ryosuke Niwa]

Created attachment 322290 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 322290 [details]
Patch for landing

Wait for WPE EWS again...

[Ryosuke Niwa]

Created attachment 322293 [details]
Fixed Mac builds

[WebKit Commit Bot]

Comment on attachment 322293 [details]
Fixed Mac builds

Clearing flags on attachment: 322293

Committed r222680: <http://trac.webkit.org/changeset/222680>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34754897>