At least until reloading/navigating.
Created attachment 312314 [details] Patch
Comment on attachment 312314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312314&action=review r=me with one change > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:208 > +bool UserMediaPermissionRequestManagerProxy::isRequestDenied(uint64_t frameID, const WebCore::SecurityOrigin& userMediaDocumentOrigin, const WebCore::SecurityOrigin& topLevelDocumentOrigin, bool needsAudio, bool needsVideo) I would call this "wasRequestDenied" because it's a record of something that happened in the past, and not necessarily a comment on what is or should be happening right now. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:216 > + if (deniedRequest.frameID != frameID) > + continue; This seems wrong to me. A page could re-prompt the user infinitely by inserting and removing iframes with getUserMedia calls. Once the user says no to a given domain, that should be the end of it.
Created attachment 312322 [details] Patch
<rdar://problem/32381046>
Thanks for the review. > > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:216 > > + if (deniedRequest.frameID != frameID) > > + continue; > > This seems wrong to me. A page could re-prompt the user infinitely by > inserting and removing iframes with getUserMedia calls. Once the user says > no to a given domain, that should be the end of it. This departs from the current model, which I think should be simplified. We can add back the flexibility if we discover the need for it. This would mean: - Only care about the main frame for both predenied/pregranted requests - Only care about the main frame security origin at UIProcess/Client level, frames with different origins are not allowed to do gum calls anyway. - Reflect in client API that we are only searching for camera/mic access, not per device choice. We should reflect all of that in our IPC and Client APIs. I will update the patch with just the deny case now.
Created attachment 312326 [details] Patch for landing
Created attachment 312333 [details] Patch for landing
> This departs from the current model, which I think should be simplified. > > We can add back the flexibility if we discover the need for it. > > This would mean: > - Only care about the main frame for both predenied/pregranted requests > - Only care about the main frame security origin at UIProcess/Client level, > frames with different origins are not allowed to do gum calls anyway. I'm not an expert on how WebRTC permissions usually work. I wonder what other browsers do. Other web APIs like storage and geolocation grant or deny permission per domain, rather than per frame or per function call. I can see some value in prompting more for WebRTC than for storage or geolocation, since we consider storage and geolocation to be optional APIs whose absence shouldn't break a site, whereas absence of WebRTC will probably break a WebRTC site. My intuition is that one prompt per main document is probably the right policy to start with.
Comment on attachment 312333 [details] Patch for landing Clearing flags on attachment: 312333 Committed r217945: <http://trac.webkit.org/changeset/217945>
All reviewed patches have been landed. Closing bug.
(In reply to WebKit Commit Bot from comment #9) > Comment on attachment 312333 [details] > Patch for landing > > Clearing flags on attachment: 312333 > > Committed r217945: <http://trac.webkit.org/changeset/217945> This broke the GTK+ build with GCC-4.9 : https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29/builds/2570/steps/compile-webkit/logs/stdio I suspect its related to the same issue with GCC-4.9 that Yusuke fixed 2 days ago https://trac.webkit.org/changeset/217829/
(In reply to Carlos Alberto Lopez Perez from comment #11) > (In reply to WebKit Commit Bot from comment #9) > > Comment on attachment 312333 [details] > > Patch for landing > > > > Clearing flags on attachment: 312333 > > > > Committed r217945: <http://trac.webkit.org/changeset/217945> > > This broke the GTK+ build with GCC-4.9 : > https://build.webkit.org/builders/GTK%20Linux%2064- > bit%20Release%20Debian%20Stable%20%28Build%29/builds/2570/steps/compile- > webkit/logs/stdio > > I suspect its related to the same issue with GCC-4.9 that Yusuke fixed 2 > days ago https://trac.webkit.org/changeset/217829/ Ah, sorry about that. I won't be able to fix it until tomorrow...
Looking at Yusuke patch, you might indeed be right. If nobody beats me, I'll try to fix it tomorrow
(In reply to youenn fablet from comment #13) > Looking at Yusuke patch, you might indeed be right. If nobody beats me, I'll > try to fix it tomorrow (In reply to youenn fablet from comment #12) > (In reply to Carlos Alberto Lopez Perez from comment #11) > > (In reply to WebKit Commit Bot from comment #9) > > > Comment on attachment 312333 [details] > > > Patch for landing > > > > > > Clearing flags on attachment: 312333 > > > > > > Committed r217945: <http://trac.webkit.org/changeset/217945> > > > > This broke the GTK+ build with GCC-4.9 : > > https://build.webkit.org/builders/GTK%20Linux%2064- > > bit%20Release%20Debian%20Stable%20%28Build%29/builds/2570/steps/compile- > > webkit/logs/stdio > > > > I suspect its related to the same issue with GCC-4.9 that Yusuke fixed 2 > > days ago https://trac.webkit.org/changeset/217829/ > > Ah, sorry about that. I won't be able to fix it until tomorrow... No problem: The GTK+ EWS and the GTK+ release and debug bots run a more recent GCC version (6.3). We have this bot with GCC 4.9 because its our minimum version supported of the compiler, so we can test that WebKit remains build-able with that. So, no hurries at all.
I think in this particular case the default initializers are redundant so it's safe to remove them. The struct is only initialized in one place with all members explicitly. I've committed https://trac.webkit.org/changeset/217974/webkit, please feel free to change if you disagree and prefer to have the default initializers and a constructor as well.
All struct or class members should always be initialized by default or by the constructor, we should not be playing games with possible use of uninitialized memory. The code instantiating the struct should not have to remember to initialize everything. We know this does not work in practice.