Clear a page's cached gUM prompt state, so the user will be prompted again on the next call to getUserMedia, after: - a stream has been active for 24 hours - no stream has been active for a platform specific interval (defaults to 1 minute on iOS, 10 minutes on other platforms, settable with a WK2 setting)
<rdar://problem/32742356>
Created attachment 324739 [details] Proposed patch.
Comment on attachment 324739 [details] Proposed patch. Patch lgtm, not applying so... View in context: https://bugs.webkit.org/attachment.cgi?id=324739&action=review > Source/WebKit/Shared/WebPreferencesDefinitions.h:315 > + macro(MediaCaptureInactiveStreamRepromptInterval, mediaCaptureInactiveStreamRepromptInterval, Double, double, DEFAULT_MEDIA_CAPTURE_INTERACTIVE_STREAM_REPROMPT_INTERVAL, "", "") \ Are we usually using minutes and not seconds for intervals/timeouts values? > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:414 > + interval = Seconds::fromHours(24); Shouldn't we try to put the default long timeout value in a better place, maybe in a macro in WebPreferenceDefinitions? > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:422 > + m_watchdogTimer.startOneShot(m_currentWatchdogInterval); Are there cases where we should stop the timer like in destructor or is it handled on its own? > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:434 > + ASSERT(!(m_captureState & activeCaptureMask)); Cannot this assertion happens if there is a live webrtc session for more than 24 hours? > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:437 > + m_pregrantedRequests.clear(); We should also clear m_deniedRequests. > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:438 > + line not needed > Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:444 > +// Defaults to 8 minutes. Is it still true?
Comment on attachment 324739 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=324739&action=review >> Source/WebKit/Shared/WebPreferencesDefinitions.h:315 >> + macro(MediaCaptureInactiveStreamRepromptInterval, mediaCaptureInactiveStreamRepromptInterval, Double, double, DEFAULT_MEDIA_CAPTURE_INTERACTIVE_STREAM_REPROMPT_INTERVAL, "", "") \ > > Are we usually using minutes and not seconds for intervals/timeouts values? I don't think there is a preference one way or the other, and I think expressing 5 minutes as "5*60" is helpful, but I changed the preference name to "inactiveMediaCaptureSteamRepromptIntervalInMinutes" to make the units obvious. >> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:414 >> + interval = Seconds::fromHours(24); > > Shouldn't we try to put the default long timeout value in a better place, maybe in a macro in WebPreferenceDefinitions? I added a preference for it. >> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:422 >> + m_watchdogTimer.startOneShot(m_currentWatchdogInterval); > > Are there cases where we should stop the timer like in destructor or is it handled on its own? A timer's destructor stops itself. >> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:434 >> + ASSERT(!(m_captureState & activeCaptureMask)); > > Cannot this assertion happens if there is a live webrtc session for more than 24 hours? Indeed, removed! >> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:437 >> + m_pregrantedRequests.clear(); > > We should also clear m_deniedRequests. I disagree. If a user disallows capture, we should remember that setting until the page is reloaded. >> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:438 >> + > > line not needed Fixed. >> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:444 >> +// Defaults to 8 minutes. > > Is it still true? Nope, fixed.
Created attachment 324860 [details] Rebased and updated patch.
Comment on attachment 324860 [details] Rebased and updated patch. LGTM. I am still not sure whether we should not remove the denied requests decisions after some timeouts. The timer will only fire if user granted either audio or video. So there will be a prompt for one or the other. Then, why not allowing the application to request both?
(In reply to youenn fablet from comment #6) > Comment on attachment 324860 [details] > Rebased and updated patch. > > LGTM. > > I am still not sure whether we should not remove the denied requests > decisions after some timeouts. > The timer will only fire if user granted either audio or video. > So there will be a prompt for one or the other. > Then, why not allowing the application to request both? Lets land this and talk about the issue.
Comment on attachment 324860 [details] Rebased and updated patch. Clearing flags on attachment: 324860 Committed r223988: <https://trac.webkit.org/changeset/223988>
All reviewed patches have been landed. Closing bug.
The API test added with this change is failing: FAIL WebKit2.GetUserMediaReprompt /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/WebKit/GetUserMediaReprompt.mm:80 Value of: [result boolValue] Actual: false Expected: true /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/WebKit/GetUserMediaReprompt.mm:89 Value of: [result boolValue] Actual: false Expected: true https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20(Tests)/builds/5281
Created attachment 325029 [details] Test fix.
Comment on attachment 325029 [details] Test fix. Clearing flags on attachment: 325029 Committed r224044: <https://trac.webkit.org/changeset/224044>