Desktop chrome tab capture-chooseDesktopMedia()

chrome/browser/media/desktop_media_list_ash.cc

Index: chrome/browser/media/desktop_media_list_ash.cc
diff --git a/chrome/browser/media/desktop_media_list_ash.cc b/chrome/browser/media/desktop_media_list_ash.cc
index 7bbd85ca20adb2b6845db9c3b82650a6a92e133f..ed4a320caaa5f986731faeb08cb9703169ce99b0 100644
--- a/chrome/browser/media/desktop_media_list_ash.cc
+++ b/chrome/browser/media/desktop_media_list_ash.cc
@@ -13,8 +13,14 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "chrome/browser/media/desktop_media_list_observer.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/favicon/content/content_favicon_driver.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_process_host.h"
 #include "media/base/video_util.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/compositor/dip_util.h"
@@ -169,6 +175,61 @@ void DesktopMediaListAsh::EnumerateWindowsForRoot(
   }
 }
 
+void DesktopMediaListAsh::EnumerateTabs(
+    std::vector<DesktopMediaListAsh::SourceDescription>* sources) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  Profile* profile = ProfileManager::GetLastUsedProfileAllowedByPolicy();
+  std::vector<Browser*> browsers = FindAllTabbedBrowsersWithProfile(
+      profile, chrome::HOST_DESKTOP_TYPE_NATIVE);
+
+  std::map<base::TimeTicks, SourceDescription> tab_map;
+
+  for (auto browser : browsers) {

[Sergey Ulanov, 2015/12/18 19:45:04]

This code is duplicated in DesktopMediaListAsh and NativeDesktopMediaList and
it's best to avoid this. 
I suggest you keep DesktopMediaListAsh and NativeDesktopMediaList the way they
are and add two new classes: TabDesktopMediaList and CombinedDesktopMediaList. 
TabDesktopMediaList will contain just list of tabs. CombinedDesktopMediaList
will merge 2 lists together.

[GeorgeZ, 2015/12/18 21:57:26]

You are right, the duplication is not good. However, introducing
CombinedDesktopMediaList may lead to major modification of DesktopMediaListAsh
and NativeDesktopMediaList. DesktopMeidaPickerView is the observer of those two
classes. After introducing CombinedDesktopMediaList, the observer and associated
callback need to be stripped from DesktopMediaListAsh and NativeDesktopMediaList
to CombinedDesktopMediaList to make logical simple and clear.

DesktopMediaListAsh and NativeDesktopMediaList behave differently regarding to
the observer. Two threads are associated with startupdateing() of
NativeDesktopMediaList. observer will be notified all media list first and then
all associate thumbnail. While a single thread is associated with
startUpdating() of DesktopMediaListAsh. Observer is notified each medial and its
thumbnail one by one.

How about I add a utility class to contain all the common code of tab capture
for DesktopMediaListAsh and NativeDesktopMediaList. This will be a relative
simple solution. 

[GeorgeZ, 2015/12/21 16:17:37]

After I further think about this duplication issue, my understanding is as
below: since DesktopMediaList is an interface class, all the implementations are
pushed to its derived children. This leads to duplication in the children. For
examples several set functions, sorting codes in
NativeDesktopMediaList::OnSourcesList() and DesktopMediaListAsh::Refresh() are
duplicated. Adding tab capture support makes duplication worse. Though adding
TabDesktopMediaList class will solve the duplication for tab capture, it make
the other duplications ever worse. Adding CombinedDesktopMediaList class also
will make coding logic more complex.

One possible solution is to do common implementation in DesktopMediaList class.
But this will break the design that DesktopMediaList is an interface class.
Another solution is  all an DesktopMediaListImpl class that hold all common code
and let NativeDesktopMediaList and DesktopMediaListAsh derived from it.

Let me know what's your opinion.

[Sergey Ulanov, 2016/01/04 18:59:29]

With CombinedDesktopMediaList you get more classes, but each of them will be
much simpler, and that will allow you to test each of them independently.
This might be acceptable (except that it would be better to call the base class
DesktopMediaListBase, as it's not full implementation), But I would still prefer
the solution I suggested.

[Sergey Ulanov, 2016/01/04 18:59:29]

Yes, CombinedDesktopMediaList will need to handle callbacks (i.e. implement
Observer interface) for the 2 underlying implementations of DesktopMediaList and
then call the observer that it has (which is normally the picker dialog). If you
implement observer interface directly in CombinedDesktopMediaList then it will
need to distinguish between the calls from the two sublists. You can solve this
by adding an argument in each Observer call to indicate the caller, i.e.:

class DesktopMediaListObserver {
 public:
  virtual void OnSourceAdded(DesktopMediaList* list, int index) = 0;
  virtual void OnSourceRemoved(DesktopMediaList* list, int index) = 0;
  virtual void OnSourceMoved(DesktopMediaList* list, int old_index, int
new_index) = 0;
  virtual void OnSourceNameChanged(DesktopMediaList* list, int index) = 0;
  virtual void OnSourceThumbnailChanged(DesktopMediaList* list, int index) = 0;
}

There will be other cases when CombinedDesktopMediaList will be useful. E.g. see
crbug.com/552123 - to fix it we will need to have a list of both ash and native
window. That would be very easy to solve with CombinedDesktopMediaList.
I think the code for CombinedDesktopMediaList should be very simple. Let me know
if it's not clear how to implement it - I will help you.
I don't think that would make any difference. The Observer should not depend on
these details.

[GeorgeZ, 2016/01/06 22:41:39]

How about I address this issue in another CL right after this one. This CL was
expanded and quit big now.

+    TabStripModel* tab_strip_model = browser->tab_strip_model();
+    DCHECK(tab_strip_model);
+    for (int i = 0; i < tab_strip_model->count(); i++) {
+      // Create DesktopMediaID.
+      content::WebContents* contents = tab_strip_model->GetWebContentsAt(i);
+      gfx::NativeWindow tab_window = contents->GetTopLevelNativeWindow();
+      DesktopMediaID media_id = DesktopMediaID::RegisterAuraWindow(
+          DesktopMediaID::TYPE_TAB, tab_window);
+
+      content::RenderFrameHost* const main_frame = contents->GetMainFrame();
+      DCHECK(main_frame);
+
+      media_id.tab_id.render_process_id = main_frame->GetProcess()->GetID();
+      media_id.tab_id.main_render_frame_id = main_frame->GetRoutingID();
+
+      // Get tab title.
+      base::string16 title = l10n_util::GetStringFUTF16(
+          IDS_DESKTOP_MEDIA_PICKER_CHROME_TAB_TITLE, contents->GetTitle());
+
+      // Get tab's last active time stamp.
+      base::TimeTicks t = contents->GetLastActiveTime();
+      tab_map.insert(std::make_pair(t, SourceDescription(media_id, title)));
+
+      // Create thumbnail based on favicon for tab.
+      favicon::FaviconDriver* favicon_driver =
+          favicon::ContentFaviconDriver::FromWebContents(contents);
+      if (favicon_driver) {
+        ++pending_window_capture_requests_;
+        gfx::Image tab_icon = favicon_driver->GetFavicon();

[mark a. foltz, 2016/01/04 20:11:41]

What if the tab does not have a favicon?

Nit: In the code base, a "thumbnail" refers to a scaled down image of the
WebContents-as-rendered.  I think it's confusing to refer to a favicon as a
thumbnail.

[GeorgeZ, 2016/01/06 22:41:39]

If there is no favicon, there is no drawing in the thumbnail area. The area will
be shown as white background.

We use snapshot shot for media types other than tab. Original I was going the
same way for tab/webcontents. However I was advised not to load and capture
inactive tab/webcontent since it is too cost. Therefore, as an alternative, I
choose favicon.

+        tab_map[t].thumbnail =

[mark a. foltz, 2016/01/04 20:11:41]

How expensive is the creation of these favicon images?  It appears to be run in
the code that renders the desktop picker window, i.e. the UI main thread?

[GeorgeZ, 2016/01/06 22:41:39]

We use screen shot for media type of screen and windows. favicon for tab costs
much less than screen shot. favicon has to be collected in UI thread.

+            CreateEnlargedFaviconImage(thumbnail_size_, tab_icon);
+      }
+    }
+  }
+
+  // Add timely sorted tab sources into vector. Most recent one first.
+  for (auto it = tab_map.rbegin(); it != tab_map.rend(); ++it) {
+    SourceDescription& source = it->second;
+    sources->push_back(source);
+    if (!it->second.thumbnail.isNull())
+      OnThumbnailCaptured(source.id, gfx::Image(source.thumbnail));
+  }
+}
+
 void DesktopMediaListAsh::EnumerateSources(
     std::vector<DesktopMediaListAsh::SourceDescription>* sources) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -209,6 +270,9 @@ void DesktopMediaListAsh::EnumerateSources(
       EnumerateWindowsForRoot(
           sources, root_windows[i], ash::kShellWindowId_DockedContainer);
     }
+
+    if (source_types_ & TABS)
+      EnumerateTabs(sources);
   }
 }