Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2123863004:
ScreenCapture for Android phase1, part II (Closed)
|
Created:
4 years, 5 months ago by braveyao Modified:
4 years, 2 months ago Reviewers:
Peter Kasting, Sergey Ulanov, piman, Ted C, tsergeant, raymes, miu, Alexei Svitkine (slow), gone CC:
chromium-reviews, msramek+watch_chromium.org, asvitkine+watch_chromium.org, posciak+watch_chromium.org, jam, raymes+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, mlamouri+watch-permissions_chromium.org, markusheintz_, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionScreenCapture for Android phase1, part II
The capture part has been landed in cl https://codereview.chromium.org/2125973002/.
This cl is to implement the control part in chrome/ and content/. The document
is here, https://goo.gl/QNH29g.
This cl proposes a pemission design on Android, including permission info bar
and head up notification, which will show up each time at starting screen capture
and user can check/stop the capture without leaving current page.
And all of these is behind a flag as an experimental feature.
BUG=487935
Committed: https://crrev.com/dbb85e451ee0ffe9d18300befb9c0d0664b5ee94
Cr-Commit-Position: refs/heads/master@{#420679}
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix problem with adding content setting type #
Total comments: 4
Patch Set 3 : address comments in components/content_settings/ #
Total comments: 15
Patch Set 4 : address comments #Patch Set 5 : rebase to #407492(Jul 25) #
Total comments: 27
Patch Set 6 : rebase to #409214 (Aug 2) #Patch Set 7 : address comments #Patch Set 8 : UI tweaking and rebase #
Total comments: 20
Patch Set 9 : address comments and rebase #
Total comments: 25
Patch Set 10 : address comments #Patch Set 11 : rebase to #412340(Aug16) #
Total comments: 6
Patch Set 12 : address comments #
Total comments: 19
Patch Set 13 : address comments #Patch Set 14 : remove modification to peer connection tracker #Patch Set 15 : rebase to Aug 30 #
Total comments: 2
Patch Set 16 : rebase to Sep 9 #Patch Set 17 : fix nits #
Total comments: 2
Patch Set 18 : adopt base::Feature instead of switches #
Total comments: 8
Patch Set 19 : add a new infobarDelegate for screen capture #
Total comments: 4
Patch Set 20 : rebase to Sep15 to make trybots working #Patch Set 21 : fix nits #Patch Set 22 : rebase to Sep19 to pass dry run #
Total comments: 2
Patch Set 23 : fix a rebase error in histograms.xml: moved two newly added definitions #
Total comments: 2
Patch Set 24 : address comments #Patch Set 25 : add entry in LoginCustomFlags as bots request #
Total comments: 38
Patch Set 26 : address comments #Patch Set 27 : address comments #Patch Set 28 : rebase to Sep21 #Patch Set 29 : adopt ResetAndReturn #
Total comments: 12
Patch Set 30 : address comments #Messages
Total messages: 231 (164 generated)
braveyao@chromium.org changed reviewers: + miu@chromium.org, sergeyu@chromium.org
Hi Sergey and Miu, Here is the part II of screen capture on Android. Could you please take an initial look?
msramek@chromium.org changed reviewers: + msramek@chromium.org
https://codereview.chromium.org/2123863004/diff/1/components/content_settings... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/2123863004/diff/1/components/content_settings... components/content_settings/core/common/content_settings_types.h:35: CONTENT_SETTINGS_TYPE_MEDIASTREAM_SCREEN, Drive by! 1. The platform #ifdef does not belong here, please remove it (instead, make a corresponding entry in ContentSettingsRegistry and specify the platform there). 2. Please add a corresponding entry to the histogram enum in content_settings_types.cc. 3. New content settings are normally added to the end of the enum. I can imagine making an exception here, but since new entries in kHistogramOrder[] in ContentSettingsType *must* be added to the end, perhaps it's better to add this to the end as well to keep them in sync.
Hi msramek@, thanks so much for your comments. I didn't notice there are some other content settings which aren't used to store any data. Now I followed their practice here too. Please help to take a look the changes in components/content_settings. https://codereview.chromium.org/2123863004/diff/1/components/content_settings... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/2123863004/diff/1/components/content_settings... components/content_settings/core/common/content_settings_types.h:35: CONTENT_SETTINGS_TYPE_MEDIASTREAM_SCREEN, On 2016/07/08 10:56:48, msramek wrote: > Drive by! > > 1. The platform #ifdef does not belong here, please remove it (instead, make a > corresponding entry in ContentSettingsRegistry and specify the platform there). > > 2. Please add a corresponding entry to the histogram enum in > content_settings_types.cc. > > 3. New content settings are normally added to the end of the enum. I can imagine > making an exception here, but since new entries in kHistogramOrder[] in > ContentSettingsType *must* be added to the end, perhaps it's better to add this > to the end as well to keep them in sync. Done.
raymes@chromium.org changed reviewers: + raymes@chromium.org
On 2016/07/11 21:42:58, wrote: > Hi msramek@, thanks so much for your comments. I didn't > notice there are some other content settings which aren't > used to store any data. Now I followed their practice here too. Hey braveyao! Have you chatted about the UI for this permission with UX or the security enamel team? It could be good to email chrome-security-enamel with your plans. I'm also curious about what you mean by "I didn't notice there are some other content settings which aren't used to store any data". Will this content setting not be used to store any data? If not, what will it be used for? Thanks! https://codereview.chromium.org/2123863004/diff/20001/components/content_sett... File components/content_settings/core/browser/content_settings_registry_unittest.cc (right): https://codereview.chromium.org/2123863004/diff/20001/components/content_sett... components/content_settings/core/browser/content_settings_registry_unittest.cc:58: #endif nit: this should have an #else too (as above) https://codereview.chromium.org/2123863004/diff/20001/components/content_sett... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/2123863004/diff/20001/components/content_sett... components/content_settings/core/common/content_settings.cc:59: #if defined(OS_ANDROID) nit: I think we can remove the ifdef here
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
The CQ bit was unchecked by braveyao@chromium.org
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
The CQ bit was unchecked by braveyao@chromium.org
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
The CQ bit was unchecked by braveyao@chromium.org
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
raymes@, thanks for the comments! PTAL. The reason for adding CONTENT_SETTINGS_TYPE_MEDIASTREAM_SCREEN is simply to distinguish the stream of screen capture from stream of camera/mic, (CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA/MIC, which are used for stream type too), because they are similar and will go through same code path. For screen capture, we will show the permission bar every time when it's requested. So no data will be stored. And we (actually my manager did mostly) did discussed this proposal with several other teams since it's very security sensitive. https://codereview.chromium.org/2123863004/diff/20001/components/content_sett... File components/content_settings/core/browser/content_settings_registry_unittest.cc (right): https://codereview.chromium.org/2123863004/diff/20001/components/content_sett... components/content_settings/core/browser/content_settings_registry_unittest.cc:58: #endif On 2016/07/13 02:44:09, raymes wrote: > nit: this should have an #else too (as above) Done. https://codereview.chromium.org/2123863004/diff/20001/components/content_sett... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/2123863004/diff/20001/components/content_sett... components/content_settings/core/common/content_settings.cc:59: #if defined(OS_ANDROID) On 2016/07/13 02:44:09, raymes wrote: > nit: I think we can remove the ifdef here Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Thanks braveyao@ I'm curious who you've chatted to from the UX side? Also, will there be an option to toggle screen capture globally in the android settings? Thanks!
On 2016/07/14 03:33:43, raymes wrote: > Thanks braveyao@ > > I'm curious who you've chatted to from the UX side? > > Also, will there be an option to toggle screen capture globally in the android > settings? > > Thanks! Hi raymes, I don't have any contact. niklase@ should have. I can check with him when he is back from vacation. Currently MediaProjection API, which this screen capture relies on, has its own permission implementation that is before the runtime permissions were built. You must call startActivityForResult() to popup a dialogue for user input for each screen capture request before you can retrieve MediaProjection. Android team is planning to move the MediaProjection infrastructure over to runtime permissions in O release. Then it will be unified with Camera/Mic permissions, I suppose. The doc, https://goo.gl/QNH29g, has more details and concerns. whatever, there will be many modifications to this experimental feature in the future.
I'm not that qualified to code review all the UX aspects of this change. sergeyu@ (or maybe others?) would be more experienced with those aspects. Also, have you talked to a UX designer? Messaging and consistency w.r.t. related product feature are important considerations here. That said, one major concern I have is that you seem to be mixing the concepts of tab capture with screen capture (especially in MediaStreamCaptureIndicator). I'm don't understand why Android screen capture would be associated with *any* WebContents instance, since screen capture spans ALL browser tabs and ALL apps on the device. Other than that, here are a few specific things I noticed while doing a quick scan on the rest of the change: https://codereview.chromium.org/2123863004/diff/60001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2123863004/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2396: The <ph name="URL_OF_THE_CURRENT_TAB">%1$s<ex>https://apprtc.appspot.com</ex></ph> is sharing your screen now Please end this sentence with a period. https://codereview.chromium.org/2123863004/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2396: The <ph name="URL_OF_THE_CURRENT_TAB">%1$s<ex>https://apprtc.appspot.com</ex></ph> is sharing your screen now Should this be the URL or the Title of the tab? https://codereview.chromium.org/2123863004/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2123863004/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:5593: + Enabls this option for experimental ScreenCapture feature on Android. Please fix typo: s/Enabls/Enable/ https://codereview.chromium.org/2123863004/diff/60001/chrome/browser/media/me... File chrome/browser/media/media_stream_capture_indicator.cc (right): https://codereview.chromium.org/2123863004/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_capture_indicator.cc:192: if (IsMirroringType(it->type)) { Not sure if it's safe/correct to start mixing-in desktop capture here (in what was once a tab-capture only code path). sergeyu@, WDYT? https://codereview.chromium.org/2123863004/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_capture_indicator.cc:242: bool MediaStreamCaptureIndicator::WebContentsDeviceUsage::IsMirroringType( This belongs here: https://cs.chromium.org/chromium/src/content/public/common/media_stream_reque... Also, it should include all of the types, regardless of platform. Coincidentally, I have another CL I prepared a few months back, but haven't had time to work on, that does exactly this. Here's the function I was going to add to MSR.h: CONTENT_EXPORT bool IsContentCaptureMediaType(MediaStreamType type); ...and in MSR.cc: bool IsContentCaptureMediaType(MediaStreamType type) { return (type == content::MEDIA_TAB_AUDIO_CAPTURE || type == content::MEDIA_TAB_VIDEO_CAPTURE || type == content::MEDIA_DESKTOP_AUDIO_CAPTURE || type == content::MEDIA_DESKTOP_VIDEO_CAPTURE); } https://codereview.chromium.org/2123863004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/peer_connection_tracker_host.cc (right): https://codereview.chromium.org/2123863004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/peer_connection_tracker_host.cc:118: video_constraints.find(kMediaStreamSourceScreen) != std::string::npos; This feels fragile. Is the |video_constraints| string something that needs to be parsed, because it contains formatted/serialized structured data? https://codereview.chromium.org/2123863004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/peer_connection_tracker_host.h (right): https://codereview.chromium.org/2123863004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/peer_connection_tracker_host.h:62: bool isScreenCapture_; style: bool is_screen_capture_; https://codereview.chromium.org/2123863004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2123863004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:591: #if defined(OS_ANDROID) Use #elif here instead.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
miu@, thanks so much for the helpful comments! All addressed. PTAL! And I didn't intend to mix tab capture with screen capture. Just because they seem to be same 'mirroring' type and can reuse the codes. Hope it's not a problem. sergeyu@, any more comments? https://codereview.chromium.org/2123863004/diff/60001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2123863004/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2396: The <ph name="URL_OF_THE_CURRENT_TAB">%1$s<ex>https://apprtc.appspot.com</ex></ph> is sharing your screen now On 2016/07/14 22:25:52, miu wrote: > Please end this sentence with a period. Period will be added later, not here, same as audio/video call notifications(i.e. see 'IDS_VIDEO_AUDIO_CALL_NOTIFICATION_TEXT_2') https://codereview.chromium.org/2123863004/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2396: The <ph name="URL_OF_THE_CURRENT_TAB">%1$s<ex>https://apprtc.appspot.com</ex></ph> is sharing your screen now On 2016/07/14 22:25:52, miu wrote: > Should this be the URL or the Title of the tab? It's URL. See 'IDS_MEDIA_NOTIFICATION_LINK_TEXT'. https://codereview.chromium.org/2123863004/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2123863004/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:5593: + Enabls this option for experimental ScreenCapture feature on Android. On 2016/07/14 22:25:52, miu wrote: > Please fix typo: s/Enabls/Enable/ Done. https://codereview.chromium.org/2123863004/diff/60001/chrome/browser/media/me... File chrome/browser/media/media_stream_capture_indicator.cc (right): https://codereview.chromium.org/2123863004/diff/60001/chrome/browser/media/me... chrome/browser/media/media_stream_capture_indicator.cc:242: bool MediaStreamCaptureIndicator::WebContentsDeviceUsage::IsMirroringType( On 2016/07/14 22:25:52, miu wrote: > This belongs here: > https://cs.chromium.org/chromium/src/content/public/common/media_stream_reque... > > Also, it should include all of the types, regardless of platform. > Coincidentally, I have another CL I prepared a few months back, but haven't had > time to work on, that does exactly this. Here's the function I was going to add > to MSR.h: > > CONTENT_EXPORT bool IsContentCaptureMediaType(MediaStreamType type); > > ...and in MSR.cc: > > bool IsContentCaptureMediaType(MediaStreamType type) { > return (type == content::MEDIA_TAB_AUDIO_CAPTURE || > type == content::MEDIA_TAB_VIDEO_CAPTURE || > type == content::MEDIA_DESKTOP_AUDIO_CAPTURE || > type == content::MEDIA_DESKTOP_VIDEO_CAPTURE); > } Done. https://codereview.chromium.org/2123863004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/peer_connection_tracker_host.cc (right): https://codereview.chromium.org/2123863004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/peer_connection_tracker_host.cc:118: video_constraints.find(kMediaStreamSourceScreen) != std::string::npos; On 2016/07/14 22:25:53, miu wrote: > This feels fragile. Is the |video_constraints| string something that needs to be > parsed, because it contains formatted/serialized structured data? Done. Found a more solid method: parse the blink::WebUserMediaRequest in peer_connection_tracker.cc and send the source type to here. https://codereview.chromium.org/2123863004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/peer_connection_tracker_host.h (right): https://codereview.chromium.org/2123863004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/peer_connection_tracker_host.h:62: bool isScreenCapture_; On 2016/07/14 22:25:53, miu wrote: > style: bool is_screen_capture_; Done. https://codereview.chromium.org/2123863004/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2123863004/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:591: #if defined(OS_ANDROID) On 2016/07/14 22:25:53, miu wrote: > Use #elif here instead. Done.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/18 20:46:31, braveyao wrote: > miu@, thanks so much for the helpful comments! All addressed. PTAL! Looks good now, thanks. > And I didn't intend to mix tab capture with screen capture. Just because they > seem to be same 'mirroring' type and can reuse the codes. Hope it's not a > problem. > sergeyu@, any more comments? I'm curious to see what Sergey says about these changes since I, personally, need more context/understanding to feel comfortable giving this change an ell-gee-tee-em.
Looking at this CL now.
https://codereview.chromium.org/2123863004/diff/100001/chrome/browser/media/m... File chrome/browser/media/media_stream_capture_indicator.h (right): https://codereview.chromium.org/2123863004/diff/100001/chrome/browser/media/m... chrome/browser/media/media_stream_capture_indicator.h:58: // Called when STOP button in medida capture notification is clicked. typo: medida https://codereview.chromium.org/2123863004/diff/100001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2123863004/diff/100001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:205: if (request.video_type == content::MEDIA_DESKTOP_VIDEO_CAPTURE) { nit: Other single-line if statements in this file don't have braces, remove them here for consistency https://codereview.chromium.org/2123863004/diff/100001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:256: message_id = IDS_MEDIA_SCREEN_CAPTURE_CONFIRMATION_TEXT; please add braces here and for else-if cases below. https://codereview.chromium.org/2123863004/diff/100001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:500: devices.push_back(content::MediaStreamDevice( This reads as if we always allow screen capture, even when access was denied. This looks looks wrong to me. Overall the logic in this function isn't very clear. Maybe update the comment above? https://codereview.chromium.org/2123863004/diff/100001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/2123863004/diff/100001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:339: Register(CONTENT_SETTINGS_TYPE_MEDIASTREAM_SCREEN, "media-stream-screen", Do we need to register it when the feature is not enabled? https://codereview.chromium.org/2123863004/diff/100001/components/content_set... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/2123863004/diff/100001/components/content_set... components/content_settings/core/common/content_settings_types.h:49: CONTENT_SETTINGS_TYPE_MEDIASTREAM_SCREEN, Why do we need this content setting? Is it only for the case the user checks "Don't show again" check box on the permission prompt? https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.cc (right): https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.cc:117: is_screen_capture_ = There can be multiple media streams, and as far as I can tell the same PeerConnectionTrackerHost() will be used for all of them, so I don't think you can store the state of in a single flag like this. Beside that you set the value on IO thread here, but then use it on UI thread without any synchronization. https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.cc:131: if (host && !is_screen_capture_) See my comment above. I think you always want to send OnSuspend message, but handle it differently in the renderer, depending on media stream type. https://codereview.chromium.org/2123863004/diff/100001/content/public/common/... File content/public/common/media_stream_request.cc (right): https://codereview.chromium.org/2123863004/diff/100001/content/public/common/... content/public/common/media_stream_request.cc:28: type == content::MEDIA_DESKTOP_VIDEO_CAPTURE); don't need to specify namespace, because this code is in content namespace. Also remove content:: from he functions above. https://codereview.chromium.org/2123863004/diff/100001/content/public/common/... File content/public/common/media_stream_request.h (right): https://codereview.chromium.org/2123863004/diff/100001/content/public/common/... content/public/common/media_stream_request.h:90: CONTENT_EXPORT bool IsContentCaptureMediaType(MediaStreamType type); ContentCapture is ambiguous. Maybe call it IsDesktopCaptureMediaType()?
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks so much sergeyu@. All comments are addressed. PTAL! BTW: We didn't contact UX team before. Now we are discussing with Rebecca Rolfe(rolfe@). There will be a little tweaking of icons in next patchset. https://codereview.chromium.org/2123863004/diff/100001/chrome/browser/media/m... File chrome/browser/media/media_stream_capture_indicator.h (right): https://codereview.chromium.org/2123863004/diff/100001/chrome/browser/media/m... chrome/browser/media/media_stream_capture_indicator.h:58: // Called when STOP button in medida capture notification is clicked. On 2016/08/01 18:20:36, Sergey Ulanov wrote: > typo: medida Done. https://codereview.chromium.org/2123863004/diff/100001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2123863004/diff/100001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:205: if (request.video_type == content::MEDIA_DESKTOP_VIDEO_CAPTURE) { On 2016/08/01 18:20:36, Sergey Ulanov wrote: > nit: Other single-line if statements in this file don't have braces, remove them > here for consistency Done. https://codereview.chromium.org/2123863004/diff/100001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:256: message_id = IDS_MEDIA_SCREEN_CAPTURE_CONFIRMATION_TEXT; On 2016/08/01 18:20:36, Sergey Ulanov wrote: > please add braces here and for else-if cases below. Done. https://codereview.chromium.org/2123863004/diff/100001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller.cc:500: devices.push_back(content::MediaStreamDevice( On 2016/08/01 18:20:36, Sergey Ulanov wrote: > This reads as if we always allow screen capture, even when access was denied. > This looks looks wrong to me. > Overall the logic in this function isn't very clear. Maybe update the comment > above? Done. https://codereview.chromium.org/2123863004/diff/100001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/2123863004/diff/100001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:339: Register(CONTENT_SETTINGS_TYPE_MEDIASTREAM_SCREEN, "media-stream-screen", On 2016/08/01 18:20:36, Sergey Ulanov wrote: > Do we need to register it when the feature is not enabled? This setting is invisible to users. Should be OK. https://codereview.chromium.org/2123863004/diff/100001/components/content_set... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/2123863004/diff/100001/components/content_set... components/content_settings/core/common/content_settings_types.h:49: CONTENT_SETTINGS_TYPE_MEDIASTREAM_SCREEN, On 2016/08/01 18:20:36, Sergey Ulanov wrote: > Why do we need this content setting? Is it only for the case the user checks > "Don't show again" check box on the permission prompt? The reason is we use the definition to distinguish the stream type too. See chrome/browser/media/media_stream_infobar_delegate_android.cc. That's the only reason I add it here. Otherwise it's not needed since no content setting for screen capture at all. https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.cc (right): https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.cc:117: is_screen_capture_ = On 2016/08/01 18:20:36, Sergey Ulanov wrote: > There can be multiple media streams, and as far as I can tell the same > PeerConnectionTrackerHost() will be used for all of them, so I don't think you > can store the state of in a single flag like this. Beside that you set the value > on IO thread here, but then use it on UI thread without any synchronization. Done. Single flag is necessary on Android. During screen sharing, Chrome may be minimized to backgound which will trigger the OnSuspend() callback here. Since MediaProjection doesn't have audio capture, there usually is another peerconnection for audio/video on the same page, e.g. under same PeerConnectionTrackerHost. We shouldn't cut screensharing, also shouldn't cut the audio session. (video session will be mute after Chrome goes to background), which means we have to keep these two peerconnection running for such a scenario. https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.cc:131: if (host && !is_screen_capture_) On 2016/08/01 18:20:36, Sergey Ulanov wrote: > See my comment above. I think you always want to send OnSuspend message, but > handle it differently in the renderer, depending on media stream type. see comments above. https://codereview.chromium.org/2123863004/diff/100001/content/public/common/... File content/public/common/media_stream_request.cc (right): https://codereview.chromium.org/2123863004/diff/100001/content/public/common/... content/public/common/media_stream_request.cc:28: type == content::MEDIA_DESKTOP_VIDEO_CAPTURE); On 2016/08/01 18:20:36, Sergey Ulanov wrote: > don't need to specify namespace, because this code is in content namespace. Also > remove content:: from he functions above. Done. https://codereview.chromium.org/2123863004/diff/100001/content/public/common/... File content/public/common/media_stream_request.h (right): https://codereview.chromium.org/2123863004/diff/100001/content/public/common/... content/public/common/media_stream_request.h:90: CONTENT_EXPORT bool IsContentCaptureMediaType(MediaStreamType type); On 2016/08/01 18:20:36, Sergey Ulanov wrote: > ContentCapture is ambiguous. Maybe call it IsDesktopCaptureMediaType()? Done. I think content capture is better, since tab capture and desktop capture sounds like two individual types, but both means Content. Or call it IsScreenCaptureMediaType()?
https://codereview.chromium.org/2123863004/diff/100001/content/public/common/... File content/public/common/media_stream_request.h (right): https://codereview.chromium.org/2123863004/diff/100001/content/public/common/... content/public/common/media_stream_request.h:90: CONTENT_EXPORT bool IsContentCaptureMediaType(MediaStreamType type); On 2016/08/03 00:23:47, braveyao wrote: > On 2016/08/01 18:20:36, Sergey Ulanov wrote: > > ContentCapture is ambiguous. Maybe call it IsDesktopCaptureMediaType()? > Done. > I think content capture is better, since tab capture and desktop capture sounds > like two individual types, but both means Content. > Or call it IsScreenCaptureMediaType()? IsDesktopCaptureMediaType() is wrong since it's inaccurate. How about IsScreenCaptureMediaType() then?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:160001) has been deleted
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
braveyao@chromium.org changed reviewers: + dfalcantara@chromium.org - msramek@chromium.org
Thanks for all the pre-review comments! After discussion with UI team, I tweaked the UI a bit and updated the doc accordingly. Now it's time to kick off the review! sergeyu@, please take a look at chrome/browser/ and other related. dfalcantara@, please take a look at chrome/android/ and other related. These two are the major parts of this cl relied on by other modifications, so please review first. Thanks a lot! https://codereview.chromium.org/2123863004/diff/100001/content/public/common/... File content/public/common/media_stream_request.h (right): https://codereview.chromium.org/2123863004/diff/100001/content/public/common/... content/public/common/media_stream_request.h:90: CONTENT_EXPORT bool IsContentCaptureMediaType(MediaStreamType type); On 2016/08/03 00:36:52, miu wrote: > On 2016/08/03 00:23:47, braveyao wrote: > > On 2016/08/01 18:20:36, Sergey Ulanov wrote: > > > ContentCapture is ambiguous. Maybe call it IsDesktopCaptureMediaType()? > > Done. > > I think content capture is better, since tab capture and desktop capture > sounds > > like two individual types, but both means Content. > > Or call it IsScreenCaptureMediaType()? > > IsDesktopCaptureMediaType() is wrong since it's inaccurate. How about > IsScreenCaptureMediaType() then? Done. Prefer to use IsScreenCaptureMediaType().
More questions than comments. Has this gone through UX, at least for a string pass? https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java (right): https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:40: private static final String ACTION_MEDIA_CAPTURE_UPDATE = nit: Alphabetize these. https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:202: // To screen capture notification, add Stop action button and enable head up 1) Add a "Stop" button to the screen capture notification. If necessary, turn the notification into a high priority one. 2) "heads-up", not "head up". "headsup" for the variable name. https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:205: // Enable the head up notification. nit: Don't need to say that you're enabling the heads-up notification immediately after the other comment. https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java (right): https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java:79: private static SparseArray<WebContents> sWebContentsMap = new SparseArray<WebContents>(); 1) private static final 2) Use javadoc syntax: /** Static array for blah blah. */ https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java:237: sWebContentsMap.put(mTab.getId(), mTab.getWebContents()); 1) Tabs can swap their WebContents, but you don't seem to be accounting for this. 2) You use mTab.getWebContents() here and in isCapturingScreen(), but you use the map in notifyStopped. Why? https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/android/... File chrome/android/java/strings/android_chrome_strings.grd (right): https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/android/... chrome/android/java/strings/android_chrome_strings.grd:2407: The <ph name="URL_OF_THE_CURRENT_TAB">%1$s<ex>https://apprtc.appspot.com</ex></ph> is sharing your screen now This string doesn't make sense, grammatically. Has this been run through UX? "The https://google.com is sharing your screen now" https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/app/gene... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/app/gene... chrome/app/generated_resources.grd:5613: + <message name="IDS_FLAGS_MEDIA_SCREEN_CAPTURE_NAME" desc="Name of chrome:flags option to enable screen capture."> Add this to both of your about:flags strings: translateable="false" If you don't, translators will try to translate this. https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/browser/... File chrome/browser/android/tab_web_contents_delegate_android.cc (right): https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/browser/... chrome/browser/android/tab_web_contents_delegate_android.cc:495: ->GetMediaStreamCaptureIndicator(); Pull this code out instead of duplicating it four times (IsCapturingAudio/video/Screen & NotifyStopped all use it). https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/browser/... File chrome/browser/media/permission_bubble_media_access_handler.cc (right): https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/browser/... chrome/browser/media/permission_bubble_media_access_handler.cc:117: #if BUILDFLAG(ANDROID_JAVA_UI) Why are you using ANDROID_JAVA_UI instead of OS_ANDROID? https://chromiumcodereview.appspot.com/2123863004/diff/180001/chrome/browser/... chrome/browser/media/permission_bubble_media_access_handler.cc:158: !controller->IsAskingForScreenCapture()) { Why is this new check necessary? Nothing below (not even the comment) changed.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Patchset #9 (id:200001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Looks better, but you missed some of my questions on your previous patch set. https://chromiumcodereview.appspot.com/2123863004/diff/220001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java (right): https://chromiumcodereview.appspot.com/2123863004/diff/220001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java:491: **/ nit: bottom javadoc never has two * https://chromiumcodereview.appspot.com/2123863004/diff/220001/chrome/app/gene... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/2123863004/diff/220001/chrome/app/gene... chrome/app/generated_resources.grd:5616: + <message name="IDS_FLAGS_MEDIA_SCREEN_CAPTURE_NAME" desc="Name of chrome:flags option to enable screen capture." translateable="false"> nit: use single space before translateable="false" instead of two https://chromiumcodereview.appspot.com/2123863004/diff/220001/chrome/app/gene... chrome/app/generated_resources.grd:13010: + share your screen Shouldn't "share" be capitalized? https://chromiumcodereview.appspot.com/2123863004/diff/220001/chrome/browser/... File chrome/browser/media/permission_bubble_media_access_handler.cc (right): https://chromiumcodereview.appspot.com/2123863004/diff/220001/chrome/browser/... chrome/browser/media/permission_bubble_media_access_handler.cc:82: #if BUILDFLAG(ANDROID_JAVA_UI) Same question as before about why you're not using OS_ANDROID. https://chromiumcodereview.appspot.com/2123863004/diff/220001/chrome/browser/... chrome/browser/media/permission_bubble_media_access_handler.cc:158: !controller->IsAskingForScreenCapture()) { Same question as the previous patch set.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Patchset #9 (id:220001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
braveyao@chromium.org changed reviewers: + tedchoc@chromium.org
Thanks so much, dfalcantara@! All comments are addressed/answered. PTAL! Also add tedchoc@ as suggested for another look! https://codereview.chromium.org/2123863004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java (right): https://codereview.chromium.org/2123863004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:40: private static final String ACTION_MEDIA_CAPTURE_UPDATE = On 2016/08/10 19:04:02, dfalcantara wrote: > nit: Alphabetize these. Done. https://codereview.chromium.org/2123863004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:202: // To screen capture notification, add Stop action button and enable head up On 2016/08/10 19:04:02, dfalcantara wrote: > 1) Add a "Stop" button to the screen capture notification. If necessary, turn > the > notification into a high priority one. > > 2) "heads-up", not "head up". "headsup" for the variable name. Done. https://codereview.chromium.org/2123863004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:205: // Enable the head up notification. On 2016/08/10 19:04:02, dfalcantara wrote: > nit: Don't need to say that you're enabling the heads-up notification > immediately after the other comment. Done. https://codereview.chromium.org/2123863004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java (right): https://codereview.chromium.org/2123863004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java:79: private static SparseArray<WebContents> sWebContentsMap = new SparseArray<WebContents>(); On 2016/08/10 19:04:02, dfalcantara wrote: > 1) private static final > 2) Use javadoc syntax: > /** Static array for blah blah. */ Done. Removed. https://codereview.chromium.org/2123863004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java:237: sWebContentsMap.put(mTab.getId(), mTab.getWebContents()); On 2016/08/10 19:04:02, dfalcantara wrote: > 1) Tabs can swap their WebContents, but you don't seem to be accounting for > this. > > 2) You use mTab.getWebContents() here and in isCapturingScreen(), but you use > the map > in notifyStopped. Why? Done. Use the newly added TabWindowManager#getTabById() API now. https://codereview.chromium.org/2123863004/diff/180001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2123863004/diff/180001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2407: The <ph name="URL_OF_THE_CURRENT_TAB">%1$s<ex>https://apprtc.appspot.com</ex></ph> is sharing your screen now On 2016/08/10 19:04:02, dfalcantara wrote: > This string doesn't make sense, grammatically. Has this been run through UX? > > "The https://google.com is sharing your screen now" Done. Change it to "www.site.com is sharing your screen" as suggested by UX. https://codereview.chromium.org/2123863004/diff/180001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2123863004/diff/180001/chrome/app/generated_r... chrome/app/generated_resources.grd:5613: + <message name="IDS_FLAGS_MEDIA_SCREEN_CAPTURE_NAME" desc="Name of chrome:flags option to enable screen capture."> On 2016/08/10 19:04:03, dfalcantara wrote: > Add this to both of your about:flags strings: > > translateable="false" > > If you don't, translators will try to translate this. Done. https://codereview.chromium.org/2123863004/diff/180001/chrome/browser/android... File chrome/browser/android/tab_web_contents_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/180001/chrome/browser/android... chrome/browser/android/tab_web_contents_delegate_android.cc:495: ->GetMediaStreamCaptureIndicator(); On 2016/08/10 19:04:03, dfalcantara wrote: > Pull this code out instead of duplicating it four times > (IsCapturingAudio/video/Screen & NotifyStopped all use it). As discussed offline, keep it as is. https://codereview.chromium.org/2123863004/diff/180001/chrome/browser/media/p... File chrome/browser/media/permission_bubble_media_access_handler.cc (right): https://codereview.chromium.org/2123863004/diff/180001/chrome/browser/media/p... chrome/browser/media/permission_bubble_media_access_handler.cc:117: #if BUILDFLAG(ANDROID_JAVA_UI) On 2016/08/10 19:04:03, dfalcantara wrote: > Why are you using ANDROID_JAVA_UI instead of OS_ANDROID? It's because only ANDROID_JAVA_UI is used in this file. Should I replace all of them with OS_ANDROID? https://codereview.chromium.org/2123863004/diff/180001/chrome/browser/media/p... chrome/browser/media/permission_bubble_media_access_handler.cc:158: !controller->IsAskingForScreenCapture()) { On 2016/08/10 19:04:03, dfalcantara wrote: > Why is this new check necessary? Nothing below (not even the comment) changed. It's necessary. Otherwise a screen capture will satisfy "if (!controller->IsAskingForAudio() && !controller->IsAskingForVideo())" and hit the 'return' in the if section. Then no infobar will be created. https://codereview.chromium.org/2123863004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java (right): https://codereview.chromium.org/2123863004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java:491: */ >nit: bottom javadoc never has two * Done https://codereview.chromium.org/2123863004/diff/240001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2123863004/diff/240001/chrome/app/generated_r... chrome/app/generated_resources.grd:5616: + <message name="IDS_FLAGS_MEDIA_SCREEN_CAPTURE_NAME" desc="Name of chrome:flags option to enable screen capture." translateable="false"> >nit: use single space before translateable="false" instead of two Done https://codereview.chromium.org/2123863004/diff/240001/chrome/app/generated_r... chrome/app/generated_resources.grd:13010: + Share your screen >Shouldn't "share" be capitalized? Done
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, but wait for Ted. Also: it's much easier to re-review if you address comments and then rebase in different patch sets. Can't always do that, but it helps when you can.
On 2016/08/15 20:42:22, dfalcantara wrote: > lgtm, but wait for Ted. > > Also: it's much easier to re-review if you address comments and then rebase in > different patch sets. Can't always do that, but it helps when you can. dfalantara@, Noted with thanks! sergeyu@, please take a look to chrome/browser/ and other related. tedchoc@, please take another look to chrome/android/ and other related. Thanks a lot!
https://codereview.chromium.org/2123863004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java (right): https://codereview.chromium.org/2123863004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:98: case ACTION_MEDIA_CAPTURE_UPDATE: since we only have two actions, I think if (ACTION_MEDIA_CAPTURE_UPDATE.equals(action)) { ... } else if (ACTION_SCREEN_CAPTURE_STOP.equals(action)) { ... } is clearer and has less code. https://codereview.chromium.org/2123863004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:146: createNotification(notificationId, mediaType, url, true); this is only ever called once...and we always pass true. let's get rid of the new boolean and simply key it off of MEDIATYPE_SCREEN_CAPTURE https://codereview.chromium.org/2123863004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:195: new StringBuilder(mContext.getResources().getString(notificationContentTextId, url)) While extra arguments are dropped in the underlying format call, I'm not a big fan of passing the url as an argument that is only used by one of the 4 options. Maybe we should just pull out two helper functions getNotificationContentText and getNotificationIconId Then we can make this a bit clearer. https://codereview.chromium.org/2123863004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:207: builder.setVibrate(new long[0]); we don't vibrate for any other notifications...seems odd to do it here. And I think we should pass an explicit duration of what we think is best instead, maybe 50 or 100 https://codereview.chromium.org/2123863004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:209: builder.addAction(R.drawable.ic_vidcontrol_stop, "Stop", "Stop" should be translated...and we might already have a Stop that you can use (ACCESSIBILITY_STOP for example) https://codereview.chromium.org/2123863004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:210: buildPendingIntent(ACTION_SCREEN_CAPTURE_STOP, notificationId, mediaType, again, let's not make this overly generic for now. just do buildStopCapturePendingIntent(int notificationId) and infer the rest https://codereview.chromium.org/2123863004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:211: url)); clang formatting doesn't work perfectly in java...should be indented 8 from the start of the previous line https://codereview.chromium.org/2123863004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:308: Context context, int tabId, int mediaType, String fullUrl) { update javadoc https://codereview.chromium.org/2123863004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java (right): https://codereview.chromium.org/2123863004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:160: public boolean tabExistsInAnySelector(int tabId) { can you change this to be: return getTabById(tabId) != null; https://codereview.chromium.org/2123863004/diff/240001/chrome/browser/android... File chrome/browser/android/tab_web_contents_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/240001/chrome/browser/android... chrome/browser/android/tab_web_contents_delegate_android.cc:503: content::WebContents::FromJavaWebContents(java_web_contents); Any chance (in a follow up), you can pull these four methods out of this class into a new java util class w/ a separate c++ counterpart. This is getting to be big enough to justify it imo.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Ted, thanks so much for the comments! All addressed. PTAL. https://chromiumcodereview.appspot.com/2123863004/diff/240001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java (right): https://chromiumcodereview.appspot.com/2123863004/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:98: case ACTION_MEDIA_CAPTURE_UPDATE: On 2016/08/16 00:19:19, Ted C wrote: > since we only have two actions, I think > > if (ACTION_MEDIA_CAPTURE_UPDATE.equals(action)) { > ... > } else if (ACTION_SCREEN_CAPTURE_STOP.equals(action)) { > ... > } > > is clearer and has less code. Done. https://chromiumcodereview.appspot.com/2123863004/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:146: createNotification(notificationId, mediaType, url, true); On 2016/08/16 00:19:19, Ted C wrote: > this is only ever called once...and we always pass true. let's get rid of the > new boolean and simply key it off of MEDIATYPE_SCREEN_CAPTURE Done. Once there was another action added, which is removed according to UX suggestion. I intended to still keep things generic here and there. Sorry about that. https://chromiumcodereview.appspot.com/2123863004/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:195: new StringBuilder(mContext.getResources().getString(notificationContentTextId, url)) On 2016/08/16 00:19:19, Ted C wrote: > While extra arguments are dropped in the underlying format call, I'm not a big > fan of passing the url as an argument that is only used by one of the 4 options. > > Maybe we should just pull out two helper functions getNotificationContentText > and getNotificationIconId > > Then we can make this a bit clearer. Done. https://chromiumcodereview.appspot.com/2123863004/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:207: builder.setVibrate(new long[0]); On 2016/08/16 00:19:19, Ted C wrote: > we don't vibrate for any other notifications...seems odd to do it here. And I > think we should pass an explicit duration of what we think is best instead, > maybe 50 or 100 We don't vibrate for screen share too. 'setVibrate' is just necessary to show the headsup notification. https://chromiumcodereview.appspot.com/2123863004/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:209: builder.addAction(R.drawable.ic_vidcontrol_stop, "Stop", On 2016/08/16 00:19:19, Ted C wrote: > "Stop" should be translated...and we might already have a Stop that you can use > (ACCESSIBILITY_STOP for example) Done. https://chromiumcodereview.appspot.com/2123863004/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:210: buildPendingIntent(ACTION_SCREEN_CAPTURE_STOP, notificationId, mediaType, On 2016/08/16 00:19:19, Ted C wrote: > again, let's not make this overly generic for now. just do > buildStopCapturePendingIntent(int notificationId) and infer the rest Done. https://chromiumcodereview.appspot.com/2123863004/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:211: url)); On 2016/08/16 00:19:19, Ted C wrote: > clang formatting doesn't work perfectly in java...should be indented 8 from the > start of the previous line Done. https://chromiumcodereview.appspot.com/2123863004/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:308: Context context, int tabId, int mediaType, String fullUrl) { On 2016/08/16 00:19:19, Ted C wrote: > update javadoc Done. https://chromiumcodereview.appspot.com/2123863004/diff/240001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java (right): https://chromiumcodereview.appspot.com/2123863004/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:160: public boolean tabExistsInAnySelector(int tabId) { On 2016/08/16 00:19:19, Ted C wrote: > can you change this to be: > > return getTabById(tabId) != null; Done. And there is no easy way to test this line: if (AsyncTabParamsManager.hasParamsForTabId(tabId)) { return AsyncTabParamsManager.getAsyncTabParams().get(tabId).getTabTo Reparent(); } So the TabWindowManagerTest#testTabExistsInAnySelector has to be revised a bit. https://chromiumcodereview.appspot.com/2123863004/diff/240001/chrome/browser/... File chrome/browser/android/tab_web_contents_delegate_android.cc (right): https://chromiumcodereview.appspot.com/2123863004/diff/240001/chrome/browser/... chrome/browser/android/tab_web_contents_delegate_android.cc:503: content::WebContents::FromJavaWebContents(java_web_contents); On 2016/08/16 00:19:19, Ted C wrote: > Any chance (in a follow up), you can pull these four methods out of this class > into a new java util class w/ a separate c++ counterpart. This is getting to be > big enough to justify it imo. Sure.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2123863004/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabWindowManagerTest.java (right): https://codereview.chromium.org/2123863004/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabWindowManagerTest.java:258: final TabReparentingParams dummyParams = new TabReparentingParams(null, null, null, false); I think this was failing because the tab in the params is null. What happens if you change the first param to "new Tab(0, false, null)", does this work again? I think we should keep this, we just have to figure out how to make sure the tab is returned properly from the params. https://codereview.chromium.org/2123863004/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java (right): https://codereview.chromium.org/2123863004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:170: StringBuilder contentText = getNotificationContext(mediaType, url); let's have this just return the String w/o the . appended and do the string building here. https://codereview.chromium.org/2123863004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:206: private StringBuilder getNotificationContext(int mediaType, String url) { I'd call this getNotificationTextContent. Context is very overloaded in Android, so I want to distinguish this from the os Context concept. https://codereview.chromium.org/2123863004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:219: new StringBuilder(mContext.getResources().getString(notificationContentTextId, url)) I think I'd prefer a check at the top for MEDIATYPE_SCREEN_CAPTURE to be clear it is the only one that uses the url. if (mediaType == MEDIATYPE_SCREEN_CAPTURE) { return mContext.getResources().getString( R.string.screen_capture_notification_text, url); } int notificationContentTextId = 0; if (mediaType == MEDIATYPE_AUDIO_AND_VIDEO) { ... ... ... Then it clearly splits the usage of url to only the one message that is expected to use it.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Ted, thanks so much!!! PTAL. https://codereview.chromium.org/2123863004/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabWindowManagerTest.java (right): https://codereview.chromium.org/2123863004/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabWindowManagerTest.java:258: final TabReparentingParams dummyParams = new TabReparentingParams(null, null, null, false); On 2016/08/18 03:55:01, Ted C wrote: > I think this was failing because the tab in the params is null. What happens if > you change the first param to "new Tab(0, false, null)", does this work again? > I think we should keep this, we just have to figure out how to make sure the tab > is returned properly from the params. Done. I must read the codes wrong and thought Intent of the tab can't be null to be added as tabToReparent. https://codereview.chromium.org/2123863004/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java (right): https://codereview.chromium.org/2123863004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:170: StringBuilder contentText = getNotificationContext(mediaType, url); On 2016/08/18 03:55:02, Ted C wrote: > let's have this just return the String w/o the . appended and do the string > building here. Done. https://codereview.chromium.org/2123863004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:206: private StringBuilder getNotificationContext(int mediaType, String url) { On 2016/08/18 03:55:01, Ted C wrote: > I'd call this getNotificationTextContent. Context is very overloaded in > Android, so I want to distinguish this from the os Context concept. Done. https://codereview.chromium.org/2123863004/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java:219: new StringBuilder(mContext.getResources().getString(notificationContentTextId, url)) On 2016/08/18 03:55:01, Ted C wrote: > I think I'd prefer a check at the top for MEDIATYPE_SCREEN_CAPTURE to be clear > it is the only one that uses the url. > > if (mediaType == MEDIATYPE_SCREEN_CAPTURE) { > return mContext.getResources().getString( > R.string.screen_capture_notification_text, url); > } > > int notificationContentTextId = 0; > if (mediaType == MEDIATYPE_AUDIO_AND_VIDEO) { > ... > ... > ... > > Then it clearly splits the usage of url to only the one message that is expected > to use it. Done.
lgtm - android-y bits
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.cc (right): https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.cc:117: is_screen_capture_ = On 2016/08/03 00:23:47, braveyao wrote: > On 2016/08/01 18:20:36, Sergey Ulanov wrote: > > There can be multiple media streams, and as far as I can tell the same > > PeerConnectionTrackerHost() will be used for all of them, so I don't think you > > can store the state of in a single flag like this. Beside that you set the > value > > on IO thread here, but then use it on UI thread without any synchronization. > > Done. > > Single flag is necessary on Android. During screen sharing, Chrome may be > minimized to backgound which will trigger the OnSuspend() callback here. Since > MediaProjection doesn't have audio capture, there usually is another > peerconnection for audio/video on the same page, e.g. under same > PeerConnectionTrackerHost. We shouldn't cut screensharing, also shouldn't cut > the audio session. (video session will be mute after Chrome goes to background), > which means we have to keep these two peerconnection running for such a > scenario. Then I still think this code won't work correctly, particularly in the following state: 1. GetUserMedia() is called for screen 2. GetUserMedia() is called for camera/mic after (2) is_screen_capture_ will be set to false, but you want it to be set to true in this case. https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_manager.cc:595: video_capture_device = DesktopCaptureDevice::Create(desktop_id); DesktopCapture device is not applicable on Android so I think this code should look as follows #if ANDROIRD create ScreenCaptureDeviceAndroid #else #if AURA try create aura device #endif create default capture device. #endif https://codereview.chromium.org/2123863004/diff/300001/chrome/browser/media/m... File chrome/browser/media/media_stream_capture_indicator.cc (right): https://codereview.chromium.org/2123863004/diff/300001/chrome/browser/media/m... chrome/browser/media/media_stream_capture_indicator.cc:319: if (usage) Should this be a DCHECK()? https://codereview.chromium.org/2123863004/diff/300001/chrome/browser/media/m... File chrome/browser/media/media_stream_devices_controller_browsertest.cc (right): https://codereview.chromium.org/2123863004/diff/300001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller_browsertest.cc:113: // microphone and/or camera device. update comment? https://codereview.chromium.org/2123863004/diff/300001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller_browsertest.cc:114: bool DevicesContains(bool needs_audio, bool needs_video) { I think it would be cleaner to replace this with a function that compares single type. So instead of EXPECT_TRUE(DevicesContains(true, false)); the tests would do something like this: CheckDevicesListContains(content::MEDIA_DEVICE_AUDIO_CAPTURE, true); CheckDevicesListContains(content::MEDIA_DEVICE_VIDEO_CAPTURE, false); Right now this function confuses MEDIA_DEVICE_VIDEO_CAPTURE and MEDIA_DESKTOP_VIDEO_CAPTURE, which is not desirable. https://codereview.chromium.org/2123863004/diff/300001/chrome/browser/media/m... chrome/browser/media/media_stream_devices_controller_browsertest.cc:749: IN_PROC_BROWSER_TEST_F(MediaStreamDevicesControllerTest, RequestScreenCapture) { Also add a test for the case when access is denied. https://codereview.chromium.org/2123863004/diff/300001/chrome/browser/media/p... File chrome/browser/media/permission_bubble_media_access_handler.cc (right): https://codereview.chromium.org/2123863004/diff/300001/chrome/browser/media/p... chrome/browser/media/permission_bubble_media_access_handler.cc:127: #endif // BUILDFLAG(ANDROID_JAVA_UI) https://codereview.chromium.org/2123863004/diff/300001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2123863004/diff/300001/content/browser/BUILD.... content/browser/BUILD.gn:199: if (is_linux || is_mac || is_win || is_android) { I think you want to keep this line as it was before - see my comments below. You just need to define ENABLE_SCREEN_CAPTURE on all platforms (or get rid of it completely) https://codereview.chromium.org/2123863004/diff/300001/content/browser/BUILD.... content/browser/BUILD.gn:201: "media/capture/desktop_capture_device.cc", This class is not useful on android (because you are not implementing webrtc::DesktopCapturer interface). https://codereview.chromium.org/2123863004/diff/300001/content/browser/BUILD.... content/browser/BUILD.gn:212: defines += [ "ENABLE_SCREEN_CAPTURE=1" ] Just move this line out of the if block or remove this define completely (since now screen capture enabled on all platforms) https://codereview.chromium.org/2123863004/diff/300001/content/browser/BUILD.... content/browser/BUILD.gn:213: deps += [ "//third_party/webrtc/modules/desktop_capture" ] don't really need this on Android. https://codereview.chromium.org/2123863004/diff/300001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.cc (right): https://codereview.chromium.org/2123863004/diff/300001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.cc:111: base::AutoLock lock(lock_); Why do you need this lock? If it's just for is_screen_capture_ then you don't want to acquire it before calling WebRTCInternals::GetInstance()->OnGetUserMedia(). https://codereview.chromium.org/2123863004/diff/300001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.h (right): https://codereview.chromium.org/2123863004/diff/300001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.h:68: base::Lock lock_; Add a comment to explain why this lock is necessary.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi sergeyu@, all comments are addressed. PTAL! https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.cc (right): https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.cc:117: is_screen_capture_ = On 2016/08/19 05:54:15, Sergey Ulanov wrote: > On 2016/08/03 00:23:47, braveyao wrote: > > On 2016/08/01 18:20:36, Sergey Ulanov wrote: > > > There can be multiple media streams, and as far as I can tell the same > > > PeerConnectionTrackerHost() will be used for all of them, so I don't think > you > > > can store the state of in a single flag like this. Beside that you set the > > value > > > on IO thread here, but then use it on UI thread without any synchronization. > > > > Done. > > > > Single flag is necessary on Android. During screen sharing, Chrome may be > > minimized to backgound which will trigger the OnSuspend() callback here. Since > > MediaProjection doesn't have audio capture, there usually is another > > peerconnection for audio/video on the same page, e.g. under same > > PeerConnectionTrackerHost. We shouldn't cut screensharing, also shouldn't cut > > the audio session. (video session will be mute after Chrome goes to > background), > > which means we have to keep these two peerconnection running for such a > > scenario. > > Then I still think this code won't work correctly, particularly in the following > state: > 1. GetUserMedia() is called for screen > 2. GetUserMedia() is called for camera/mic > > after (2) is_screen_capture_ will be set to false, but you want it to be set to > true in this case. Yes, this is a problem. Here is a temp solution and add a TODO. I'm taking crbug/513633 now and will handle both cases together later. https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_manager.cc:595: video_capture_device = DesktopCaptureDevice::Create(desktop_id); On 2016/08/19 05:54:15, Sergey Ulanov wrote: > DesktopCapture device is not applicable on Android so I think this code should > look as follows > > #if ANDROIRD > create ScreenCaptureDeviceAndroid > #else > #if AURA > try create aura device > #endif > create default capture device. > #endif Done. https://codereview.chromium.org/2123863004/diff/300001/chrome/browser/media/m... File chrome/browser/media/media_stream_capture_indicator.cc (right): https://codereview.chromium.org/2123863004/diff/300001/chrome/browser/media/m... chrome/browser/media/media_stream_capture_indicator.cc:319: if (usage) On 2016/08/19 05:54:15, Sergey Ulanov wrote: > Should this be a DCHECK()? Done. https://codereview.chromium.org/2123863004/diff/300001/chrome/browser/media/p... File chrome/browser/media/permission_bubble_media_access_handler.cc (right): https://codereview.chromium.org/2123863004/diff/300001/chrome/browser/media/p... chrome/browser/media/permission_bubble_media_access_handler.cc:127: #endif On 2016/08/19 05:54:15, Sergey Ulanov wrote: > // BUILDFLAG(ANDROID_JAVA_UI) Done. https://codereview.chromium.org/2123863004/diff/300001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2123863004/diff/300001/content/browser/BUILD.... content/browser/BUILD.gn:199: if (is_linux || is_mac || is_win || is_android) { On 2016/08/19 05:54:15, Sergey Ulanov wrote: > I think you want to keep this line as it was before - see my comments below. You > just need to define ENABLE_SCREEN_CAPTURE on all platforms (or get rid of it > completely) Done. I suppose we still need to exclude ios, right? https://codereview.chromium.org/2123863004/diff/300001/content/browser/BUILD.... content/browser/BUILD.gn:201: "media/capture/desktop_capture_device.cc", On 2016/08/19 05:54:15, Sergey Ulanov wrote: > This class is not useful on android (because you are not implementing > webrtc::DesktopCapturer interface). Done. https://codereview.chromium.org/2123863004/diff/300001/content/browser/BUILD.... content/browser/BUILD.gn:212: defines += [ "ENABLE_SCREEN_CAPTURE=1" ] On 2016/08/19 05:54:15, Sergey Ulanov wrote: > Just move this line out of the if block or remove this define completely (since > now screen capture enabled on all platforms) Done. https://codereview.chromium.org/2123863004/diff/300001/content/browser/BUILD.... content/browser/BUILD.gn:213: deps += [ "//third_party/webrtc/modules/desktop_capture" ] On 2016/08/19 05:54:15, Sergey Ulanov wrote: > don't really need this on Android. Done. https://codereview.chromium.org/2123863004/diff/300001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.cc (right): https://codereview.chromium.org/2123863004/diff/300001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.cc:111: base::AutoLock lock(lock_); On 2016/08/19 05:54:15, Sergey Ulanov wrote: > Why do you need this lock? If it's just for is_screen_capture_ then you don't > want to acquire it before calling > WebRTCInternals::GetInstance()->OnGetUserMedia(). Done. https://codereview.chromium.org/2123863004/diff/300001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.h (right): https://codereview.chromium.org/2123863004/diff/300001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.h:68: base::Lock lock_; On 2016/08/19 05:54:15, Sergey Ulanov wrote: > Add a comment to explain why this lock is necessary. Done.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sergeyu@, all the comments are addressed in patchset 13&14. Please have another look when you are back. https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.cc (right): https://codereview.chromium.org/2123863004/diff/100001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.cc:117: is_screen_capture_ = On 2016/08/23 21:18:54, braveyao wrote: > On 2016/08/19 05:54:15, Sergey Ulanov wrote: > > On 2016/08/03 00:23:47, braveyao wrote: > > > On 2016/08/01 18:20:36, Sergey Ulanov wrote: > > > > There can be multiple media streams, and as far as I can tell the same > > > > PeerConnectionTrackerHost() will be used for all of them, so I don't think > > you > > > > can store the state of in a single flag like this. Beside that you set the > > > value > > > > on IO thread here, but then use it on UI thread without any > synchronization. > > > > > > Done. > > > > > > Single flag is necessary on Android. During screen sharing, Chrome may be > > > minimized to backgound which will trigger the OnSuspend() callback here. > Since > > > MediaProjection doesn't have audio capture, there usually is another > > > peerconnection for audio/video on the same page, e.g. under same > > > PeerConnectionTrackerHost. We shouldn't cut screensharing, also shouldn't > cut > > > the audio session. (video session will be mute after Chrome goes to > > background), > > > which means we have to keep these two peerconnection running for such a > > > scenario. > > > > Then I still think this code won't work correctly, particularly in the > following > > state: > > 1. GetUserMedia() is called for screen > > 2. GetUserMedia() is called for camera/mic > > > > after (2) is_screen_capture_ will be set to false, but you want it to be set > to > > true in this case. > > Yes, this is a problem. Here is a temp solution and add a TODO. I'm taking > crbug/513633 now and will handle both cases together later. Done.
braveyao@chromium.org changed reviewers: + tommi@chromium.org
Hi tommi@, could you please take a look at chrome/browser/ and other related since sergeyu@ is absent for quite a while?
lgtm
braveyao@chromium.org changed reviewers: + asvitkine@chromium.org, piman@chromium.org
Thanks so much sergeyu@! I was so confused by your GTalk status :) raymes@, could you please take another look at components/content_settings/? piman@, could you please take a look at content/browser/? asvitkine@, could you please take a look at tools/metrics/histograms/histograms.xml? Thanks a lot!
lgtm https://codereview.chromium.org/2123863004/diff/360001/content/browser/render... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2123863004/diff/360001/content/browser/render... content/browser/renderer_host/media/video_capture_manager.cc:628: video_capture_device = base::WrapUnique(new ScreenCaptureDeviceAndroid()); nit: base::MakeUnique<ScreenCaptureDeviceAndroid>()
tommi@chromium.org changed reviewers: + magjed@chromium.org
Magnus - can you take my place?
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== ScreenCapture for Android phase1, part II The capture part has been landed in cl https://codereview.chromium.org/2125973002/. This cl is to implement the control part in chrome/ and content/. The document is here, https://goo.gl/QNH29g. This cl proposes a pemission design on Android, including permission info bar and head up notification, which will show up each time at starting screen capture and user can check/stop the capture without leaving current page. And all of these is behind a flag as an experimental feature. BUG=487935 ========== to ========== ScreenCapture for Android phase1, part II The capture part has been landed in cl https://codereview.chromium.org/2125973002/. This cl is to implement the control part in chrome/ and content/. The document is here, https://goo.gl/QNH29g. This cl proposes a pemission design on Android, including permission info bar and head up notification, which will show up each time at starting screen capture and user can check/stop the capture without leaving current page. And all of these is behind a flag as an experimental feature. BUG=487935 ==========
tommi@chromium.org changed reviewers: - tommi@chromium.org
https://codereview.chromium.org/2123863004/diff/400001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2123863004/diff/400001/chrome/browser/about_f... chrome/browser/about_flags.cc:714: SINGLE_VALUE_TYPE(switches::kEnableUserMediaScreenCapturing)}, Have you considered using base::Feature instead? We recommend using that instead of adding new flags.
Patchset #18 (id:420001) has been deleted
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Patchset #18 (id:440001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
asvitkine@, thanks so much for the recommendation. I had no idea before. Done. Please take another look! https://codereview.chromium.org/2123863004/diff/360001/content/browser/render... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2123863004/diff/360001/content/browser/render... content/browser/renderer_host/media/video_capture_manager.cc:628: video_capture_device = base::WrapUnique(new ScreenCaptureDeviceAndroid()); On 2016/09/09 00:52:54, piman OOO back 2016-09-12 wrote: > nit: base::MakeUnique<ScreenCaptureDeviceAndroid>() Done. https://codereview.chromium.org/2123863004/diff/400001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2123863004/diff/400001/chrome/browser/about_f... chrome/browser/about_flags.cc:714: SINGLE_VALUE_TYPE(switches::kEnableUserMediaScreenCapturing)}, On 2016/09/12 16:56:57, Alexei Svitkine (very slow) wrote: > Have you considered using base::Feature instead? > > We recommend using that instead of adding new flags. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2123863004/diff/460001/components/content_set... File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/2123863004/diff/460001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:329: Register(CONTENT_SETTINGS_TYPE_MEDIASTREAM_SCREEN, "media-stream-screen", Could we call it CONTNET_SETTING_TYPE_SCREEN_CAPTURE and screen-capture ? https://codereview.chromium.org/2123863004/diff/460001/components/content_set... components/content_settings/core/browser/content_settings_registry.cc:331: WhitelistedSchemes(), ValidSettings(), It's strange to have an empty ValidSettings. That implies that no settings are valid. Perhaps have ALLOW and BLOCK here
tsergeant@chromium.org changed reviewers: + tsergeant@chromium.org
Raymes asked me to explain an alternative method of showing the permission request, see below for our suggestion. https://codereview.chromium.org/2123863004/diff/460001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2123863004/diff/460001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_devices_controller.cc:499: if (IsAskingForScreenCapture()) { My suggestion would be to avoid modifying media_stream_devices_controller and media_stream_infobar_delegate if possible. There's a lot of logic in there already to handle the ways in which video/audio permissions interact, which you probably don't want to get involved with. One way to do this would be to add a new ConfirmInfoBarDelegate subclass. Hopefully that won't be too much work, since the critical parts are just setting up the strings, icons and buttons (which are mostly static), and using roughly the same callback method as is used here. Using a new infobar delegate means that you can get the behavior you want here without worrying about how it interacts with the existing permissions. You also wouldn't need to worry about adding a new content setting. https://codereview.chromium.org/2123863004/diff/460001/chrome/browser/media/w... File chrome/browser/media/webrtc/permission_bubble_media_access_handler.cc (right): https://codereview.chromium.org/2123863004/diff/460001/chrome/browser/media/w... chrome/browser/media/webrtc/permission_bubble_media_access_handler.cc:150: std::unique_ptr<MediaStreamDevicesController> controller( If you added a new InfoBarDelegate class, you would use it here instead of creating a controller.
On 2016/09/09 10:46:15, tommi (chrömium) wrote: > Magnus - can you take my place? I could review the Java MediaProjection parts, but that has already landed in a separate CL. This CL is about chrome permission and control parts which I'm unfortunately not familiar with.
magjed@chromium.org changed reviewers: - magjed@chromium.org, tsergeant@chromium.org
lgtm https://codereview.chromium.org/2123863004/diff/460001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2123863004/diff/460001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:85630: + <int value="-905334658" label="enable-usermedia-screen-capturing"/> You can remove this line, as it's not needed when use base::Feature API.
Patchset #19 (id:480001) has been deleted
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #19 (id:500001) has been deleted
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
raymes@ and tsergeant@, Thanks so much for the suggestions! Please take a look at patchset19 for the modifications. (patchset 20 is a rebase to make trybots working, and also solved a definition conflict in components/infobars/core/infobar_delegate.h) https://codereview.chromium.org/2123863004/diff/460001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_stream_devices_controller.cc (right): https://codereview.chromium.org/2123863004/diff/460001/chrome/browser/media/w... chrome/browser/media/webrtc/media_stream_devices_controller.cc:499: if (IsAskingForScreenCapture()) { On 2016/09/13 05:06:23, tsergeant wrote: > My suggestion would be to avoid modifying media_stream_devices_controller and > media_stream_infobar_delegate if possible. There's a lot of logic in there > already to > handle the ways in which video/audio permissions interact, which you probably > don't want to get involved with. > > One way to do this would be to add a new ConfirmInfoBarDelegate subclass. > Hopefully that won't be too much work, since the critical parts are just setting > up the strings, icons and buttons (which are mostly static), and using roughly > the same callback method as is used here. > > Using a new infobar delegate means that you can get the behavior you want here > without worrying about how it interacts with the existing permissions. You also > wouldn't need to worry about adding a new content setting. Done. https://codereview.chromium.org/2123863004/diff/460001/chrome/browser/media/w... File chrome/browser/media/webrtc/permission_bubble_media_access_handler.cc (right): https://codereview.chromium.org/2123863004/diff/460001/chrome/browser/media/w... chrome/browser/media/webrtc/permission_bubble_media_access_handler.cc:150: std::unique_ptr<MediaStreamDevicesController> controller( On 2016/09/13 05:06:23, tsergeant wrote: > If you added a new InfoBarDelegate class, you would use it here instead of > creating a controller. Done. https://codereview.chromium.org/2123863004/diff/460001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2123863004/diff/460001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:85630: + <int value="-905334658" label="enable-usermedia-screen-capturing"/> On 2016/09/13 14:54:48, Alexei Svitkine (very slow) wrote: > You can remove this line, as it's not needed when use base::Feature API. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I defer to tsergeant@
braveyao@chromium.org changed reviewers: + tsergeant@chromium.org
Oops, forgot to add tsergeant@ as a reviewer... tsergeant@, please take a look at patchset19 for the modifications. (patchset 20 is a rebase to make trybots working, and also solved a definition conflict in components/infobars/core/infobar_delegate.h)
Thanks for doing this, the result is much simpler. lgtm with a couple of small nits. https://codereview.chromium.org/2123863004/diff/520001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h (right): https://codereview.chromium.org/2123863004/diff/520001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:16: // user's device screen. The user is shown a message asking whether he wishes to Please try and avoid gender-specific pronouns in comments. Here, you can use: "The user is shown a message asking whether they wish to share their screen with the current page." https://codereview.chromium.org/2123863004/diff/520001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:50: const ResponseCallback granted_callback_; Nit: I would just call this `callback_` rather than `granted_callback_`, since it is used even when the permission is denied.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tsergeant@, Thanks so much for the quick review! Fixed the nits and added two sanity checks to the "callback_" in /screen_capture_infobar_delegate_android.cc. Please check if they require another look. https://codereview.chromium.org/2123863004/diff/520001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h (right): https://codereview.chromium.org/2123863004/diff/520001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:16: // user's device screen. The user is shown a message asking whether he wishes to On 2016/09/19 23:27:02, tsergeant wrote: > Please try and avoid gender-specific pronouns in comments. Here, you can use: > > "The user is shown a message asking whether they wish to share their screen with > the current page." Done. https://codereview.chromium.org/2123863004/diff/520001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:50: const ResponseCallback granted_callback_; On 2016/09/19 23:27:02, tsergeant wrote: > Nit: I would just call this `callback_` rather than `granted_callback_`, since > it is used even when the permission is denied. Done.
Ah, sorry, a couple more things: https://codereview.chromium.org/2123863004/diff/580001/components/infobars/co... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2123863004/diff/580001/components/infobars/co... components/infobars/core/infobar_delegate.h:65: // KEEP IN SYNC WITH THE InfoBarIdentifier ENUM IN histograms.xml. You'll need to modify this enum since you've added a new Identifier type. https://codereview.chromium.org/2123863004/diff/600001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/600001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:58: ScreenCaptureInfoBarDelegateAndroid::~ScreenCaptureInfoBarDelegateAndroid() {} The callback should be run here if it is non-null. I think this can happen if the tab is closed without making a decision on the infobar. (Eg: https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/media_stream...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tsergeant@, thanks for the reminding. All addressed. https://codereview.chromium.org/2123863004/diff/580001/components/infobars/co... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2123863004/diff/580001/components/infobars/co... components/infobars/core/infobar_delegate.h:65: // KEEP IN SYNC WITH THE InfoBarIdentifier ENUM IN histograms.xml. On 2016/09/20 01:17:55, tsergeant wrote: > You'll need to modify this enum since you've added a new Identifier type. Done. https://codereview.chromium.org/2123863004/diff/600001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/600001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:58: ScreenCaptureInfoBarDelegateAndroid::~ScreenCaptureInfoBarDelegateAndroid() {} On 2016/09/20 01:17:55, tsergeant wrote: > The callback should be run here if it is non-null. I think this can happen if > the tab is closed without making a decision on the infobar. > > (Eg: > https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/media_stream...) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, still lgtm
The CQ bit was checked by braveyao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org, tedchoc@chromium.org, sergeyu@chromium.org, piman@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2123863004/#ps640001 (title: "add entry in LoginCustomFlags as bots request")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
braveyao@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@, could you please take a look at components/infobars/core/infobar_delegate.xx files?
https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:35: for (size_t i = 0; i < infobar_service->infobar_count(); ++i) { Why do you need this block? Are you expecting to get multiple calls with different callbacks, so the latter should obsolete the former, or something? https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:53: DCHECK(request.video_type == content::MEDIA_DESKTOP_VIDEO_CAPTURE); Nit: DCHECK_EQ(content::MEDIA_DESKTOP_VIDEO_CAPTURE, request.video_type); https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:60: std::unique_ptr<content::MediaStreamUI>()); Nit: Just use nullptr? https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:106: CHECK(!callback_.is_null()); Nit: Prefer DCHECK except for debugging or security-critical issues Consider also a DCHECK in the constructor, since this sort of implies that. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:110: content::DesktopMediaID screen_id; Nit: Combine these two lines https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:124: base::ResetAndReturn(&callback_).Run(devices, result, std::move(ui)); Is there a reason you need to use ResetAndReturn() instead of just Run() and then Reset()? https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h (right): https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:18: // screen to the page, or to deny access to it. Nit: We can make this less wordy and less passive; how about just: An infobar that allows the user to share their screen with the current page. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:21: typedef content::MediaResponseCallback ResponseCallback; Nit: Does this really buy much over just using the original type directly? If it really does, prefer a type alias to a typedef. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:23: // Static method to create the object Nit: This comment adds nothing to the function declaration. Something like this instead: Creates a screen capture infobar and delegate and adds the infobar to the InfoBarService associated with |web_contents|. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:28: explicit ScreenCaptureInfoBarDelegateAndroid( Do not make the constructor public; that's the point of the Create() method. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:34: // ConfirmInfoBarDelegate: Nit: These should be private. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:46: void RunCallback(content::MediaStreamRequestResult result); Nit: This deserves a comment, since the implementation is nontrivial.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pkasting@, thanks so much for the comments! All addressed/responded. PTAL. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:35: for (size_t i = 0; i < infobar_service->infobar_count(); ++i) { On 2016/09/20 23:50:28, Peter Kasting wrote: > Why do you need this block? Are you expecting to get multiple calls with > different callbacks, so the latter should obsolete the former, or something? Yes I think so. Same as here, https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/media_stream.... Since they should be handled same, except no content_setting/grouped_permission for screen capture. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:53: DCHECK(request.video_type == content::MEDIA_DESKTOP_VIDEO_CAPTURE); On 2016/09/20 23:50:28, Peter Kasting wrote: > Nit: DCHECK_EQ(content::MEDIA_DESKTOP_VIDEO_CAPTURE, request.video_type); Done. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:60: std::unique_ptr<content::MediaStreamUI>()); On 2016/09/20 23:50:28, Peter Kasting wrote: > Nit: Just use nullptr? Done. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:106: CHECK(!callback_.is_null()); On 2016/09/20 23:50:28, Peter Kasting wrote: > Nit: Prefer DCHECK except for debugging or security-critical issues > > Consider also a DCHECK in the constructor, since this sort of implies that. Done. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:110: content::DesktopMediaID screen_id; On 2016/09/20 23:50:28, Peter Kasting wrote: > Nit: Combine these two lines Done. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:124: base::ResetAndReturn(&callback_).Run(devices, result, std::move(ui)); On 2016/09/20 23:50:29, Peter Kasting wrote: > Is there a reason you need to use ResetAndReturn() instead of just Run() and > then Reset()? Because ResetAndReturn() is used here too, https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/media_stream.... Also I see it's used lots of other places. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h (right): https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:18: // screen to the page, or to deny access to it. On 2016/09/20 23:50:29, Peter Kasting wrote: > Nit: We can make this less wordy and less passive; how about just: > > An infobar that allows the user to share their screen with the current page. Done. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:21: typedef content::MediaResponseCallback ResponseCallback; On 2016/09/20 23:50:29, Peter Kasting wrote: > Nit: Does this really buy much over just using the original type directly? > > If it really does, prefer a type alias to a typedef. Done. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:23: // Static method to create the object On 2016/09/20 23:50:29, Peter Kasting wrote: > Nit: This comment adds nothing to the function declaration. Something like this > instead: > > Creates a screen capture infobar and delegate and adds the infobar to the > InfoBarService associated with |web_contents|. Done. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:28: explicit ScreenCaptureInfoBarDelegateAndroid( On 2016/09/20 23:50:29, Peter Kasting wrote: > Do not make the constructor public; that's the point of the Create() method. Done. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:34: // ConfirmInfoBarDelegate: On 2016/09/20 23:50:29, Peter Kasting wrote: > Nit: These should be private. Done. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:46: void RunCallback(content::MediaStreamRequestResult result); On 2016/09/20 23:50:29, Peter Kasting wrote: > Nit: This deserves a comment, since the implementation is nontrivial. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:35: for (size_t i = 0; i < infobar_service->infobar_count(); ++i) { On 2016/09/21 00:35:55, braveyao wrote: > On 2016/09/20 23:50:28, Peter Kasting wrote: > > Why do you need this block? Are you expecting to get multiple calls with > > different callbacks, so the latter should obsolete the former, or something? > Yes I think so. Can you say more about this? Normally we prevent duplicate infobars automatically, so it's very unusual to run code like this. You need to have a good reason for doing so (and I'm not completely sure why the code you linked to is doing it either). https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:124: base::ResetAndReturn(&callback_).Run(devices, result, std::move(ui)); On 2016/09/21 00:35:55, braveyao wrote: > On 2016/09/20 23:50:29, Peter Kasting wrote: > > Is there a reason you need to use ResetAndReturn() instead of just Run() and > > then Reset()? > > Because ResetAndReturn() is used here too, > https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/media_stream.... > Also I see it's used lots of other places. That doesn't answer my question. Don't do something just because you see other code doing it, do it because you need it. ResetAndReturn() is meant for cases where running the callback could change something out from under you in a way that results in memory corruption or similar. Using it implies to the reader that something tricky is going on with lifetime management. If that's not what you actually need, do things the simple and direct route.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pkasting@, thanks so much for the clarification! PTAL. Referring the existing codes sounds the best choice to me so far :) I'm glad to know more. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:35: for (size_t i = 0; i < infobar_service->infobar_count(); ++i) { On 2016/09/21 04:07:27, Peter Kasting wrote: > On 2016/09/21 00:35:55, braveyao wrote: > > On 2016/09/20 23:50:28, Peter Kasting wrote: > > > Why do you need this block? Are you expecting to get multiple calls with > > > different callbacks, so the latter should obsolete the former, or something? > > Yes I think so. > > Can you say more about this? Normally we prevent duplicate infobars > automatically, so it's very unusual to run code like this. You need to have a > good reason for doing so (and I'm not completely sure why the code you linked to > is doing it either). Done. It's really good to know it's handled internally. I referred several delegate implementations and some of them do the same thing too, so I thought it's a good thing to do. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:124: base::ResetAndReturn(&callback_).Run(devices, result, std::move(ui)); On 2016/09/21 04:07:27, Peter Kasting wrote: > On 2016/09/21 00:35:55, braveyao wrote: > > On 2016/09/20 23:50:29, Peter Kasting wrote: > > > Is there a reason you need to use ResetAndReturn() instead of just Run() and > > > then Reset()? > > > > Because ResetAndReturn() is used here too, > > > https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/media_stream.... > > Also I see it's used lots of other places. > > That doesn't answer my question. Don't do something just because you see other > code doing it, do it because you need it. ResetAndReturn() is meant for cases > where running the callback could change something out from under you in a way > that results in memory corruption or similar. Using it implies to the reader > that something tricky is going on with lifetime management. If that's not what > you actually need, do things the simple and direct route. Done. I just noticed that in chrome/browser/media/media_stream_devices_controller.cc it's changed to ResetAndReturn() not very long ago, in cl https://codereview.chromium.org/1362803002. Also there are so many of it in other places. Then I think it's a new practice to adopt. Thanks so much for the clarification.
https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:35: for (size_t i = 0; i < infobar_service->infobar_count(); ++i) { On 2016/09/21 21:04:02, braveyao wrote: > On 2016/09/21 04:07:27, Peter Kasting wrote: > > On 2016/09/21 00:35:55, braveyao wrote: > > > On 2016/09/20 23:50:28, Peter Kasting wrote: > > > > Why do you need this block? Are you expecting to get multiple calls with > > > > different callbacks, so the latter should obsolete the former, or > something? > > > Yes I think so. > > > > Can you say more about this? Normally we prevent duplicate infobars > > automatically, so it's very unusual to run code like this. You need to have a > > good reason for doing so (and I'm not completely sure why the code you linked > to > > is doing it either). > > Done. > It's really good to know it's handled internally. I referred several delegate > implementations and some of them do the same thing too, so I thought it's a good > thing to do. Well, what matters is what you want to happen when we have infobar A and we try to create infobar A'. If A' is substantially the same as A, the default implementation should (but you should test this!) just avoid creating A', leaving A onscreen. But in some cases, we actually want A' to replace A, because it has a different callback or whatever. In those cases, you need code like what you added. So this isn't necessarily wrong, but it's wrong to do it blindly. You need to understand how you want to react when one request is similar to an outstanding previous request: dump the old one or dump the new one? https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:124: base::ResetAndReturn(&callback_).Run(devices, result, std::move(ui)); On 2016/09/21 21:04:02, braveyao wrote: > On 2016/09/21 04:07:27, Peter Kasting wrote: > > On 2016/09/21 00:35:55, braveyao wrote: > > > On 2016/09/20 23:50:29, Peter Kasting wrote: > > > > Is there a reason you need to use ResetAndReturn() instead of just Run() > and > > > > then Reset()? > > > > > > Because ResetAndReturn() is used here too, > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/media_stream.... > > > Also I see it's used lots of other places. > > > > That doesn't answer my question. Don't do something just because you see > other > > code doing it, do it because you need it. ResetAndReturn() is meant for cases > > where running the callback could change something out from under you in a way > > that results in memory corruption or similar. Using it implies to the reader > > that something tricky is going on with lifetime management. If that's not > what > > you actually need, do things the simple and direct route. > Done. > > I just noticed that in chrome/browser/media/media_stream_devices_controller.cc > it's changed to ResetAndReturn() not very long ago, in cl > https://codereview.chromium.org/1362803002. Also there are so many of it in > other places. Then I think it's a new practice to adopt. > Thanks so much for the clarification. I don't think you understood my objection. I'm not objecting to using ResetAndReturn() when you actually need to reset the callback before running it. I'm asking why you need to reset before running it. You changed to doing the same thing ResetAndReturn() was doing, except manually, which is strictly worse. What I'm asking is why you're resetting the callback before running it instead of after. The answer to this isn't necessarily to just reset after running, it's to explain the rationale behind _why_ you need to do one or the other. Or in other words: copying code from me is no better than copying code from elsewhere :)
Patchset #27 (id:680001) has been deleted
pkasting@, Thanks again for the clarification. Deleted patchset27. Please check my responses to the two concerns. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:35: for (size_t i = 0; i < infobar_service->infobar_count(); ++i) { On 2016/09/21 21:39:43, Peter Kasting wrote: > On 2016/09/21 21:04:02, braveyao wrote: > > On 2016/09/21 04:07:27, Peter Kasting wrote: > > > On 2016/09/21 00:35:55, braveyao wrote: > > > > On 2016/09/20 23:50:28, Peter Kasting wrote: > > > > > Why do you need this block? Are you expecting to get multiple calls > with > > > > > different callbacks, so the latter should obsolete the former, or > > something? > > > > Yes I think so. > > > > > > Can you say more about this? Normally we prevent duplicate infobars > > > automatically, so it's very unusual to run code like this. You need to have > a > > > good reason for doing so (and I'm not completely sure why the code you > linked > > to > > > is doing it either). > > > > Done. > > It's really good to know it's handled internally. I referred several delegate > > implementations and some of them do the same thing too, so I thought it's a > good > > thing to do. > > Well, what matters is what you want to happen when we have infobar A and we try > to create infobar A'. > > If A' is substantially the same as A, the default implementation should (but you > should test this!) just avoid creating A', leaving A onscreen. > > But in some cases, we actually want A' to replace A, because it has a different > callback or whatever. In those cases, you need code like what you added. > > So this isn't necessarily wrong, but it's wrong to do it blindly. You need to > understand how you want to react when one request is similar to an outstanding > previous request: dump the old one or dump the new one? Dumping the old one sounds more proper here. So keeping these codes. GetUserMedia calls on one page will be executed one by one. If the previous one is not called back, the next won't be handled. Logically if A' is created for some reason, it should mean the A is not valid any more, so I'll stick with the newer one. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:124: base::ResetAndReturn(&callback_).Run(devices, result, std::move(ui)); On 2016/09/21 21:39:43, Peter Kasting wrote: > On 2016/09/21 21:04:02, braveyao wrote: > > On 2016/09/21 04:07:27, Peter Kasting wrote: > > > On 2016/09/21 00:35:55, braveyao wrote: > > > > On 2016/09/20 23:50:29, Peter Kasting wrote: > > > > > Is there a reason you need to use ResetAndReturn() instead of just Run() > > and > > > > > then Reset()? > > > > > > > > Because ResetAndReturn() is used here too, > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/media_stream.... > > > > Also I see it's used lots of other places. > > > > > > That doesn't answer my question. Don't do something just because you see > > other > > > code doing it, do it because you need it. ResetAndReturn() is meant for > cases > > > where running the callback could change something out from under you in a > way > > > that results in memory corruption or similar. Using it implies to the > reader > > > that something tricky is going on with lifetime management. If that's not > > what > > > you actually need, do things the simple and direct route. > > Done. > > > > I just noticed that in chrome/browser/media/media_stream_devices_controller.cc > > it's changed to ResetAndReturn() not very long ago, in cl > > https://codereview.chromium.org/1362803002. Also there are so many of it in > > other places. Then I think it's a new practice to adopt. > > Thanks so much for the clarification. > > I don't think you understood my objection. > > I'm not objecting to using ResetAndReturn() when you actually need to reset the > callback before running it. I'm asking why you need to reset before running it. > You changed to doing the same thing ResetAndReturn() was doing, except > manually, which is strictly worse. What I'm asking is why you're resetting the > callback before running it instead of after. > > The answer to this isn't necessarily to just reset after running, it's to > explain the rationale behind _why_ you need to do one or the other. Or in other > words: copying code from me is no better than copying code from elsewhere :) In my case, Run-Reset and Reset-run are same thing. But Reset-Run looks more safe and ResetAndReturn().Run does everything in one line. So keep it here.
https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:35: for (size_t i = 0; i < infobar_service->infobar_count(); ++i) { On 2016/09/21 22:42:21, braveyao wrote: > On 2016/09/21 21:39:43, Peter Kasting wrote: > > On 2016/09/21 21:04:02, braveyao wrote: > > > On 2016/09/21 04:07:27, Peter Kasting wrote: > > > > On 2016/09/21 00:35:55, braveyao wrote: > > > > > On 2016/09/20 23:50:28, Peter Kasting wrote: > > > > > > Why do you need this block? Are you expecting to get multiple calls > > with > > > > > > different callbacks, so the latter should obsolete the former, or > > > something? > > > > > Yes I think so. > > > > > > > > Can you say more about this? Normally we prevent duplicate infobars > > > > automatically, so it's very unusual to run code like this. You need to > have > > a > > > > good reason for doing so (and I'm not completely sure why the code you > > linked > > > to > > > > is doing it either). > > > > > > Done. > > > It's really good to know it's handled internally. I referred several > delegate > > > implementations and some of them do the same thing too, so I thought it's a > > good > > > thing to do. > > > > Well, what matters is what you want to happen when we have infobar A and we > try > > to create infobar A'. > > > > If A' is substantially the same as A, the default implementation should (but > you > > should test this!) just avoid creating A', leaving A onscreen. > > > > But in some cases, we actually want A' to replace A, because it has a > different > > callback or whatever. In those cases, you need code like what you added. > > > > So this isn't necessarily wrong, but it's wrong to do it blindly. You need to > > understand how you want to react when one request is similar to an outstanding > > previous request: dump the old one or dump the new one? > > Dumping the old one sounds more proper here. So keeping these codes. > GetUserMedia calls on one page will be executed one by one. If the previous one > is not called back, the next won't be handled. Logically if A' is created for > some reason, it should mean the A is not valid any more, so I'll stick with the > newer one. Wait, so if they're called one by one, and the next isn't called until the previous request gets a call back, then how can you reach here with a new request while the old one is outstanding? Can that happen? If for example the page cancels the request (however that happens -- not familiar with the web APIs here) shouldn't the infobar be automatically dismissed? So when will we have two outstanding requests? I'm asking all this because we shouldn't have code to handle multiple outstanding simultaneous requests if that can't occur. See also my next reply. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:124: base::ResetAndReturn(&callback_).Run(devices, result, std::move(ui)); On 2016/09/21 22:42:21, braveyao wrote: > On 2016/09/21 21:39:43, Peter Kasting wrote: > > On 2016/09/21 21:04:02, braveyao wrote: > > > On 2016/09/21 04:07:27, Peter Kasting wrote: > > > > On 2016/09/21 00:35:55, braveyao wrote: > > > > > On 2016/09/20 23:50:29, Peter Kasting wrote: > > > > > > Is there a reason you need to use ResetAndReturn() instead of just > Run() > > > and > > > > > > then Reset()? > > > > > > > > > > Because ResetAndReturn() is used here too, > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/media_stream.... > > > > > Also I see it's used lots of other places. > > > > > > > > That doesn't answer my question. Don't do something just because you see > > > other > > > > code doing it, do it because you need it. ResetAndReturn() is meant for > > cases > > > > where running the callback could change something out from under you in a > > way > > > > that results in memory corruption or similar. Using it implies to the > > reader > > > > that something tricky is going on with lifetime management. If that's not > > > what > > > > you actually need, do things the simple and direct route. > > > Done. > > > > > > I just noticed that in > chrome/browser/media/media_stream_devices_controller.cc > > > it's changed to ResetAndReturn() not very long ago, in cl > > > https://codereview.chromium.org/1362803002. Also there are so many of it in > > > other places. Then I think it's a new practice to adopt. > > > Thanks so much for the clarification. > > > > I don't think you understood my objection. > > > > I'm not objecting to using ResetAndReturn() when you actually need to reset > the > > callback before running it. I'm asking why you need to reset before running > it. > > You changed to doing the same thing ResetAndReturn() was doing, except > > manually, which is strictly worse. What I'm asking is why you're resetting > the > > callback before running it instead of after. > > > > The answer to this isn't necessarily to just reset after running, it's to > > explain the rationale behind _why_ you need to do one or the other. Or in > other > > words: copying code from me is no better than copying code from elsewhere :) > > In my case, Run-Reset and Reset-run are same thing. Then do the latter. > But Reset-Run looks more safe This is precisely why I don't want you to do it if you don't need it. It implies to the reader that you _do_ need it: it looks as if the other pattern is unsafe. If the other pattern isn't unsafe, we don't want that implication. The analogy here is to reflexively null-checking pointers. We never want to null-check pointers that can't be null because it tells the reader they can be null when they can't.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pkasting@, got your concern now. Maybe I'm over-cautious to unfamiliar fields here. PTAL. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:35: for (size_t i = 0; i < infobar_service->infobar_count(); ++i) { On 2016/09/21 23:00:32, Peter Kasting wrote: > On 2016/09/21 22:42:21, braveyao wrote: > > On 2016/09/21 21:39:43, Peter Kasting wrote: > > > On 2016/09/21 21:04:02, braveyao wrote: > > > > On 2016/09/21 04:07:27, Peter Kasting wrote: > > > > > On 2016/09/21 00:35:55, braveyao wrote: > > > > > > On 2016/09/20 23:50:28, Peter Kasting wrote: > > > > > > > Why do you need this block? Are you expecting to get multiple calls > > > with > > > > > > > different callbacks, so the latter should obsolete the former, or > > > > something? > > > > > > Yes I think so. > > > > > > > > > > Can you say more about this? Normally we prevent duplicate infobars > > > > > automatically, so it's very unusual to run code like this. You need to > > have > > > a > > > > > good reason for doing so (and I'm not completely sure why the code you > > > linked > > > > to > > > > > is doing it either). > > > > > > > > Done. > > > > It's really good to know it's handled internally. I referred several > > delegate > > > > implementations and some of them do the same thing too, so I thought it's > a > > > good > > > > thing to do. > > > > > > Well, what matters is what you want to happen when we have infobar A and we > > try > > > to create infobar A'. > > > > > > If A' is substantially the same as A, the default implementation should (but > > you > > > should test this!) just avoid creating A', leaving A onscreen. > > > > > > But in some cases, we actually want A' to replace A, because it has a > > different > > > callback or whatever. In those cases, you need code like what you added. > > > > > > So this isn't necessarily wrong, but it's wrong to do it blindly. You need > to > > > understand how you want to react when one request is similar to an > outstanding > > > previous request: dump the old one or dump the new one? > > > > Dumping the old one sounds more proper here. So keeping these codes. > > GetUserMedia calls on one page will be executed one by one. If the previous > one > > is not called back, the next won't be handled. Logically if A' is created for > > some reason, it should mean the A is not valid any more, so I'll stick with > the > > newer one. > > Wait, so if they're called one by one, and the next isn't called until the > previous request gets a call back, then how can you reach here with a new > request while the old one is outstanding? Can that happen? If for example the > page cancels the request (however that happens -- not familiar with the web APIs > here) shouldn't the infobar be automatically dismissed? So when will we have > two outstanding requests? > > I'm asking all this because we shouldn't have code to handle multiple > outstanding simultaneous requests if that can't occur. See also my next reply. Done. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:124: base::ResetAndReturn(&callback_).Run(devices, result, std::move(ui)); On 2016/09/21 23:00:32, Peter Kasting wrote: > On 2016/09/21 22:42:21, braveyao wrote: > > On 2016/09/21 21:39:43, Peter Kasting wrote: > > > On 2016/09/21 21:04:02, braveyao wrote: > > > > On 2016/09/21 04:07:27, Peter Kasting wrote: > > > > > On 2016/09/21 00:35:55, braveyao wrote: > > > > > > On 2016/09/20 23:50:29, Peter Kasting wrote: > > > > > > > Is there a reason you need to use ResetAndReturn() instead of just > > Run() > > > > and > > > > > > > then Reset()? > > > > > > > > > > > > Because ResetAndReturn() is used here too, > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/media_stream.... > > > > > > Also I see it's used lots of other places. > > > > > > > > > > That doesn't answer my question. Don't do something just because you > see > > > > other > > > > > code doing it, do it because you need it. ResetAndReturn() is meant for > > > cases > > > > > where running the callback could change something out from under you in > a > > > way > > > > > that results in memory corruption or similar. Using it implies to the > > > reader > > > > > that something tricky is going on with lifetime management. If that's > not > > > > what > > > > > you actually need, do things the simple and direct route. > > > > Done. > > > > > > > > I just noticed that in > > chrome/browser/media/media_stream_devices_controller.cc > > > > it's changed to ResetAndReturn() not very long ago, in cl > > > > https://codereview.chromium.org/1362803002. Also there are so many of it > in > > > > other places. Then I think it's a new practice to adopt. > > > > Thanks so much for the clarification. > > > > > > I don't think you understood my objection. > > > > > > I'm not objecting to using ResetAndReturn() when you actually need to reset > > the > > > callback before running it. I'm asking why you need to reset before running > > it. > > > You changed to doing the same thing ResetAndReturn() was doing, except > > > manually, which is strictly worse. What I'm asking is why you're resetting > > the > > > callback before running it instead of after. > > > > > > The answer to this isn't necessarily to just reset after running, it's to > > > explain the rationale behind _why_ you need to do one or the other. Or in > > other > > > words: copying code from me is no better than copying code from elsewhere :) > > > > In my case, Run-Reset and Reset-run are same thing. > > Then do the latter. > > > But Reset-Run looks more safe > > This is precisely why I don't want you to do it if you don't need it. It > implies to the reader that you _do_ need it: it looks as if the other pattern is > unsafe. If the other pattern isn't unsafe, we don't want that implication. > > The analogy here is to reflexively null-checking pointers. We never want to > null-check pointers that can't be null because it tells the reader they can be > null when they can't. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:124: base::ResetAndReturn(&callback_).Run(devices, result, std::move(ui)); On 2016/09/21 23:43:18, braveyao wrote: > On 2016/09/21 23:00:32, Peter Kasting wrote: > > On 2016/09/21 22:42:21, braveyao wrote: > > > On 2016/09/21 21:39:43, Peter Kasting wrote: > > > > On 2016/09/21 21:04:02, braveyao wrote: > > > > > On 2016/09/21 04:07:27, Peter Kasting wrote: > > > > > > On 2016/09/21 00:35:55, braveyao wrote: > > > > > > > On 2016/09/20 23:50:29, Peter Kasting wrote: > > > > > > > > Is there a reason you need to use ResetAndReturn() instead of just > > > Run() > > > > > and > > > > > > > > then Reset()? > > > > > > > > > > > > > > Because ResetAndReturn() is used here too, > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/media_stream.... > > > > > > > Also I see it's used lots of other places. > > > > > > > > > > > > That doesn't answer my question. Don't do something just because you > > see > > > > > other > > > > > > code doing it, do it because you need it. ResetAndReturn() is meant > for > > > > cases > > > > > > where running the callback could change something out from under you > in > > a > > > > way > > > > > > that results in memory corruption or similar. Using it implies to the > > > > reader > > > > > > that something tricky is going on with lifetime management. If that's > > not > > > > > what > > > > > > you actually need, do things the simple and direct route. > > > > > Done. > > > > > > > > > > I just noticed that in > > > chrome/browser/media/media_stream_devices_controller.cc > > > > > it's changed to ResetAndReturn() not very long ago, in cl > > > > > https://codereview.chromium.org/1362803002. Also there are so many of it > > in > > > > > other places. Then I think it's a new practice to adopt. > > > > > Thanks so much for the clarification. > > > > > > > > I don't think you understood my objection. > > > > > > > > I'm not objecting to using ResetAndReturn() when you actually need to > reset > > > the > > > > callback before running it. I'm asking why you need to reset before > running > > > it. > > > > You changed to doing the same thing ResetAndReturn() was doing, except > > > > manually, which is strictly worse. What I'm asking is why you're > resetting > > > the > > > > callback before running it instead of after. > > > > > > > > The answer to this isn't necessarily to just reset after running, it's to > > > > explain the rationale behind _why_ you need to do one or the other. Or in > > > other > > > > words: copying code from me is no better than copying code from elsewhere > :) > > > > > > In my case, Run-Reset and Reset-run are same thing. > > > > Then do the latter. > > > > > But Reset-Run looks more safe > > > > This is precisely why I don't want you to do it if you don't need it. It > > implies to the reader that you _do_ need it: it looks as if the other pattern > is > > unsafe. If the other pattern isn't unsafe, we don't want that implication. > > > > The analogy here is to reflexively null-checking pointers. We never want to > > null-check pointers that can't be null because it tells the reader they can be > > null when they can't. > > Done. So, I just wrote an email thread to chromium-dev asking for other perspectives about this case, and it seems the consensus there is that using ResetAndReturn() here is fine, and perhaps even slightly preferable. So, I reverse myself. Go ahead and do that :) I am sorry to yank you back and forth on this, and I appreciate how accommodating you're trying to be. My general underlying advice -- "understand what your needs are and write code you know addresses them; don't trust what other code does or other people suggest until you verify it matches your case" -- is, hopefully, still good.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pkasting@, thanks a lot for all your help as it's a good learning. PTAL. https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/640001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:124: base::ResetAndReturn(&callback_).Run(devices, result, std::move(ui)); On 2016/09/22 00:12:38, Peter Kasting wrote: > On 2016/09/21 23:43:18, braveyao wrote: > > On 2016/09/21 23:00:32, Peter Kasting wrote: > > > On 2016/09/21 22:42:21, braveyao wrote: > > > > On 2016/09/21 21:39:43, Peter Kasting wrote: > > > > > On 2016/09/21 21:04:02, braveyao wrote: > > > > > > On 2016/09/21 04:07:27, Peter Kasting wrote: > > > > > > > On 2016/09/21 00:35:55, braveyao wrote: > > > > > > > > On 2016/09/20 23:50:29, Peter Kasting wrote: > > > > > > > > > Is there a reason you need to use ResetAndReturn() instead of > just > > > > Run() > > > > > > and > > > > > > > > > then Reset()? > > > > > > > > > > > > > > > > Because ResetAndReturn() is used here too, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/media_stream.... > > > > > > > > Also I see it's used lots of other places. > > > > > > > > > > > > > > That doesn't answer my question. Don't do something just because > you > > > see > > > > > > other > > > > > > > code doing it, do it because you need it. ResetAndReturn() is meant > > for > > > > > cases > > > > > > > where running the callback could change something out from under you > > in > > > a > > > > > way > > > > > > > that results in memory corruption or similar. Using it implies to > the > > > > > reader > > > > > > > that something tricky is going on with lifetime management. If > that's > > > not > > > > > > what > > > > > > > you actually need, do things the simple and direct route. > > > > > > Done. > > > > > > > > > > > > I just noticed that in > > > > chrome/browser/media/media_stream_devices_controller.cc > > > > > > it's changed to ResetAndReturn() not very long ago, in cl > > > > > > https://codereview.chromium.org/1362803002. Also there are so many of > it > > > in > > > > > > other places. Then I think it's a new practice to adopt. > > > > > > Thanks so much for the clarification. > > > > > > > > > > I don't think you understood my objection. > > > > > > > > > > I'm not objecting to using ResetAndReturn() when you actually need to > > reset > > > > the > > > > > callback before running it. I'm asking why you need to reset before > > running > > > > it. > > > > > You changed to doing the same thing ResetAndReturn() was doing, except > > > > > manually, which is strictly worse. What I'm asking is why you're > > resetting > > > > the > > > > > callback before running it instead of after. > > > > > > > > > > The answer to this isn't necessarily to just reset after running, it's > to > > > > > explain the rationale behind _why_ you need to do one or the other. Or > in > > > > other > > > > > words: copying code from me is no better than copying code from > elsewhere > > :) > > > > > > > > In my case, Run-Reset and Reset-run are same thing. > > > > > > Then do the latter. > > > > > > > But Reset-Run looks more safe > > > > > > This is precisely why I don't want you to do it if you don't need it. It > > > implies to the reader that you _do_ need it: it looks as if the other > pattern > > is > > > unsafe. If the other pattern isn't unsafe, we don't want that implication. > > > > > > The analogy here is to reflexively null-checking pointers. We never want to > > > null-check pointers that can't be null because it tells the reader they can > be > > > null when they can't. > > > > Done. > > So, I just wrote an email thread to chromium-dev asking for other perspectives > about this case, and it seems the consensus there is that using ResetAndReturn() > here is fine, and perhaps even slightly preferable. So, I reverse myself. Go > ahead and do that :) > > I am sorry to yank you back and forth on this, and I appreciate how > accommodating you're trying to be. My general underlying advice -- "understand > what your needs are and write code you know addresses them; don't trust what > other code does or other people suggest until you verify it matches your case" > -- is, hopefully, still good. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:27: InfoBarService::FromWebContents(web_contents); Nit: It's kinda ugly, but it's shorter to just inline this below: InfoBarService::FromWebContents(web_contents)->AddInfoBar( infobar_service->CreateConfirmInfoBar( std::unique_ptr<ConfirmInfoBarDelegate>( new ScreenCaptureInfoBarDelegateAndroid(web_contents, request, callback)))); https://codereview.chromium.org/2123863004/diff/740001/components/infobars/co... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2123863004/diff/740001/components/infobars/co... components/infobars/core/infobar_delegate.h:226: AsScreenCaptureInfoBarDelegateAndroid(); You no longer need this method, remove it.
Apparently I had more comments. Still LGTM tho. https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:95: content::MediaStreamDevices devices = content::MediaStreamDevices(); Nit: Omit "= content::MediaStreamDevices();", it does nothing. https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:105: if (!devices.empty()) { Nit: Isn't this true iff the above conditional succeeds? So just combine them: content::MediaStreamDevices devices; std::unique_ptr<content::MediaStreamUI> ui; if (result == content::MEDIA_DEVICE_OK) { ...; // Init both the above } base::ResetAndReturn(&callback_).Run(devices, result, std::move(ui)); https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h (right): https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:25: explicit ScreenCaptureInfoBarDelegateAndroid( Nit: No need for explicit here https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:43: // notify WebRTC with corresponding screen device and UI object. Nit: These sound like two separate actions, and technically nothing about this class knows the callback is to WebRTC code. How about: Runs |callback_|, passing it the |result|, and (if permission was granted) the appropriate stream device and UI object for video capture.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
All addressed/responded. Thanks so much! https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:27: InfoBarService::FromWebContents(web_contents); On 2016/09/23 01:07:19, Peter Kasting wrote: > Nit: It's kinda ugly, but it's shorter to just inline this below: > > InfoBarService::FromWebContents(web_contents)->AddInfoBar( > infobar_service->CreateConfirmInfoBar( > std::unique_ptr<ConfirmInfoBarDelegate>( > new ScreenCaptureInfoBarDelegateAndroid(web_contents, request, > callback)))); |infobar_service| is used twice. It isn't better to invoke |InfoBarService::FromWebContents(web_contents)| twice instead. https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:95: content::MediaStreamDevices devices = content::MediaStreamDevices(); On 2016/09/23 01:17:58, Peter Kasting wrote: > Nit: Omit "= content::MediaStreamDevices();", it does nothing. Done. https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:105: if (!devices.empty()) { On 2016/09/23 01:17:58, Peter Kasting wrote: > Nit: Isn't this true iff the above conditional succeeds? So just combine them: > > content::MediaStreamDevices devices; > std::unique_ptr<content::MediaStreamUI> ui; > if (result == content::MEDIA_DEVICE_OK) { > ...; // Init both the above > } > base::ResetAndReturn(&callback_).Run(devices, result, std::move(ui)); Done. https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h (right): https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:25: explicit ScreenCaptureInfoBarDelegateAndroid( On 2016/09/23 01:17:58, Peter Kasting wrote: > Nit: No need for explicit here Done. https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h:43: // notify WebRTC with corresponding screen device and UI object. On 2016/09/23 01:17:58, Peter Kasting wrote: > Nit: These sound like two separate actions, and technically nothing about this > class knows the callback is to WebRTC code. How about: > > Runs |callback_|, passing it the |result|, and (if permission was granted) the > appropriate stream device and UI object for video capture. Done.
The CQ bit was checked by braveyao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, piman@chromium.org, tedchoc@chromium.org, tsergeant@chromium.org, asvitkine@chromium.org, dfalcantara@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2123863004/#ps760001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... File chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc (right): https://codereview.chromium.org/2123863004/diff/740001/chrome/browser/media/w... chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc:27: InfoBarService::FromWebContents(web_contents); On 2016/09/23 18:40:55, braveyao wrote: > On 2016/09/23 01:07:19, Peter Kasting wrote: > > Nit: It's kinda ugly, but it's shorter to just inline this below: > > > > InfoBarService::FromWebContents(web_contents)->AddInfoBar( > > infobar_service->CreateConfirmInfoBar( > > std::unique_ptr<ConfirmInfoBarDelegate>( > > new ScreenCaptureInfoBarDelegateAndroid(web_contents, request, > > callback)))); > > |infobar_service| is used twice. It isn't better to invoke > |InfoBarService::FromWebContents(web_contents)| twice instead. Oh, yeah, nm, I had looked for a second invocation and missed it.
Message was sent while issue was closed.
Description was changed from ========== ScreenCapture for Android phase1, part II The capture part has been landed in cl https://codereview.chromium.org/2125973002/. This cl is to implement the control part in chrome/ and content/. The document is here, https://goo.gl/QNH29g. This cl proposes a pemission design on Android, including permission info bar and head up notification, which will show up each time at starting screen capture and user can check/stop the capture without leaving current page. And all of these is behind a flag as an experimental feature. BUG=487935 ========== to ========== ScreenCapture for Android phase1, part II The capture part has been landed in cl https://codereview.chromium.org/2125973002/. This cl is to implement the control part in chrome/ and content/. The document is here, https://goo.gl/QNH29g. This cl proposes a pemission design on Android, including permission info bar and head up notification, which will show up each time at starting screen capture and user can check/stop the capture without leaving current page. And all of these is behind a flag as an experimental feature. BUG=487935 ==========
Message was sent while issue was closed.
Committed patchset #30 (id:760001)
Message was sent while issue was closed.
Description was changed from ========== ScreenCapture for Android phase1, part II The capture part has been landed in cl https://codereview.chromium.org/2125973002/. This cl is to implement the control part in chrome/ and content/. The document is here, https://goo.gl/QNH29g. This cl proposes a pemission design on Android, including permission info bar and head up notification, which will show up each time at starting screen capture and user can check/stop the capture without leaving current page. And all of these is behind a flag as an experimental feature. BUG=487935 ========== to ========== ScreenCapture for Android phase1, part II The capture part has been landed in cl https://codereview.chromium.org/2125973002/. This cl is to implement the control part in chrome/ and content/. The document is here, https://goo.gl/QNH29g. This cl proposes a pemission design on Android, including permission info bar and head up notification, which will show up each time at starting screen capture and user can check/stop the capture without leaving current page. And all of these is behind a flag as an experimental feature. BUG=487935 Committed: https://crrev.com/dbb85e451ee0ffe9d18300befb9c0d0664b5ee94 Cr-Commit-Position: refs/heads/master@{#420679} ==========
Message was sent while issue was closed.
Patchset 30 (id:??) landed as https://crrev.com/dbb85e451ee0ffe9d18300befb9c0d0664b5ee94 Cr-Commit-Position: refs/heads/master@{#420679} |
