Chromium Code Reviews
Help | Chromium Project | Sign in
(1113)

Issue 12208010: Adding device selection menus to the content setting bubble (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 2 months ago by xians1
Modified:
1 year, 1 month ago
CC:
chromium-reviews_chromium.org, tfarina
Visibility:
Public.

Description

This patch adds mic/camera selection menu to the content setting bubble.
It allows the users to select a different device from the omnibox since the device selection UI in content setting is not noticable to the normal users.
Note that a reload action is required to have the new setting take effect.

Relevant design doc is in https://docs.google.com/a/google.com/presentation/d/1Ohksd45T-1fjQpdElKx0cRrnZin3j85oxDKbuBILnuk/edit#slide=id.g9a0c6e39_00 page 2

BUG=174357
TEST=unit_tests --gtest_filter="ContentSettingBubbleModelTest*" and
go to https://webrtc-demos.appspot.com/html/pc1.html, allow the media request, click the camera icon on the omnibox, then select a different device, reload the page, then the new setting should kick in.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183597

Patch Set 1 #

Patch Set 2 : some cleanup and fixed the unittest #

Patch Set 3 : some more cleanup and ready for review. #

Total comments: 32

Patch Set 4 : addressed Markus' comments and rebased. #

Patch Set 5 : Used callback instead of inheriting from a virtual interface #

Total comments: 6

Patch Set 6 : addressed tfarina's comments. #

Patch Set 7 : fixed the windows code. #

Total comments: 67

Patch Set 8 : addressed Peter and Evan's comments. #

Total comments: 2

Patch Set 9 : rebased and fixed the windows code Markus found. #

Total comments: 10

Patch Set 10 : addressed Peter's comments. #

Patch Set 11 : rebased #

Total comments: 1

Patch Set 12 : used dynamic sizing for the menu buttons and TOPLEFT alignment #

Total comments: 43

Patch Set 13 : #

Total comments: 12

Patch Set 14 : #

Total comments: 6

Patch Set 15 : addressed the final comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+971 lines, -426 lines) Lint Patch
M chrome/browser/ui/content_settings/content_setting_bubble_model.h View 1 2 3 6 chunks +18 lines, -0 lines 0 comments 0 errors Download
M chrome/browser/ui/content_settings/content_setting_bubble_model.cc View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +100 lines, -0 lines 0 comments 0 errors Download
M chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments 0 errors Download
A chrome/browser/ui/content_settings/content_setting_media_menu_model.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +50 lines, -0 lines 0 comments 3 errors Download
A chrome/browser/ui/content_settings/content_setting_media_menu_model.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +60 lines, -0 lines 0 comments 1 errors Download
M chrome/browser/ui/gtk/content_setting_bubble_gtk.h View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +31 lines, -0 lines 0 comments 1 errors Download
M chrome/browser/ui/gtk/content_setting_bubble_gtk.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 7 chunks +98 lines, -1 line 0 comments 1 errors Download
M chrome/browser/ui/views/content_setting_bubble_contents.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +122 lines, -95 lines 0 comments 3 errors Download
M chrome/browser/ui/views/content_setting_bubble_contents.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +489 lines, -330 lines 0 comments 3 errors Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -0 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 40
xians1
Hi Markus and Evan, could you please review this CL? It is a feature adding ...
1 year, 2 months ago #1
markusheintz_
LG manly typos and nits. Please replace the "unit_tests" in the TEST section of the ...
1 year, 2 months ago #2
tfarina
https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content_settings/content_setting_media_menu_model.h File chrome/browser/ui/content_settings/content_setting_media_menu_model.h (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content_settings/content_setting_media_menu_model.h#newcode24 chrome/browser/ui/content_settings/content_setting_media_menu_model.h:24: class Observer { Don't use nested classes for Observer/Listener ...
1 year, 2 months ago #3
xians1
Markus, please take another look. Thanks, SX https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content_settings/content_setting_bubble_model.cc File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content_settings/content_setting_bubble_model.cc#newcode69 chrome/browser/ui/content_settings/content_setting_bubble_model.cc:69: // Could ...
1 year, 2 months ago #4
xians1
https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content_settings/content_setting_media_menu_model.h File chrome/browser/ui/content_settings/content_setting_media_menu_model.h (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content_settings/content_setting_media_menu_model.h#newcode24 chrome/browser/ui/content_settings/content_setting_media_menu_model.h:24: class Observer { On 2013/02/06 13:31:36, tfarina wrote: > ...
1 year, 2 months ago #5
markusheintz_
LGTM Thanks https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content_settings/content_setting_media_menu_model.cc File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content_settings/content_setting_media_menu_model.cc#newcode27 chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:27: BuildMenu(); On 2013/02/06 13:31:52, xians1 wrote: > ...
1 year, 2 months ago #6
tfarina
https://codereview.chromium.org/12208010/diff/7019/chrome/browser/ui/content_settings/content_setting_media_menu_model.cc File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/7019/chrome/browser/ui/content_settings/content_setting_media_menu_model.cc#newcode1 chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights ...
1 year, 2 months ago #7
xians1
tfarina, I have already addressed your comments, please take another look. Peter, could you please ...
1 year, 2 months ago #8
Evan Stade
https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/content_setting_bubble_gtk.cc File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/content_setting_bubble_gtk.cc#newcode85 chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:85: it->second->label.Destroy(); I don't think this should be necessary, and ...
1 year, 2 months ago #9
Peter Kasting
I can't review GTK. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_settings/content_setting_bubble_model.cc File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_settings/content_setting_bubble_model.cc#newcode61 chrome/browser/ui/content_settings/content_setting_bubble_model.cc:61: const std::string& device_id, const content::MediaStreamDevices& ...
1 year, 2 months ago #10
xians1
Hi reviewers, thanks for the help. I think I have already addressed all your comments, ...
1 year, 2 months ago #11
markusheintz_
https://codereview.chromium.org/12208010/diff/9009/chrome/browser/ui/views/content_setting_bubble_contents.cc File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/9009/chrome/browser/ui/views/content_setting_bubble_contents.cc#newcode321 chrome/browser/ui/views/content_setting_bubble_contents.cc:321: Profile* profile = FromBrowserContext(web_contents_->GetBrowserContext()); Please use: Profile::FromBrowserContext(web_contents_->GetBrowserContext());
1 year, 2 months ago #12
xians1
https://codereview.chromium.org/12208010/diff/9009/chrome/browser/ui/views/content_setting_bubble_contents.cc File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/9009/chrome/browser/ui/views/content_setting_bubble_contents.cc#newcode321 chrome/browser/ui/views/content_setting_bubble_contents.cc:321: Profile* profile = FromBrowserContext(web_contents_->GetBrowserContext()); On 2013/02/07 16:45:06, markusheintz_ wrote: ...
1 year, 2 months ago #13
xians1
I was told that Nico is OOO for today and tomorrow, added thestig for the ...
1 year, 2 months ago #14
Lei Zhang
On 2013/02/07 17:25:06, xians1 wrote: > I was told that Nico is OOO for today ...
1 year, 2 months ago #15
Peter Kasting
https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/content_setting_bubble_contents.cc File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/content_setting_bubble_contents.cc#newcode58 chrome/browser/ui/views/content_setting_bubble_contents.cc:58: const int kMediaMenuButtonWidth = 300; On 2013/02/07 16:25:19, xians1 ...
1 year, 2 months ago #16
xians1
Peter, please see answers inline. I will send you the screenshots by email. Thanks, SX ...
1 year, 2 months ago #17
Peter Kasting
https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/content_setting_bubble_contents.cc File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/content_setting_bubble_contents.cc#newcode58 chrome/browser/ui/views/content_setting_bubble_contents.cc:58: const int kMediaMenuButtonWidth = 300; On 2013/02/08 12:32:39, xians1 ...
1 year, 2 months ago #18
Nico
gypi lgtm
1 year, 2 months ago #19
xians1
Hello Petter, please see answer inline. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/content_setting_bubble_contents.cc File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/content_setting_bubble_contents.cc#newcode58 chrome/browser/ui/views/content_setting_bubble_contents.cc:58: const int kMediaMenuButtonWidth ...
1 year, 2 months ago #20
xians1
Hello Petter, please see answer inline.
1 year, 2 months ago #21
Peter Kasting
https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/content_setting_bubble_contents.cc File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/content_setting_bubble_contents.cc#newcode58 chrome/browser/ui/views/content_setting_bubble_contents.cc:58: const int kMediaMenuButtonWidth = 300; On 2013/02/09 14:53:51, xians1 ...
1 year, 2 months ago #22
xians1
We actually made a document for this work : UI, https://docs.google.com/a/google.com/presentation/d/1Ohksd45T-1fjQpdElKx0cRrnZin3j85oxDKbuBILnuk/edit#slide=id.g9a0c6e39_00 + Glen since the ...
1 year, 2 months ago #23
markusheintz_
On 2013/02/09 22:13:25, xians1 wrote: > We actually made a document for this work : ...
1 year, 2 months ago #24
Peter Kasting
On 2013/02/11 15:39:41, markusheintz_ wrote: > Given the length of these strings (see http://imgur.com/FmKunEJ) I'm ...
1 year, 2 months ago #25
Evan Stade
gtk lgtm https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/content_setting_bubble_gtk.cc File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/content_setting_bubble_gtk.cc#newcode85 chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:85: it->second->label.Destroy(); On 2013/02/07 16:25:19, xians1 wrote: > ...
1 year, 2 months ago #26
xians1
Peter, I have already addressed the dynamic size and alignment for the buttons. please take ...
1 year, 2 months ago #27
Peter Kasting
Seems mostly in good shape https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content_settings/content_setting_bubble_model.cc File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content_settings/content_setting_bubble_model.cc#newcode742 chrome/browser/ui/content_settings/content_setting_bubble_model.cc:742: camera_menu.default_device = GetMediaDeviceById(preferred_camera, Nit: ...
1 year, 2 months ago #28
xians1
https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content_settings/content_setting_bubble_model.cc File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content_settings/content_setting_bubble_model.cc#newcode742 chrome/browser/ui/content_settings/content_setting_bubble_model.cc:742: camera_menu.default_device = GetMediaDeviceById(preferred_camera, On 2013/02/16 02:55:13, Peter Kasting wrote: ...
1 year, 1 month ago #29
tfarina
https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/content_setting_bubble_contents.h File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/content_setting_bubble_contents.h#newcode50 chrome/browser/ui/views/content_setting_bubble_contents.h:50: : public views::BubbleDelegateView, On 2013/02/18 17:20:04, xians1 wrote: > ...
1 year, 1 month ago #30
xians1
On 2013/02/18 17:33:50, tfarina wrote: > https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/content_setting_bubble_contents.h > File chrome/browser/ui/views/content_setting_bubble_contents.h (right): > > https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/content_setting_bubble_contents.h#newcode50 > ...
1 year, 1 month ago #31
Nico
On Mon, Feb 18, 2013 at 6:54 PM, <xians@chromium.org> wrote: > On 2013/02/18 17:33:50, tfarina ...
1 year, 1 month ago #32
Peter Kasting
https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/content_setting_bubble_contents.cc File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/content_setting_bubble_contents.cc#newcode59 chrome/browser/ui/views/content_setting_bubble_contents.cc:59: // The width of the media menu button. On ...
1 year, 1 month ago #33
xians1
https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/content_setting_bubble_contents.cc File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/content_setting_bubble_contents.cc#newcode336 chrome/browser/ui/views/content_setting_bubble_contents.cc:336: menu_width = menu_width <= kMinMediaMenuButtonWidth ? On 2013/02/18 18:23:18, ...
1 year, 1 month ago #34
Peter Kasting
LGTM except for one substantive comment. https://codereview.chromium.org/12208010/diff/46004/chrome/browser/ui/content_settings/content_setting_media_menu_model.cc File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/46004/chrome/browser/ui/content_settings/content_setting_media_menu_model.cc#newcode29 chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:29: devices = dispatcher->GetVideoCaptureDevices(); ...
1 year, 1 month ago #35
xians1
Thanks for all. I am going to land it now. https://codereview.chromium.org/12208010/diff/46004/chrome/browser/ui/content_settings/content_setting_media_menu_model.cc File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/46004/chrome/browser/ui/content_settings/content_setting_media_menu_model.cc#newcode29 ...
1 year, 1 month ago #36
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/12208010/54001
1 year, 1 month ago #37
I haz the power (commit-bot)
Retried try job too often on win_rel for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=113554
1 year, 1 month ago #38
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/12208010/54001
1 year, 1 month ago #39
I haz the power (commit-bot)
1 year, 1 month ago #40
Message was sent while issue was closed.
Change committed as 183597
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1275:d14800f88434