Bug 169805 – [WK1] Support animated transitions when performing a data interaction operation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Wenson Hsieh]

Support animated transitions when performing a data interaction operation in a WK1 web view.

[Wenson Hsieh]

<rdar://problem/31045767>

[Wenson Hsieh]

Created attachment 304751 [details]
First pass

[Megan Gardner]

Comment on attachment 304751 [details]
First pass

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

> Source/WebKit/mac/WebCoreSupport/WebDragClient.h:52
>  private:

Is this change needed, the implementations are empty.

> Source/WebKit/mac/WebCoreSupport/WebDragClient.mm:82
> +

empty implementations, do these belong in this patch?

> Source/WebKit/mac/WebCoreSupport/WebDragClient.mm:196
> +

empty implementations, do these belong in this patch?

> Source/WebKit/mac/WebView/WebView.mm:669
> +            _contentImageWithoutSelectionRectInRootViewCoordinates = indicatorData.contentImageWithoutSelectionRectInRootViewCoordinates;

retain here, also release this and _contentImageWithoutSelection in dealloc.

> Source/WebKit/mac/WebView/WebView.mm:1912
>  #endif // ENABLE(DATA_INTERACTION) && defined(__cplusplus)

empty method? what is it used for?

> Source/WebKit/mac/WebView/WebViewData.h:292
>  #endif

is this the best way to transfer this data?

> Source/WebKit/mac/WebView/WebViewInternal.h:262
>  #endif

The implementation is empty, is this used?

> Source/WebKit/mac/WebView/WebViewPrivate.h:-59
> -

Make sure that this removed from where it is needed first, so as to not cause code to stop compiling.

> Source/WebKit/mac/WebView/WebViewPrivate.h:194
>  @end

Is everything above being used? Can we re-purpose some of the existing properties?

[Wenson Hsieh]

Comment on attachment 304751 [details]
First pass

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

>> Source/WebKit/mac/WebCoreSupport/WebDragClient.h:52
>>  private:
> 
> Is this change needed, the implementations are empty.

The implementation we care about is in WebKitAdditions. It just takes a snapshot using TextIndicator and sets dataOperationTextIndicator -- I will post the patch later.

>> Source/WebKit/mac/WebCoreSupport/WebDragClient.mm:82
>> +
> 
> empty implementations, do these belong in this patch?

Yes. didConcludeEditDrag() was formerly implemented in the header, now it's just declared there and defined in the implementation.

>> Source/WebKit/mac/WebCoreSupport/WebDragClient.mm:196
>> +
> 
> empty implementations, do these belong in this patch?

Yes. didConcludeEditDrag() was formerly implemented in the header, now it's just declared there and defined in the implementation.

>> Source/WebKit/mac/WebView/WebView.mm:669
>> +            _contentImageWithoutSelectionRectInRootViewCoordinates = indicatorData.contentImageWithoutSelectionRectInRootViewCoordinates;
> 
> retain here, also release this and _contentImageWithoutSelection in dealloc.

This is a CGRect, and isn't something we can or should retain.

Fixed the _contentImageWithoutSelection leak though -- good catch!

>> Source/WebKit/mac/WebView/WebView.mm:1912
>>  #endif // ENABLE(DATA_INTERACTION) && defined(__cplusplus)
> 
> empty method? what is it used for?

Refer to the other empty methods preceding this. Although on second glance, it looks like these are just for staging if <WebKitAdditions/WebViewAdditions.mm> is not available yet? In which case we should just remove the empty implementations for _requestStartDataInteraction:globalPosition: ... -_endedDataInteraction:global: instead...

>> Source/WebKit/mac/WebView/WebViewData.h:292
>>  #endif
> 
> is this the best way to transfer this data?

I believe so, for the same reason textIndicatorData is a member of WebViewPrivate.

Also, we need access to this in two places in UIWebDocumentView anyways (refer to the UIKit change) so even if we made -[WebView _performDataInteraction:client:global:operation:], we'd have to store dataOperationTextIndicator somewhere that UIWebDocumentView can access.

>> Source/WebKit/mac/WebView/WebViewInternal.h:262
>>  #endif
> 
> The implementation is empty, is this used?

The implementation we care about is in WebKitAdditions (patch not yet posted).

>> Source/WebKit/mac/WebView/WebViewPrivate.h:-59
>> -
> 
> Make sure that this removed from where it is needed first, so as to not cause code to stop compiling.

👍

>> Source/WebKit/mac/WebView/WebViewPrivate.h:194
>>  @end
> 
> Is everything above being used? Can we re-purpose some of the existing properties?

No, we definitely should not repurpose the existing properties (though it might make staging a bit easier). WebUITextIndicatorData is meant to mirror TextIndicatorData.

[Wenson Hsieh]

Comment on attachment 304751 [details]
First pass

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

>>> Source/WebKit/mac/WebView/WebView.mm:1912
>>>  #endif // ENABLE(DATA_INTERACTION) && defined(__cplusplus)
>> 
>> empty method? what is it used for?
> 
> Refer to the other empty methods preceding this. Although on second glance, it looks like these are just for staging if <WebKitAdditions/WebViewAdditions.mm> is not available yet? In which case we should just remove the empty implementations for _requestStartDataInteraction:globalPosition: ... -_endedDataInteraction:global: instead...

On second glance, this is not just a staging hack -- we'll need to keep this, and also add a stub implementation for -_dataOperationTextIndicator (this appears to be the cause of the EWS build failures).

[Wenson Hsieh]

Created attachment 304783 [details]
Fix OpenSource build

[Wenson Hsieh]

(In reply to comment #5)
> Comment on attachment 304751 [details]
> First pass
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304751&action=review
> 
> >>> Source/WebKit/mac/WebView/WebView.mm:1912
> >>>  #endif // ENABLE(DATA_INTERACTION) && defined(__cplusplus)
> >> 
> >> empty method? what is it used for?
> > 
> > Refer to the other empty methods preceding this. Although on second glance, it looks like these are just for staging if <WebKitAdditions/WebViewAdditions.mm> is not available yet? In which case we should just remove the empty implementations for _requestStartDataInteraction:globalPosition: ... -_endedDataInteraction:global: instead...
> 
> On second glance, this is not just a staging hack -- we'll need to keep
> this, and also add a stub implementation for -_dataOperationTextIndicator
> (this appears to be the cause of the EWS build failures).

Hm...it looks like we'll actually need to stub -_dataOperationTextIndicator and not -_didConcludeEditDataInteraction, since the former is SPI but the latter is IPI.

[Wenson Hsieh]

Created attachment 304787 [details]
Fix OpenSource iOS builds

[Wenson Hsieh]

Created attachment 304802 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 304802 [details]
Patch for landing

Clearing flags on attachment: 304802

Committed r214111: <http://trac.webkit.org/changeset/214111>