Bug 158285 – Get rid of StringCapture

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Get rid of StringCapture

With all of the C++14 lambda work lately, it's already almost gone.

We don't need it anymore.

[Brady Eidson]

Created attachment 280280 [details]
Patch

[Brady Eidson]

This may rely on https://bugs.webkit.org/show_bug.cgi?id=158277 landing first, which I had in my tree.

It will probably fail EWS, but feel free to review.

[Brady Eidson]

Will resubmit for EWS once 158277 is resolved and landed.

[Brady Eidson]

Created attachment 280303 [details]
Patch

[Chris Dumez]

Does not build on GTK.

[Brady Eidson]

Created attachment 280304 [details]
Patch

[Brady Eidson]

(In reply to comment #5)
> Does not build on GTK.

Or anywhere.

Due to the wonders of git (and my inability to navigate its mysteries) I lost a line. *sigh*

New one churning through EWS...

[Chris Dumez]

Comment on attachment 280304 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:434
> +    dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {

Hmm, this is a block, not a NoncopyableFunction. I don't understand how having a Ref<> in there works. I thought constructing a block from a lambda always copied the lambda.

[Chris Dumez]

(In reply to comment #8)
> Comment on attachment 280304 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280304&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:434
> > +    dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
> 
> Hmm, this is a block, not a NoncopyableFunction. I don't understand how
> having a Ref<> in there works. I thought constructing a block from a lambda
> always copied the lambda.

We'll see if the iOS EWS is happy with it.

[Chris Dumez]

(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 280304 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=280304&action=review
> > 
> > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:434
> > > +    dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
> > 
> > Hmm, this is a block, not a NoncopyableFunction. I don't understand how
> > having a Ref<> in there works. I thought constructing a block from a lambda
> > always copied the lambda.
> 
> We'll see if the iOS EWS is happy with it.

Ahah! I knew it was too good to be true:
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:47: error: call to implicitly-deleted copy constructor of 'const (lambda at /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:47)'
    dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:48: note: copy constructor of '' is implicitly deleted because field '' has a deleted copy constructor
    dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
                                               ^
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:35:
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/html/TimeRanges.h:29:
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/PlatformTimeRanges.h:31:
In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/usr/local/include/wtf/PassRefPtr.h:25:
/Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/usr/local/include/wtf/Ref.h:69:5: note: 'Ref' has been explicitly marked deleted here
    Ref(const Ref& other) = delete;
    ^
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:47: note: implicit capture of lambda object due to conversion to block pointer here
    dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
                                              ^
1 error generated.

[Chris Dumez]

Comment on attachment 280304 [details]
Patch

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

>>>> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:434
>>>> +    dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
>>> 
>>> Hmm, this is a block, not a NoncopyableFunction. I don't understand how having a Ref<> in there works. I thought constructing a block from a lambda always copied the lambda.
>> 
>> We'll see if the iOS EWS is happy with it.
> 
> Ahah! I knew it was too good to be true:
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:47: error: call to implicitly-deleted copy constructor of 'const (lambda at /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:47)'
>     dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
>                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:48: note: copy constructor of '' is implicitly deleted because field '' has a deleted copy constructor
>     dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
>                                                ^
> In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:35:
> In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/html/TimeRanges.h:29:
> In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/PlatformTimeRanges.h:31:
> In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/usr/local/include/wtf/PassRefPtr.h:25:
> /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/usr/local/include/wtf/Ref.h:69:5: note: 'Ref' has been explicitly marked deleted here
>     Ref(const Ref& other) = delete;
>     ^
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:47: note: implicit capture of lambda object due to conversion to block pointer here
>     dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
>                                               ^
> 1 error generated.

I am not 100% sure but would using callOnMainThread() be OK in this case?

[Brady Eidson]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Comment on attachment 280304 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=280304&action=review
> > > 
> > > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:434
> > > > +    dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
> > > 
> > > Hmm, this is a block, not a NoncopyableFunction. I don't understand how
> > > having a Ref<> in there works. I thought constructing a block from a lambda
> > > always copied the lambda.
> > 
> > We'll see if the iOS EWS is happy with it.
> 
> Ahah! I knew it was too good to be true:
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/
> WebVideoFullscreenControllerAVKit.mm:437:47: error: call to
> implicitly-deleted copy constructor of 'const (lambda at
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/
> WebVideoFullscreenControllerAVKit.mm:437:47)'
>     dispatch_async(dispatch_get_main_queue(), [protectedThis =
> Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type,
> localizedDeviceName = localizedDeviceName.isolatedCopy()] {
>                                              
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/
> WebVideoFullscreenControllerAVKit.mm:437:48: note: copy constructor of '' is
> implicitly deleted because field '' has a deleted copy constructor
>     dispatch_async(dispatch_get_main_queue(), [protectedThis =
> Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type,
> localizedDeviceName = localizedDeviceName.isolatedCopy()] {
>                                                ^
> In file included from
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/
> WebVideoFullscreenControllerAVKit.mm:35:
> In file included from
> /Volumes/Data/EWS/WebKit/Source/WebCore/html/TimeRanges.h:29:
> In file included from
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/PlatformTimeRanges.
> h:31:
> In file included from
> /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/usr/local/
> include/wtf/PassRefPtr.h:25:
> /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/usr/local/
> include/wtf/Ref.h:69:5: note: 'Ref' has been explicitly marked deleted here
>     Ref(const Ref& other) = delete;
>     ^
> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/
> WebVideoFullscreenControllerAVKit.mm:437:47: note: implicit capture of
> lambda object due to conversion to block pointer here
>     dispatch_async(dispatch_get_main_queue(), [protectedThis =
> Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type,
> localizedDeviceName = localizedDeviceName.isolatedCopy()] {
>                                               ^
> 1 error generated.

I *thought* I'd mangled the compiler guards locally to actually see it build on my Mac, but I guess I failed!

Will fix.

[Chris Dumez]

Comment on attachment 280304 [details]
Patch

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

>>>>>> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:434
>>>>>> +    dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
>>>>> 
>>>>> Hmm, this is a block, not a NoncopyableFunction. I don't understand how having a Ref<> in there works. I thought constructing a block from a lambda always copied the lambda.
>>>> 
>>>> We'll see if the iOS EWS is happy with it.
>>> 
>>> Ahah! I knew it was too good to be true:
>>> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:47: error: call to implicitly-deleted copy constructor of 'const (lambda at /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:47)'
>>>     dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
>>>                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:48: note: copy constructor of '' is implicitly deleted because field '' has a deleted copy constructor
>>>     dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
>>>                                                ^
>>> In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:35:
>>> In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/html/TimeRanges.h:29:
>>> In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/PlatformTimeRanges.h:31:
>>> In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/usr/local/include/wtf/PassRefPtr.h:25:
>>> /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/usr/local/include/wtf/Ref.h:69:5: note: 'Ref' has been explicitly marked deleted here
>>>     Ref(const Ref& other) = delete;
>>>     ^
>>> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:47: note: implicit capture of lambda object due to conversion to block pointer here
>>>     dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
>>>                                               ^
>>> 1 error generated.
>> 
>> I am not 100% sure but would using callOnMainThread() be OK in this case?
> 
> I *thought* I'd mangled the compiler guards locally to actually see it build on my Mac, but I guess I failed!
> 
> Will fix.

Otherwise, we can keep using the same API (which is safer) and allocated a NonCopyableFunction on the heap and capture that.

[Brady Eidson]

(In reply to comment #11)
> Comment on attachment 280304 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280304&action=review
> 
> >>>> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:434
> >>>> +    dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
> >>> 
> >>> Hmm, this is a block, not a NoncopyableFunction. I don't understand how having a Ref<> in there works. I thought constructing a block from a lambda always copied the lambda.
> >> 
> >> We'll see if the iOS EWS is happy with it.
> > 
> > Ahah! I knew it was too good to be true:
> > /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:47: error: call to implicitly-deleted copy constructor of 'const (lambda at /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:47)'
> >     dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
> >                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:48: note: copy constructor of '' is implicitly deleted because field '' has a deleted copy constructor
> >     dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
> >                                                ^
> > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:35:
> > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/html/TimeRanges.h:29:
> > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/PlatformTimeRanges.h:31:
> > In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/usr/local/include/wtf/PassRefPtr.h:25:
> > /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/usr/local/include/wtf/Ref.h:69:5: note: 'Ref' has been explicitly marked deleted here
> >     Ref(const Ref& other) = delete;
> >     ^
> > /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:47: note: implicit capture of lambda object due to conversion to block pointer here
> >     dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
> >                                               ^
> > 1 error generated.
> 
> I am not 100% sure but would using callOnMainThread() be OK in this case?

Seeing as this is iOS and I'm trying to make this be a 100% mechanical task, I'm not willing to try that change.

I think we *really* need to sort out the whole RunLoop::main vs callOnMainThread issue. It would simply be better for the project if callOnMainThread could be ripped out and we only relied on RunLoop dispatch.

[Brady Eidson]

(In reply to comment #13)
> >>> /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/usr/local/include/wtf/Ref.h:69:5: note: 'Ref' has been explicitly marked deleted here
> >>>     Ref(const Ref& other) = delete;
> >>>     ^
> >>> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:47: note: implicit capture of lambda object due to conversion to block pointer here
> >>>     dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
> >>>                                               ^
> >>> 1 error generated.
> >> 
> >> I am not 100% sure but would using callOnMainThread() be OK in this case?
> > 
> > I *thought* I'd mangled the compiler guards locally to actually see it build on my Mac, but I guess I failed!
> > 
> > Will fix.
> 
> Otherwise, we can keep using the same API (which is safer) and allocated a
> NonCopyableFunction on the heap and capture that.

Yah... kinda ugly, but I'm doing that. Stay tuned.

[Brady Eidson]

Created attachment 280305 [details]
Patch

[Brady Eidson]

(In reply to comment #15)
> (In reply to comment #13)
> > >>> /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/usr/local/include/wtf/Ref.h:69:5: note: 'Ref' has been explicitly marked deleted here
> > >>>     Ref(const Ref& other) = delete;
> > >>>     ^
> > >>> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:437:47: note: implicit capture of lambda object due to conversion to block pointer here
> > >>>     dispatch_async(dispatch_get_main_queue(), [protectedThis = Ref<WebVideoFullscreenControllerContext>(*this), this, enabled, type, localizedDeviceName = localizedDeviceName.isolatedCopy()] {
> > >>>                                               ^
> > >>> 1 error generated.
> > >> 
> > >> I am not 100% sure but would using callOnMainThread() be OK in this case?
> > > 
> > > I *thought* I'd mangled the compiler guards locally to actually see it build on my Mac, but I guess I failed!
> > > 
> > > Will fix.
> > 
> > Otherwise, we can keep using the same API (which is safer) and allocated a
> > NonCopyableFunction on the heap and capture that.
> 
> Yah... kinda ugly, but I'm doing that. Stay tuned.

Actually, screw that.

callOnMainThread() should be fine.

From the Concurrency Programming guide (https://developer.apple.com/library/mac/documentation/General/Conceptual/ConcurrencyProgrammingGuide/OperationQueues/OperationQueues.html#//apple_ref/doc/uid/TP40008091-CH102-SW2):

"The main dispatch queue is a globally available serial queue that executes tasks on the application’s main thread."

[Chris Dumez]

Comment on attachment 280305 [details]
Patch

r=me as long as the bots are happy.

[Brady Eidson]

Current test failures on EWS appear to be the typical media controls failures that have been plaguing us lately.

On retries they're clearing up.

[Brady Eidson]

http://trac.webkit.org/changeset/201594