ScreenCapture for Android phase1, part II

chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/media/webrtc/screen_capture_infobar_delegate_android.h"
+
+#include "base/callback_helpers.h"
+#include "chrome/browser/infobars/infobar_service.h"
+#include "chrome/browser/media/webrtc/desktop_streams_registry.h"
+#include "chrome/browser/media/webrtc/media_capture_devices_dispatcher.h"
+#include "chrome/browser/media/webrtc/media_stream_capture_indicator.h"
+#include "chrome/grit/generated_resources.h"
+#include "chrome/grit/theme_resources.h"
+#include "components/infobars/core/infobar.h"
+#include "components/url_formatter/elide_url.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/common/media_stream_request.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_capture_types.h"
+#include "ui/base/l10n/l10n_util.h"
+
+// static
+void ScreenCaptureInfoBarDelegateAndroid::Create(
+    content::WebContents* web_contents,
+    const content::MediaStreamRequest& request,
+    const ResponseCallback& callback) {
+  InfoBarService* infobar_service =
+      InfoBarService::FromWebContents(web_contents);
+
+  std::unique_ptr<infobars::InfoBar> new_infobar(
+      infobar_service->CreateConfirmInfoBar(
+          std::unique_ptr<ConfirmInfoBarDelegate>(
+              new ScreenCaptureInfoBarDelegateAndroid(web_contents, request,
+                                                      callback))));
+
+  for (size_t i = 0; i < infobar_service->infobar_count(); ++i) {

[Peter Kasting, 2016/09/20 23:50:28]

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?

[braveyao, 2016/09/21 00:35:55]

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.

[Peter Kasting, 2016/09/21 04:07:27]

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).

[braveyao, 2016/09/21 21:04:02]

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.

[Peter Kasting, 2016/09/21 21:39:43]

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?

[braveyao, 2016/09/21 22:42:21]

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.

[Peter Kasting, 2016/09/21 23:00:32]

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.

[braveyao, 2016/09/21 23:43:18]

Done.

+    infobars::InfoBar* old_infobar = infobar_service->infobar_at(i);
+    ScreenCaptureInfoBarDelegateAndroid* delegate =
+        old_infobar->delegate()->AsScreenCaptureInfoBarDelegateAndroid();
+    if (delegate != nullptr) {
+      infobar_service->ReplaceInfoBar(old_infobar, std::move(new_infobar));
+      return;
+    }
+  }
+
+  infobar_service->AddInfoBar(std::move(new_infobar));
+}
+
+ScreenCaptureInfoBarDelegateAndroid::ScreenCaptureInfoBarDelegateAndroid(
+    content::WebContents* web_contents,
+    const content::MediaStreamRequest& request,
+    const ResponseCallback& callback)
+    : web_contents_(web_contents), request_(request), callback_(callback) {
+  DCHECK(request.video_type == content::MEDIA_DESKTOP_VIDEO_CAPTURE);

[Peter Kasting, 2016/09/20 23:50:28]

Nit: DCHECK_EQ(content::MEDIA_DESKTOP_VIDEO_CAPTURE, request.video_type);

[braveyao, 2016/09/21 00:35:55]

Done.

+}
+
+ScreenCaptureInfoBarDelegateAndroid::~ScreenCaptureInfoBarDelegateAndroid() {
+  if (!callback_.is_null()) {
+    callback_.Run(content::MediaStreamDevices(),
+                  content::MEDIA_DEVICE_FAILED_DUE_TO_SHUTDOWN,
+                  std::unique_ptr<content::MediaStreamUI>());

[Peter Kasting, 2016/09/20 23:50:28]

Nit: Just use nullptr?

[braveyao, 2016/09/21 00:35:55]

Done.

+  }
+}
+
+infobars::InfoBarDelegate::InfoBarIdentifier
+ScreenCaptureInfoBarDelegateAndroid::GetIdentifier() const {
+  return SCREEN_CAPTURE_INFOBAR_DELEGATE_ANDROID;
+}
+
+ScreenCaptureInfoBarDelegateAndroid*
+ScreenCaptureInfoBarDelegateAndroid::AsScreenCaptureInfoBarDelegateAndroid() {
+  return this;
+}
+
+base::string16 ScreenCaptureInfoBarDelegateAndroid::GetMessageText() const {
+  return l10n_util::GetStringFUTF16(
+      IDS_MEDIA_CAPTURE_SCREEN,
+      url_formatter::FormatUrlForSecurityDisplay(request_.security_origin));
+}
+
+int ScreenCaptureInfoBarDelegateAndroid::GetIconId() const {
+  return IDR_INFOBAR_MEDIA_STREAM_SCREEN;
+}
+
+base::string16 ScreenCaptureInfoBarDelegateAndroid::GetButtonLabel(
+    InfoBarButton button) const {
+  return l10n_util::GetStringUTF16((button == BUTTON_OK) ? IDS_PERMISSION_ALLOW
+                                                         : IDS_PERMISSION_DENY);
+}
+
+bool ScreenCaptureInfoBarDelegateAndroid::Accept() {
+  RunCallback(content::MEDIA_DEVICE_OK);
+  return true;
+}
+
+bool ScreenCaptureInfoBarDelegateAndroid::Cancel() {
+  RunCallback(content::MEDIA_DEVICE_PERMISSION_DENIED);
+  return true;
+}
+
+void ScreenCaptureInfoBarDelegateAndroid::InfoBarDismissed() {
+  RunCallback(content::MEDIA_DEVICE_PERMISSION_DISMISSED);
+}
+
+void ScreenCaptureInfoBarDelegateAndroid::RunCallback(
+    content::MediaStreamRequestResult result) {
+  CHECK(!callback_.is_null());

[Peter Kasting, 2016/09/20 23:50:28]

Nit: Prefer DCHECK except for debugging or security-critical issues

Consider also a DCHECK in the constructor, since this sort of implies that.

[braveyao, 2016/09/21 00:35:55]

Done.

+
+  content::MediaStreamDevices devices = content::MediaStreamDevices();
+  if (result == content::MEDIA_DEVICE_OK) {
+    content::DesktopMediaID screen_id;

[Peter Kasting, 2016/09/20 23:50:28]

Nit: Combine these two lines

[braveyao, 2016/09/21 00:35:55]

Done.

+    screen_id = content::DesktopMediaID(content::DesktopMediaID::TYPE_SCREEN,
+                                        webrtc::kFullDesktopScreenId);
+    devices.push_back(content::MediaStreamDevice(
+        content::MEDIA_DESKTOP_VIDEO_CAPTURE, screen_id.ToString(), "Screen"));
+  }
+
+  std::unique_ptr<content::MediaStreamUI> ui;
+  if (!devices.empty()) {
+    ui = MediaCaptureDevicesDispatcher::GetInstance()
+             ->GetMediaStreamCaptureIndicator()
+             ->RegisterMediaStream(web_contents_, devices);
+  }
+
+  base::ResetAndReturn(&callback_).Run(devices, result, std::move(ui));

[Peter Kasting, 2016/09/20 23:50:29]

Is there a reason you need to use ResetAndReturn() instead of just Run() and
then Reset()?

[braveyao, 2016/09/21 00:35:55]

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. 

[Peter Kasting, 2016/09/21 04:07:27]

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.

[braveyao, 2016/09/21 21:04:02]

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.

[Peter Kasting, 2016/09/21 21:39:43]

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 :)

[braveyao, 2016/09/21 22:42:21]

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. 

[Peter Kasting, 2016/09/21 23:00:32]

Then do the latter.
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.

[braveyao, 2016/09/21 23:43:18]

Done.

[Peter Kasting, 2016/09/22 00:12:38]

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.

[braveyao, 2016/09/22 00:30:23]

Done.

+}