Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1503563004:
Desktop chrome tab capture-chooseDesktopMedia() (Closed)
|
Created:
5 years ago by GeorgeZ Modified:
4 years, 11 months ago Reviewers:
qiangchen, Ken Rockot(use gerrit already), Avi (use Gerrit), Sergey Ulanov, mark a. foltz, msw, DaleCurtis, miu CC:
chromium-apps-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, posciak+watch_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis is the second of three CLs for desktop chrome tab capture.
WebRTC today supports media capture and sharing for "screen" and "window" types through Javascript APIs chooseDesktopMedia/GetUserMedia.
This Cl adds support for chrome tab for JavaScript API chooseDesktopMedia(). If "tab" is enabled in chooseDesktopMedia(), the desktop media pick dialog will list all chrome tabs for user to select. No tab video/audio will be actually captured until the third CL.
The design document can be found at https://goo.gl/uoHnv0
BUG=557222
Committed: https://crrev.com/7194a6fa14a2fea0db9c20602b4a16913d7295f3
Cr-Commit-Position: refs/heads/master@{#369447}
Patch Set 1 #
Total comments: 47
Patch Set 2 : review round 1 #
Total comments: 25
Patch Set 3 : review round2 #
Total comments: 12
Patch Set 4 : review round 3 #
Total comments: 8
Patch Set 5 : review round 4 #
Total comments: 9
Patch Set 6 : review patch 5 #
Total comments: 53
Patch Set 7 : #Patch Set 8 : rebase #
Total comments: 6
Patch Set 9 : #Patch Set 10 : #
Total comments: 23
Patch Set 11 : fixed a unit test bug #Patch Set 12 : #Patch Set 13 : #
Total comments: 10
Patch Set 14 : #
Total comments: 2
Patch Set 15 : #Patch Set 16 : #
Total comments: 18
Patch Set 17 : #
Total comments: 4
Patch Set 18 : #Messages
Total messages: 66 (11 generated)
Description was changed from ========== treat for no capture merge for cl2 add a sanity check a review update restore bluetooth_utility test with ash order tab for ash a little order Enclose favicon and disable audio capture order tabs based on their time stamps Use Fovicon for all tab previous fixed a conflict BUG=557222 ========== to ========== This is the second of three CLs for desktop chrome tab capture. WebRTC today supports media capture and sharing for "screen" and "window" types through Javascript APIs chooseDesktopMedia/GetUserMedia. This Cl adds support for chrome tab for JavaScript API chooseDesktopMedia(). If "tab" is enabled in chooseDesktopMedia(), the desktop media pick dialog will list all chrome tabs for user to select. No tab video/audio will be actually captured until the third CL. The design document can be found at https://goo.gl/uoHnv0 BUG=557222 ==========
gyzhou@chromium.org changed reviewers: + mcasas@chromium.org
Miguel, Please have a review of this CL before I send it out of our group for further review. Thanks, George
gyzhou@chromium.org changed reviewers: + qiangchen@chromium.org - mcasas@chromium.org
Qiang, Please have a review of this CL. Thanks, George
https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... File chrome/browser/media/desktop_media_list.h (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list.h:66: static gfx::ImageSkia CreateImageEncloseFavicon(gfx::Size size, Better to separate the definition from declaration, as this function is large. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list.h:69: int scale = 3; const? https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list.h:71: int thick = 5; const? https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list.h:94: int bpp = result.bytesPerPixel(); Again my question, the follow code seems to assume bpp = 4. Is that always true? https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... File chrome/browser/media/desktop_media_list_ash.cc (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list_ash.cc:208: base::UTF8ToUTF16("Chrome tab_") + contents->GetTitle(); Just wonder do we need to throw "Chrome tab_" to resource for translation? https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... File chrome/browser/media/desktop_media_list_ash.h (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list_ash.h:33: public: Any reason to move SourceTypes to the parent class? https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:207: // Favicon has already captured for tabs. Consider moving the Favicon "capturing" here. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:317: // Get tab source list Consider moving this block to Worker::Refresh. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:331: bool NativeDesktopMediaList::GetTabSourceDescription( GetTabSourceDescriptions is better. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:461: void NativeDesktopMediaList::OnSourceTabThumbnail(SourceDescription source) { Is it possible to remove this function, and merge functionality into OnSourceThumbNail? https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... File chrome/browser/media/native_desktop_media_list_unittest.cc (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list_unittest.cc:241: scoped_ptr<webrtc::WindowCapturer>(), false)); Consider adding a test case say "TabOnly" https://codereview.chromium.org/1503563004/diff/1/content/public/browser/desk... File content/public/browser/desktop_media_id.cc (right): https://codereview.chromium.org/1503563004/diff/1/content/public/browser/desk... content/public/browser/desktop_media_id.cc:160: media_id.render_process_id = rend_process_id; Consider adding a constructor for DesktopMediaID. https://codereview.chromium.org/1503563004/diff/1/content/public/browser/desk... File content/public/browser/desktop_media_id.h (right): https://codereview.chromium.org/1503563004/diff/1/content/public/browser/desk... content/public/browser/desktop_media_id.h:32: static const int kINVALIDID = -1; Any reason kNullId isn't sufficient to use?
miu@chromium.org changed reviewers: + miu@chromium.org
https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... File chrome/browser/media/desktop_media_list_ash.cc (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list_ash.cc:192: const int kNumTabs = tab_strip_model->count(); naming: This variable is not assigned from a literal value. Please call it |number_of_tabs| instead. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list_ash.cc:208: base::UTF8ToUTF16("Chrome tab_") + contents->GetTitle(); 1. This string needs to be in generated_resources.grd so it can be translated into multiple languages. 2. "Chrome tab_" is kind of weird. How about "Tab -- "? Or, maybe a UX designer can help you with this? https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list_ash.cc:214: // Get thumbnail for tab. These aren't tab thumbnails. They're favicons. Please use consistent terminology in all of the variable/function names and code comments throughout this CL. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... File chrome/browser/media/desktop_streams_registry.cc (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_streams_registry.cc:45: id = source.ToString(); This is a security violation. If you need this information, please add it to the ApprovedDesktopMediaStream struct. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:207: // Favicon has already captured for tabs. On 2015/12/07 22:45:56, qiangchenC wrote: > Consider moving the Favicon "capturing" here. IIUC, I think that has to be done on the UI thread, not here. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:322: LOG(ERROR) << "Failed to get tab sources description"; Please remove this logging statement. This operation can't fail (see comment below). https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:331: bool NativeDesktopMediaList::GetTabSourceDescription( This method should not return anything because it will always succeed. (Note that you never "return false" anywhere.) https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:363: base::UTF8ToUTF16("Chrome tab_") + contents->GetTitle(); ditto: Make this a string resource. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... File chrome/browser/media/native_desktop_media_list.h (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.h:78: bool tab_capture_enabled_; Should be: const bool tab_capture_enabled_ (since this value should not be changed after construction) https://codereview.chromium.org/1503563004/diff/1/content/public/browser/desk... File content/public/browser/desktop_media_id.cc (right): https://codereview.chromium.org/1503563004/diff/1/content/public/browser/desk... content/public/browser/desktop_media_id.cc:77: const char kTabPrefix[] = "web-contents-media-stream"; This and a lot of the other changes in this file duplicate the code found in content/browser/media/capture/web_contents_capture_util.*. Instead, please move web_contents_capture_util.* to content/public/browser/web_contents_media_capture_id.* and have everything use that so the logic is centralized. https://codereview.chromium.org/1503563004/diff/1/content/public/browser/desk... File content/public/browser/desktop_media_id.h (right): https://codereview.chromium.org/1503563004/diff/1/content/public/browser/desk... content/public/browser/desktop_media_id.h:32: static const int kINVALIDID = -1; style: kInvalidId
Qian and Yuri, A patch is uploaded based your reviews. Thanks, George https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... File chrome/browser/media/desktop_media_list.h (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list.h:66: static gfx::ImageSkia CreateImageEncloseFavicon(gfx::Size size, On 2015/12/07 22:45:56, qiangchenC wrote: > Better to separate the definition from declaration, as this function is large. Done. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list.h:69: int scale = 3; On 2015/12/07 22:45:56, qiangchenC wrote: > const? Done. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list.h:71: int thick = 5; On 2015/12/07 22:45:56, qiangchenC wrote: > const? Done. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list.h:94: int bpp = result.bytesPerPixel(); On 2015/12/07 22:45:56, qiangchenC wrote: > Again my question, the follow code seems to assume bpp = 4. > Is that always true? result.allocN32Pixels() guarantees result image's bpp=4; (icon.colorType() != result.colorType()) return gfx::ImageSkia(); guarantees result image's bpp == icon image's bpp https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... File chrome/browser/media/desktop_media_list_ash.cc (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list_ash.cc:192: const int kNumTabs = tab_strip_model->count(); On 2015/12/08 01:54:38, miu wrote: > naming: This variable is not assigned from a literal value. Please call it > |number_of_tabs| instead. Changed for loop to for (int i = 0; i < tab_strip_model->count(); i++) { to remove this variable https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list_ash.cc:208: base::UTF8ToUTF16("Chrome tab_") + contents->GetTitle(); On 2015/12/07 22:45:56, qiangchenC wrote: > Just wonder do we need to throw "Chrome tab_" to resource for translation? Done. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list_ash.cc:208: base::UTF8ToUTF16("Chrome tab_") + contents->GetTitle(); On 2015/12/08 01:54:38, miu wrote: > 1. This string needs to be in generated_resources.grd so it can be translated > into multiple languages. > > 2. "Chrome tab_" is kind of weird. How about "Tab -- "? Or, maybe a UX > designer can help you with this? For 1, I moved string into resources.grd. For 2, I used to used "tab_", however some reviewers think "chrome tab_" is better. Any way, there is going to have UI refactor later and prefix string most likely will disappear. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list_ash.cc:214: // Get thumbnail for tab. On 2015/12/08 01:54:38, miu wrote: > These aren't tab thumbnails. They're favicons. Please use consistent > terminology in all of the variable/function names and code comments throughout > this CL. The whole block is for thumbnail creation. I will change comment to "Create thumbnail based on favicon for tab." to remove ambiguous. I Created a background thumbnail and put the scaled/Enlarged favicon in its Center. One reason to do so is that if I use Favicon directly for tab_map[t].thumbnail. Mac and windows system will behave diffently. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... File chrome/browser/media/desktop_media_list_ash.h (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_media_list_ash.h:33: public: On 2015/12/07 22:45:56, qiangchenC wrote: > Any reason to move SourceTypes to the parent class? This definition once shared with a sibling class. I will move it back to child class. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... File chrome/browser/media/desktop_streams_registry.cc (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/deskto... chrome/browser/media/desktop_streams_registry.cc:45: id = source.ToString(); On 2015/12/08 01:54:38, miu wrote: > This is a security violation. If you need this information, please add it to > the ApprovedDesktopMediaStream struct. I will remove this one. I remembered this change is not definite needed. It is just better to have. if I am wrong, I will figure it out in the next CL that is going to hook up webcontentscapture. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:207: // Favicon has already captured for tabs. On 2015/12/07 22:45:56, qiangchenC wrote: > Consider moving the Favicon "capturing" here. This is a no UI thread. Favicon capture has to to done in UI thread. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:317: // Get tab source list On 2015/12/07 22:45:56, qiangchenC wrote: > Consider moving this block to Worker::Refresh. Many work has to be done in UI thread here. worker is in a no UI thread. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:322: LOG(ERROR) << "Failed to get tab sources description"; On 2015/12/08 01:54:38, miu wrote: > Please remove this logging statement. This operation can't fail (see comment > below). Done. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:331: bool NativeDesktopMediaList::GetTabSourceDescription( On 2015/12/07 22:45:56, qiangchenC wrote: > GetTabSourceDescriptions is better. Done. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:331: bool NativeDesktopMediaList::GetTabSourceDescription( On 2015/12/08 01:54:38, miu wrote: > This method should not return anything because it will always succeed. (Note > that you never "return false" anywhere.) Done. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:363: base::UTF8ToUTF16("Chrome tab_") + contents->GetTitle(); On 2015/12/08 01:54:38, miu wrote: > ditto: Make this a string resource. Done. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.cc:461: void NativeDesktopMediaList::OnSourceTabThumbnail(SourceDescription source) { On 2015/12/07 22:45:56, qiangchenC wrote: > Is it possible to remove this function, and merge functionality into > OnSourceThumbNail? Favicon is capture and stored in a UI thead. While desktop snapshot is capture in none UI Thread. It can be awkward to try to merge those two functions into one, since I have to pass necessary info from one thread to another. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... File chrome/browser/media/native_desktop_media_list.h (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list.h:78: bool tab_capture_enabled_; On 2015/12/08 01:54:38, miu wrote: > Should be: const bool tab_capture_enabled_ (since this value should not be > changed after construction) Done. https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... File chrome/browser/media/native_desktop_media_list_unittest.cc (right): https://codereview.chromium.org/1503563004/diff/1/chrome/browser/media/native... chrome/browser/media/native_desktop_media_list_unittest.cc:241: scoped_ptr<webrtc::WindowCapturer>(), false)); On 2015/12/07 22:45:56, qiangchenC wrote: > Consider adding a test case say "TabOnly" Need modify unit tests desktop_meida_list_ash_uninttest.cc and native_desktop_meida_list_uninttest.cc and associated desktop_meida_list_ash.cc and native_desktop_meida_list.cc to cover tab cases. How about that I create another CL to cover units test to limit the scope of this CL? https://codereview.chromium.org/1503563004/diff/1/content/public/browser/desk... File content/public/browser/desktop_media_id.cc (right): https://codereview.chromium.org/1503563004/diff/1/content/public/browser/desk... content/public/browser/desktop_media_id.cc:77: const char kTabPrefix[] = "web-contents-media-stream"; On 2015/12/08 01:54:38, miu wrote: > This and a lot of the other changes in this file duplicate the code found in > content/browser/media/capture/web_contents_capture_util.*. Instead, please move > web_contents_capture_util.* to > content/public/browser/web_contents_media_capture_id.* and have everything use > that so the logic is centralized. After modification, DesktopMediaID has an instance of WebContentsMediaCaptureId to take care of render_process_id and main_render_frame_id. https://codereview.chromium.org/1503563004/diff/1/content/public/browser/desk... content/public/browser/desktop_media_id.cc:160: media_id.render_process_id = rend_process_id; On 2015/12/07 22:45:56, qiangchenC wrote: > Consider adding a constructor for DesktopMediaID. Done. https://codereview.chromium.org/1503563004/diff/1/content/public/browser/desk... File content/public/browser/desktop_media_id.h (right): https://codereview.chromium.org/1503563004/diff/1/content/public/browser/desk... content/public/browser/desktop_media_id.h:32: static const int kINVALIDID = -1; On 2015/12/07 22:45:56, qiangchenC wrote: > Any reason kNullId isn't sufficient to use? This is to make the code safe. render_processor_id and main_render_frame_id cannot be negative value logically. However there is no definitely guarantee that they cannot be 0. https://codereview.chromium.org/1503563004/diff/1/content/public/browser/desk... content/public/browser/desktop_media_id.h:32: static const int kINVALIDID = -1; On 2015/12/08 01:54:38, miu wrote: > style: kInvalidId Done.
Yuri and Qiang, Please review the new patch based on your comments. Thanks, George
Looking much better. A few remaining things: https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/de... File chrome/browser/media/desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/de... chrome/browser/media/desktop_media_list.cc:11: const int scale = 3; style nit: kScale https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/de... chrome/browser/media/desktop_media_list.cc:13: const int thick = 5; style nit: kBoundaryThickness https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/de... chrome/browser/media/desktop_media_list.cc:15: // Sanity check. If failed, return a empty image. The entire rest of this function is re-inventing common operations provided by Skia already. Please remove all this code and replace it with something cleaner like the following: // Create a bitmap and a canvas that draws to it. SkBitmap result; result.allocPixels(SkImageInfo::MakeN32Premul(size.width(), size.height())); SkCanvas canvas(result); // Fill with black. canvas.drawARGB(255, 0, 0, 0); // Draw a scaled favicon image such that it occupies the middle three-quarters of the result image. SkRect dest_rect; dest_rect.set(result.width() * 1/8, // left result.height() * 1/8, // top result.width() * 7/8, // right result.height() * 7/8); //bottom canvas.drawImageRect(favicon, nullptr, dest_rect, nullptr); // Draw white border. SkPaint paint; paint.setARGB(255, 255, 255, 255); paint.setStyle(SkPaint::kStroke_Style); paint.setStrokeWidth(kThickness); canvas.drawRectCoords(0 + kThickness / 2, // left 0 + kThickness / 2, // top result.width() - kThickness / 2, // right result.height() - kThickness / 2, // bottom paint); https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/de... File chrome/browser/media/desktop_media_list.h (right): https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/de... chrome/browser/media/desktop_media_list.h:65: static gfx::ImageSkia CreateImageEncloseFavicon(gfx::Size size, naming nit: How about: CreateEnlargedFaviconImage() https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:103: std::vector<SourceDescription> tab_sources); Please use "const std::vector<SourceDescription>&" for the 3rd argument to avoid unnecessary copying. https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:328: std::vector<SourceDescription>& tab_sources) { style: Cannot pass by ref. Use pointers instead for output arguments. https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:367: int window_id = (media_id.tab_id.main_render_frame_id << 12) + This is fragile. Just use std::pair<int, int> instead of risking overflow problems. https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:461: int window_id = (source.id.tab_id.main_render_frame_id << 12) + ditto: Use std::pair<int, int> instead. https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... File chrome/browser/media/native_desktop_media_list.h (right): https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.h:62: void GetTabSourceDescriptions(std::vector<SourceDescription>& tab_sources); style guide: You must pass by pointer, not by reference. https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.h:81: std::map<int, gfx::ImageSkia> tab_icon_map_; Please make the key a std::pair<int, int> instead. See comment below. https://codereview.chromium.org/1503563004/diff/20001/content/public/browser/... File content/public/browser/web_contents_media_capture_id.cc (right): https://codereview.chromium.org/1503563004/diff/20001/content/public/browser/... content/public/browser/web_contents_media_capture_id.cc:43: if(ExtractTabCaptureTarget(str, &render_process_id, &main_render_frame_id)) style: Space after "if" and before open parenthesis. https://codereview.chromium.org/1503563004/diff/20001/content/public/browser/... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/20001/content/public/browser/... content/public/browser/web_contents_media_capture_id.h:9: #include <tuple> nit: Put the <tuple> include in the .cc file since it is not used in this header file. https://codereview.chromium.org/1503563004/diff/20001/content/public/browser/... content/public/browser/web_contents_media_capture_id.h:38: Please add a field for the auto-throttling option and change all the methods/functions to account for it. Probably a bool would suffice.
Miu, Updated based on your comments. Thanks, George https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/de... File chrome/browser/media/desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/de... chrome/browser/media/desktop_media_list.cc:11: const int scale = 3; On 2015/12/12 00:00:33, miu wrote: > style nit: kScale variable is removed https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/de... chrome/browser/media/desktop_media_list.cc:13: const int thick = 5; On 2015/12/12 00:00:33, miu wrote: > style nit: kBoundaryThickness variable is removed https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/de... chrome/browser/media/desktop_media_list.cc:15: // Sanity check. If failed, return a empty image. On 2015/12/12 00:00:33, miu wrote: > The entire rest of this function is re-inventing common operations provided by > Skia already. Please remove all this code and replace it with something cleaner > like the following: > > // Create a bitmap and a canvas that draws to it. > SkBitmap result; > result.allocPixels(SkImageInfo::MakeN32Premul(size.width(), size.height())); > SkCanvas canvas(result); > > // Fill with black. > canvas.drawARGB(255, 0, 0, 0); > > // Draw a scaled favicon image such that it occupies the middle three-quarters > of the result image. > SkRect dest_rect; > dest_rect.set(result.width() * 1/8, // left > result.height() * 1/8, // top > result.width() * 7/8, // right > result.height() * 7/8); //bottom > canvas.drawImageRect(favicon, nullptr, dest_rect, nullptr); > > // Draw white border. > SkPaint paint; > paint.setARGB(255, 255, 255, 255); > paint.setStyle(SkPaint::kStroke_Style); > paint.setStrokeWidth(kThickness); > canvas.drawRectCoords(0 + kThickness / 2, // left > 0 + kThickness / 2, // top > result.width() - kThickness / 2, // right > result.height() - kThickness / 2, // bottom > paint); Done. https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:103: std::vector<SourceDescription> tab_sources); On 2015/12/12 00:00:33, miu wrote: > Please use "const std::vector<SourceDescription>&" for the 3rd argument to avoid > unnecessary copying. Done. https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:328: std::vector<SourceDescription>& tab_sources) { On 2015/12/12 00:00:33, miu wrote: > style: Cannot pass by ref. Use pointers instead for output arguments. Done. https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:367: int window_id = (media_id.tab_id.main_render_frame_id << 12) + On 2015/12/12 00:00:33, miu wrote: > This is fragile. Just use std::pair<int, int> instead of risking overflow > problems. Done. https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.cc:461: int window_id = (source.id.tab_id.main_render_frame_id << 12) + On 2015/12/12 00:00:33, miu wrote: > ditto: Use std::pair<int, int> instead. Done. https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... File chrome/browser/media/native_desktop_media_list.h (right): https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.h:62: void GetTabSourceDescriptions(std::vector<SourceDescription>& tab_sources); On 2015/12/12 00:00:33, miu wrote: > style guide: You must pass by pointer, not by reference. Done. https://codereview.chromium.org/1503563004/diff/20001/chrome/browser/media/na... chrome/browser/media/native_desktop_media_list.h:81: std::map<int, gfx::ImageSkia> tab_icon_map_; On 2015/12/12 00:00:33, miu wrote: > Please make the key a std::pair<int, int> instead. See comment below. Done. https://codereview.chromium.org/1503563004/diff/20001/content/public/browser/... File content/public/browser/web_contents_media_capture_id.cc (right): https://codereview.chromium.org/1503563004/diff/20001/content/public/browser/... content/public/browser/web_contents_media_capture_id.cc:43: if(ExtractTabCaptureTarget(str, &render_process_id, &main_render_frame_id)) On 2015/12/12 00:00:33, miu wrote: > style: Space after "if" and before open parenthesis. Done. https://codereview.chromium.org/1503563004/diff/20001/content/public/browser/... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/20001/content/public/browser/... content/public/browser/web_contents_media_capture_id.h:9: #include <tuple> On 2015/12/12 00:00:33, miu wrote: > nit: Put the <tuple> include in the .cc file since it is not used in this header > file. Done. https://codereview.chromium.org/1503563004/diff/20001/content/public/browser/... content/public/browser/web_contents_media_capture_id.h:38: On 2015/12/12 00:00:33, miu wrote: > Please add a field for the auto-throttling option and change all the > methods/functions to account for it. Probably a bool would suffice. Done.
Just a few things left: https://codereview.chromium.org/1503563004/diff/40001/chrome/browser/media/de... File chrome/browser/media/desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/40001/chrome/browser/media/de... chrome/browser/media/desktop_media_list.cc:41: const SkImage* temp_image = SkImage::NewFromBitmap(favicon.AsBitmap()); Make sure you delete the SkImage to avoid memory leaks. Suggestion: const scoped_ptr<SkImage> temp_image(SkImage::NewFromBitmap(...)); https://codereview.chromium.org/1503563004/diff/40001/content/public/browser/... File content/public/browser/web_contents_media_capture_id.cc (right): https://codereview.chromium.org/1503563004/diff/40001/content/public/browser/... content/public/browser/web_contents_media_capture_id.cc:17: bool WebContentsMediaCaptureId::operator<( Please account for |enable_auto_throttling| here. https://codereview.chromium.org/1503563004/diff/40001/content/public/browser/... content/public/browser/web_contents_media_capture_id.cc:23: bool WebContentsMediaCaptureId::operator==( And need to account for |enable_auto_throttling| here too. https://codereview.chromium.org/1503563004/diff/40001/content/public/browser/... content/public/browser/web_contents_media_capture_id.cc:51: return WebContentsMediaCaptureId(render_process_id, main_render_frame_id); Please account for |enable_auto_throttling| here too. https://codereview.chromium.org/1503563004/diff/40001/content/public/browser/... content/public/browser/web_contents_media_capture_id.cc:71: device_id_param.substr(arraysize(kTabPrefix) - 1); With the change from kDeviceScheme to kTabPrefix, the offset here is now incorrect. I suggest putting this at the top of the method and using it instead of kTabPrefix: const std::string device_scheme = std::string(kTabPrefix) + "://"; https://codereview.chromium.org/1503563004/diff/40001/content/public/browser/... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/40001/content/public/browser/... content/public/browser/web_contents_media_capture_id.h:13: const char kTabPrefix[] = "web-contents-media-stream"; naming nit: kWebContentsCaptureScheme
Miu, Updated based on your comments. Thanks for a few catches. George https://codereview.chromium.org/1503563004/diff/40001/chrome/browser/media/de... File chrome/browser/media/desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/40001/chrome/browser/media/de... chrome/browser/media/desktop_media_list.cc:41: const SkImage* temp_image = SkImage::NewFromBitmap(favicon.AsBitmap()); On 2015/12/14 20:21:08, miu wrote: > Make sure you delete the SkImage to avoid memory leaks. Suggestion: > > const scoped_ptr<SkImage> temp_image(SkImage::NewFromBitmap(...)); good catch. https://codereview.chromium.org/1503563004/diff/40001/content/public/browser/... File content/public/browser/web_contents_media_capture_id.cc (right): https://codereview.chromium.org/1503563004/diff/40001/content/public/browser/... content/public/browser/web_contents_media_capture_id.cc:17: bool WebContentsMediaCaptureId::operator<( On 2015/12/14 20:21:08, miu wrote: > Please account for |enable_auto_throttling| here. Done. https://codereview.chromium.org/1503563004/diff/40001/content/public/browser/... content/public/browser/web_contents_media_capture_id.cc:23: bool WebContentsMediaCaptureId::operator==( On 2015/12/14 20:21:08, miu wrote: > And need to account for |enable_auto_throttling| here too. Done. https://codereview.chromium.org/1503563004/diff/40001/content/public/browser/... content/public/browser/web_contents_media_capture_id.cc:51: return WebContentsMediaCaptureId(render_process_id, main_render_frame_id); On 2015/12/14 20:21:08, miu wrote: > Please account for |enable_auto_throttling| here too. Done. https://codereview.chromium.org/1503563004/diff/40001/content/public/browser/... content/public/browser/web_contents_media_capture_id.cc:71: device_id_param.substr(arraysize(kTabPrefix) - 1); On 2015/12/14 20:21:08, miu wrote: > With the change from kDeviceScheme to kTabPrefix, the offset here is now > incorrect. > > I suggest putting this at the top of the method and using it instead of > kTabPrefix: > > const std::string device_scheme = std::string(kTabPrefix) + "://"; Good catch https://codereview.chromium.org/1503563004/diff/40001/content/public/browser/... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/40001/content/public/browser/... content/public/browser/web_contents_media_capture_id.h:13: const char kTabPrefix[] = "web-contents-media-stream"; On 2015/12/14 20:21:08, miu wrote: > naming nit: kWebContentsCaptureScheme Done.
Miu, Updated based on your comments. Thanks for a few catches. George
gyzhou@chromium.org changed reviewers: + dalecurtis@chromium.org, rockot@chromium.org, sergeyu@chromium.org
gyzhou@chromium.org changed reviewers: + mfoltz@chromium.org, msw@chromium.org
rockot, sergeyu, dalecurtis, msw, and mofoltz, Would you please have a review for this CL. rockot, as owner of chrome/common/extensions/docs/examples/api/desktopCapture/app.js, please have a look of this file. dalecurtis, as owner of content/browser/renderer_host/media/audio_input_renderer_host.cc and content/browser/renderer_host/media/media_stream_manager.cc, please have a look for those files. mfoltz, as owner, please specifically for files under content/public/browser folder. msw, you are the owner of files under chrome/browser/ui. Sergeyu, you pretty much covers most of everything here. Thanks for your help, George
https://codereview.chromium.org/1503563004/diff/60001/chrome/common/extension... File chrome/common/extensions/docs/examples/api/desktopCapture/app.js (right): https://codereview.chromium.org/1503563004/diff/60001/chrome/common/extension... chrome/common/extensions/docs/examples/api/desktopCapture/app.js:7: const DESKTOP_MEDIA = ['screen', 'window', "tab"]; nit: Please keep quote styles consistent
lgtm % one thing I forgot to mention last time around: https://codereview.chromium.org/1503563004/diff/60001/content/public/browser/... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/60001/content/public/browser/... content/public/browser/web_contents_media_capture_id.h:13: const char kWebContentsCaptureScheme[] = "web-contents-media-stream"; This string should be defined in the .cc file. In the .h file (here), you declare it as: extern const char kWebContentsCaptureScheme[];
AIRH and MSM lgtm
c/b/ui lgtm with nits. https://codereview.chromium.org/1503563004/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_finder.cc (right): https://codereview.chromium.org/1503563004/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder.cc:168: if (matches.size()) nit: if (!matches.empty()) https://codereview.chromium.org/1503563004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1503563004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_media_picker_views.cc:525: // Tab capture will be enabled by next CL for it. nit: remove this line; maybe note something similar above, like: "// Temporarily disable tab capture; this will be enabled later."
A patch is uploaded based on review comments of miu, rockot and msw. Thanks, George https://codereview.chromium.org/1503563004/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_finder.cc (right): https://codereview.chromium.org/1503563004/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder.cc:168: if (matches.size()) On 2015/12/15 21:30:09, msw wrote: > nit: if (!matches.empty()) Done. https://codereview.chromium.org/1503563004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1503563004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_media_picker_views.cc:525: // Tab capture will be enabled by next CL for it. On 2015/12/15 21:30:09, msw wrote: > nit: remove this line; maybe note something similar above, like: > "// Temporarily disable tab capture; this will be enabled later." Done. https://codereview.chromium.org/1503563004/diff/60001/chrome/common/extension... File chrome/common/extensions/docs/examples/api/desktopCapture/app.js (right): https://codereview.chromium.org/1503563004/diff/60001/chrome/common/extension... chrome/common/extensions/docs/examples/api/desktopCapture/app.js:7: const DESKTOP_MEDIA = ['screen', 'window', "tab"]; On 2015/12/15 00:06:16, Ken Rockot wrote: > nit: Please keep quote styles consistent Done. https://codereview.chromium.org/1503563004/diff/60001/content/public/browser/... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/60001/content/public/browser/... content/public/browser/web_contents_media_capture_id.h:13: const char kWebContentsCaptureScheme[] = "web-contents-media-stream"; On 2015/12/15 01:38:53, miu wrote: > This string should be defined in the .cc file. In the .h file (here), you > declare it as: > > extern const char kWebContentsCaptureScheme[]; This string is also used by DesktopMediaId to judge the type of media. Therefore its need in .h file so that Desktop_media_id.h can include it.
Sergey and mfoltz, Would you please have a review of this CL. Thanks, George
Just a few comments. https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... File chrome/browser/media/desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... chrome/browser/media/desktop_media_list.cc:43: delete temp_image; Consider using scoped_ptr to avoid an explicit delete. https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... File chrome/browser/media/desktop_media_list_ash.cc (right): https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... chrome/browser/media/desktop_media_list_ash.cc:207: contents->GetTitle(); I'm not sure of google's I18N policy. But in my mind, we should avoid this string concatenation as much as we can. Doing a place holder in the resource string is more preferred, as it gives translator more freedom. Perhaps for this simple prefix is fine. Double check it. Bad Example: I_HAVE --> I have STH --> something GetString(I_HAVE) + GetString(STH) + "." Good Example: I_HAVE --> I have {0}. STH --> something String.format(GetString(I_HAVE), GetString(STH)) https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... File chrome/browser/media/desktop_streams_registry.cc (right): https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... chrome/browser/media/desktop_streams_registry.cc:42: We can remove this line, and then remove this file from CL. https://codereview.chromium.org/1503563004/diff/80001/content/browser/media/c... File content/browser/media/capture/web_contents_video_capture_device_unittest.cc (right): https://codereview.chromium.org/1503563004/diff/80001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:22: #include "content/public/browser/web_contents_media_capture_id.h" I did not see any essential code change in this file. Can you verify whether we can remove this include or not?
Qiang, A patch is uploaded based on your review comments. Thanks, George https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... File chrome/browser/media/desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... chrome/browser/media/desktop_media_list.cc:43: delete temp_image; On 2015/12/16 21:08:36, qiangchenC wrote: > Consider using scoped_ptr to avoid an explicit delete. Done. https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... File chrome/browser/media/desktop_media_list_ash.cc (right): https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... chrome/browser/media/desktop_media_list_ash.cc:207: contents->GetTitle(); On 2015/12/16 21:08:36, qiangchenC wrote: > I'm not sure of google's I18N policy. But in my mind, we should avoid this > string concatenation as much as we can. Doing a place holder in the resource > string is more preferred, as it gives translator more freedom. > Perhaps for this simple prefix is fine. Double check it. > > Bad Example: > I_HAVE --> I have > STH --> something > > GetString(I_HAVE) + GetString(STH) + "." > > > Good Example: > I_HAVE --> I have {0}. > STH --> something > > String.format(GetString(I_HAVE), GetString(STH)) Done. https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... File chrome/browser/media/desktop_streams_registry.cc (right): https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... chrome/browser/media/desktop_streams_registry.cc:42: On 2015/12/16 21:08:36, qiangchenC wrote: > We can remove this line, and then remove this file from CL. Done. https://codereview.chromium.org/1503563004/diff/80001/content/browser/media/c... File content/browser/media/capture/web_contents_video_capture_device_unittest.cc (right): https://codereview.chromium.org/1503563004/diff/80001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:22: #include "content/public/browser/web_contents_media_capture_id.h" On 2015/12/16 21:08:36, qiangchenC wrote: > I did not see any essential code change in this file. Can you verify whether we > can remove this include or not? content/public/browser/web_contents_media_capture_id.h was added to cover the content of content/browser/media/capture/web_contents_capture_util.h. This was suggested by a reviewer.
On 2015/12/16 22:38:40, GeorgeZ wrote: > Qiang, > > A patch is uploaded based on your review comments. > > Thanks, > > George > > https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... > File chrome/browser/media/desktop_media_list.cc (right): > > https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... > chrome/browser/media/desktop_media_list.cc:43: delete temp_image; > On 2015/12/16 21:08:36, qiangchenC wrote: > > Consider using scoped_ptr to avoid an explicit delete. > > Done. > > https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... > File chrome/browser/media/desktop_media_list_ash.cc (right): > > https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... > chrome/browser/media/desktop_media_list_ash.cc:207: contents->GetTitle(); > On 2015/12/16 21:08:36, qiangchenC wrote: > > I'm not sure of google's I18N policy. But in my mind, we should avoid this > > string concatenation as much as we can. Doing a place holder in the resource > > string is more preferred, as it gives translator more freedom. > > Perhaps for this simple prefix is fine. Double check it. > > > > Bad Example: > > I_HAVE --> I have > > STH --> something > > > > GetString(I_HAVE) + GetString(STH) + "." > > > > > > Good Example: > > I_HAVE --> I have {0}. > > STH --> something > > > > String.format(GetString(I_HAVE), GetString(STH)) > > Done. > > https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... > File chrome/browser/media/desktop_streams_registry.cc (right): > > https://codereview.chromium.org/1503563004/diff/80001/chrome/browser/media/de... > chrome/browser/media/desktop_streams_registry.cc:42: > On 2015/12/16 21:08:36, qiangchenC wrote: > > We can remove this line, and then remove this file from CL. > > Done. > > https://codereview.chromium.org/1503563004/diff/80001/content/browser/media/c... > File content/browser/media/capture/web_contents_video_capture_device_unittest.cc > (right): > > https://codereview.chromium.org/1503563004/diff/80001/content/browser/media/c... > content/browser/media/capture/web_contents_video_capture_device_unittest.cc:22: > #include "content/public/browser/web_contents_media_capture_id.h" > On 2015/12/16 21:08:36, qiangchenC wrote: > > I did not see any essential code change in this file. Can you verify whether > we > > can remove this include or not? > > content/public/browser/web_contents_media_capture_id.h was added to cover the > content of content/browser/media/capture/web_contents_capture_util.h. This was > suggested by a reviewer. looks good to me now.
Sergey and mfoltz, Would you please have a review of this CL. Ken, Is JavaScript modification based on your comment OK? Thanks, George
JS change lgtm
It looks like you can split this CL into multiple CLs. At very list changes in DesktopMediaList implementations can be isolated. I looked only in chrome/browser/media/ - see my comments about design there. Also, can you please add unittests for the code you are adding. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list.h (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list.h:65: static gfx::ImageSkia CreateEnlargedFaviconImage(gfx::Size size, I don't think this is the right place for this function. This is an abstract interface. It shouldn't have any methods, even protected static. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list_ash.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list_ash.cc:188: for (auto browser : browsers) { 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. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/n... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:329: // Only support mac, linux, windows and chrome OS, the last three use aura. Are there any other platforms where this code is compiled? I don't think you need this ifdef. Even if this file was compiled on other platforms (e.g. android) it would would with this change because you have unreachable code after return.
Sergey, This CL used to contain half of current files. A reviewer's comment of moving a class and associate files leaded to add about a dozen more files into this CL. Associated unittests for DesktopMediaList Classes are pretty much ready but are not included in CL, since it is already too big. A separated CL will be created for Unit tests. Regarding your review comments, I would like you to clarify a little bit more before I can go forward. Thanks for your help. George https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list.h (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list.h:65: static gfx::ImageSkia CreateEnlargedFaviconImage(gfx::Size size, On 2015/12/18 19:45:04, Sergey Ulanov wrote: > I don't think this is the right place for this function. > This is an abstract interface. It shouldn't have any methods, even protected > static. I will move this function away from here. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list_ash.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list_ash.cc:188: for (auto browser : browsers) { On 2015/12/18 19:45:04, Sergey Ulanov wrote: > 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. 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. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/n... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:329: // Only support mac, linux, windows and chrome OS, the last three use aura. On 2015/12/18 19:45:05, Sergey Ulanov wrote: > Are there any other platforms where this code is compiled? I don't think you > need this ifdef. Even if this file was compiled on other platforms (e.g. > android) it would would with this change because you have unreachable code after > return. This file is compiled by Android platform. Without ifdef, the android try bolt will fail because some associated libs are not available. Do you have any better solution for this case?
Sergey, I added one more comment about the code duplication. Let me know your opinion. Thanks, George https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list_ash.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list_ash.cc:188: for (auto browser : browsers) { On 2015/12/18 21:57:26, GeorgeZ wrote: > On 2015/12/18 19:45:04, Sergey Ulanov wrote: > > 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. > > 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. 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 and mfoltz, Would you please give some feedback for this CL. Thanks, George
My objections have to do mainly with NativeDesktopMediaList and DesktopMediaListAsh. So to make further progress on this CL maybe keep these classes as is in this CL and update them in a separate CL? https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list_ash.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list_ash.cc:188: for (auto browser : browsers) { On 2015/12/21 16:17:37, GeorgeZ wrote: > On 2015/12/18 21:57:26, GeorgeZ wrote: > > On 2015/12/18 19:45:04, Sergey Ulanov wrote: > > > 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. > > > > 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. > > 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. 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. > > 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. 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. > > Let me know what's your opinion. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list_ash.cc:188: for (auto browser : browsers) { On 2015/12/18 21:57:26, GeorgeZ wrote: > On 2015/12/18 19:45:04, Sergey Ulanov wrote: > > 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. > > 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. 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. > > 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. I don't think that would make any difference. The Observer should not depend on these details. > > 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. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/n... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:329: // Only support mac, linux, windows and chrome OS, the last three use aura. On 2015/12/18 21:57:26, GeorgeZ wrote: > On 2015/12/18 19:45:05, Sergey Ulanov wrote: > > Are there any other platforms where this code is compiled? I don't think you > > need this ifdef. Even if this file was compiled on other platforms (e.g. > > android) it would would with this change because you have unreachable code > after > > return. > > This file is compiled by Android platform. Without ifdef, the android try bolt > will fail because some associated libs are not available. Do you have any better > solution for this case? Exclude this file on android? There is no reason to compile it there. It's likely it will still fail to compile even with this ifdef. https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... File content/public/browser/desktop_media_id.h (right): https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/desktop_media_id.h:50: DesktopMediaID(Type type, I don't think this constructor is necessary given that you have the one above that takes WebContentsMediaCaptureId https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:45: bool enable_auto_throttling = false; Why is this part of the window identifier?
So here is proof-of-concept implementation of CombinedDesktopMediaList: https://codereview.chromium.org/1554243002/ As you can see the code is super simple. The .cc file is fewer than 100 lines. To reiterate, here the benefits of the design that I proposed: - DesktopMediaListAsh and NativeDesktopMediaList stay the way they are. I.e. they will be simpler and there is no change of breaking any of the existing code. - No code duplication. - The new tab-capture code you are adding will be simpler and easier to test. - There are other cases when CombinedDesktopMediaList will be useful, specifically crbug.com/552123. Another benefit is that it will be easier to reuse the code you are adding on other platforms. E.g. currently neither DesktopMediaListAsh nor NativeDesktopMediaList work on Android. But if you add a separate TabDesktopMediaList it will be reusable on Android.
This feature seems to largely duplicate what is possible through chrome.tabCapture with the additional feature of specifying a specific WebContents to capture via (RFID,RFHID). Did you consider a design which allows a tab_id to be passed from the picker to chrome.tabCapture? This means you don't need magic URLs with magic schemes that fall outside the standard usage of gUM. What is the security story? Is this capability whitelisted to extensions with a certain permission? Certainly we don't want to expose this to the open Web. I think the UX needs some work before it's ready to launch. In particular many sites don't have favicons, or use low-resolution 16x16 or 32x32 sites, which won't look great when scaled up in the picker window. I recommend experimentation on some displays (especially HiDPI ones) in real world browsing scenarios, especially with large numbers of tabs. The picker needs to handle the following cases: - no favicon - empty hostname or nonsense hostname - no page title I didn't see any logic to exclude certain tabs from this feature, like chrome://, which usually have some protections against extension APIs. We should probably be consistent with what chrome.tabCapture does - miu@ are there any restrictions on what can be captured? If this is planned as a dev-channel only feature while it's worked on then this is okay to land, but I'd like to see plans to address these issues. I have some concerns about code duplication in *_desktop_media_list.cc and browser_finder.cc. There's little to no test coverage added in this patch. What is your test plan? https://codereview.chromium.org/1503563004/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:13775: + chrome tab_<ph name="CHROME_TAB_TITLE">$1<ex>YouTube</ex></ph> This would show "chrome tab_YouTube"? That isn't a sensible string to show users. Why not just something like "YouTube (tab - youtube.com)", or something along those lines, including fallback cases for when the tab does not have a title or host. Has the picker (including strings) been through UX review? https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. s/2015/2016/ https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list.cc:14: result.allocN32Pixels(size.width(), size.height(), true); What restrictions are there on |size|? https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list.cc:33: // 3/4 of result image. Will there be a black border, inset by a white border, with the favicon drawn on top? What is the background this will be drawn on? https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list.h (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list.h:64: // The function is used to create a thumbnail for chrome tab preview. There is already an API to generate tab thumbnails, see https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... Is there a reason to add a second one here, or at a minimum can the implementation be shared? https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list_ash.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list_ash.cc:217: gfx::Image tab_icon = favicon_driver->GetFavicon(); 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. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list_ash.cc:218: tab_map[t].thumbnail = 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? https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/n... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:343: for (auto browser : browsers) { Is the Browser* guaranteed to stay alive while this loop executes? https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_finder.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder.cc:150: std::vector<Browser*> FindAllBrowsersWithTabbedOrAnyType( It seems like you are adding duplicate implementations of what's already here that returns a vector<> instead of the first match. If this API were written entirely in terms of predicates and iterators, callers could use standard library algorithms instead (i.e. std::find_if, std::remove_if). I don't know if this can be addressed without blowing up this patchset. Maybe if you can find the original authors, might be worth starting a thread on why this API is the way it is. https://codereview.chromium.org/1503563004/diff/100001/chrome/common/extensio... File chrome/common/extensions/docs/examples/api/desktopCapture/app.js (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/common/extensio... chrome/common/extensions/docs/examples/api/desktopCapture/app.js:7: const DESKTOP_MEDIA = ['screen', 'window', 'tab']; Maybe don't expose this until you've landed all necessary patches? https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... File content/public/browser/web_contents_media_capture_id.cc (right): https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. s/2012/2016/ https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.cc:34: return (render_process_id < 0) || (main_render_frame_id < 0); What about kInvalidId? https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.cc:44: if (enable_auto_throttling) This doesn't have anything to do with the target of capture, it's a parameter that probably belongs in the video constraints object. https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.cc:67: bool WebContentsMediaCaptureId::ExtractTabCaptureTarget( How is a client supposed to come up with the (render_frame_id,render_host_id) to construct this magic URL? Why can't you take a tab_id like the other extensions APIs in chrome.tabs? https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. s/2015/2016/ https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:15: struct CONTENT_EXPORT WebContentsMediaCaptureId { Where is the syntax of this ID defined? Can you include documentation or a link? https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:51: static bool IsWebContentsDeviceId(const std::string& device_id); Why static? https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:55: static bool ExtractTabCaptureTarget(const std::string& device_id, Ditto https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:61: static bool IsAutoThrottlingOptionSet(const std::string& device_id); Ditto
https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:15: struct CONTENT_EXPORT WebContentsMediaCaptureId { On 2016/01/04 20:11:41, mfoltz_OOO_until_1.4 wrote: > Where is the syntax of this ID defined? Can you include documentation or a > link? mfoltz: There is none. Note that gyzhou@ has simply moved content/browser/media/capture/web_contents_util.* into this file/class at my earlier request because he needs to use it for this new feature as well. Before this change, there were only two code points using this (chrome/browser/extensions/api/tab_capture/tab_capture_api.cc and content/browser/media/capture/web_contents_util.cc). Eventually, we should do some housekeeping in the tab capture API code to eliminate the need for this ID (https://code.google.com/p/chromium/issues/detail?id=163100#c19, see comment 19).
Sergey and Mark, I updated the code based on your comments. I would like use one or two separated CLs for refactor of DesktopMediaList*.* and unit tests, Since this CL is quite big now. Thanks, George https://codereview.chromium.org/1503563004/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:13775: + chrome tab_<ph name="CHROME_TAB_TITLE">$1<ex>YouTube</ex></ph> On 2016/01/04 20:11:40, mfoltz_OOO_until_1.4 wrote: > This would show "chrome tab_YouTube"? That isn't a sensible string to show > users. Why not just something like "YouTube (tab - youtube.com)", or something > along those lines, including fallback cases for when the tab does not have a > title or host. > > Has the picker (including strings) been through UX review? We had consulted with UX persons before this CL. In design document https://goo.gl/uoHnv0, we are going to make the feature work first and then revise the UI completely. For fallback case, I am going to display text "no title available" for now. I will collect suggestion from UX persons. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/04 20:11:40, mfoltz_OOO_until_1.4 wrote: > s/2015/2016/ Done. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list.cc:14: result.allocN32Pixels(size.width(), size.height(), true); On 2016/01/04 20:11:40, mfoltz_OOO_until_1.4 wrote: > What restrictions are there on |size|? Added a restriction for minimal size. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list.cc:33: // 3/4 of result image. On 2016/01/04 20:11:40, mfoltz_OOO_until_1.4 wrote: > Will there be a black border, inset by a white border, with the favicon drawn on > top? > What is the background this will be drawn on? There are examples in design doc https://goo.gl/uoHnv0. Again, as planned, functionalities will be addressed first. UI will be revised later after feedback is collected. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list.h (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list.h:64: // The function is used to create a thumbnail for chrome tab preview. On 2016/01/04 20:11:40, mfoltz_OOO_until_1.4 wrote: > There is already an API to generate tab thumbnails, see > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... > > Is there a reason to add a second one here, or at a minimum can the > implementation be shared? The code you mentioned is to capture visible area of an active tab/Webcontents. In our case, most tabs are inactive and invisible. Inactive tabs may not be loaded and loading them will be cost. Therefore, we use a favicon as the thumbnail instead of actually loading and capturing the previous of a tab. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list.h:65: static gfx::ImageSkia CreateEnlargedFaviconImage(gfx::Size size, On 2015/12/18 21:57:26, GeorgeZ wrote: > On 2015/12/18 19:45:04, Sergey Ulanov wrote: > > I don't think this is the right place for this function. > > This is an abstract interface. It shouldn't have any methods, even protected > > static. > > I will move this function away from here. Done. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list_ash.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list_ash.cc:188: for (auto browser : browsers) { On 2016/01/04 18:59:29, Sergey Ulanov wrote: > On 2015/12/18 21:57:26, GeorgeZ wrote: > > On 2015/12/18 19:45:04, Sergey Ulanov wrote: > > > 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. > > > > 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. > > 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. > > > > > > 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. > > I don't think that would make any difference. The Observer should not depend on > these details. > > > > > > 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. > How about I address this issue in another CL right after this one. This CL was expanded and quit big now. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list_ash.cc:217: gfx::Image tab_icon = favicon_driver->GetFavicon(); On 2016/01/04 20:11:41, mfoltz_OOO_until_1.4 wrote: > 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. > 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. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/d... chrome/browser/media/desktop_media_list_ash.cc:218: tab_map[t].thumbnail = On 2016/01/04 20:11:41, mfoltz_OOO_until_1.4 wrote: > 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? 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. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/n... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:329: // Only support mac, linux, windows and chrome OS, the last three use aura. On 2016/01/04 18:59:29, Sergey Ulanov wrote: > On 2015/12/18 21:57:26, GeorgeZ wrote: > > On 2015/12/18 19:45:05, Sergey Ulanov wrote: > > > Are there any other platforms where this code is compiled? I don't think you > > > need this ifdef. Even if this file was compiled on other platforms (e.g. > > > android) it would would with this change because you have unreachable code > > after > > > return. > > > > This file is compiled by Android platform. Without ifdef, the android try bolt > > will fail because some associated libs are not available. Do you have any > better > > solution for this case? > > Exclude this file on android? There is no reason to compile it there. > It's likely it will still fail to compile even with this ifdef. Android builds in git cl try passed with "#if !define ...". I just modified gypi. to avoid those file to be built for android instead of using "#if !defined ..." https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:343: for (auto browser : browsers) { On 2016/01/04 20:11:41, mark a. foltz wrote: > Is the Browser* guaranteed to stay alive while this loop executes? Thanks to point out this issue. I modified the code that I immediately enumerate and register all webcontents* after I get Browser*, so that Browser* only need be alive for a very short period of time. I added a WebContentsRegistry class that will register webcontents * into a integer id. The webcontents* can be gotten from registered id if webcontents hasn't been destroyed. Otherwise a nullptr will be return. This will largely avoid visiting a destroyed webcontents*. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_finder.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder.cc:150: std::vector<Browser*> FindAllBrowsersWithTabbedOrAnyType( On 2016/01/04 20:11:41, mark a. foltz wrote: > It seems like you are adding duplicate implementations of what's already here > that returns a vector<> instead of the first match. If this API were written > entirely in terms of predicates and iterators, callers could use standard > library algorithms instead (i.e. std::find_if, std::remove_if). > > I don't know if this can be addressed without blowing up this patchset. Maybe > if you can find the original authors, might be worth starting a thread on why > this API is the way it is. browser matching using function. bool BrowserMatches(Browser* browser, Profile* profile, Browser::WindowFeature window_feature, uint32_t match_types) This function is not very straightforward to convert into a C++ predicate. In order to merge FindBrowserMatching() and FindAllBrowserMatching() so that FindBrowserMatching() can be called iteratively to find all matched browsers. One more argument for search start position need to be added. merging FindBrowserWithTabbedOrAnyType() and FindALLBrowserWithTabbedOrAnyType() will need add two more arguments. One for search start point and the other to identify whether search all browsers or just active browsers. I feel merge may not make code simpler and cleaner. I also worried about introducing bugs in important functions here if I touch them. https://codereview.chromium.org/1503563004/diff/100001/chrome/common/extensio... File chrome/common/extensions/docs/examples/api/desktopCapture/app.js (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/common/extensio... chrome/common/extensions/docs/examples/api/desktopCapture/app.js:7: const DESKTOP_MEDIA = ['screen', 'window', 'tab']; On 2016/01/04 20:11:41, mfoltz_OOO_until_1.4 wrote: > Maybe don't expose this until you've landed all necessary patches? Done. https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... File content/public/browser/desktop_media_id.h (right): https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/desktop_media_id.h:50: DesktopMediaID(Type type, On 2016/01/04 18:59:29, Sergey Ulanov wrote: > I don't think this constructor is necessary given that you have the one above > that takes WebContentsMediaCaptureId Done. https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2016/01/04 20:11:41, mfoltz_OOO_until_1.4 wrote: > s/2015/2016/ Done. https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:45: bool enable_auto_throttling = false; On 2016/01/04 18:59:29, Sergey Ulanov wrote: > Why is this part of the window identifier? This file was moved from content/browser/media/capture/web_contents_capture_util.* as a comment of a reviewer and author of the file. He asked me to keep this parameter otherwise some of his code will break. https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:51: static bool IsWebContentsDeviceId(const std::string& device_id); On 2016/01/04 20:11:41, mfoltz_OOO_until_1.4 wrote: > Why static? This file was moved from content/browser/media/capture/web_contents_capture_util.* as a comment of a reviewer and author of the file. This function is used by his code as a static function. https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:55: static bool ExtractTabCaptureTarget(const std::string& device_id, On 2016/01/04 20:11:41, mfoltz_OOO_until_1.4 wrote: > Ditto same as previous. https://codereview.chromium.org/1503563004/diff/100001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:61: static bool IsAutoThrottlingOptionSet(const std::string& device_id); On 2016/01/04 20:11:41, mfoltz_OOO_until_1.4 wrote: > Ditto Same as previous.
gyzhou@chromium.org changed reviewers: + avi@chromium.org
Avi, As an owner, would you please have a review of content/browser/web_contents/web_contents_impl.cc content/public/browser/web_contents_observer.h content/public/browser/desktop_media_id.h content/public/browser/desktop_media_id.cc content/public/browser/web_contents_media_capture_id.h content/public/browser/web_contents_media_capture_id.cc The last 4 files have been reviewed by at least one author of the files. Therefore you may not have to pay too much attention. Thanks, George
Two massive objections here. 1. Your UI strings are abysmal. I would withhold LG on that basis alone. You need to commit *real* UI, and that is terrible. My proposal is to be consistent with the existing Task Manager naming, but if you insist that you can't, you need to talk to the UI people. 2. Do not change the shutdown code of WebContents. Your proposal makes no sense. Why do you add a version of WebContentsDestroyed that takes a WebContents parameter? Just override the one that exists today, and call web_contents() to get the value you want. https://codereview.chromium.org/1503563004/diff/140001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1503563004/diff/140001/chrome/app/generated_r... chrome/app/generated_resources.grd:13741: + chrome tab_no title available Gah. This is *awful*. As I noted in the design document, we already have a convention in the Task Manager of "Tab: <<page name>>". We should re-use that for consistency. https://codereview.chromium.org/1503563004/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1503563004/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:504: FOR_EACH_OBSERVER(WebContentsObserver, observers_, ResetWebContents()); No. https://codereview.chromium.org/1503563004/diff/140001/content/public/browser... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/1503563004/diff/140001/content/public/browser... content/public/browser/web_contents_observer.h:392: virtual void WebContentsDestroyed(WebContents* content) {} What? No.
Avi, For your point 1, I modified the string for tab title display to follow your suggestion which is a much better way. After we consulted with UI person weeks ago, we decided to work on functionalities first and then revise UI later. This is mentioned at the end of design description. For your point 2, I really appreciate your pointed out the issue. I thought WebContentsObserver can observe multiple objects just like aura::WindowObserver. Apparently, I was wrong. I stepped back and let content/browser/web_contents/web_contents_impl.cc content/public/browser/web_contents_observer.h untouched. Thanks, George https://codereview.chromium.org/1503563004/diff/140001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1503563004/diff/140001/chrome/app/generated_r... chrome/app/generated_resources.grd:13741: + chrome tab_no title available On 2016/01/07 01:22:33, Avi wrote: > Gah. This is *awful*. > > As I noted in the design document, we already have a convention in the Task > Manager of "Tab: <<page name>>". We should re-use that for consistency. Done. https://codereview.chromium.org/1503563004/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1503563004/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:504: FOR_EACH_OBSERVER(WebContentsObserver, observers_, ResetWebContents()); On 2016/01/07 01:22:33, Avi wrote: > No. Undo and removed. https://codereview.chromium.org/1503563004/diff/140001/content/public/browser... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/1503563004/diff/140001/content/public/browser... content/public/browser/web_contents_observer.h:392: virtual void WebContentsDestroyed(WebContents* content) {} On 2016/01/07 01:22:34, Avi wrote: > What? No. Undo and removed it.
https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list.h (right): https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/d... chrome/browser/media/desktop_media_list.h:67: class MediaListWebContentsObserver : public content::WebContentsObserver { This is not a useful class; it exists but overrides nothing. You use the automatic nulling out of the pointer, but I've never seen that done before. Its use doesn't make sense either; I'm commenting there. You should remove it. https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list_ash.cc (right): https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/d... chrome/browser/media/desktop_media_list_ash.cc:186: // Get all tabs/webcontents' ids for a user profile Full sentences; end it with a period. https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/d... chrome/browser/media/desktop_media_list_ash.cc:207: // If the WebContents has been destroyed, a nullptr will return; How could the WebContents have been destroyed from the time you grabbed it ten lines up, and now? We're on the UI thread, and WebContents creation and destruction happens on that thread. It's completely OK to walk the TabStripModel, get all the WebContents, and do stuff with them. There's no danger of them disappearing on you, so don't worry :) Remove the MediaListWebContentsObserver class and all of this paranoid code. It's not needed. https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/n... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:185: // Add tab sources to all media sources Full sentences; end with a . https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:323: // Get tab source list . https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:363: content::WebContents* contents = observers[i]->web_contents(); Same objection here. This code is useless for the same reason. WebContents aren't going to disappear out from under you. https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/ui/brow... File chrome/browser/ui/browser_finder.h (right): https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder.h:53: HostDesktopType type); I feel skeptical when a feature needs new functionality in the core files of Browser. Why does this need to be here rather than at the point of use, using BrowserList and BrowserIterator? https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... File content/public/browser/desktop_media_id.h (right): https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... content/public/browser/desktop_media_id.h:26: enum Type { TYPE_NONE, TYPE_SCREEN, TYPE_WINDOW, TYPE_TAB }; Whoa, whoa. Now we're getting into sketchy territory. Content knows about screens and windows. Once you get into having tabs, that's a ui decision that belongs to the embedder, and I'm not comfortable having that knowledge leak into Content. I'm not seeing the full picture here, but I'm a bit weirded out. https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... File content/public/browser/web_contents_media_capture_id.cc (right): https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... content/public/browser/web_contents_media_capture_id.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. No (c), and it's 2016. https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. No (c) https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:19: static const int kInvalidId = -1; But those already have invalid constants: MSG_ROUTING_NONE
Avi, Thanks for your pointing out some key issues. I uploaded patches to address your comments. Thanks, George https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/n... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:343: for (auto browser : browsers) { On 2016/01/06 22:41:39, GeorgeZ wrote: > On 2016/01/04 20:11:41, mark a. foltz wrote: > > Is the Browser* guaranteed to stay alive while this loop executes? > > Thanks to point out this issue. I modified the code that I immediately enumerate > and register all webcontents* after I get Browser*, so that Browser* only need > be alive for a very short period of time. I added a WebContentsRegistry class > that will register webcontents * into a integer id. The webcontents* can be > gotten from registered id if webcontents hasn't been destroyed. Otherwise a > nullptr will be return. This will largely avoid visiting a destroyed > webcontents*. According to Avi@, the browser* and Webcontents* will be alive in the lifetime of this function, since this function, creation and destroy of browser* and Webcontents* are in the same thread. https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_finder.cc (right): https://codereview.chromium.org/1503563004/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder.cc:150: std::vector<Browser*> FindAllBrowsersWithTabbedOrAnyType( On 2016/01/06 22:41:39, GeorgeZ wrote: > On 2016/01/04 20:11:41, mark a. foltz wrote: > > It seems like you are adding duplicate implementations of what's already here > > that returns a vector<> instead of the first match. If this API were written > > entirely in terms of predicates and iterators, callers could use standard > > library algorithms instead (i.e. std::find_if, std::remove_if). > > > > I don't know if this can be addressed without blowing up this patchset. Maybe > > if you can find the original authors, might be worth starting a thread on why > > this API is the way it is. > > browser matching using function. > bool BrowserMatches(Browser* browser, > Profile* profile, > Browser::WindowFeature window_feature, > uint32_t match_types) > This function is not very straightforward to convert into a C++ predicate. > > In order to merge FindBrowserMatching() and FindAllBrowserMatching() so that > FindBrowserMatching() can be called iteratively to find all matched browsers. > One more argument for search start position need to be added. > > merging FindBrowserWithTabbedOrAnyType() and FindALLBrowserWithTabbedOrAnyType() > will need add two more arguments. One for search start point and the other to > identify whether search all browsers or just active browsers. > > I feel merge may not make code simpler and cleaner. I also worried about > introducing bugs in important functions here if I touch them. According Avi@, I removed all change in browser_finder.*. Instead, I used browser_list and browser_iterator. https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list.h (right): https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/d... chrome/browser/media/desktop_media_list.h:67: class MediaListWebContentsObserver : public content::WebContentsObserver { On 2016/01/08 17:11:38, Avi wrote: > This is not a useful class; it exists but overrides nothing. You use the > automatic nulling out of the pointer, but I've never seen that done before. Its > use doesn't make sense either; I'm commenting there. > > You should remove it. Done. https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/d... File chrome/browser/media/desktop_media_list_ash.cc (right): https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/d... chrome/browser/media/desktop_media_list_ash.cc:186: // Get all tabs/webcontents' ids for a user profile On 2016/01/08 17:11:38, Avi wrote: > Full sentences; end it with a period. Done. https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/d... chrome/browser/media/desktop_media_list_ash.cc:207: // If the WebContents has been destroyed, a nullptr will return; On 2016/01/08 17:11:38, Avi wrote: > How could the WebContents have been destroyed from the time you grabbed it ten > lines up, and now? We're on the UI thread, and WebContents creation and > destruction happens on that thread. > > It's completely OK to walk the TabStripModel, get all the WebContents, and do > stuff with them. There's no danger of them disappearing on you, so don't worry > :) > > Remove the MediaListWebContentsObserver class and all of this paranoid code. > It's not needed. Thanks for the explanation here. Done. https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/n... File chrome/browser/media/native_desktop_media_list.cc (right): https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:185: // Add tab sources to all media sources On 2016/01/08 17:11:38, Avi wrote: > Full sentences; end with a . Done. https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:323: // Get tab source list On 2016/01/08 17:11:38, Avi wrote: > . Done. https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/media/n... chrome/browser/media/native_desktop_media_list.cc:363: content::WebContents* contents = observers[i]->web_contents(); On 2016/01/08 17:11:38, Avi wrote: > Same objection here. This code is useless for the same reason. WebContents > aren't going to disappear out from under you. Done. https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/ui/brow... File chrome/browser/ui/browser_finder.h (right): https://codereview.chromium.org/1503563004/diff/180001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder.h:53: HostDesktopType type); On 2016/01/08 17:11:39, Avi wrote: > I feel skeptical when a feature needs new functionality in the core files of > Browser. Why does this need to be here rather than at the point of use, using > BrowserList and BrowserIterator? This is a very good point. I had no knowledge of BrowserList and BrowserIterator. I have removed all the changes in browser_finder.* and followed your instruction. Thanks. https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... File content/public/browser/desktop_media_id.h (right): https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... content/public/browser/desktop_media_id.h:26: enum Type { TYPE_NONE, TYPE_SCREEN, TYPE_WINDOW, TYPE_TAB }; On 2016/01/08 17:11:39, Avi wrote: > Whoa, whoa. Now we're getting into sketchy territory. > > Content knows about screens and windows. Once you get into having tabs, that's a > ui decision that belongs to the embedder, and I'm not comfortable having that > knowledge leak into Content. > > I'm not seeing the full picture here, but I'm a bit weirded out This class actually is a part of the implementation to support JaveScript chrome.desktopCapture https://developer.chrome.com/extensions/desktopCapture. in the document, DesktopCaptureSourceType enums "screen", "window", or "tab". Sharing a tab should hide private information such as bookmarks if a browser window is shared instead. If this didn't address your question, please clarify it. https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... File content/public/browser/web_contents_media_capture_id.cc (right): https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... content/public/browser/web_contents_media_capture_id.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/01/08 17:11:39, Avi wrote: > No (c), and it's 2016. Done. https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/01/08 17:11:39, Avi wrote: > No (c) Done. https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:19: static const int kInvalidId = -1; On 2016/01/08 17:11:39, Avi wrote: > But those already have invalid constants: MSG_ROUTING_NONE Done.
https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... File content/public/browser/desktop_media_id.h (right): https://codereview.chromium.org/1503563004/diff/180001/content/public/browser... content/public/browser/desktop_media_id.h:26: enum Type { TYPE_NONE, TYPE_SCREEN, TYPE_WINDOW, TYPE_TAB }; On 2016/01/08 19:39:02, GeorgeZ wrote: > On 2016/01/08 17:11:39, Avi wrote: > > Whoa, whoa. Now we're getting into sketchy territory. > > > > Content knows about screens and windows. Once you get into having tabs, that's > a > > ui decision that belongs to the embedder, and I'm not comfortable having that > > knowledge leak into Content. > > > > I'm not seeing the full picture here, but I'm a bit weirded out > > This class actually is a part of the implementation to support JaveScript > chrome.desktopCapture https://developer.chrome.com/extensions/desktopCapture. in > the document, DesktopCaptureSourceType enums "screen", "window", or "tab". > Sharing a tab should hide private information such as bookmarks if a browser > window is shared instead. > > If this didn't address your question, please clarify it. Avi makes a good point here, and I should have caught this myself in a prior round of this code review (sorry). What he means is that libcontent has no knowledge of tabs. Tabs is a UI concept in the Chrome browser, but other embedders of libcontent (such as content_shell) don't use the "tabs" concept. Therefore, any libcontent code should instead be referring to a WebContents (a WebContents instance is 1:1 with a tab in the Chrome browser). So, maybe TYPE_WEB_CONTENT or TYPE_PAGE would be more appropriate here? Also, consider updating your design so that the "tab" DesktopCaptureSourceType enum is something like "content" or "page" instead.
Sergey, Would you please provide some feedback of new patches. Thanks, George
No major issues, just minor ones now. Thanks. https://codereview.chromium.org/1503563004/diff/240001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1503563004/diff/240001/chrome/app/generated_r... chrome/app/generated_resources.grd:6111: + Enable the experimental implementation of the "Cache-Control: stale-while-revalidate" directive. This permits servers to specify that some resources may be revalidated in the background to improve latency. Your editor stripped the space from the end of this line. While it shouldn't have been there in the first place, if you touch this line you'll force a translation. Drop this modification chunk. https://codereview.chromium.org/1503563004/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1503563004/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_media_picker_views.cc:526: // Temporarily disable tab capture. This will be enabled later. "enabled later" meaning: 1) It's in the code, and will happen later in the flow, or 2) It will be added in a later CL? If 1, OK, but if 2 then turn this into a TODO. https://codereview.chromium.org/1503563004/diff/240001/content/public/browser... File content/public/browser/desktop_media_id.cc (right): https://codereview.chromium.org/1503563004/diff/240001/content/public/browser... content/public/browser/desktop_media_id.cc:136: type = TYPE_TAB; Yeah, the TYPE_WEB_CONTENTS fits better here. https://codereview.chromium.org/1503563004/diff/240001/content/public/browser... File content/public/browser/desktop_media_id.h (right): https://codereview.chromium.org/1503563004/diff/240001/content/public/browser... content/public/browser/desktop_media_id.h:26: enum Type { TYPE_NONE, TYPE_SCREEN, TYPE_WINDOW, TYPE_TAB }; TYPE_WEB_CONTENTS, not TYPE_TAB. https://codereview.chromium.org/1503563004/diff/240001/content/public/browser... content/public/browser/desktop_media_id.h:70: // This id contains information for tab capture. WebContents capture, etc.
Avi, I uploaded a patch based on your comments. Thanks, George https://codereview.chromium.org/1503563004/diff/240001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1503563004/diff/240001/chrome/app/generated_r... chrome/app/generated_resources.grd:6111: + Enable the experimental implementation of the "Cache-Control: stale-while-revalidate" directive. This permits servers to specify that some resources may be revalidated in the background to improve latency. On 2016/01/11 16:27:57, Avi wrote: > Your editor stripped the space from the end of this line. While it shouldn't > have been there in the first place, if you touch this line you'll force a > translation. Drop this modification chunk. Done. https://codereview.chromium.org/1503563004/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1503563004/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_media_picker_views.cc:526: // Temporarily disable tab capture. This will be enabled later. On 2016/01/11 16:27:57, Avi wrote: > "enabled later" meaning: > 1) It's in the code, and will happen later in the flow, or > 2) It will be added in a later CL? > > If 1, OK, but if 2 then turn this into a TODO. It is 2. Therefore I use TODO. https://codereview.chromium.org/1503563004/diff/240001/content/public/browser... File content/public/browser/desktop_media_id.cc (right): https://codereview.chromium.org/1503563004/diff/240001/content/public/browser... content/public/browser/desktop_media_id.cc:136: type = TYPE_TAB; On 2016/01/11 16:27:57, Avi wrote: > Yeah, the TYPE_WEB_CONTENTS fits better here. Done. https://codereview.chromium.org/1503563004/diff/240001/content/public/browser... File content/public/browser/desktop_media_id.h (right): https://codereview.chromium.org/1503563004/diff/240001/content/public/browser... content/public/browser/desktop_media_id.h:26: enum Type { TYPE_NONE, TYPE_SCREEN, TYPE_WINDOW, TYPE_TAB }; On 2016/01/11 16:27:57, Avi wrote: > TYPE_WEB_CONTENTS, not TYPE_TAB. Done. https://codereview.chromium.org/1503563004/diff/240001/content/public/browser... content/public/browser/desktop_media_id.h:70: // This id contains information for tab capture. On 2016/01/11 16:27:57, Avi wrote: > WebContents capture, etc. Done.
LGTM Thanks!
Sergey, I would need your review and approve to land this CL. Thanks, George
On 2016/01/12 00:15:13, GeorgeZ wrote: > Sergey, > > I would need your review and approve to land this CL. > > Thanks, > > George I don't like the idea of landing the changes in desktop_media_list_ash.cc and native_desktop_media_list.cc and leaving refactoring for separate changes. There is duplicated code and no unittests. As I suggested before, please revert the changes desktop_media_list_ash.cc and native_desktop_media_list.cc . Then implement the approach I suggested in a separate CL.
lgtm % nit: https://codereview.chromium.org/1503563004/diff/260001/content/public/browser... File content/public/browser/desktop_media_id.cc (right): https://codereview.chromium.org/1503563004/diff/260001/content/public/browser... content/public/browser/desktop_media_id.cc:124: // for no WebContentsand aura: screen:"window_id" or window:"window_id" s/WebContentsand/WebContents and/
Sergey, I Reverted all changes associated DesktopMediaList classes as you suggested. Thanks, George https://codereview.chromium.org/1503563004/diff/260001/content/public/browser... File content/public/browser/desktop_media_id.cc (right): https://codereview.chromium.org/1503563004/diff/260001/content/public/browser... content/public/browser/desktop_media_id.cc:124: // for no WebContentsand aura: screen:"window_id" or window:"window_id" On 2016/01/12 22:05:03, miu wrote: > s/WebContentsand/WebContents and/ Done.
https://codereview.chromium.org/1503563004/diff/300001/chrome/browser/extensi... File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc (right): https://codereview.chromium.org/1503563004/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:95: show_tabs = true; tab capture is still not implemented yet. leave this change for later, when the feature is fully implemented. https://codereview.chromium.org/1503563004/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:114: const int source_types = (show_screens ? DesktopMediaListAsh::SCREENS : 0) | Move this inside the if statement below, but I'm not sure if you need this at all. https://codereview.chromium.org/1503563004/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1503563004/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_media_picker_views.cc:529: if (parent_ && source.type != DesktopMediaID::TYPE_WEB_CONTENTS) This would be confusing behavior. It would be better to keep the feature not supported, until it's fully implemented. I.e. revert the change in desktop_capture_base.cc. https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... File content/public/browser/desktop_media_id.cc (right): https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... content/public/browser/desktop_media_id.cc:86: type == TYPE_WEB_CONTENTS); 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. https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... content/public/browser/desktop_media_id.cc:148: return DesktopMediaID(type, 0, temp_id); 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. https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... content/public/browser/desktop_media_id.cc:160: int64_t tempid; 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. https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... File content/public/browser/web_contents_media_capture_id.cc (right): https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... content/public/browser/web_contents_media_capture_id.cc:14: const char kEnableFlag[] = "?throttling=auto"; Rename this to kEnableThrottlingFlag please. https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... content/public/browser/web_contents_media_capture_id.cc:14: const char kEnableFlag[] = "?throttling=auto"; Make this static. https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:14: const char kWebContentsCaptureScheme[] = "web-contents-media-stream"; This doesn't need to be in .h file. Ideally it should be internal detail of how ToString() and Parse() are implemented. Also see my comments in desktop_media_id.cc for how you can avoid using it there.
Sergey, I added a patch to address your comments. Thanks, George https://codereview.chromium.org/1503563004/diff/300001/chrome/browser/extensi... File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc (right): https://codereview.chromium.org/1503563004/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:95: show_tabs = true; On 2016/01/13 19:06:11, Sergey Ulanov wrote: > tab capture is still not implemented yet. leave this change for later, when the > feature is fully implemented. Done. https://codereview.chromium.org/1503563004/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:114: const int source_types = (show_screens ? DesktopMediaListAsh::SCREENS : 0) | On 2016/01/13 19:06:11, Sergey Ulanov wrote: > Move this inside the if statement below, but I'm not sure if you need this at > all. Done. https://codereview.chromium.org/1503563004/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1503563004/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/desktop_media_picker_views.cc:529: if (parent_ && source.type != DesktopMediaID::TYPE_WEB_CONTENTS) On 2016/01/13 19:06:11, Sergey Ulanov wrote: > This would be confusing behavior. It would be better to keep the feature not > supported, until it's fully implemented. I.e. revert the change in > desktop_capture_base.cc. Done. https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... File content/public/browser/desktop_media_id.cc (right): https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... content/public/browser/desktop_media_id.cc:86: type == TYPE_WEB_CONTENTS); On 2016/01/13 19:06:11, Sergey Ulanov wrote: > 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. Done. https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... content/public/browser/desktop_media_id.cc:148: return DesktopMediaID(type, 0, temp_id); On 2016/01/13 19:06:11, Sergey Ulanov wrote: > 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. Good idea. https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... content/public/browser/desktop_media_id.cc:160: int64_t tempid; On 2016/01/13 19:06:11, Sergey Ulanov wrote: > 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. Done. https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... File content/public/browser/web_contents_media_capture_id.cc (right): https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... content/public/browser/web_contents_media_capture_id.cc:14: const char kEnableFlag[] = "?throttling=auto"; On 2016/01/13 19:06:11, Sergey Ulanov wrote: > Rename this to kEnableThrottlingFlag please. Done. https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... content/public/browser/web_contents_media_capture_id.cc:14: const char kEnableFlag[] = "?throttling=auto"; On 2016/01/13 19:06:11, Sergey Ulanov wrote: > Make this static. Done. https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/300001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:14: const char kWebContentsCaptureScheme[] = "web-contents-media-stream"; On 2016/01/13 19:06:11, Sergey Ulanov wrote: > This doesn't need to be in .h file. Ideally it should be internal detail of how > ToString() and Parse() are implemented. Also see my comments in > desktop_media_id.cc for how you can avoid using it there. Good Point
lgtm https://codereview.chromium.org/1503563004/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc (right): https://codereview.chromium.org/1503563004/diff/320001/chrome/browser/extensi... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:79: bool show_tabs = false; remove this. You don't need any changes in this file in this CL https://codereview.chromium.org/1503563004/diff/320001/content/public/browser... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/320001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:14: struct CONTENT_EXPORT WebContentsMediaCaptureId { nit: add empty line here
Sergey. Done and thanks, George https://codereview.chromium.org/1503563004/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc (right): https://codereview.chromium.org/1503563004/diff/320001/chrome/browser/extensi... chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:79: bool show_tabs = false; On 2016/01/13 22:53:16, Sergey Ulanov wrote: > remove this. You don't need any changes in this file in this CL Done. https://codereview.chromium.org/1503563004/diff/320001/content/public/browser... File content/public/browser/web_contents_media_capture_id.h (right): https://codereview.chromium.org/1503563004/diff/320001/content/public/browser... content/public/browser/web_contents_media_capture_id.h:14: struct CONTENT_EXPORT WebContentsMediaCaptureId { On 2016/01/13 22:53:16, Sergey Ulanov wrote: > nit: add empty line here Done.
The CQ bit was checked by gyzhou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, dalecurtis@chromium.org, rockot@chromium.org, avi@chromium.org, miu@chromium.org, sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/1503563004/#ps340001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1503563004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1503563004/340001
Message was sent while issue was closed.
Description was changed from ========== This is the second of three CLs for desktop chrome tab capture. WebRTC today supports media capture and sharing for "screen" and "window" types through Javascript APIs chooseDesktopMedia/GetUserMedia. This Cl adds support for chrome tab for JavaScript API chooseDesktopMedia(). If "tab" is enabled in chooseDesktopMedia(), the desktop media pick dialog will list all chrome tabs for user to select. No tab video/audio will be actually captured until the third CL. The design document can be found at https://goo.gl/uoHnv0 BUG=557222 ========== to ========== This is the second of three CLs for desktop chrome tab capture. WebRTC today supports media capture and sharing for "screen" and "window" types through Javascript APIs chooseDesktopMedia/GetUserMedia. This Cl adds support for chrome tab for JavaScript API chooseDesktopMedia(). If "tab" is enabled in chooseDesktopMedia(), the desktop media pick dialog will list all chrome tabs for user to select. No tab video/audio will be actually captured until the third CL. The design document can be found at https://goo.gl/uoHnv0 BUG=557222 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== This is the second of three CLs for desktop chrome tab capture. WebRTC today supports media capture and sharing for "screen" and "window" types through Javascript APIs chooseDesktopMedia/GetUserMedia. This Cl adds support for chrome tab for JavaScript API chooseDesktopMedia(). If "tab" is enabled in chooseDesktopMedia(), the desktop media pick dialog will list all chrome tabs for user to select. No tab video/audio will be actually captured until the third CL. The design document can be found at https://goo.gl/uoHnv0 BUG=557222 ========== to ========== This is the second of three CLs for desktop chrome tab capture. WebRTC today supports media capture and sharing for "screen" and "window" types through Javascript APIs chooseDesktopMedia/GetUserMedia. This Cl adds support for chrome tab for JavaScript API chooseDesktopMedia(). If "tab" is enabled in chooseDesktopMedia(), the desktop media pick dialog will list all chrome tabs for user to select. No tab video/audio will be actually captured until the third CL. The design document can be found at https://goo.gl/uoHnv0 BUG=557222 Committed: https://crrev.com/7194a6fa14a2fea0db9c20602b4a16913d7295f3 Cr-Commit-Position: refs/heads/master@{#369447} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/7194a6fa14a2fea0db9c20602b4a16913d7295f3 Cr-Commit-Position: refs/heads/master@{#369447}
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/1503563004/diff/80001/content/browser/media/c... File content/browser/media/capture/web_contents_video_capture_device_unittest.cc (right): https://codereview.chromium.org/1503563004/diff/80001/content/browser/media/c... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:22: #include "content/public/browser/web_contents_media_capture_id.h" On 2015/12/16 22:38:40, GeorgeZ wrote: > On 2015/12/16 21:08:36, qiangchenC wrote: > > I did not see any essential code change in this file. Can you verify whether > we > > can remove this include or not? > > content/public/browser/web_contents_media_capture_id.h was added to cover the > content of content/browser/media/capture/web_contents_capture_util.h. This was > suggested by a reviewer. Acknowledged. |
