Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1661063002:
Add message and Help Center link to the chooser UI (Closed)
|
Created:
4 years, 10 months ago by juncai Modified:
4 years, 10 months ago CC:
ortuno, chromium-reviews, Jeffrey Yasskin, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, scheib, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd message and Help Center link to the chooser UI
This patch added message "Not seeing your device? Get
help" at the bottom of the chooser UI, and the "Get help"
links to a Help Center URL. The message is added below
the "Connect" and "Cancel" button with a grey separator.
BUG=492204, 583479
Committed: https://crrev.com/e26da162b7de08b3918352deff9f5897f1e2cf2a
Cr-Commit-Position: refs/heads/master@{#377216}
Patch Set 1 : add privacy message to the chooser UI #Patch Set 2 : added chooser UI comments #
Total comments: 2
Patch Set 3 : updated message description #Patch Set 4 : updated comments #Patch Set 5 : added get help link for chooser UI #Patch Set 6 : address rolfe@'s comments about the chooser UI #Patch Set 7 : updated kChooserUIGetHelpURL string constant #Patch Set 8 : added GetHelpCenterURL function to ChooserBubbleDelegate #Patch Set 9 : added OpenHelpCenterURL function to ChooserBubbleDelegate #
Total comments: 15
Patch Set 10 : fixed some typos #
Total comments: 20
Patch Set 11 : address comments #Patch Set 12 : cleaned up code #
Total comments: 27
Patch Set 13 : address comments #Patch Set 14 : removed debug code #Patch Set 15 : added GetOptionDescription virtual function to ChooserBubbleDelegate #
Total comments: 6
Patch Set 16 : address jyasskin@'s comments #Patch Set 17 : merged changes from master and resolved conflicts #
Total comments: 10
Patch Set 18 : address estade@'s comments #Dependent Patchsets: Messages
Total messages: 53 (19 generated)
juncai@chromium.org changed reviewers: + reillyg@chromium.org
Please review this patch.
https://codereview.chromium.org/1661063002/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1661063002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:14607: + <message name="IDS_CHOOSER_BUBBLE_PRIVACY_MESSAGE_TEXT" desc="The text that is shown on the bottom of the chooser popup."> This description doesn't give the translator enough context to understand the message. Please read this guide for creating good message descriptions: https://www.chromium.org/developers/design-documents/ui-localization?pli=1#TO... I suggest, "This text is shown at the bottom of the chooser popup and informs the user of the privacy implications of the choice they will be making."
Description was changed from ========== add privacy message to the chooser UI This patch added privacy message to the chooser UI. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ========== to ========== Add privacy message to the chooser UI This patch added privacy message to the chooser UI. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ==========
https://codereview.chromium.org/1661063002/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1661063002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:14607: + <message name="IDS_CHOOSER_BUBBLE_PRIVACY_MESSAGE_TEXT" desc="The text that is shown on the bottom of the chooser popup."> On 2016/02/03 18:49:08, Reilly Grant wrote: > This description doesn't give the translator enough context to understand the > message. Please read this guide for creating good message descriptions: > https://www.chromium.org/developers/design-documents/ui-localization?pli=1#TO... > > I suggest, "This text is shown at the bottom of the chooser popup and informs > the user of the privacy implications of the choice they will be making." Done.
Looks reasonable to me but I'm not a //chrome/browser/ui/views/website_settings OWNER.
juncai@chromium.org changed reviewers: + palmer@chromium.org
palmer@chromium.org: Please review changes in chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc TBR=grt@chromium.org
Description was changed from ========== Add privacy message to the chooser UI This patch added privacy message to the chooser UI. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ========== to ========== Add privacy message to the chooser UI This patch added privacy message to the chooser UI. The message is added below the "Connect" and "Cancel" button with a grey separator. TBR=grt@chromium.org BUG=492204, 583479 ==========
Description was changed from ========== Add privacy message to the chooser UI This patch added privacy message to the chooser UI. The message is added below the "Connect" and "Cancel" button with a grey separator. TBR=grt@chromium.org BUG=492204, 583479 ========== to ========== Add privacy message to the chooser UI This patch added privacy message to the chooser UI. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ==========
Can you please post a screenshot of what it looks like on the bug?
On 2016/02/05 20:11:26, palmer wrote: > Can you please post a screenshot of what it looks like on the bug? I posted a screenshot at bug 583497 page: https://code.google.com/p/chromium/issues/detail?id=583479
On 2016/02/05 20:34:00, juncai wrote: > On 2016/02/05 20:11:26, palmer wrote: > > Can you please post a screenshot of what it looks like on the bug? > > I posted a screenshot at bug 583497 page: > https://code.google.com/p/chromium/issues/detail?id=583479 typo: bug 583497 should be: bug 583479.
LGTM (thanks!). I added some relevant UX people to the bug; maybe wait on landing this CL until they weigh in. (Or, if they already have, then go for it.)
Please review the changes. I uploaded a screenshot at: https://code.google.com/p/chromium/issues/detail?id=583479
Description was changed from ========== Add privacy message to the chooser UI This patch added privacy message to the chooser UI. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ========== to ========== Add privacy message to the chooser UI This patch added message "Not seeing your device? Get help" at the bottom of the chooser UI, and the "Get help" links to a Helper Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ==========
Description was changed from ========== Add privacy message to the chooser UI This patch added message "Not seeing your device? Get help" at the bottom of the chooser UI, and the "Get help" links to a Helper Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ========== to ========== Add privacy message to the chooser UI This patch added message "Not seeing your device? Get help" at the bottom of the chooser UI, and the "Get help" links to a Helper Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ==========
Description was changed from ========== Add privacy message to the chooser UI This patch added message "Not seeing your device? Get help" at the bottom of the chooser UI, and the "Get help" links to a Helper Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ========== to ========== Add message and Helper Center link to the chooser UI This patch added message "Not seeing your device? Get help" at the bottom of the chooser UI, and the "Get help" links to a Helper Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ==========
Description was changed from ========== Add message and Helper Center link to the chooser UI This patch added message "Not seeing your device? Get help" at the bottom of the chooser UI, and the "Get help" links to a Helper Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ========== to ========== Add message and Helper Center link to the chooser UI This patch added message "Not seeing your device? Get help." at the bottom of the chooser UI, and the "Get help." links to a Helper Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ==========
juncai@chromium.org changed reviewers: + msw@chromium.org
msw@chromium.org: Please review changes in //chrome/browser/ui/bluetooth/ palmer@chromium.org: Please review changes in //chrome/browser/ui/views/website_settings/ //chrome/browser/ui/website_settings/ reillyg@chromium.org: Please review changes in //chrome/browser/usb/
msw@chromium.org changed reviewers: + estade@chromium.org
+estade for weird bubble (see 583479) https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:58: const int kTextLabelSpacing = 3; Try to use ui/views/layout/layout_constants.h here and elsewhere. https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:161: // ------------------------------------ This looks so close to DialogClientView (content, ok/cancel, footnote view), I would prefer to see that re-used here instead of partially rebuilt. https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:171: // | [ Connect] [ Cancel ] | nit: space after connect... https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:173: // | Not seeing your device? Get help | Is "device" relevant terminology for every chooser bubble? https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:249: // Lay out the message label and link. Putting the link right of the text isn't always correct (think about internationalization). Use StyledLabel, I guess. https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:161: // ------------------------------------ Why is this bubble a darker gray than others? https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:273: get_help_link_->SetEnabledColor(SkColorSetRGB(66, 133, 255)); Where did this color come from? Can you use something from the native theme? https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_bubble_delegate.cc:166: return GURL(chrome::kWebUsbHelpCenterURL); Does it make sense to link to a general support url for now?
jyasskin@chromium.org changed reviewers: + jyasskin@chromium.org
https://codereview.chromium.org/1661063002/diff/180001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1661063002/diff/180001/chrome/app/generated_r... chrome/app/generated_resources.grd:14572: + Get help Putting two strings next to each other tends to be bad for internationalization. I don't have a concrete example where this one will cause problems, but it's dangerous in general. It looks like views::StyledLabel is the class to use for labels where part of the string is supposed to be linked. https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ch... has an example of doing this sort of embedding. It's not quite as nice as Android's parsed messages (https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/android/ja... and https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/android/ja...), but it's better than freezing the order in code. https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:23: GetBrowser()->OpenURL( It seems wrong to conjure a browser window out of nothing. Shouldn't it be the same window as the bubble's showing in? https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:24: content::OpenURLParams(url, content::Referrer(), NEW_BACKGROUND_TAB, I think we want this to be a FOREGROUND_TAB, to make sure the user sees it. (OMG, who decided that enum didn't need comments??) https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:25: ui::PAGE_TRANSITION_AUTO_TOPLEVEL, true)); Always comment naked boolean arguments like this. I usually use /*arg_name=*/true, but I don't care much as long as we can tell what "true" means.
https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:23: GetBrowser()->OpenURL( On 2016/02/17 01:33:45, Jeffrey Yasskin wrote: > It seems wrong to conjure a browser window out of nothing. Shouldn't it be the > same window as the bubble's showing in? FWIW: The What Do These Mean? link in the (soon to go away) Connection Tab in the Origin Info Bubble opens a new (foreground) tab. I don't know if that's the best thing, but it seems bad to destroy/lose any context in the current tab/window. https://codereview.chromium.org/1661063002/diff/180001/chrome/common/url_cons... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/1661063002/diff/180001/chrome/common/url_cons... chrome/common/url_constants.cc:766: // TODO(juncai): Change to a WebUsb help center URL when one is available. Attach a specific bug ID to this TODO. We definitely wouldn't want to ship to stable and still be pointing to this generic help page.
ortuno@chromium.org changed reviewers: + ortuno@chromium.org
https://codereview.chromium.org/1661063002/diff/180001/chrome/common/url_cons... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/1661063002/diff/180001/chrome/common/url_cons... chrome/common/url_constants.cc:763: const char kWebBluetoothHelpCenterURL[] = Would you mind changing this to "kChooserBluetoothOverviewURL"? We have many links in our android chooser so it would help if the names are more specific.
https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:58: const int kTextLabelSpacing = 3; On 2016/02/17 01:31:35, msw wrote: > Try to use ui/views/layout/layout_constants.h here and elsewhere. Done. https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:161: // ------------------------------------ On 2016/02/17 01:31:35, msw wrote: > This looks so close to DialogClientView (content, ok/cancel, footnote view), I > would prefer to see that re-used here instead of partially rebuilt. I filed a bug and added a TODO here to try it in a new patch once this patch is done. https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:171: // | [ Connect] [ Cancel ] | On 2016/02/17 01:31:35, msw wrote: > nit: space after connect... Done. https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:173: // | Not seeing your device? Get help | On 2016/02/17 01:31:35, msw wrote: > Is "device" relevant terminology for every chooser bubble? It is relevant for the two components that use this UI now: WebUsb and WebBluetooth. This is part of the UI that was discussed at: http://code.google.com/p/chromium/issues/detail?id=583479 https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:249: // Lay out the message label and link. On 2016/02/17 01:31:35, msw wrote: > Putting the link right of the text isn't always correct (think about > internationalization). Use StyledLabel, I guess. Done. https://codereview.chromium.org/1661063002/diff/180001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1661063002/diff/180001/chrome/app/generated_r... chrome/app/generated_resources.grd:14572: + Get help On 2016/02/17 01:33:45, Jeffrey Yasskin wrote: > Putting two strings next to each other tends to be bad for internationalization. > I don't have a concrete example where this one will cause problems, but it's > dangerous in general. > > It looks like views::StyledLabel is the class to use for labels where part of > the string is supposed to be linked. > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ch... > has an example of doing this sort of embedding. It's not quite as nice as > Android's parsed messages > (https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/android/ja... > and > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/android/ja...), > but it's better than freezing the order in code. Done. https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:161: // ------------------------------------ On 2016/02/17 01:31:35, msw wrote: > Why is this bubble a darker gray than others? The chooser bubble color is the same color as other permission bubble such as geolocation. https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:273: get_help_link_->SetEnabledColor(SkColorSetRGB(66, 133, 255)); On 2016/02/17 01:31:35, msw wrote: > Where did this color come from? Can you use something from the native theme? Done. https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:23: GetBrowser()->OpenURL( On 2016/02/17 02:19:53, palmer wrote: > On 2016/02/17 01:33:45, Jeffrey Yasskin wrote: > > It seems wrong to conjure a browser window out of nothing. Shouldn't it be the > > same window as the bubble's showing in? > > FWIW: The What Do These Mean? link in the (soon to go away) Connection Tab in > the Origin Info Bubble opens a new (foreground) tab. I don't know if that's the > best thing, but it seems bad to destroy/lose any context in the current > tab/window. Since ChooserBubbleDelegate has a Browser* member variable, I use it to open the URL instead. https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:24: content::OpenURLParams(url, content::Referrer(), NEW_BACKGROUND_TAB, On 2016/02/17 01:33:45, Jeffrey Yasskin wrote: > I think we want this to be a FOREGROUND_TAB, to make sure the user sees it. > (OMG, who decided that enum didn't need comments??) Done. https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:25: ui::PAGE_TRANSITION_AUTO_TOPLEVEL, true)); On 2016/02/17 01:33:45, Jeffrey Yasskin wrote: > Always comment naked boolean arguments like this. I usually use > /*arg_name=*/true, but I don't care much as long as we can tell what "true" > means. Done. https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/usb/usb... File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/usb/usb... chrome/browser/usb/usb_chooser_bubble_delegate.cc:166: return GURL(chrome::kWebUsbHelpCenterURL); On 2016/02/17 01:31:35, msw wrote: > Does it make sense to link to a general support url for now? A WebUsb help center URL has been requested, once the URL is available, I will change it to that URL. I filed a bug to track it: https://crbug.com/587525 https://codereview.chromium.org/1661063002/diff/180001/chrome/common/url_cons... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/1661063002/diff/180001/chrome/common/url_cons... chrome/common/url_constants.cc:763: const char kWebBluetoothHelpCenterURL[] = On 2016/02/17 16:02:59, ortuno wrote: > Would you mind changing this to "kChooserBluetoothOverviewURL"? We have many > links in our android chooser so it would help if the names are more specific. Done. https://codereview.chromium.org/1661063002/diff/180001/chrome/common/url_cons... chrome/common/url_constants.cc:766: // TODO(juncai): Change to a WebUsb help center URL when one is available. On 2016/02/17 02:19:53, palmer wrote: > Attach a specific bug ID to this TODO. We definitely wouldn't want to ship to > stable and still be pointing to this generic help page. Done.
lgtm with nits and minor comments. https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:173: // | Not seeing your device? Get help | On 2016/02/17 22:40:39, juncai wrote: > On 2016/02/17 01:31:35, msw wrote: > > Is "device" relevant terminology for every chooser bubble? > > It is relevant for the two components that use this UI now: WebUsb and > WebBluetooth. This is part of the UI that was discussed at: > http://code.google.com/p/chromium/issues/detail?id=583479 Okay, perhaps this should be a DeviceChooserBubbleUiViewDelegate... ha. https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... chrome/app/generated_resources.grd:14571: + <message name="IDS_CHOOSER_BUBBLE_GET_HELP_LINK_TEXT" desc="Text of the Get help link at the bottom of the chooser popup."> nit: 'Get help' https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... chrome/app/generated_resources.grd:14572: + Get help Maybe use IDS_GET_HELP? https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:175: layout->StartRow(0, 0); Should the table's row be resizable? (if the bubble is shorter than expected?) https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:233: new views::Separator(views::Separator::HORIZONTAL); nit: inline in AddView() call below; remove |separator|. https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:249: GetNativeTheme()->GetSystemColor(ui::NativeTheme::kColorId_LinkEnabled); aside: odd that CreateForLink doesn't do this.
hope you don't mind the driveby, I've been doing a bit of bubble cleanup of late. https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:173: views::GridLayout::USE_PREF, 0, 0); so |layout| only has one column set with one column in it... this should be a box layout https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:204: button_columns->AddColumn(views::GridLayout::TRAILING, this also seems like it should be a (horizontal) box layout https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:249: GetNativeTheme()->GetSystemColor(ui::NativeTheme::kColorId_LinkEnabled); I don't think you can call GetNativeTheme() yet (and get a good return value) because you haven't been added to a parent view or widget hierarchy. This probably breaks on Linux either in gtk theme mode or classic theme mode. https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.h:57: void OpenHelpCenterURL() const; nit: should be Url not URL
Don't wait for my lgtm on this, but here are a few more comments. https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1661063002/diff/180001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:23: GetBrowser()->OpenURL( On 2016/02/17 22:40:40, juncai wrote: > On 2016/02/17 02:19:53, palmer wrote: > > On 2016/02/17 01:33:45, Jeffrey Yasskin wrote: > > > It seems wrong to conjure a browser window out of nothing. Shouldn't it be > the > > > same window as the bubble's showing in? > > > > FWIW: The What Do These Mean? link in the (soon to go away) Connection Tab in > > the Origin Info Bubble opens a new (foreground) tab. I don't know if that's > the > > best thing, but it seems bad to destroy/lose any context in the current > > tab/window. > > Since ChooserBubbleDelegate has a Browser* member variable, I use it to open the > URL instead. Awesome, thanks. https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:175: layout->StartRow(0, 0); On 2016/02/17 22:58:16, msw wrote: > Should the table's row be resizable? (if the bubble is shorter than expected?) The UX folks are still figuring out how to size the bubble appropriately: https://groups.google.com/a/google.com/d/msg/chrome-ui-review/AHU4T6B-8sU/kBU... Bluetooth, especially, causes a problem because it discovers devices over time, which could cause the buttons to jump around. https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:249: GetNativeTheme()->GetSystemColor(ui::NativeTheme::kColorId_LinkEnabled); On 2016/02/17 22:58:16, msw wrote: > aside: odd that CreateForLink doesn't do this. It does, by causing the created label at https://code.google.com/p/chromium/codesearch/#chromium/src/ui/views/controls... to be a Link, which has a default color at https://code.google.com/p/chromium/codesearch/#chromium/src/ui/views/controls.... Jun, if you remove this line, does the link look like a link? https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:20: ui::PAGE_TRANSITION_AUTO_TOPLEVEL, /*is_renderer_initiated=*/true)); It seems like is_renderer_initiated should be false, for this click that was handled by the browser process. Why did you set it to 'true' initially?
Description was changed from ========== Add message and Helper Center link to the chooser UI This patch added message "Not seeing your device? Get help." at the bottom of the chooser UI, and the "Get help." links to a Helper Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ========== to ========== Add message and Helper Center link to the chooser UI This patch added message "Not seeing your device? Get help" at the bottom of the chooser UI, and the "Get help" links to a Help Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ==========
Description was changed from ========== Add message and Helper Center link to the chooser UI This patch added message "Not seeing your device? Get help" at the bottom of the chooser UI, and the "Get help" links to a Help Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ========== to ========== Add message and Help Center link to the chooser UI This patch added message "Not seeing your device? Get help" at the bottom of the chooser UI, and the "Get help" links to a Help Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ==========
https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:173: // | Not seeing your device? Get help | On 2016/02/17 22:58:16, msw wrote: > On 2016/02/17 22:40:39, juncai wrote: > > On 2016/02/17 01:31:35, msw wrote: > > > Is "device" relevant terminology for every chooser bubble? > > > > It is relevant for the two components that use this UI now: WebUsb and > > WebBluetooth. This is part of the UI that was discussed at: > > http://code.google.com/p/chromium/issues/detail?id=583479 > > Okay, perhaps this should be a DeviceChooserBubbleUiViewDelegate... ha. Added a GetOptionDescription virtual function to ChooserBubbleDelegate and subclass can override it with different option description. Also updated the comments. Done. https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... chrome/app/generated_resources.grd:14571: + <message name="IDS_CHOOSER_BUBBLE_GET_HELP_LINK_TEXT" desc="Text of the Get help link at the bottom of the chooser popup."> On 2016/02/17 22:58:16, msw wrote: > nit: 'Get help' Done. https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... chrome/app/generated_resources.grd:14572: + Get help On 2016/02/17 22:58:16, msw wrote: > Maybe use IDS_GET_HELP? There is already an IDS_GET_HELP: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat... https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:173: views::GridLayout::USE_PREF, 0, 0); On 2016/02/17 23:09:01, Evan Stade wrote: > so |layout| only has one column set with one column in it... this should be a > box layout Tried using the following code to add a TableView to BoxLayout: views::View* table_parent = table_view_->CreateParentIfNecessary(); AddChildView(table_parent); layout->SetFlexForView(table_parent, 1); But somehow the table view is not shown. Any suggestions? Thanks! https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:204: button_columns->AddColumn(views::GridLayout::TRAILING, On 2016/02/17 23:09:01, Evan Stade wrote: > this also seems like it should be a (horizontal) box layout Done. https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:233: new views::Separator(views::Separator::HORIZONTAL); On 2016/02/17 22:58:16, msw wrote: > nit: inline in AddView() call below; remove |separator|. Done. https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:249: GetNativeTheme()->GetSystemColor(ui::NativeTheme::kColorId_LinkEnabled); On 2016/02/17 23:54:25, Jeffrey Yasskin wrote: > On 2016/02/17 22:58:16, msw wrote: > > aside: odd that CreateForLink doesn't do this. > > It does, by causing the created label at > https://code.google.com/p/chromium/codesearch/#chromium/src/ui/views/controls... > to be a Link, which has a default color at > https://code.google.com/p/chromium/codesearch/#chromium/src/ui/views/controls.... > > Jun, if you remove this line, does the link look like a link? If I remove this line, the link still looks like a link, but the color is red instead of blue. https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:249: GetNativeTheme()->GetSystemColor(ui::NativeTheme::kColorId_LinkEnabled); On 2016/02/17 23:09:01, Evan Stade wrote: > I don't think you can call GetNativeTheme() yet (and get a good return value) > because you haven't been added to a parent view or widget hierarchy. This > probably breaks on Linux either in gtk theme mode or classic theme mode. override views::BubbleDelegateView::OnNativeThemeChanged to set the link text color. Done. https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:20: ui::PAGE_TRANSITION_AUTO_TOPLEVEL, /*is_renderer_initiated=*/true)); On 2016/02/17 23:54:25, Jeffrey Yasskin wrote: > It seems like is_renderer_initiated should be false, for this click that was > handled by the browser process. Why did you set it to 'true' initially? Done. https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.h:57: void OpenHelpCenterURL() const; On 2016/02/17 23:09:01, Evan Stade wrote: > nit: should be Url not URL Done.
not lgtm, for the translation problems. https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... chrome/app/generated_resources.grd:14572: + Get help On 2016/02/19 01:51:00, juncai wrote: > On 2016/02/17 22:58:16, msw wrote: > > Maybe use IDS_GET_HELP? > > There is already an IDS_GET_HELP: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat... I think msw is suggesting that you use that existing message instead of adding a new message. You'll have to double-check that it works, since it has an embedded '&' to set the shortcut character. https://codereview.chromium.org/1661063002/diff/280001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1661063002/diff/280001/chrome/app/generated_r... chrome/app/generated_resources.grd:14568: + Not seeing your <ph name="OPTION_DESCRIPTION">$1<ex>device</ex></ph>? <ph name="GET_HELP_LINK">$2<ex>Get help</ex></ph> Translation doesn't work like this either. Some languages require, for example, a different form of 'your' depending on what word fills in 'device'. You have to translate at least whole sentences at a time, and ideally whole concepts. https://codereview.chromium.org/1661063002/diff/280001/chrome/browser/ui/blue... File chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1661063002/diff/280001/chrome/browser/ui/blue... chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc:66: return base::ASCIIToUTF16("device"); This needs to be translated. https://codereview.chromium.org/1661063002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:257: void ChooserBubbleUiViewDelegate::OnNativeThemeChanged( I don't see any other uses of this with RangeStyleInfo::CreateForLink(). See what's happening with one of the other CreateForLink() calls, and imitate that?
https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:173: // | Not seeing your device? Get help | On 2016/02/17 22:58:16, msw wrote: > On 2016/02/17 22:40:39, juncai wrote: > > On 2016/02/17 01:31:35, msw wrote: > > > Is "device" relevant terminology for every chooser bubble? > > > > It is relevant for the two components that use this UI now: WebUsb and > > WebBluetooth. This is part of the UI that was discussed at: > > http://code.google.com/p/chromium/issues/detail?id=583479 > > Okay, perhaps this should be a DeviceChooserBubbleUiViewDelegate... ha. We may not need to generalize the chooser UI yet since usb/bluetooth use it and have the same UI footnote text. So we will hold off this change for now. https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... chrome/app/generated_resources.grd:14572: + Get help On 2016/02/19 21:25:40, Jeffrey Yasskin wrote: > On 2016/02/19 01:51:00, juncai wrote: > > On 2016/02/17 22:58:16, msw wrote: > > > Maybe use IDS_GET_HELP? > > > > There is already an IDS_GET_HELP: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat... > > I think msw is suggesting that you use that existing message instead of adding a > new message. You'll have to double-check that it works, since it has an embedded > '&' to set the shortcut character. I tried using IDS_GET_HELP, it didn't work. Also it has uppercase 'H', but the UI team suggested using lowercase 'h'. https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:249: GetNativeTheme()->GetSystemColor(ui::NativeTheme::kColorId_LinkEnabled); On 2016/02/17 22:58:16, msw wrote: > aside: odd that CreateForLink doesn't do this. ok, this line of code is not needed, estade@ pointed out that I was testing the bubble UI using the the gtk theme and the link text is red. In classic theme the link text is blue. I changed it to classic theme and it works. https://codereview.chromium.org/1661063002/diff/280001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1661063002/diff/280001/chrome/app/generated_r... chrome/app/generated_resources.grd:14568: + Not seeing your <ph name="OPTION_DESCRIPTION">$1<ex>device</ex></ph>? <ph name="GET_HELP_LINK">$2<ex>Get help</ex></ph> On 2016/02/19 21:25:40, Jeffrey Yasskin wrote: > Translation doesn't work like this either. Some languages require, for example, > a different form of 'your' depending on what word fills in 'device'. You have to > translate at least whole sentences at a time, and ideally whole concepts. Done. https://codereview.chromium.org/1661063002/diff/280001/chrome/browser/ui/blue... File chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1661063002/diff/280001/chrome/browser/ui/blue... chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc:66: return base::ASCIIToUTF16("device"); On 2016/02/19 21:25:40, Jeffrey Yasskin wrote: > This needs to be translated. Removed this member function since it is not needed for now. https://codereview.chromium.org/1661063002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:257: void ChooserBubbleUiViewDelegate::OnNativeThemeChanged( On 2016/02/19 21:25:40, Jeffrey Yasskin wrote: > I don't see any other uses of this with RangeStyleInfo::CreateForLink(). See > what's happening with one of the other CreateForLink() calls, and imitate that? This function is not overridden, since I was using gtk theme to test the bubble UI. By changing it to classic theme, the link text color is blue.
lgtm now. Thanks. https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:173: // | Not seeing your device? Get help | On 2016/02/20 02:25:04, juncai wrote: > On 2016/02/17 22:58:16, msw wrote: > > On 2016/02/17 22:40:39, juncai wrote: > > > On 2016/02/17 01:31:35, msw wrote: > > > > Is "device" relevant terminology for every chooser bubble? > > > > > > It is relevant for the two components that use this UI now: WebUsb and > > > WebBluetooth. This is part of the UI that was discussed at: > > > http://code.google.com/p/chromium/issues/detail?id=583479 > > > > Okay, perhaps this should be a DeviceChooserBubbleUiViewDelegate... ha. > > We may not need to generalize the chooser UI yet since usb/bluetooth use it and > have the same UI footnote text. So we will hold off this change for now. It'd be better to ask if that's all right with Mike. I suggested holding off on the change because YAGNI, but Mike has more experience with this area and might disagree. https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... chrome/app/generated_resources.grd:14572: + Get help On 2016/02/20 02:25:04, juncai wrote: > On 2016/02/19 21:25:40, Jeffrey Yasskin wrote: > > On 2016/02/19 01:51:00, juncai wrote: > > > On 2016/02/17 22:58:16, msw wrote: > > > > Maybe use IDS_GET_HELP? > > > > > > There is already an IDS_GET_HELP: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat... > > > > I think msw is suggesting that you use that existing message instead of adding > a > > new message. You'll have to double-check that it works, since it has an > embedded > > '&' to set the shortcut character. > > I tried using IDS_GET_HELP, it didn't work. Also it has uppercase 'H', but the > UI team suggested using lowercase 'h'. "It didn't work" doesn't help the reviewers figure out what's going wrong or what alternative would work. If, as I assume, the '&' was visible in the UI, please say that explicitly. If something else about it didn't work, that's even more important to say.
https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:173: // | Not seeing your device? Get help | On 2016/02/22 19:54:14, Jeffrey Yasskin wrote: > On 2016/02/20 02:25:04, juncai wrote: > > On 2016/02/17 22:58:16, msw wrote: > > > On 2016/02/17 22:40:39, juncai wrote: > > > > On 2016/02/17 01:31:35, msw wrote: > > > > > Is "device" relevant terminology for every chooser bubble? > > > > > > > > It is relevant for the two components that use this UI now: WebUsb and > > > > WebBluetooth. This is part of the UI that was discussed at: > > > > http://code.google.com/p/chromium/issues/detail?id=583479 > > > > > > Okay, perhaps this should be a DeviceChooserBubbleUiViewDelegate... ha. > > > > We may not need to generalize the chooser UI yet since usb/bluetooth use it > and > > have the same UI footnote text. So we will hold off this change for now. > > It'd be better to ask if that's all right with Mike. I suggested holding off on > the change because YAGNI, but Mike has more experience with this area and might > disagree. Since the chooser is limited to bluetooth and usb device choosing, this seems acceptable for now. https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... chrome/app/generated_resources.grd:14572: + Get help On 2016/02/22 19:54:15, Jeffrey Yasskin wrote: > On 2016/02/20 02:25:04, juncai wrote: > > On 2016/02/19 21:25:40, Jeffrey Yasskin wrote: > > > On 2016/02/19 01:51:00, juncai wrote: > > > > On 2016/02/17 22:58:16, msw wrote: > > > > > Maybe use IDS_GET_HELP? > > > > > > > > There is already an IDS_GET_HELP: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat... > > > > > > I think msw is suggesting that you use that existing message instead of > adding > > a > > > new message. You'll have to double-check that it works, since it has an > > embedded > > > '&' to set the shortcut character. > > > > I tried using IDS_GET_HELP, it didn't work. Also it has uppercase 'H', but the > > UI team suggested using lowercase 'h'. > > "It didn't work" doesn't help the reviewers figure out what's going wrong or > what alternative would work. If, as I assume, the '&' was visible in the UI, > please say that explicitly. If something else about it didn't work, that's even > more important to say. I don't think there's a common pattern for removing the '&' mnemonic prefix outside of menu code. (the gfx::Canvas flag HIDE_PREFIX isn't something you can readily specify with a [styled]label or link). It's not worth adding code to remove that char for string re-use here, nor is it worth pushing back on the UI team's preference with the capitalization of the string... So adding this new almost-duplicate resource seems fine for now. https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/blue... File chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/blue... chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc:62: return base::ASCIIToUTF16(chrome::kChooserBluetoothOverviewURL); Avoid converting to string16 before creating the GURL. https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right): https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.h:84: virtual base::string16 GetHelpCenterUrl() const = 0; Why doesn't this return a GURL?
https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1661063002/diff/220001/chrome/app/generated_r... chrome/app/generated_resources.grd:14572: + Get help On 2016/02/22 19:54:15, Jeffrey Yasskin wrote: > On 2016/02/20 02:25:04, juncai wrote: > > On 2016/02/19 21:25:40, Jeffrey Yasskin wrote: > > > On 2016/02/19 01:51:00, juncai wrote: > > > > On 2016/02/17 22:58:16, msw wrote: > > > > > Maybe use IDS_GET_HELP? > > > > > > > > There is already an IDS_GET_HELP: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/generat... > > > > > > I think msw is suggesting that you use that existing message instead of > adding > > a > > > new message. You'll have to double-check that it works, since it has an > > embedded > > > '&' to set the shortcut character. > > > > I tried using IDS_GET_HELP, it didn't work. Also it has uppercase 'H', but the > > UI team suggested using lowercase 'h'. > > "It didn't work" doesn't help the reviewers figure out what's going wrong or > what alternative would work. If, as I assume, the '&' was visible in the UI, > please say that explicitly. If something else about it didn't work, that's even > more important to say. Thanks. The '&' was visible in the UI when I tried using IDS_GET_HELP. https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/blue... File chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc (right): https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/blue... chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc:62: return base::ASCIIToUTF16(chrome::kChooserBluetoothOverviewURL); On 2016/02/22 20:13:11, msw wrote: > Avoid converting to string16 before creating the GURL. Explained in another comment for why using base::string16. https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right): https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.h:84: virtual base::string16 GetHelpCenterUrl() const = 0; On 2016/02/22 20:13:11, msw wrote: > Why doesn't this return a GURL? There is a follow-up patch for Mac: https://codereview.chromium.org/1665343003/ And it uses HyperlinkTextView: https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/cocoa/cont... which has a NSString* parameter, so I think that returns a base::string16 is more convenient to use.
lgtm
ping reillyg@, :).
chrome/browser/usb lgtm
https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:63: class ChooserBubbleUiViewDelegate : public views::BubbleDelegateView, I find it very confusing how this is a BubbleDelegateView and also owns a BubbleDelegate. I filed crbug.com/588868 because to fix this. (Not your fault.) Regardless of the nomenclature in components/bubble, to better reflect its responsibilities and clarify roles, I think you should rename ChooserBubbleDelegate to ChooserBubbleController and chooser_bubble_delegate_ to controller_. https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:188: table_view_->SetEnabled(false); nit: table_view_->SetEnabled(chooser_bubble_delegate_->NumOptions() > 0); https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right): https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.h:84: virtual base::string16 GetHelpCenterUrl() const = 0; On 2016/02/22 22:06:37, juncai wrote: > On 2016/02/22 20:13:11, msw wrote: > > Why doesn't this return a GURL? > > There is a follow-up patch for Mac: > https://codereview.chromium.org/1665343003/ > And it uses HyperlinkTextView: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/cocoa/cont... > which has a NSString* parameter, so I think that returns a base::string16 is > more convenient to use. That is a platform-specific implementation detail. This should return a GURL and let mac convert if it needs to. This is logically a URL. The type should match that.
https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:63: class ChooserBubbleUiViewDelegate : public views::BubbleDelegateView, On 2016/02/22 23:15:16, Evan Stade wrote: > I find it very confusing how this is a BubbleDelegateView and also owns a > BubbleDelegate. I filed crbug.com/588868 because to fix this. (Not your fault.) > > Regardless of the nomenclature in components/bubble, to better reflect its > responsibilities and clarify roles, I think you should rename > ChooserBubbleDelegate to ChooserBubbleController and chooser_bubble_delegate_ to > controller_. Since the class name change will also touch the code on Mac side: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... I filed a bug and added a TODO to do it in a separate patch. https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:188: table_view_->SetEnabled(false); On 2016/02/22 23:15:16, Evan Stade wrote: > nit: > > table_view_->SetEnabled(chooser_bubble_delegate_->NumOptions() > 0); Done. https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right): https://codereview.chromium.org/1661063002/diff/320001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/chooser_bubble_delegate.h:84: virtual base::string16 GetHelpCenterUrl() const = 0; On 2016/02/22 23:15:16, Evan Stade wrote: > On 2016/02/22 22:06:37, juncai wrote: > > On 2016/02/22 20:13:11, msw wrote: > > > Why doesn't this return a GURL? > > > > There is a follow-up patch for Mac: > > https://codereview.chromium.org/1665343003/ > > And it uses HyperlinkTextView: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/cocoa/cont... > > which has a NSString* parameter, so I think that returns a base::string16 is > > more convenient to use. > > That is a platform-specific implementation detail. This should return a GURL and > let mac convert if it needs to. This is logically a URL. The type should match > that. Done.
lgtm
The CQ bit was checked by juncai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, reillyg@chromium.org, msw@chromium.org, jyasskin@chromium.org Link to the patchset: https://codereview.chromium.org/1661063002/#ps340001 (title: "address estade@'s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661063002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661063002/340001
Message was sent while issue was closed.
Description was changed from ========== Add message and Help Center link to the chooser UI This patch added message "Not seeing your device? Get help" at the bottom of the chooser UI, and the "Get help" links to a Help Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ========== to ========== Add message and Help Center link to the chooser UI This patch added message "Not seeing your device? Get help" at the bottom of the chooser UI, and the "Get help" links to a Help Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Add message and Help Center link to the chooser UI This patch added message "Not seeing your device? Get help" at the bottom of the chooser UI, and the "Get help" links to a Help Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 ========== to ========== Add message and Help Center link to the chooser UI This patch added message "Not seeing your device? Get help" at the bottom of the chooser UI, and the "Get help" links to a Help Center URL. The message is added below the "Connect" and "Cancel" button with a grey separator. BUG=492204, 583479 Committed: https://crrev.com/e26da162b7de08b3918352deff9f5897f1e2cf2a Cr-Commit-Position: refs/heads/master@{#377216} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/e26da162b7de08b3918352deff9f5897f1e2cf2a Cr-Commit-Position: refs/heads/master@{#377216} |
