Desktop chrome tab capture-chooseDesktopMedia()

content/public/browser/desktop_media_id.cc

Index: content/public/browser/desktop_media_id.cc
diff --git a/content/public/browser/desktop_media_id.cc b/content/public/browser/desktop_media_id.cc
index 523c61d39bc696c912a50f7253076bf912091354..c1b68d8ebb6c209fc9b7b8f65af6fa9481b897be 100644
--- a/content/public/browser/desktop_media_id.cc
+++ b/content/public/browser/desktop_media_id.cc
@@ -82,7 +82,8 @@ const char kWindowPrefix[] = "window";
 // static
 DesktopMediaID DesktopMediaID::RegisterAuraWindow(DesktopMediaID::Type type,
                                                   aura::Window* window) {
-  DCHECK(type == TYPE_SCREEN || type == TYPE_WINDOW);
+  DCHECK(type == TYPE_SCREEN || type == TYPE_WINDOW ||
+         type == TYPE_WEB_CONTENTS);

[Sergey Ulanov, 2016/01/13 19:06:11]

I don't think TYPE_WEB_CONTENTS makes sense here. aura::Window can be a screen
or a window, but not web contents. I.e. RegisterAuraWindow() shouldn't be called
for tabs.

[GeorgeZ, 2016/01/13 22:29:02]

Done.

   DCHECK(window);
   DesktopMediaID media_id(type, kNullId);
   media_id.aura_id = AuraWindowRegistry::GetInstance()->RegisterWindow(window);
@@ -96,45 +97,84 @@ aura::Window* DesktopMediaID::GetAuraWindowById(const DesktopMediaID& id) {
 
 #endif  // defined(USE_AURA)
 
+bool DesktopMediaID::operator<(const DesktopMediaID& other) const {
+#if defined(USE_AURA)
+  return std::tie(type, id, aura_id, web_contents_id) <
+         std::tie(other.type, other.id, other.aura_id, other.web_contents_id);
+#else
+  return std::tie(type, id, web_contents_id) <
+         std::tie(other.type, other.id, web_contents_id);
+#endif
+}
+
+bool DesktopMediaID::operator==(const DesktopMediaID& other) const {
+#if defined(USE_AURA)
+  return type == other.type && id == other.id && aura_id == other.aura_id &&
+         web_contents_id == other.web_contents_id;
+#else
+  return type == other.type && id == other.id &&
+         web_contents_id == other.web_contents_id;
+#endif
+}
+
 // static
+// Input string should in format:
+// for WebContents:
+// web-contents-media-stream://"render_process_id":"render_process_id"
+// for no WebContents and aura: screen:"window_id" or window:"window_id"
+// for no web_contents and no aura: screen:"window_id:aura_id" or
+//                         window:"window_id:aura_id".
 DesktopMediaID DesktopMediaID::Parse(const std::string& str) {
   std::vector<std::string> parts = base::SplitString(
       str, ":", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
 
-#if defined(USE_AURA)
-  if (parts.size() != 3)
-    return DesktopMediaID();
-#else
-  if (parts.size() != 2)
+  if (parts.size() < 2)
     return DesktopMediaID();
-#endif
 
   Type type = TYPE_NONE;
   if (parts[0] == kScreenPrefix) {
     type = TYPE_SCREEN;
   } else if (parts[0] == kWindowPrefix) {
     type = TYPE_WINDOW;
+  } else if (parts[0] == kWebContentsCaptureScheme) {
+    type = TYPE_WEB_CONTENTS;
   } else {
     return DesktopMediaID();
   }
 
-  int64_t id;
-  if (!base::StringToInt64(parts[1], &id))
+  // WebContents type.
+  if (type == TYPE_WEB_CONTENTS) {
+    WebContentsMediaCaptureId temp_id = WebContentsMediaCaptureId::Parse(str);
+    return DesktopMediaID(type, 0, temp_id);

[Sergey Ulanov, 2016/01/13 19:06:11]

WebContentsMediaCaptureId::Parse() may fail and that case is not handled here. 
This code would be simpler if you first try to parse the id as a web content id,
and then do everythin else otherwise. I.e. something like this:

DesktopMediaID DesktopMediaID::Parse(const std::string& str) {
  WebContentsMediaCaptureId web_id = WebContentsMediaCaptureId::Parse(str);
  if (!web_id.is_null())
    return DesktopMediaID(TYPE_WEB_CONTENTS, 0, web_id);

  ... Rest of the code to parse screen/window ids the way it was before.
}

In your current implementation you check that the id starts with
web-contents-media-stream: twice and this isn't necessary.

[GeorgeZ, 2016/01/13 22:29:02]

Good idea.

+  }
+
+// Screen and window types.
+#if defined(USE_AURA)
+  if (parts.size() != 3)
+    return DesktopMediaID();
+#else
+  if (parts.size() != 2)
+    return DesktopMediaID();
+#endif  // defined(USE_AURA)
+
+  int64_t tempid;

[Sergey Ulanov, 2016/01/13 19:06:11]

Please keep this called 'id' instead of 'tempid'. Adding "temp" prefix doesn't
make this code easier to understand. And then according to style guide the
proper name would be temporary_id, not tempid.

[GeorgeZ, 2016/01/13 22:29:02]

Done.

+  if (!base::StringToInt64(parts[1], &tempid))
     return DesktopMediaID();
 
-  DesktopMediaID media_id(type, id);
+  DesktopMediaID media_id(type, tempid);
 
 #if defined(USE_AURA)
   int64_t aura_id;
   if (!base::StringToInt64(parts[2], &aura_id))
     return DesktopMediaID();
+
   media_id.aura_id = aura_id;
 #endif  // defined(USE_AURA)
 
   return media_id;
 }
 
-std::string DesktopMediaID::ToString() {
+std::string DesktopMediaID::ToString() const {
   std::string prefix;
   switch (type) {
     case TYPE_NONE:
@@ -146,9 +186,13 @@ std::string DesktopMediaID::ToString() {
     case TYPE_WINDOW:
       prefix = kWindowPrefix;
       break;
+    case TYPE_WEB_CONTENTS:
+      return web_contents_id.ToString();
+      break;
   }
   DCHECK(!prefix.empty());
 
+  // Screen and Window types.
   prefix.append(":");
   prefix.append(base::Int64ToString(id));