Get rid of StringCapture With all of the C++14 lambda work lately, it's already almost gone. We don't need it anymore.
Created attachment 280280 [details] Patch
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.
Will resubmit for EWS once 158277 is resolved and landed.
Created attachment 280303 [details] Patch
Does not build on GTK.
Created attachment 280304 [details] Patch
(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...
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.
(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.
(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.
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?
(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.
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.
(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.
(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.
Created attachment 280305 [details] Patch
(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."
Comment on attachment 280305 [details] Patch r=me as long as the bots are happy.
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.
http://trac.webkit.org/changeset/201594