Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12208010:
Adding device selection menus to the content setting bubble (Closed)
|
Created:
7 years, 10 months ago by no longer working on chromium Modified:
7 years, 9 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis 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. #Messages
Total messages: 40 (0 generated)
Hi Markus and Evan, could you please review this CL? It is a feature adding a device selection dropdowns to the content setting bubble in linux and windows for M26. Evan: gtk code Markus: content setting I will ask Peter to review the windows layout code after verifying the UI on windows. Thanks, SX
LG manly typos and nits. Please replace the "unit_tests" in the TEST section of the CL description with "ContentSettingBubbleModelTest*" Also could you please add some screenshots? Either to the issue or add some links to the CL description. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:69: // Could not find any device with |device_id|, likely the device has been Nit; changing this is up to you: A device with the |device_id| was not found. It is likely that the device ... https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:672: const content::MediaStreamDevices& micophones = typo: please s/micophones/microphones/g https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_bubble_model.h:66: typedef std::map<content::MediaStreamType, MediaMenu> MediaMenuMap; Please add "#include <map>" in line 8 https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_bubble_model.h:108: virtual void OnMeiaMenuClicked(content::MediaStreamType type, Please fix the typo here. s/OnMeiaMenuClicked/OnMediaMenuClicked https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:27: BuildMenu(); I guess we can inline BuildMenu here. I don't see that it is called anywhere else. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:41: bool ContentSettingMediaMenuModel::GetAcceleratorForCommandId( Is it possible to open navigate and close the menu without a mouse and the keyboard only? If not let's file a bug for this because this would be an accessibility issue. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.h (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.h:57: Observer* observer_; This is a weak pointer too. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/gtk/con... File chrome/browser/ui/gtk/content_setting_bubble_gtk.h (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/gtk/con... chrome/browser/ui/gtk/content_setting_bubble_gtk.h:57: // A map from a GtkWidget* to a MediaMenuGtk*. MediaMenuGtk struct is used See my comment in the views code. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/gtk/con... chrome/browser/ui/gtk/content_setting_bubble_gtk.h:59: struct MediaMenuGtk { Now I see we you named the equivalent in the views code MediaMenuView. I guess I would rename this to MediaMenuPartsGtk. But it probably does not matter here. If Evan has no comments about this then, I'm fine with MediaMenuGtk https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:127: // Free any MediaMenuVoew objects left over. Typo s/MediaMenuVoew/MediaMenuView https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:151: DLOG(WARNING) << "TODO SET THE TITLE FOR THE MENU"; Is this still an active TODO? Looks like some dbg code Please remove the LOG statement and either add a comment with a TODO and an owner, or even better file a bug for the TODO. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:424: ContentSettingBubbleContents::MediaMenuView::MediaMenuView() {} |menu_model| initialized to NULL thanks to the scoped_ptr. But |type| is not initialized. So I think we may want to initialize |type| to some default value here. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.h:75: // A map from a views::MenuButton* to a MediaMenuView*. MediaMenuView struct Since the map is defined in line 87 either move the first sentence to this line or delete it. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.h:77: struct MediaMenuView { The suffix "View" is a bit misleading here, since this struct does not inherit from views::View. Maybe MediaMenuParts, MediaMenu or something similar.
https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.h (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.h:24: class Observer { Don't use nested classes for Observer/Listener (whatever you call it)! https://codereview.chromium.org/12039058/diff/1/content/public/browser/host_z... When you inherit from it in chrome/browser/ui/views/content_setting_bubble_contents.h you see that none of them are using this pattern? Are all FooObserver/FooListener rather than Foo::Observer, Foo::Listener. I hope John's recommendation above is strong enough to support my suggestion here.
Markus, please take another look. Thanks, SX https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:69: // Could not find any device with |device_id|, likely the device has been On 2013/02/06 11:03:26, markusheintz_ wrote: > Nit; changing this is up to you: > A device with the |device_id| was not found. It is likely that the device ... Done. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:672: const content::MediaStreamDevices& micophones = On 2013/02/06 11:03:26, markusheintz_ wrote: > typo: please s/micophones/microphones/g Done. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_bubble_model.h:66: typedef std::map<content::MediaStreamType, MediaMenu> MediaMenuMap; On 2013/02/06 11:03:26, markusheintz_ wrote: > Please add "#include <map>" in line 8 Done. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_bubble_model.h:108: virtual void OnMeiaMenuClicked(content::MediaStreamType type, On 2013/02/06 11:03:26, markusheintz_ wrote: > Please fix the typo here. > s/OnMeiaMenuClicked/OnMediaMenuClicked Done. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:27: BuildMenu(); On 2013/02/06 11:03:26, markusheintz_ wrote: > I guess we can inline BuildMenu here. I don't see that it is called anywhere > else. We can do this. But from the readability perspective, it is good to wrap the code into a private function to keep the constructor small. I hope it is fine to keep it this way, but please let me know if you strongly want it to be inline, I can change it. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:41: bool ContentSettingMediaMenuModel::GetAcceleratorForCommandId( On 2013/02/06 11:03:26, markusheintz_ wrote: > Is it possible to open navigate and close the menu without a mouse and the > keyboard only? If not let's file a bug for this because this would be an > accessibility issue. Not sure here, I just filed a bug 174615. Do you know if we can use the keyboard to open the bubble? https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.h (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.h:57: Observer* observer_; On 2013/02/06 11:03:26, markusheintz_ wrote: > This is a weak pointer too. Done. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/gtk/con... File chrome/browser/ui/gtk/content_setting_bubble_gtk.h (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/gtk/con... chrome/browser/ui/gtk/content_setting_bubble_gtk.h:59: struct MediaMenuGtk { On 2013/02/06 11:03:26, markusheintz_ wrote: > Now I see we you named the equivalent in the views code MediaMenuView. I guess I > would rename this to MediaMenuPartsGtk. But it probably does not matter here. If > Evan has no comments about this then, I'm fine with MediaMenuGtk Thanks, MediaMenu has been used by the content_setting_bubble_model, so I won't want the same name in different places. Lets keep it open and I can adapt to Evan's comment on this. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:127: // Free any MediaMenuVoew objects left over. On 2013/02/06 11:03:26, markusheintz_ wrote: > Typo s/MediaMenuVoew/MediaMenuView Done. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:151: DLOG(WARNING) << "TODO SET THE TITLE FOR THE MENU"; On 2013/02/06 11:03:26, markusheintz_ wrote: > Is this still an active TODO? Looks like some dbg code > > Please remove the LOG statement and either add a comment with a TODO and an > owner, or even better file a bug for the TODO. Removed. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:424: ContentSettingBubbleContents::MediaMenuView::MediaMenuView() {} On 2013/02/06 11:03:26, markusheintz_ wrote: > |menu_model| initialized to NULL thanks to the scoped_ptr. But |type| is not > initialized. So I think we may want to initialize |type| to some default value > here. Done. I added the type as the input to the constructor. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.h:75: // A map from a views::MenuButton* to a MediaMenuView*. MediaMenuView struct On 2013/02/06 11:03:26, markusheintz_ wrote: > Since the map is defined in line 87 either move the first sentence to this line > or delete it. Done. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.h:77: struct MediaMenuView { On 2013/02/06 11:03:26, markusheintz_ wrote: > The suffix "View" is a bit misleading here, since this struct does not inherit > from views::View. > > Maybe MediaMenuParts, MediaMenu or something similar. Done.
https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.h (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.h:24: class Observer { On 2013/02/06 13:31:36, tfarina wrote: > Don't use nested classes for Observer/Listener (whatever you call it)! > > https://codereview.chromium.org/12039058/diff/1/content/public/browser/host_z... > > When you inherit from it in > chrome/browser/ui/views/content_setting_bubble_contents.h > you see that none of them are using this pattern? Are all > FooObserver/FooListener rather than Foo::Observer, Foo::Listener. > > I hope John's recommendation above is strong enough to support my suggestion > here. Thanks tfarina, I got hint from the thread you attached that a callback is preferred since there is one interface. Please check the code again if you want to.
LGTM Thanks https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:27: BuildMenu(); On 2013/02/06 13:31:52, xians1 wrote: > On 2013/02/06 11:03:26, markusheintz_ wrote: > > I guess we can inline BuildMenu here. I don't see that it is called anywhere > > else. > > We can do this. But from the readability perspective, it is good to wrap the > code into a private function to keep the constructor small. I hope it is fine to > keep it this way, but please let me know if you strongly want it to be inline, I > can change it. SGTM. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:41: bool ContentSettingMediaMenuModel::GetAcceleratorForCommandId( On 2013/02/06 13:31:52, xians1 wrote: > On 2013/02/06 11:03:26, markusheintz_ wrote: > > Is it possible to open navigate and close the menu without a mouse and the > > keyboard only? If not let's file a bug for this because this would be an > > accessibility issue. > > Not sure here, I just filed a bug 174615. > Do you know if we can use the keyboard to open the bubble? Got to double check this. Thanks for filing the bug. https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/gtk/con... File chrome/browser/ui/gtk/content_setting_bubble_gtk.h (right): https://codereview.chromium.org/12208010/diff/23001/chrome/browser/ui/gtk/con... chrome/browser/ui/gtk/content_setting_bubble_gtk.h:59: struct MediaMenuGtk { On 2013/02/06 13:31:52, xians1 wrote: > On 2013/02/06 11:03:26, markusheintz_ wrote: > > Now I see we you named the equivalent in the views code MediaMenuView. I guess > I > > would rename this to MediaMenuPartsGtk. But it probably does not matter here. > If > > Evan has no comments about this then, I'm fine with MediaMenuGtk > > Thanks, MediaMenu has been used by the content_setting_bubble_model, so I won't > want the same name in different places. > Lets keep it open and I can adapt to Evan's comment on this. SGTM
https://codereview.chromium.org/12208010/diff/7019/chrome/browser/ui/content_... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/7019/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. no (c), the new copyright doesn't have it. crbug.com/140977 https://codereview.chromium.org/12208010/diff/7019/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:12: #include "chrome/browser/ui/content_settings/content_setting_media_menu_model.h" nit: this is the same include of line 5. https://codereview.chromium.org/12208010/diff/7019/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:14: nit: extra blank line
tfarina, I have already addressed your comments, please take another look. Peter, could you please review the gtk and views/ layout code. I might further tweak the win UI code a bit to make the buttons looking nicer, but the main work has been done. Thanks, SX https://codereview.chromium.org/12208010/diff/7019/chrome/browser/ui/content_... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/7019/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/02/06 14:56:08, tfarina wrote: > no (c), the new copyright doesn't have it. > > crbug.com/140977 Done. https://codereview.chromium.org/12208010/diff/7019/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:12: #include "chrome/browser/ui/content_settings/content_setting_media_menu_model.h" On 2013/02/06 14:56:08, tfarina wrote: > nit: this is the same include of line 5. Done. https://codereview.chromium.org/12208010/diff/7019/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:14: On 2013/02/06 14:56:08, tfarina wrote: > nit: extra blank line Done.
https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/cont... File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/cont... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:85: it->second->label.Destroy(); I don't think this should be necessary, and instead of using a for loop you should be able to call STLMapDestroy or something like that. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/cont... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:99: break; make this a return and put a NOTREACHED at the end of the fn. That way you can define the iterator inside the for loop header. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/cont... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:233: i(content.media_menus.begin()); i != content.media_menus.end(); 4 more indent and put the termination condition on a new line https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/cont... File chrome/browser/ui/gtk/content_setting_bubble_gtk.h (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/cont... chrome/browser/ui/gtk/content_setting_bubble_gtk.h:32: ^H
I can't review GTK. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:61: const std::string& device_id, const content::MediaStreamDevices& devices) { Nit: One arg per line https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:62: DCHECK(devices.size()); Nit: !empty() https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:570: if (it->second.selected_device.id != it->second.default_device.id) { Nit: {} not necessary https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:654: content::MediaStreamType type, const std::string& device) { Nit: One arg per line https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:656: switch (type) { Nit: Shorter: DCHECK((type == content::MEDIA_DEVICE_AUDIO_CAPTURE) || (type == content::MEDIA_DEVICE_VIDEO_CAPTURE)); prefs->SetString((type == content::MEDIA_DEVICE_AUDIO_CAPTURE) ? prefs::kDefaultAudioCaptureDevice : prefs::kDefaultVideoCaptureDevice, device); You could also do this with ifs: if (type == content::MEDIA_DEVICE_AUDIO_CAPTURE) { prefs->SetString(prefs::kDefaultAudioCaptureDevice, device); } else { DCHECK_EQ(content::MEDIA_DEVICE_VIDEO_CAPTURE, type); prefs->SetString(prefs::kDefaultVideoCaptureDevice, device); } https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:677: if (microphones.size()) { Nit: !empty() (2 places) https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:706: DCHECK(bubble_content().media_menus.find(type) != Nit: Use count() instead of find() (shorter) https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:710: content::MediaStreamDevices devices; Nit: I suggest using ?: to initialize this at its declaration point (add an optional DCHECK above it if it makes you feel safer). This prevents someone adding a future case that fails to initialize this, and is shorter to boot. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:20: profile_(profile), You shouldn't be taking a Profile* when you depend on a PrefService*. Take a PrefService* directly. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:68: NOTREACHED(); Nit: Simpler: if (type_ == content::MEDIA_DEVICE_AUDIO_CAPTURE) { ... } else { DCHECK_EQ(content::MEDIA_DEVICE_VIDEO_CAPTURE, type_); ... } https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:72: int command_id = commands_.size(); Nit: Inline into next statement https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... File chrome/browser/ui/content_settings/content_setting_media_menu_model.h (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.h:20: : public ui::SimpleMenuModel, Can we use composition here or in the callers/users of this class to avoid multiple inheritance? It seems kind of weird to me to have a menu model be its own delegate as well, though maybe I'm just not familiar with this practice. Nit: Place base class declaration on end of previous line, a la class A : public B, public C { https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:45: namespace { Nit: Add blank line below https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:58: const int kMediaMenuButtonWidth = 300; This seems arbitrary. How was this decided? Should this instead be calculated based on the bubble's current width or max contents width, or based on the longest string in any of the menus, or something? https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:139: // Free any MediaMenuParts objects left over. Nit: Comment unnecessary. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:142: delete it->second; Nit: I believe STLDeleteValues() does what you want. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:159: MediaMenuPartsMap::const_iterator it = media_menus_.begin(); Nit: Declare this in the loop declaration. You can still assert that we find the desired item if you want, by changing "break;" to "return;" and the final DCHECK to a NOTREACHED. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:287: UTF8ToUTF16(i->second.label)); Nit: Bizarre indenting (all over this section; did you use tabs instead of spaces?) https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:292: NULL, UTF8ToUTF16((i->second.selected_device.name)), this, false); Nit: 80 columns (due to indenting) https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:302: // Store the menu view to the map. Nit: This comment adds nothing. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:428: return; We return here anyway. There's no need to explicitly do so. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.h:79: struct MediaMenuParts { Does this need to be defined here? Seems like it can be forward-declared like Favicon. Regardless, you should move the declaration above the PopupLinks typedef. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.h:90: // is used to store the UI members that a media menu owns. Nit: First sentence is unnecessary since it merely restates the code. The second sentence is not terribly helpful. I'd just nuke this. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.h:100: // views::MenuButtonListener interface. Nit: Use "// Base class:" like the other comments here. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.h:112: // The active profile. Do we need to take this as a constructor arg and store it as a member? Can we dynamically get it off the WebContents* or something?
Hi reviewers, thanks for the help. I think I have already addressed all your comments, please take another look. We hope to make it for M26, if possible. Nico, can I have your owner stamp for the gypi file? Thanks again, SX https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:61: const std::string& device_id, const content::MediaStreamDevices& devices) { On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: One arg per line Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:62: DCHECK(devices.size()); On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: !empty() Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:570: if (it->second.selected_device.id != it->second.default_device.id) { On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: {} not necessary Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:654: content::MediaStreamType type, const std::string& device) { On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: One arg per line Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:656: switch (type) { On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: Shorter: > > DCHECK((type == content::MEDIA_DEVICE_AUDIO_CAPTURE) || > (type == content::MEDIA_DEVICE_VIDEO_CAPTURE)); > prefs->SetString((type == content::MEDIA_DEVICE_AUDIO_CAPTURE) ? > prefs::kDefaultAudioCaptureDevice : prefs::kDefaultVideoCaptureDevice, > device); > > You could also do this with ifs: > > if (type == content::MEDIA_DEVICE_AUDIO_CAPTURE) { > prefs->SetString(prefs::kDefaultAudioCaptureDevice, device); > } else { > DCHECK_EQ(content::MEDIA_DEVICE_VIDEO_CAPTURE, type); > prefs->SetString(prefs::kDefaultVideoCaptureDevice, device); > } Done with using the ifs. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:677: if (microphones.size()) { On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: !empty() (2 places) Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:706: DCHECK(bubble_content().media_menus.find(type) != On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: Use count() instead of find() (shorter) Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:710: content::MediaStreamDevices devices; On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: I suggest using ?: to initialize this at its declaration point (add an > optional DCHECK above it if it makes you feel safer). > > This prevents someone adding a future case that fails to initialize this, and is > shorter to boot. Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:20: profile_(profile), On 2013/02/06 22:11:18, Peter Kasting wrote: > You shouldn't be taking a Profile* when you depend on a PrefService*. Take a > PrefService* directly. Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:68: NOTREACHED(); On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: Simpler: > > if (type_ == content::MEDIA_DEVICE_AUDIO_CAPTURE) { > ... > } else { > DCHECK_EQ(content::MEDIA_DEVICE_VIDEO_CAPTURE, type_); > ... > } Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:68: NOTREACHED(); On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: Simpler: > > if (type_ == content::MEDIA_DEVICE_AUDIO_CAPTURE) { > ... > } else { > DCHECK_EQ(content::MEDIA_DEVICE_VIDEO_CAPTURE, type_); > ... > } Done, I moved the DCHECK to the constructor. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:72: int command_id = commands_.size(); On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: Inline into next statement Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... File chrome/browser/ui/content_settings/content_setting_media_menu_model.h (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_media_menu_model.h:20: : public ui::SimpleMenuModel, On 2013/02/06 22:11:18, Peter Kasting wrote: > Can we use composition here or in the callers/users of this class to avoid > multiple inheritance? It seems kind of weird to me to have a menu model be its > own delegate as well, though maybe I'm just not familiar with this practice. > > Nit: Place base class declaration on end of previous line, a la > > class A : public B, > public C { Yes, we can, but then we need to expose the SimpleMenuModel member to the UI clients. This way is actually the easiest and also clean way to achieve the same purpose. Since the same solution is also used by many clients like: NotificationOptionsMenuModel, OptionsMenuModel, SuggestionsMenuModel, PermissionMenuModel, LauncherApplicationMenuItemModel, RecentTabsSubMenuModel, EncodingMenuModel, ..etc. I hope it is fine to keep it. But please let me know if you strongly think we should break it down. Done with changing the declaration. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/cont... File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/cont... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:85: it->second->label.Destroy(); On 2013/02/06 22:07:07, Evan Stade wrote: > I don't think this should be necessary, and instead of using a for loop you > should be able to call STLMapDestroy or something like that. I changed the code to use STLDeleteValues. The comment in OwnedWidgetGtk::Own() is very confusing, it clearly states that: It is valid to never call Own(), in which case Destroy() will do nothing. If Own() has been called, you must explicitly call Destroy(). But the destructor of OwnedWidgetGtk is calling Destroy(), so I suppose the comment is not valid anymore, right? https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/cont... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:99: break; On 2013/02/06 22:07:07, Evan Stade wrote: > make this a return and put a NOTREACHED at the end of the fn. That way you can > define the iterator inside the for loop header. Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/cont... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:233: i(content.media_menus.begin()); i != content.media_menus.end(); On 2013/02/06 22:07:07, Evan Stade wrote: > 4 more indent and put the termination condition on a new line Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/cont... File chrome/browser/ui/gtk/content_setting_bubble_gtk.h (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/cont... chrome/browser/ui/gtk/content_setting_bubble_gtk.h:32: On 2013/02/06 22:07:07, Evan Stade wrote: > ^H Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:45: namespace { On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: Add blank line below Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:58: const int kMediaMenuButtonWidth = 300; On 2013/02/06 22:11:18, Peter Kasting wrote: > This seems arbitrary. How was this decided? Should this instead be calculated > based on the bubble's current width or max contents width, or based on the > longest string in any of the menus, or something? The width of the button is decided based on how the UI looks like, I changed it to 150 after turning how the UI looks. We can't set this value based on the calculation, since the length of the strings in the menus can be varying. For example, some device has a short name like Default, while others might have a pretty long name. With this fixed width, when the string is longer than it, it will shortcut the string to something like Microphone (SB ARen... https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:139: // Free any MediaMenuParts objects left over. On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: Comment unnecessary. > Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:142: delete it->second; On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: I believe STLDeleteValues() does what you want. Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:159: MediaMenuPartsMap::const_iterator it = media_menus_.begin(); On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: Declare this in the loop declaration. > > You can still assert that we find the desired item if you want, by changing > "break;" to "return;" and the final DCHECK to a NOTREACHED. Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:287: UTF8ToUTF16(i->second.label)); On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: Bizarre indenting (all over this section; did you use tabs instead of > spaces?) likely, I just got my windows working, so the VS editor is still using tab. I will get rid of all of them. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:292: NULL, UTF8ToUTF16((i->second.selected_device.name)), this, false); On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: 80 columns (due to indenting) Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:302: // Store the menu view to the map. On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: This comment adds nothing. Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:428: return; On 2013/02/06 22:11:18, Peter Kasting wrote: > We return here anyway. There's no need to explicitly do so. We need it to get rid of a warning which complains the returned value from RunMenuAt is ignored. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.h:79: struct MediaMenuParts { On 2013/02/06 22:11:18, Peter Kasting wrote: > Does this need to be defined here? Seems like it can be forward-declared like > Favicon. > > Regardless, you should move the declaration above the PopupLinks typedef. Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.h:90: // is used to store the UI members that a media menu owns. On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: First sentence is unnecessary since it merely restates the code. The > second sentence is not terribly helpful. I'd just nuke this. Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.h:100: // views::MenuButtonListener interface. On 2013/02/06 22:11:18, Peter Kasting wrote: > Nit: Use "// Base class:" like the other comments here. Done. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.h:112: // The active profile. On 2013/02/06 22:11:18, Peter Kasting wrote: > Do we need to take this as a constructor arg and store it as a member? Can we > dynamically get it off the WebContents* or something? Done.
https://codereview.chromium.org/12208010/diff/9009/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/9009/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:321: Profile* profile = FromBrowserContext(web_contents_->GetBrowserContext()); Please use: Profile::FromBrowserContext(web_contents_->GetBrowserContext());
https://codereview.chromium.org/12208010/diff/9009/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/9009/chrome/browser/ui/views/co... 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: > Please use: > > Profile::FromBrowserContext(web_contents_->GetBrowserContext()); Done.
I was told that Nico is OOO for today and tomorrow, added thestig for the owner stamps of chrome/chrome_browser_ui.gypi. BR, SX
On 2013/02/07 17:25:06, xians1 wrote: > I was told that Nico is OOO for today and tomorrow, added thestig for the owner > stamps of chrome/chrome_browser_ui.gypi. .gypi lgtm stamp
https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:58: const int kMediaMenuButtonWidth = 300; On 2013/02/07 16:25:19, xians1 wrote: > On 2013/02/06 22:11:18, Peter Kasting wrote: > > This seems arbitrary. How was this decided? Should this instead be > calculated > > based on the bubble's current width or max contents width, or based on the > > longest string in any of the menus, or something? > > The width of the button is decided based on how the UI looks like, I changed it > to 150 after turning how the UI looks. But that's the point: "how the UI looks" is almost certainly taking into account the factors I just mentioned. I suspect if the bubble width doubled, you'd want the value here to change. That in turn suggests that your value here should be based on such other constants, not simply hardcoded directly. For example, by using some of the existing views layout constants, you could specify that the menu should have a standard padding amount at the edges. (The goal is to minimize the number of distinct constants, which tend to fragment our UI appearance and lead to less modifiable UI when we want to update everything's appearance.) If you can't figure out how to avoid a constant here, post screenshots and I will give it a shot. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:428: return; On 2013/02/07 16:25:19, xians1 wrote: > On 2013/02/06 22:11:18, Peter Kasting wrote: > > We return here anyway. There's no need to explicitly do so. > > We need it to get rid of a warning which complains the returned value from > RunMenuAt is ignored. There's an explicit way to note that you don't care about the return value. Look at other callers of RunMenuAt(). https://codereview.chromium.org/12208010/diff/13005/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/13005/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:23: DCHECK(type == content::MEDIA_DEVICE_AUDIO_CAPTURE || Nit: Inconsistent use of |type| vs. |type_| https://codereview.chromium.org/12208010/diff/13005/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/13005/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:60: const SkColor kBorderDarkColor = SkColorSetRGB(0xaa, 0xaa, 0xaa); You can't hardcode colors. All colors need to come from the theme/system color set. https://codereview.chromium.org/12208010/diff/13005/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:301: views::Label* menu_label = new views::Label( Nit: I suggest breaking after '=' instead https://codereview.chromium.org/12208010/diff/13005/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:305: new views::MenuButton( Nit: If you want to indent the subsequent line the way you have, put this line at the end of the previous line https://codereview.chromium.org/12208010/diff/13005/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:322: Profile::FromBrowserContext(web_contents_->GetBrowserContext()); Nit: I'd just inline this into the subsequent statement
Peter, please see answers inline. I will send you the screenshots by email. Thanks, SX https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:58: const int kMediaMenuButtonWidth = 300; On 2013/02/07 23:55:04, Peter Kasting wrote: > On 2013/02/07 16:25:19, xians1 wrote: > > On 2013/02/06 22:11:18, Peter Kasting wrote: > > > This seems arbitrary. How was this decided? Should this instead be > > calculated > > > based on the bubble's current width or max contents width, or based on the > > > longest string in any of the menus, or something? > > > > The width of the button is decided based on how the UI looks like, I changed > it > > to 150 after turning how the UI looks. > > But that's the point: "how the UI looks" is almost certainly taking into account > the factors I just mentioned. > > I suspect if the bubble width doubled, you'd want the value here to change. > That in turn suggests that your value here should be based on such other > constants, not simply hardcoded directly. For example, by using some of the > existing views layout constants, you could specify that the menu should have a > standard padding amount at the edges. > We actually don't allow the users to resize the bubble. And even if the bubble width doubles, we still want to keep the buttons the same size, since the button should always look like button, and doubling the width of the button might look pretty odd. So adapting the size of button to the size of bubble might not give us a good UI. Neither we want to adapt to the length of the menu strings since they can vary a lot. > (The goal is to minimize the number of distinct constants, which tend to > fragment our UI appearance and lead to less modifiable UI when we want to update > everything's appearance.) > I agree, but I hope it is fine to set a preferred size for the buttons here given the size of the bubble won't be changed. > If you can't figure out how to avoid a constant here, post screenshots and I > will give it a shot. Sure, I am sending you photos on look it looks. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:428: return; On 2013/02/07 23:55:04, Peter Kasting wrote: > On 2013/02/07 16:25:19, xians1 wrote: > > On 2013/02/06 22:11:18, Peter Kasting wrote: > > > We return here anyway. There's no need to explicitly do so. > > > > We need it to get rid of a warning which complains the returned value from > > RunMenuAt is ignored. > > There's an explicit way to note that you don't care about the return value. > Look at other callers of RunMenuAt(). Done. https://codereview.chromium.org/12208010/diff/13005/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/13005/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:23: DCHECK(type == content::MEDIA_DEVICE_AUDIO_CAPTURE || On 2013/02/07 23:55:04, Peter Kasting wrote: > Nit: Inconsistent use of |type| vs. |type_| Oh, thanks, just missed the _ Done. https://codereview.chromium.org/12208010/diff/13005/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/13005/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:60: const SkColor kBorderDarkColor = SkColorSetRGB(0xaa, 0xaa, 0xaa); On 2013/02/07 23:55:04, Peter Kasting wrote: > You can't hardcode colors. All colors need to come from the theme/system color > set. Oh, uncleaned code from debugging. I removed it. https://codereview.chromium.org/12208010/diff/13005/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:301: views::Label* menu_label = new views::Label( On 2013/02/07 23:55:04, Peter Kasting wrote: > Nit: I suggest breaking after '=' instead Done. https://codereview.chromium.org/12208010/diff/13005/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:305: new views::MenuButton( On 2013/02/07 23:55:04, Peter Kasting wrote: > Nit: If you want to indent the subsequent line the way you have, put this line > at the end of the previous line Done. https://codereview.chromium.org/12208010/diff/13005/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:322: Profile::FromBrowserContext(web_contents_->GetBrowserContext()); On 2013/02/07 23:55:04, Peter Kasting wrote: > Nit: I'd just inline this into the subsequent statement Done.
https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:58: const int kMediaMenuButtonWidth = 300; On 2013/02/08 12:32:39, xians1 wrote: > We actually don't allow the users to resize the bubble. And even if the bubble > width doubles, we still want to keep the buttons the same size, since the button > should always look like button, and doubling the width of the button might look > pretty odd. So adapting the size of button to the size of bubble might not give > us a good UI. Neither we want to adapt to the length of the menu strings since > they can vary a lot. I'm actually worried about not adapting to the length of the menu strings. It seems from your photos like we could have real problems with long strings getting truncated. What if we did something like the following: The menu widths are the max width of all strings in both menus, except: * We never allow the width to be shorter than some minimum value, say 100 px, except subject to: * We also never allow the menu to get long enough to extend past the right-hand padding on the bubble. This would make sure the menus were even-length, stayed inside the bubble, didn't look broken when containing very short or long strings, but still avoided eliding device names as much as possible. WDYT?
gypi lgtm
Hello Petter, please see answer inline. https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:58: const int kMediaMenuButtonWidth = 300; On 2013/02/09 01:46:34, Peter Kasting wrote: > On 2013/02/08 12:32:39, xians1 wrote: > > We actually don't allow the users to resize the bubble. And even if the bubble > > width doubles, we still want to keep the buttons the same size, since the > button > > should always look like button, and doubling the width of the button might > look > > pretty odd. So adapting the size of button to the size of bubble might not > give > > us a good UI. Neither we want to adapt to the length of the menu strings since > > they can vary a lot. > > I'm actually worried about not adapting to the length of the menu strings. It > seems from your photos like we could have real problems with long strings > getting truncated. > > What if we did something like the following: > > The menu widths are the max width of all strings in both menus, except: > * We never allow the width to be shorter than some minimum value, say 100 px, > except subject to: > * We also never allow the menu to get long enough to extend past the right-hand > padding on the bubble. > > This would make sure the menus were even-length, stayed inside the bubble, > didn't look broken when containing very short or long strings, but still avoided > eliding device names as much as possible. > > WDYT? Thanks for ideas. After thinking a bit, I still think we prefer to having the truncated string rather than a long button. If you look at chrome://settings/content media, you will see we are trying to implement similar menus in the bubble. Actually, the truncated string is not a problem, since users can still see some key words from the truncated strings. And users can always get the whole strings by clicking the menus. One more thing, we have two menus, and the length of strings won't be the same, if one is long, while the other is short, adapt the width of the menus to the long one will make the short one filled with plenty of blank. Also, when users plug in new devices, I am not sure if we want the size of the bubbon changes correspondingly.
Hello Petter, please see answer inline.
https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:58: const int kMediaMenuButtonWidth = 300; On 2013/02/09 14:53:51, xians1 wrote: > Actually, the truncated string is not a problem, since users can still see some > key words from the truncated strings. What if the names of the devices are all identical for the first big chunk? > And users can always get the whole strings > by clicking the menus. It seems graphically nicer to have the menu button width match the menu dropdown width (when possible) than not. > One more thing, we have two menus, and the length of strings won't be the same, > if one is long, while the other is short, adapt the width of the menus to the > long one will make the short one filled with plenty of blank. That doesn't seem like a bad thing to me. I don't think this will look bad. > Also, when users plug in new devices, I am not sure if we want the size of the > bubbon changes correspondingly. That's actually a good point: how do you handle device insertion/removal today? Do you re-read the device list each time the user opens the menu? Or just when the bubble is shown?
We actually made a document for this work : UI, https://docs.google.com/a/google.com/presentation/d/1Ohksd45T-1fjQpdElKx0cRrn... + Glen since the UI design passes his review. Glen, we are having a discussion on how to decide the width of the buttons in the bubble. Peter suggests we should make it adapt to the max length of the strings in the dropdowns: The menu widths are the max width of all strings in both menus, except: * We never allow the width to be shorter than some minimum value, say 100 px, except subject to: * We also never allow the menu to get long enough to extend past the right-hand padding on the bubble. But my understanding is that we are going to make the menu buttons similar to those in content setting media, anyhow, not buttons that might reach the right hand padding of the bubble. Do you have any idea here? We need to make a decision soon in order to deliver the feature for M26. Peter, please also see my comment inline. On Sat, Feb 9, 2013 at 6:42 PM, <pkasting@chromium.org> wrote: > > https://codereview.chromium.**org/12208010/diff/3008/chrome/** > browser/ui/views/content_**setting_bubble_contents.cc<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<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 wrote: > >> Actually, the truncated string is not a problem, since users can still >> > see some > >> key words from the truncated strings. >> > > What if the names of the devices are all identical for the first big > chunk? > > The menus on the bubble is designed for the users to easily select another device without going to content setting. This happens when users want a different device rather than that one being used. That says, most of the users to this UI happens when the media request is allowed, then the users open the bubble to select another device, which will trigger a reload. So even if we have devices with the identical part of strings, it won't confuse the users to stop them clicking on the buttons. > > And users can always get the whole strings >> by clicking the menus. >> > > It seems graphically nicer to have the menu button width match the menu > dropdown width (when possible) than not. > > Even the button might become pretty wide? The names of the devices can be quite long and quite likely that it will reach the right hand padding of the bubble. In this case, the width of the button won't match dropdown any way. I am not sure if it is a nicer UI to have a big button, do we in chrome use similar design for some button menus? > > One more thing, we have two menus, and the length of strings won't be >> > the same, > >> if one is long, while the other is short, adapt the width of the menus >> > to the > >> long one will make the short one filled with plenty of blank. >> > > That doesn't seem like a bad thing to me. I don't think this will look > bad. > > > Also, when users plug in new devices, I am not sure if we want the >> > size of the > >> bubbon changes correspondingly. >> > > That's actually a good point: how do you handle device insertion/removal > today? Do you re-read the device list each time the user opens the > menu? Or just when the bubble is shown? > > As I explained in the first comment, most of the user cases for this UI is that users want to use another device, if open the bubble, plugin a new device, but could not find their target device, they will just close the button, reopen it, then the new device will show up. We can also set up a listener to the devices and always get the menus up-to-date, in case we need it, but this won't be for M26. Thanks for all. BR, SX > https://codereview.chromium.**org/12208010/<https://codereview.chromium.org/1... >
On 2013/02/09 22:13:25, xians1 wrote: > We actually made a document for this work : UI, > https://docs.google.com/a/google.com/presentation/d/1Ohksd45T-1fjQpdElKx0cRrn... > > + Glen since the UI design passes his review. > > Glen, we are having a discussion on how to decide the width of the buttons > in the bubble. Peter suggests we should make it adapt to the max length of > the strings in the dropdowns: > The menu widths are the max width of all strings in both menus, except: > * We never allow the width to be shorter than some minimum value, say 100 > px, except subject to: > * We also never allow the menu to get long enough to extend past the > right-hand padding on the bubble. Given the length of these strings (see http://imgur.com/FmKunEJ) I'm very confident that we will always use the max size. Even the max size available on the bubble for the select buttons is not enough to fit the strings. So I don't think we need to implement any dynamic sizing here. Can't we just start with some fixed size for M26? > > But my understanding is that we are going to make the menu buttons similar > to those in content setting media, anyhow, not buttons that might reach the > right hand padding of the bubble. Given that we already use a fixed sized version of the select buttons on "chrome://settings/content" Can't we simple keep the fixed size for M26? > > Do you have any idea here? We need to make a decision soon in order to > deliver the feature for M26. > > Peter, please also see my comment inline. > > > On Sat, Feb 9, 2013 at 6:42 PM, <mailto:pkasting@chromium.org> wrote: > > > > > https://codereview.chromium.**org/12208010/diff/3008/chrome/** > > > browser/ui/views/content_**setting_bubble_contents.cc<<crxref style="background-image: url(chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png);">https://codereview.chromium.org/12208010/</crxref>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<<crxref style="background-image: url(chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png);">https://codereview.chromium.org/12208010/</crxref>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 wrote: > > > >> Actually, the truncated string is not a problem, since users can still > >> > > see some > > > >> key words from the truncated strings. > >> > > > > What if the names of the devices are all identical for the first big > > chunk? > > > > > The menus on the bubble is designed for the users to easily select another > device without going to content setting. > This happens when users want a different device rather than that one being > used. That says, most of the users to this UI happens when the media > request is allowed, then the users open the bubble to select another > device, which will trigger a reload. > So even if we have devices with the identical part of strings, it won't > confuse the users to stop them clicking on the buttons. > > > > > > > And users can always get the whole strings > >> by clicking the menus. > >> > > > > It seems graphically nicer to have the menu button width match the menu > > dropdown width (when possible) than not. > > > > > Even the button might become pretty wide? The names of the devices can be > quite long and quite likely that it will reach the right hand padding of > the bubble. In this case, the width of the button won't match dropdown any > way. > > I am not sure if it is a nicer UI to have a big button, do we in chrome use > similar design for some button menus? > > > > > > One more thing, we have two menus, and the length of strings won't be > >> > > the same, > > > >> if one is long, while the other is short, adapt the width of the menus > >> > > to the > > > >> long one will make the short one filled with plenty of blank. > >> > > > > That doesn't seem like a bad thing to me. I don't think this will look > > bad. > > > > > > Also, when users plug in new devices, I am not sure if we want the > >> > > size of the > > > >> bubbon changes correspondingly. > >> > > > > That's actually a good point: how do you handle device insertion/removal > > today? Do you re-read the device list each time the user opens the > > menu? Or just when the bubble is shown? > > > > > As I explained in the first comment, most of the user cases for this UI is > that users want to use another device, if open the bubble, plugin a new > device, but could not find their target device, they will just close the > button, reopen it, then the new device will show up. > > We can also set up a listener to the devices and always get the menus > up-to-date, in case we need it, but this won't be for M26. > > Thanks for all. > > BR, > SX > > > > > https://codereview.chromium.**org/12208010/%3Chttps://codereview.chromium.org...> > >
On 2013/02/11 15:39:41, markusheintz_ wrote: > Given the length of these strings (see http://imgur.com/FmKunEJ) I'm very > confident that we will always use the max size. These strings make the small size in this CL look like a worse idea than ever, considering they're basically identical for the first long period. How were you planning on eliding the strings? It seems like elide-in-middle or elide-common-portion (or some combination thereof) might be a good idea. > Given that we already use a fixed sized version of the select buttons on > "chrome://settings/content" And what says that that UI is correct either? I'm being nitpicky because it's you guys' job as implementers to make whatever UI you implement _perfect_, the best it can be, better than any mocks you were given and better than any other UI in Chrome. That ought to be your standard. I'm trying to get you to hold yourself to that. It's not as important in the end what we do for M6 versus M27 or whatever, as it is that we get Chrome's UI to be the best it can be. I want you to figure out what the ideal UI would be and from there it's easier to work backwards to decide how to implement what when. (Also, never let yourself feel pressure to get a feature into a particular milestone. We ship rapid releases precisely so that you will never have an excuse to do something quick-and-dirty as opposed to just getting it right from the start. Features should make whatever release they're ready for.) Once it's clear you guys are approaching the UI from this perspective, if at that point you decide you need to break the implementation into pieces for different milestones, I don't think that will be too much of a problem.
gtk lgtm https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/cont... File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (right): https://codereview.chromium.org/12208010/diff/3008/chrome/browser/ui/gtk/cont... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:85: it->second->label.Destroy(); On 2013/02/07 16:25:19, xians1 wrote: > On 2013/02/06 22:07:07, Evan Stade wrote: > > I don't think this should be necessary, and instead of using a for loop you > > should be able to call STLMapDestroy or something like that. > > I changed the code to use STLDeleteValues. > The comment in OwnedWidgetGtk::Own() is very confusing, it clearly states that: > It is valid to never call Own(), in which case Destroy() will do nothing. If > Own() has been called, you must explicitly call Destroy(). > But the destructor of OwnedWidgetGtk is calling Destroy(), so I suppose the > comment is not valid anymore, right? yea, sounds like it. https://codereview.chromium.org/12208010/diff/23020/chrome/browser/ui/gtk/con... File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (right): https://codereview.chromium.org/12208010/diff/23020/chrome/browser/ui/gtk/con... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:408: } can put } on previous line, here and above.
Peter, I have already addressed the dynamic size and alignment for the buttons. please take another look. BR, SX
Seems mostly in good shape https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:742: camera_menu.default_device = GetMediaDeviceById(preferred_camera, Nit: I suggest breaking after '=' instead of ',' https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:754: content::MediaStreamType type, const std::string& selected_device_id) { Nit: One arg per line https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:27: BuildMenu(); I don't think there's much value in splitting this out of the constructor. You never call it from anywhere else, and the code within it can't fail. I'd just inline it here. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.h (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.h:31: Nit: Blank line probably not necessary https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.h:34: // ui::SimpleMenuModel::Delegate implementation: Nit: I suggest just "// ui::SimpleMenuModel::Delegate:" (slowly trying to standardize on this form elsewhere) https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.h:45: // Internal method to build the menu. Nit: This comment adds nothing. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.h:53: // Map of command IDs to devices. Nit: This comment also adds very little. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:59: // The width of the media menu button. Nit: width -> minimum width Also, I'm a fan of defining these constants locally where they're used, assuming they're only used within one function, as opposed to at the top of the file. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:67: class ContentSettingBubbleContents::Favicon : public views::ImageView { Nit: Since you have a number of classes defined in this file, consider putting dividers between each, as we do elsewhere: ... previous code ... // ClassName --------------------------... ... code for class ... Stylistically, we generally make the "--"s extend out to 79 characters, and we generally have two blank lines above each divider and one below. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:311: menu_button->SetEnabledColor(SK_ColorBLACK); Why do we need to set all these? Don't the default colors work? I always worry when I see people hardcoding colors, that it will cause some clash with themes or system colors in some case. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:325: // Calculate the optimal width for the menu buttons. Nit: Maybe instead of "optimal width" you could say what we're ultimately calculating (i.e. the width required by the longest item in any of the menus), and that we'll ultimately set all the menus to that width below. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:331: int avaialble_width = Nit: typo https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:336: menu_width = menu_width <= kMinMediaMenuButtonWidth ? Nit: You can write this more simply using std::max(); plus, I think this isn't quite correct, in that I think if |available_width| is smaller than kMinMediaMenuButtonWidth, you want to use |available_width|, lest you extend outside the bubble: menu_width = std::min(std::max(menu_width, kMinMediaMenuButtonWidth), available_width); https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:346: // Set the optimal width for the menu buttons. Nit: Perhaps "// Now set all the menus to the width we calculated above." https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:479: views::MenuButton* button, ui::SimpleMenuModel* menu_model) { Nit: One arg per line https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:480: // Store the current title for the button. Nit: The comment here and below are unnecessary, the code alone is clear. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:487: width = width > item_width ? width : item_width; Nit: Simpler: width = std::max(width, button->GetPreferredSize().width()); https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.h:50: : public views::BubbleDelegateView, Nit: Rewrapping doesn't look to be necessary, I think you can leave the indentation as it was. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.h:78: Nit: Blank line unnecessary
https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... 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: > Nit: I suggest breaking after '=' instead of ',' Done. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:754: content::MediaStreamType type, const std::string& selected_device_id) { On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: One arg per line Done. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:27: BuildMenu(); On 2013/02/16 02:55:13, Peter Kasting wrote: > I don't think there's much value in splitting this out of the constructor. You > never call it from anywhere else, and the code within it can't fail. I'd just > inline it here. Done. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.h (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.h:31: On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: Blank line probably not necessary Done. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.h:34: // ui::SimpleMenuModel::Delegate implementation: On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: I suggest just "// ui::SimpleMenuModel::Delegate:" (slowly trying to > standardize on this form elsewhere) Done. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.h:45: // Internal method to build the menu. On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: This comment adds nothing. Removed. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.h:53: // Map of command IDs to devices. On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: This comment also adds very little. Removed. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:59: // The width of the media menu button. On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: width -> minimum width > > Also, I'm a fan of defining these constants locally where they're used, assuming > they're only used within one function, as opposed to at the top of the file. Done changing the comment to minimum width. Regarding to a static local constant or a constant inside the namespace, I think both are OK, but since this file puts all constants inside the namespace on top, I will prefer to following the existing way. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:67: class ContentSettingBubbleContents::Favicon : public views::ImageView { On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: Since you have a number of classes defined in this file, consider putting > dividers between each, as we do elsewhere: > > ... previous code ... > > > // ClassName --------------------------... > > ... code for class ... > > Stylistically, we generally make the "--"s extend out to 79 characters, and we > generally have two blank lines above each divider and one below. Done. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:311: menu_button->SetEnabledColor(SK_ColorBLACK); On 2013/02/16 02:55:13, Peter Kasting wrote: > Why do we need to set all these? Don't the default colors work? > > I always worry when I see people hardcoding colors, that it will cause some > clash with themes or system colors in some case. It also works by using the default. I used those specific values since they were used in other places. Removed the hardcoded colors code. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:325: // Calculate the optimal width for the menu buttons. On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: Maybe instead of "optimal width" you could say what we're ultimately > calculating (i.e. the width required by the longest item in any of the menus), > and that we'll ultimately set all the menus to that width below. Done with updating the comment. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:331: int avaialble_width = On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: typo Done. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:336: menu_width = menu_width <= kMinMediaMenuButtonWidth ? On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: You can write this more simply using std::max(); plus, I think this isn't > quite correct, in that I think if |available_width| is smaller than > kMinMediaMenuButtonWidth, you want to use |available_width|, lest you extend > outside the bubble: > Done with the suggested change. But just want to make sure it is what we want, when the minimum width is bigger than the available width, don't we want to use the minimum width and force the layout to get bigger instead? > menu_width = std::min(std::max(menu_width, kMinMediaMenuButtonWidth), > available_width); https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:346: // Set the optimal width for the menu buttons. On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: Perhaps "// Now set all the menus to the width we calculated above." Done. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:479: views::MenuButton* button, ui::SimpleMenuModel* menu_model) { On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: One arg per line Done. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:480: // Store the current title for the button. On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: The comment here and below are unnecessary, the code alone is clear. Done. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:487: width = width > item_width ? width : item_width; On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: Simpler: > > width = std::max(width, button->GetPreferredSize().width()); Done. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.h:50: : public views::BubbleDelegateView, On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: Rewrapping doesn't look to be necessary, I think you can leave the > indentation as it was. I think this style is used more in chrome: class A : public B, public C, ... { And class A : public B { is mostly used when there is only one inheritance. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.h:78: On 2013/02/16 02:55:13, Peter Kasting wrote: > Nit: Blank line unnecessary Done.
https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.h:50: : public views::BubbleDelegateView, On 2013/02/18 17:20:04, xians1 wrote: > On 2013/02/16 02:55:13, Peter Kasting wrote: > > Nit: Rewrapping doesn't look to be necessary, I think you can leave the > > indentation as it was. > > I think this style is used more in chrome: > class A > : public B, > public C, > ... { > > And class A : public B { is mostly used when there is only one inheritance. No! If it fits, then just do: class Foo : public Listener1, public Listener2, public Observer1, public Observer2, public .... { ... };
On 2013/02/18 17:33:50, tfarina wrote: > https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... > File chrome/browser/ui/views/content_setting_bubble_contents.h (right): > > https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... > chrome/browser/ui/views/content_setting_bubble_contents.h:50: : public > views::BubbleDelegateView, > On 2013/02/18 17:20:04, xians1 wrote: > > On 2013/02/16 02:55:13, Peter Kasting wrote: > > > Nit: Rewrapping doesn't look to be necessary, I think you can leave the > > > indentation as it was. > > > > I think this style is used more in chrome: > > class A > > : public B, > > public C, > > ... { > > > > And class A : public B { is mostly used when there is only one inheritance. > > No! > > If it fits, then just do: > > ... > }; I might be wrong but that is what I got from my code review experience. Do you have any link or proof on the following style is preferred? > class Foo : public Listener1, > public Listener2, > public Observer1, > public Observer2, > public .... {
On Mon, Feb 18, 2013 at 6:54 PM, <xians@chromium.org> wrote: > On 2013/02/18 17:33:50, tfarina wrote: > > https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... >> >> File chrome/browser/ui/views/content_setting_bubble_contents.h (right): > > > > https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... >> >> chrome/browser/ui/views/content_setting_bubble_contents.h:50: : public >> views::BubbleDelegateView, >> On 2013/02/18 17:20:04, xians1 wrote: >> > On 2013/02/16 02:55:13, Peter Kasting wrote: >> > > Nit: Rewrapping doesn't look to be necessary, I think you can leave >> > > the >> > > indentation as it was. >> > >> > I think this style is used more in chrome: >> > class A >> > : public B, >> > public C, >> > ... { >> > >> > And class A : public B { is mostly used when there is only one >> > inheritance. > > >> No! > > >> If it fits, then just do: > > >> ... >> }; > > > I might be wrong but that is what I got from my code review experience. > > Do you have any link or proof on the following style is preferred? I've seen Thiago's style much more often too. It's also what clang-format -style=Chromium produces. > > >> class Foo : public Listener1, >> public Listener2, >> public Observer1, >> public Observer2, >> public .... { > > > > https://codereview.chromium.org/12208010/
https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:59: // The width of the media menu button. On 2013/02/18 17:20:04, xians1 wrote: > On 2013/02/16 02:55:13, Peter Kasting wrote: > > Also, I'm a fan of defining these constants locally where they're used, > assuming > > they're only used within one function, as opposed to at the top of the file. > > Regarding to a static local constant or a constant inside the namespace, I think > both are OK, but since this file puts all constants inside the namespace on top, > I will prefer to following the existing way. Yeah, I was really suggesting fixing all of these as appropriate. https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:336: menu_width = menu_width <= kMinMediaMenuButtonWidth ? On 2013/02/18 17:20:04, xians1 wrote: > On 2013/02/16 02:55:13, Peter Kasting wrote: > > Nit: You can write this more simply using std::max(); plus, I think this isn't > > quite correct, in that I think if |available_width| is smaller than > > kMinMediaMenuButtonWidth, you want to use |available_width|, lest you extend > > outside the bubble: > > > > Done with the suggested change. > > But just want to make sure it is what we want, when the minimum width is bigger > than the available width, don't we want to use the minimum width and force the > layout to get bigger instead? > > > menu_width = std::min(std::max(menu_width, kMinMediaMenuButtonWidth), > > available_width); ? You're just restating what I said: Me: "if |available_width| is smaller than kMinMediaMenuButtonWidth..." You: "when the minimum width is bigger than the available width..." Did you mistype? If we don't have room for the minimum width, then clearly we don't want to force things to the minimum width, or they'll escape the bubble. Unless the preferred size depends on the menu width? In which case making the menus larger is fine and we'll just enlarge the whole bubble, which suggests in turn that we shouldn't even be checking the available width. I don't know, how does layout here work? https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.h:50: : public views::BubbleDelegateView, On 2013/02/18 17:33:50, tfarina wrote: > On 2013/02/18 17:20:04, xians1 wrote: > > On 2013/02/16 02:55:13, Peter Kasting wrote: > > > Nit: Rewrapping doesn't look to be necessary, I think you can leave the > > > indentation as it was. > > > > I think this style is used more in chrome: > > class A > > : public B, > > public C, > > ... { > > > > And class A : public B { is mostly used when there is only one inheritance. > > No! > > If it fits, then just do: > > class Foo : public Listener1, > public Listener2, > public Observer1, > public Observer2, > public .... { > ... > }; Thiago is correct. There's no reason to rewrap to a 4-space indent when you don't need to. If things fit, you shouldn't take the extra vertical line unnecessarily. https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:30: std::string default_device; What do we even use this for? It looks like we set it but never use it. https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/gtk/con... File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (right): https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/gtk/con... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:261: // Calculate the optimal width for the menu buttons. You should apply my comments about the views code to the GTK code as well. https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:68: // Favicon -------------------------------------------------------------------- Nit: I would write the class name as "ContentSettingBubbleContents::Favicon" since it's a member class. (2 places) https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:132: gfx::NativeCursor ContentSettingBubbleContents::Favicon::GetCursor( This function needs to go with the rest of the class. https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:142: ContentSettingBubbleContents::ContentSettingBubbleContents( You should have a divider above this. https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:341: available_width); This code isn't right. The available_width you calculate is changing each iteration because max_menu_label_width is changing each iteration. Plus even if this were right it's still inefficient. You should remove all the stuff about min widths and available widths from here. Only keep track of the max label width. Then after the end of the loop, do any further calculations a single time. Furthermore I don't even understand what the available_width is calculating. You already subtract max_menu_label_width when calculating it so you're not actually calculating the amount of space available for a label. I'm confused.
https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/29001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:336: menu_width = menu_width <= kMinMediaMenuButtonWidth ? On 2013/02/18 18:23:18, Peter Kasting wrote: > On 2013/02/18 17:20:04, xians1 wrote: > > On 2013/02/16 02:55:13, Peter Kasting wrote: > > > Nit: You can write this more simply using std::max(); plus, I think this > isn't > > > quite correct, in that I think if |available_width| is smaller than > > > kMinMediaMenuButtonWidth, you want to use |available_width|, lest you extend > > > outside the bubble: > > > > > > > Done with the suggested change. > > > > But just want to make sure it is what we want, when the minimum width is > bigger > > than the available width, don't we want to use the minimum width and force the > > layout to get bigger instead? > > > > > menu_width = std::min(std::max(menu_width, kMinMediaMenuButtonWidth), > > > available_width); > > ? You're just restating what I said: > > Me: "if |available_width| is smaller than kMinMediaMenuButtonWidth..." > You: "when the minimum width is bigger than the available width..." > > Did you mistype? > No, I asked the question precisely how we should handle the case. Since personally I think we should set it to minimum width anyhow. And this will only enlarge the bubble. > If we don't have room for the minimum width, then clearly we don't want to force > things to the minimum width, or they'll escape the bubble. > > Unless the preferred size depends on the menu width? In which case making the > menus larger is fine and we'll just enlarge the whole bubble, which suggests in > turn that we shouldn't even be checking the available width. I don't know, how > does layout here work? All right, I thought you knew how the layout work here. The menus won't escape the bubble in any case, when we set them to a width more than the existing layout, the layout will automatically become bigger to fit the menus. So I think we both agree that we should just use the longest width for the menu here, in favor of avoiding the truncated strings. https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:30: std::string default_device; On 2013/02/18 18:23:18, Peter Kasting wrote: > What do we even use this for? It looks like we set it but never use it. It was used to marked the default device as a checked item, then I changed my mind not to check it since we use the title of the menu button to show the default device instead. Removed now. https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/gtk/con... File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (right): https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/gtk/con... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:261: // Calculate the optimal width for the menu buttons. On 2013/02/18 18:23:18, Peter Kasting wrote: > You should apply my comments about the views code to the GTK code as well. Done. https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:68: // Favicon -------------------------------------------------------------------- On 2013/02/18 18:23:18, Peter Kasting wrote: > Nit: I would write the class name as "ContentSettingBubbleContents::Favicon" > since it's a member class. (2 places) Done. https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:132: gfx::NativeCursor ContentSettingBubbleContents::Favicon::GetCursor( On 2013/02/18 18:23:18, Peter Kasting wrote: > This function needs to go with the rest of the class. Done. https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:142: ContentSettingBubbleContents::ContentSettingBubbleContents( On 2013/02/18 18:23:18, Peter Kasting wrote: > You should have a divider above this. Done. https://codereview.chromium.org/12208010/diff/46001/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:341: available_width); On 2013/02/18 18:23:18, Peter Kasting wrote: > This code isn't right. The available_width you calculate is changing each > iteration because max_menu_label_width is changing each iteration. Plus even if > this were right it's still inefficient. > > You should remove all the stuff about min widths and available widths from here. > Only keep track of the max label width. Then after the end of the loop, do any > further calculations a single time. > > Furthermore I don't even understand what the available_width is calculating. > You already subtract max_menu_label_width when calculating it so you're not > actually calculating the amount of space available for a label. I'm confused. The UI looks like that Microphone: menu_button Camera: menu_button The |menu_label| in the code mean Microphone: or Camera:, not the label on top of the menu. I have already changed the name to avoid confusion on this. The available width is calculating the pixel from the beginning of the menu button til the right padding of the bubble - kRelatedControlHorizontalSpacing. I also agree that we don't need to calculate the menu_width within the iteration, though the result might be the same. Anyhow, the available_width calculation has been removed from the new upload.
LGTM except for one substantive comment. https://codereview.chromium.org/12208010/diff/46004/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/46004/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:29: devices = dispatcher->GetVideoCaptureDevices(); Nit: To do initialization at declaration time, how about: content::MediaStreamDevices devices = (type_ == content::MEDIA_DEVICE_AUDIO_CAPTURE) ? dispatcher->GetAudioCaptureDevices() : dispatcher->GetVideoCaptureDevices(); https://codereview.chromium.org/12208010/diff/46004/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/46004/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:119: Nit: One more blank line https://codereview.chromium.org/12208010/diff/46004/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:328: menu_button, menu_view->menu_model.get())); Cool, this is much simpler. We may want to put a clamp on the maximum absolute size of the menu. I worry about lame Linux strings creating a bubble that is wider than the screen. Perhaps we should pick some value, say 600 px, to clamp to below this loop.
Thanks for all. I am going to land it now. https://codereview.chromium.org/12208010/diff/46004/chrome/browser/ui/content... File chrome/browser/ui/content_settings/content_setting_media_menu_model.cc (right): https://codereview.chromium.org/12208010/diff/46004/chrome/browser/ui/content... chrome/browser/ui/content_settings/content_setting_media_menu_model.cc:29: devices = dispatcher->GetVideoCaptureDevices(); On 2013/02/20 05:46:20, Peter Kasting wrote: > Nit: To do initialization at declaration time, how about: > > content::MediaStreamDevices devices = > (type_ == content::MEDIA_DEVICE_AUDIO_CAPTURE) ? > dispatcher->GetAudioCaptureDevices() : > dispatcher->GetVideoCaptureDevices(); Done. https://codereview.chromium.org/12208010/diff/46004/chrome/browser/ui/views/c... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/12208010/diff/46004/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:119: On 2013/02/20 05:46:20, Peter Kasting wrote: > Nit: One more blank line Done. https://codereview.chromium.org/12208010/diff/46004/chrome/browser/ui/views/c... chrome/browser/ui/views/content_setting_bubble_contents.cc:328: menu_button, menu_view->menu_model.get())); On 2013/02/20 05:46:20, Peter Kasting wrote: > Cool, this is much simpler. > > We may want to put a clamp on the maximum absolute size of the menu. I worry > about lame Linux strings creating a bubble that is wider than the screen. > Perhaps we should pick some value, say 600 px, to clamp to below this loop. Done. I added a maximum width (600px) for the menus in both linux and windows code.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/12208010/54001
Retried try job too often on win_rel for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/12208010/54001
Message was sent while issue was closed.
Change committed as 183597 |
