Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2541693004:
Add Information Tooltip for Public Suffix List Matches (Closed)
DescriptionAdd Information Tooltip for Public Suffix List Matches
Similarly as done for Desktop this change adds an information tooltip for PSL
matches. This tooltip shows the origin URL when tapped on.
BUG=666340
R=bauerb@chromium.org,vasilii@chromium.org
CC=vabr@chromium.org,melandory@chromium.org
Committed: https://crrev.com/875df60ed3a8b3e1dd89043953b3f8aa00cf70d6
Cr-Commit-Position: refs/heads/master@{#439098}
Patch Set 1 #Patch Set 2 : Add doc string #Patch Set 3 : UI Overhaul #
Total comments: 8
Patch Set 4 : Addressed comments. #
Total comments: 32
Patch Set 5 : Addressed new set of comments. #
Total comments: 20
Patch Set 6 : New Round of Comments #
Total comments: 1
Patch Set 7 : Addressed Nits #
Messages
Total messages: 38 (13 generated)
Dear all, please review. A screenshot of the Toast in action can be found here: http://crbug.com/666340#c9
I got feedback from the designer and am changing the Toast to a PopupWindow. Please hold off reviewing AccountChooserDialog.java for now, I will update the CL once I am done.
On 2016/12/01 12:27:15, jdoerrie wrote: > I got feedback from the designer and am changing the Toast to a PopupWindow. > Please hold off reviewing AccountChooserDialog.java for now, I will update the > CL once I am done. Please review now, also see http://crbug.com/666340#c11 for current screenshots.
Is it possible to take sizes, paddings, colors from the tooltip in the credit card dialog?
You might want to get an additional OWNER for chrome/android/java/res/, as I'm only owner for NTP UI there. https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/res... File chrome/android/java/res/drawable/psl_info_icon_background.xml (right): https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/res... chrome/android/java/res/drawable/psl_info_icon_background.xml:10: <solid android:color="#14000000" /> No spaces before the end of the self-closing tag. https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_dialog_item.xml (right): https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_dialog_item.xml:50: android:layout_width="@dimen/psl_info_icon_width" Keep the attributes sorted? https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:346: <!-- PSL info icon and tooltip dimensions --> Can you explain what PSL means? Without knowing about this CL, I'd only know pumpkin spice latte... https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java (right): https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:188: // Shift tooltip to the left by required amount, handle RTL correctly. Could you add empty lines before comments?
jdoerrie@chromium.org changed reviewers: + dfalcantara@chromium.org
I asked the UI designer regarding re-using styles from the CC dialog, will report back once I get an answer. https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/res... File chrome/android/java/res/drawable/psl_info_icon_background.xml (right): https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/res... chrome/android/java/res/drawable/psl_info_icon_background.xml:10: <solid android:color="#14000000" /> On 2016/12/06 16:46:42, Bernhard Bauer wrote: > No spaces before the end of the self-closing tag. Done. https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_dialog_item.xml (right): https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_dialog_item.xml:50: android:layout_width="@dimen/psl_info_icon_width" On 2016/12/06 16:46:42, Bernhard Bauer wrote: > Keep the attributes sorted? Done. https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:346: <!-- PSL info icon and tooltip dimensions --> On 2016/12/06 16:46:42, Bernhard Bauer wrote: > Can you explain what PSL means? Without knowing about this CL, I'd only know > pumpkin spice latte... Done. https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java (right): https://codereview.chromium.org/2541693004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:188: // Shift tooltip to the left by required amount, handle RTL correctly. On 2016/12/06 16:46:42, Bernhard Bauer wrote: > Could you add empty lines before comments? Done.
Description was changed from ========== Add Information Toast for Public Suffix List Matches Similarly as done for Desktop this change adds an information tooltip for PSL matches. This tooltip shows the origin URL when tapped on. BUG=666340 R=bauerb@chromium.org,vasilii@chromium.org CC=vabr@chromium.org,melandory@chromium.org ========== to ========== Add Information Tooltip for Public Suffix List Matches Similarly as done for Desktop this change adds an information tooltip for PSL matches. This tooltip shows the origin URL when tapped on. BUG=666340 R=bauerb@chromium.org,vasilii@chromium.org CC=vabr@chromium.org,melandory@chromium.org ==========
lgtm
Any reason why you're not using the standard Android tooltips like we do everywhere else? You're breaking from the platform standards, which means that this'll look even more out of place if Material changes again. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... File chrome/android/java/res/drawable/psl_info_icon_background.xml (right): https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... chrome/android/java/res/drawable/psl_info_icon_background.xml:9: <shape xmlns:android="http://schemas.android.com/apk/res/android"> Setting a background drawable overrides the platform-wide Material rippling effect. I don't think you (or the designer) want that. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_dialog_item.xml (right): https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_dialog_item.xml:6: <RelativeLayout Why do you switch to a RelativeLayout from a LinearLayout? You're not putting anything underneath anything else, AFAICT, so everything fits fine with a horizontal layout. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_dialog_item.xml:18: android:layout_centerVertical="true" What happens if you just set android:gravity=center_vertical for the RelativeLayout instead of setting it on each of the children individually? https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_dialog_item.xml:48: <ImageView You want an ImageButton, not an ImageView. See how we set the background for other ImageButtons so that you keep the Material rippling effect. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_dialog_item.xml:49: android:contentDescription="@null" nit: IDs first, then android:layout_* things, then everything else. Makes it easier to find individual views and understand how they're placed relative to the parent. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java (right): https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:151: pslInfoIconView.setImageResource(R.drawable.btn_info); Set the ImageResource in the XML file directly. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:152: pslInfoIconView.setClickable(true); If you use the ImageButton, this should be automatically be clickable. Ditto if you set an OnClickListener. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:156: final ImageView iconView = (ImageView) view; Pull this all out into another function called prepareIconView or something. It's indented way too deeply. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:162: TextView textView = new TextView(context); 1) Can you apply the background for the tooltip to the TextView, instead? There's (currently) nothing else in the tooltip, so it should be fine. It'd also make it . 2) Pull all this TextView stuff into an XML layout file instead of doing it here. Makes it clearer to understand how you're styling the font and dimensions. 3) Use a LayoutInflater.from(context) to inflate the TextView layout. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:171: R.dimen.psl_info_tooltip_height)); Don't set the height of the TextView directly. People who have changed the style or size of their fonts system-wide (for accessibility, e.g.) will bork your UI. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:187: textView.measure(0, 0); These should be MeasureSpecs instead of straight integer values. Use MeasureSpec#makeMeasureSpec() so that it's obvious what you're trying to do. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:194: // Shift tooltip up by required amount. What happens on small phones and long addresses? You should: 1) Clamp the maximum size of the text view so that it doesn't run off the screen. 2) Set it to single line mode, then ellipsize it so that the last part of the domain remains readable, similarly to how we have to deal with URLs in other places.
(See what we do for ToolbarLayout#showAccessibilityToast())
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please have another look at the updated patchset. Screenshots displaying a general case, left eliding of origins and RTL behavior can be found in http://crbug.com/666340#c12. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... File chrome/android/java/res/drawable/psl_info_icon_background.xml (right): https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... chrome/android/java/res/drawable/psl_info_icon_background.xml:9: <shape xmlns:android="http://schemas.android.com/apk/res/android"> On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > Setting a background drawable overrides the platform-wide Material rippling > effect. I don't think you (or the designer) want that. Acknowledged. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_dialog_item.xml (right): https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_dialog_item.xml:6: <RelativeLayout On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > Why do you switch to a RelativeLayout from a LinearLayout? You're not putting > anything underneath anything else, AFAICT, so everything fits fine with a > horizontal layout. I initially had difficulties getting a right aligned info button working. In the latest patch set I introduced a Space View for this purpose. Please let me know if this is more appropriate. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_dialog_item.xml:18: android:layout_centerVertical="true" On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > What happens if you just set android:gravity=center_vertical for the > RelativeLayout instead of setting it on each of the children individually? This would not work unfortunately, android:gravity is not defined for RelativeLayout. The doc says android:layout_centerVertical centers the child within it's parent, which is why we would need to repeat it for all elements: https://developer.android.com/reference/android/widget/RelativeLayout.LayoutP... This might be a moot point, since I reverted back to a LinearLayout in the latest patchset. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_dialog_item.xml:48: <ImageView On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > You want an ImageButton, not an ImageView. See how we set the background for > other ImageButtons so that you keep the Material rippling effect. Done. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_dialog_item.xml:49: android:contentDescription="@null" On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > nit: IDs first, then android:layout_* things, then everything else. Makes it > easier to find individual views and understand how they're placed relative to > the parent. Done. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java (right): https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:151: pslInfoIconView.setImageResource(R.drawable.btn_info); On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > Set the ImageResource in the XML file directly. Done. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:152: pslInfoIconView.setClickable(true); On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > If you use the ImageButton, this should be automatically be clickable. Ditto if > you set an OnClickListener. Acknowledged. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:156: final ImageView iconView = (ImageView) view; On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > Pull this all out into another function called prepareIconView or something. > It's indented way too deeply. Done. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:162: TextView textView = new TextView(context); On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > 1) Can you apply the background for the tooltip to the TextView, instead? > There's (currently) nothing else in the tooltip, so it should be fine. It'd > also make it . > > 2) Pull all this TextView stuff into an XML layout file instead of doing it > here. Makes it clearer to understand how you're styling the font and > dimensions. > > 3) Use a LayoutInflater.from(context) to inflate the TextView layout. Done. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:171: R.dimen.psl_info_tooltip_height)); On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > Don't set the height of the TextView directly. People who have changed the > style or size of their fonts system-wide (for accessibility, e.g.) will bork > your UI. I would like to avoid this, however the official Material Design spec suggests a fixed height of 32dp: https://material.google.com/components/tooltips.html#tooltips-tooltips-mobile How is this done in other parts of the code base? https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:194: // Shift tooltip up by required amount. On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > What happens on small phones and long addresses? You should: > > 1) Clamp the maximum size of the text view so that it doesn't run off the > screen. > 2) Set it to single line mode, then ellipsize it so that the last part of the > domain remains readable, similarly to how we have to deal with URLs in other > places. Done. https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:351: <dimen name="psl_info_tooltip_margin">8dp</dimen> I would like to avoid specifying this into so much detail, but the designer wants to see the official Material Design spec applied to the tooltip: https://material.google.com/components/tooltips.html#tooltips-tooltips-mobile Is there a nicer way to achieve the same result?
On 2016/12/13 17:24:48, jdoerrie wrote: > Please have another look at the updated patchset. Screenshots displaying a > general case, left eliding of origins and RTL behavior can be found in > http://crbug.com/666340#c12. > > https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... > File chrome/android/java/res/drawable/psl_info_icon_background.xml (right): > > https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... > chrome/android/java/res/drawable/psl_info_icon_background.xml:9: <shape > xmlns:android="http://schemas.android.com/apk/res/android"> > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > Setting a background drawable overrides the platform-wide Material rippling > > effect. I don't think you (or the designer) want that. > > Acknowledged. > > https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... > File chrome/android/java/res/layout/account_chooser_dialog_item.xml (right): > > https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... > chrome/android/java/res/layout/account_chooser_dialog_item.xml:6: > <RelativeLayout > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > Why do you switch to a RelativeLayout from a LinearLayout? You're not putting > > anything underneath anything else, AFAICT, so everything fits fine with a > > horizontal layout. > > I initially had difficulties getting a right aligned info button working. In the > latest patch set I introduced a Space View for this purpose. Please let me know > if this is more appropriate. > > https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... > chrome/android/java/res/layout/account_chooser_dialog_item.xml:18: > android:layout_centerVertical="true" > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > What happens if you just set android:gravity=center_vertical for the > > RelativeLayout instead of setting it on each of the children individually? > > This would not work unfortunately, android:gravity is not defined for > RelativeLayout. The doc says android:layout_centerVertical centers the child > within it's parent, which is why we would need to repeat it for all elements: > https://developer.android.com/reference/android/widget/RelativeLayout.LayoutP... > > This might be a moot point, since I reverted back to a LinearLayout in the > latest patchset. > > https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... > chrome/android/java/res/layout/account_chooser_dialog_item.xml:48: <ImageView > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > You want an ImageButton, not an ImageView. See how we set the background for > > other ImageButtons so that you keep the Material rippling effect. > > Done. > > https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... > chrome/android/java/res/layout/account_chooser_dialog_item.xml:49: > android:contentDescription="@null" > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > nit: IDs first, then android:layout_* things, then everything else. Makes it > > easier to find individual views and understand how they're placed relative to > > the parent. > > Done. > > https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java > (right): > > https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:151: > pslInfoIconView.setImageResource(R.drawable.btn_info); > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > Set the ImageResource in the XML file directly. > > Done. > > https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:152: > pslInfoIconView.setClickable(true); > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > If you use the ImageButton, this should be automatically be clickable. Ditto > if > > you set an OnClickListener. > > Acknowledged. > > https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:156: > final ImageView iconView = (ImageView) view; > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > Pull this all out into another function called prepareIconView or something. > > It's indented way too deeply. > > Done. > > https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:162: > TextView textView = new TextView(context); > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > 1) Can you apply the background for the tooltip to the TextView, instead? > > There's (currently) nothing else in the tooltip, so it should be fine. It'd > > also make it . > > > > 2) Pull all this TextView stuff into an XML layout file instead of doing it > > here. Makes it clearer to understand how you're styling the font and > > dimensions. > > > > 3) Use a LayoutInflater.from(context) to inflate the TextView layout. > > Done. > > https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:171: > R.dimen.psl_info_tooltip_height)); > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > Don't set the height of the TextView directly. People who have changed the > > style or size of their fonts system-wide (for accessibility, e.g.) will bork > > your UI. > > I would like to avoid this, however the official Material Design spec suggests a > fixed height of 32dp: > > https://material.google.com/components/tooltips.html#tooltips-tooltips-mobile > > How is this done in other parts of the code base? > > https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:194: > // Shift tooltip up by required amount. > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > What happens on small phones and long addresses? You should: > > > > 1) Clamp the maximum size of the text view so that it doesn't run off the > > screen. > > 2) Set it to single line mode, then ellipsize it so that the last part of the > > domain remains readable, similarly to how we have to deal with URLs in other > > places. > > Done. > > https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... > File chrome/android/java/res/values/dimens.xml (right): > > https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... > chrome/android/java/res/values/dimens.xml:351: <dimen > name="psl_info_tooltip_margin">8dp</dimen> > I would like to avoid specifying this into so much detail, but the designer > wants to see the official Material Design spec applied to the tooltip: > > https://material.google.com/components/tooltips.html#tooltips-tooltips-mobile > > Is there a nicer way to achieve the same result?
> > I would like to avoid specifying this into so much detail, but the designer > > wants to see the official Material Design spec applied to the tooltip: > > > > https://material.google.com/components/tooltips.html#tooltips-tooltips-mobile > > > > Is there a nicer way to achieve the same result? The designer should be aiming for consistency with what the platform provides. Every time we special case it, we end up with another piece of UI that's inconsistent and stays that way when the platform's version changes. Which person are you working with?
dfalcantara@, looks like your comments are not attached.
On 2016/12/13 17:55:23, dfalcantara (check my queue) wrote: > > > I would like to avoid specifying this into so much detail, but the designer > > > wants to see the official Material Design spec applied to the tooltip: > > > > > > > https://material.google.com/components/tooltips.html#tooltips-tooltips-mobile > > > > > > Is there a nicer way to achieve the same result? > > The designer should be aiming for consistency with what the platform provides. > Every time we special case it, we end up with another piece of UI that's > inconsistent and stays that way when the platform's version changes. Which > person are you working with? The designer is hwi@
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Spoke with Hwi; she wasn't aware that we used standard Android toasts everywhere else outside of the CVC dialog, and she is filing a bug to unify the styling for all of them. Can you make your toasting code slightly more generic to make my life easier when I start work on that (soon™)? https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_dialog_item.xml (right): https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_dialog_item.xml:18: android:layout_centerVertical="true" On 2016/12/13 17:24:48, jdoerrie wrote: > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > What happens if you just set android:gravity=center_vertical for the > > RelativeLayout instead of setting it on each of the children individually? > > This would not work unfortunately, android:gravity is not defined for > RelativeLayout. The doc says android:layout_centerVertical centers the child > within it's parent, which is why we would need to repeat it for all elements: > https://developer.android.com/reference/android/widget/RelativeLayout.LayoutP... > > This might be a moot point, since I reverted back to a LinearLayout in the > latest patchset. Now that you've switched to using the linear layout, does my original comment about setting this on the parent apply? https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java (right): https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:171: R.dimen.psl_info_tooltip_height)); On 2016/12/13 17:24:48, jdoerrie wrote: > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > Don't set the height of the TextView directly. People who have changed the > > style or size of their fonts system-wide (for accessibility, e.g.) will bork > > your UI. > > I would like to avoid this, however the official Material Design spec suggests a > fixed height of 32dp: > > https://material.google.com/components/tooltips.html#tooltips-tooltips-mobile > > How is this done in other parts of the code base? A fixed-height on text is generally unenforceable; most places end up specifying wrap_content to avoid clipping the text. We roughly control the size of the TextView by adjusting the font size. https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/psl_info_tooltip.xml (right): https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/psl_info_tooltip.xml:1: <?xml version="1.0" encoding="utf-8"?> Can you name this (and all related things) something generic, like material_tooltip.xml, material_tooltip_background.xml, etc? https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/psl_info_tooltip.xml:7: xmlns:android="http://schemas.android.com/apk/res/android" indent by 4s not 2s; check the XML file above https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/psl_info_tooltip.xml:11: android:layout_width="fill_parent" fill_parent is deprecated; use match_parent https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/psl_info_tooltip.xml:12: android:layout_height="fill_parent"> layout params at top https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/psl_info_tooltip.xml:15: android:layout_height="@dimen/psl_info_tooltip_height" I don't think you need the outer LinearLayout. There's only one child here. https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/psl_info_tooltip.xml:23: android:textColor="#fff" android:textColor="@android:color/white" https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:351: <dimen name="psl_info_tooltip_margin">8dp</dimen> On 2016/12/13 17:24:48, jdoerrie wrote: > I would like to avoid specifying this into so much detail, but the designer > wants to see the official Material Design spec applied to the tooltip: > > https://material.google.com/components/tooltips.html#tooltips-tooltips-mobile > > Is there a nicer way to achieve the same result? Annoyingly, no. We generally get redlines with numbers and plug them in like this. I would suggest just plugging most of these numbers directly into the layout XML, though -- they're used only that file and they're not different between tablet and phone layouts. I think margin is the only one that's used in the code now. https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java (right): https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:204: private void prepareButtonView(ImageButton buttonView, String originUrl) { This isn't really preparing a the buttonView; it's showing a tooltip. Can you make this more generic so it can be used elsewhere? 1) Call it showTooltip() 2) Take in a View instead of an ImageButton 3) Pass in a "message" instead of "originUrl", calling formatUrlForSecurityDisplay where this function is called from instead. 4) Pass in the layout ID to inflate 5) Maybe move it to UiUtils#showTooltip() https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:210: (ViewGroup) buttonView.findViewById(R.id.psl_info_tooltip)); 1) I'd avoid having a layout with the same name as the ID; gets confusing to read it later. 2) Pretty sure it's just returning null when you do a search on buttonView for that ID, anyway. You're telling the LayoutInflater to inflate the tooltip LinearLayout as a child of itself before it's been inflated. 3) You actually want to pass in null, anyway -- the tooltip is set as the View of the Toast below.
Thank you for talking to Hwi! I'm happy to help unifying the look of tooltips, just have a minor question regarding the proposed API. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_dialog_item.xml (right): https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_dialog_item.xml:18: android:layout_centerVertical="true" On 2016/12/13 23:14:29, dfalcantara (check my queue) wrote: > On 2016/12/13 17:24:48, jdoerrie wrote: > > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > > What happens if you just set android:gravity=center_vertical for the > > > RelativeLayout instead of setting it on each of the children individually? > > > > This would not work unfortunately, android:gravity is not defined for > > RelativeLayout. The doc says android:layout_centerVertical centers the child > > within it's parent, which is why we would need to repeat it for all elements: > > > https://developer.android.com/reference/android/widget/RelativeLayout.LayoutP... > > > > This might be a moot point, since I reverted back to a LinearLayout in the > > latest patchset. > > Now that you've switched to using the linear layout, does my original comment > about setting this on the parent apply? Unfortunately it does not apply, it still needs to specified on every child element. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java (right): https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:171: R.dimen.psl_info_tooltip_height)); On 2016/12/13 23:14:29, dfalcantara (check my queue) wrote: > On 2016/12/13 17:24:48, jdoerrie wrote: > > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > > Don't set the height of the TextView directly. People who have changed the > > > style or size of their fonts system-wide (for accessibility, e.g.) will bork > > > your UI. > > > > I would like to avoid this, however the official Material Design spec suggests > a > > fixed height of 32dp: > > > > https://material.google.com/components/tooltips.html#tooltips-tooltips-mobile > > > > How is this done in other parts of the code base? > > A fixed-height on text is generally unenforceable; most places end up specifying > wrap_content to avoid clipping the text. We roughly control the size of the > TextView by adjusting the font size. We could specify padding in the tooltip background to achieve a similar effect. On my device a fontsize of 14sp combined with setting a top and bottom padding of 6.5dp comes close to a total height of 32dp, but 6.5dp still seems arbitrary to me. Is there a commonly used vertical padding in other parts of the codebase? https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java (right): https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:204: private void prepareButtonView(ImageButton buttonView, String originUrl) { On 2016/12/13 23:14:30, dfalcantara (check my queue) wrote: > This isn't really preparing a the buttonView; it's showing a tooltip. Can you > make this more generic so it can be used elsewhere? > > 1) Call it showTooltip() > 2) Take in a View instead of an ImageButton > 3) Pass in a "message" instead of "originUrl", calling > formatUrlForSecurityDisplay where this function is called from instead. > 4) Pass in the layout ID to inflate > 5) Maybe move it to UiUtils#showTooltip() How do you suggest we pass gravity, xOffset, yOffset and duration to the toast? These will be probably different for each call site. I would like to avoid a lengthy signature like showTooltip(View view, String message, int layoutId, int gravity, int xOffset, int yOffset, int duration), but I don't know how we can avoid this. Maybe we should just return a Toast and the call site is responsible for showing it and setting gravity and duration?
https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_dialog_item.xml (right): https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_dialog_item.xml:18: android:layout_centerVertical="true" On 2016/12/14 16:04:01, jdoerrie wrote: > On 2016/12/13 23:14:29, dfalcantara (check my queue) wrote: > > On 2016/12/13 17:24:48, jdoerrie wrote: > > > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > > > What happens if you just set android:gravity=center_vertical for the > > > > RelativeLayout instead of setting it on each of the children individually? > > > > > > This would not work unfortunately, android:gravity is not defined for > > > RelativeLayout. The doc says android:layout_centerVertical centers the child > > > within it's parent, which is why we would need to repeat it for all > elements: > > > > > > https://developer.android.com/reference/android/widget/RelativeLayout.LayoutP... > > > > > > This might be a moot point, since I reverted back to a LinearLayout in the > > > latest patchset. > > > > Now that you've switched to using the linear layout, does my original comment > > about setting this on the parent apply? > > Unfortunately it does not apply, it still needs to specified on every child > element. We're talking about android:gravity, right? That's the one that tells the parent layout that its children should be placed however it wants? https://developer.android.com/reference/android/widget/LinearLayout.html#attr... We use it in download_item_view.xml, bookmark_widget_item.xml, and a bunch of other places, so I'm not sure why your case wouldn't work. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java (right): https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:171: R.dimen.psl_info_tooltip_height)); On 2016/12/14 16:04:01, jdoerrie wrote: > On 2016/12/13 23:14:29, dfalcantara (check my queue) wrote: > > On 2016/12/13 17:24:48, jdoerrie wrote: > > > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > > > Don't set the height of the TextView directly. People who have changed > the > > > > style or size of their fonts system-wide (for accessibility, e.g.) will > bork > > > > your UI. > > > > > > I would like to avoid this, however the official Material Design spec > suggests > > a > > > fixed height of 32dp: > > > > > > > https://material.google.com/components/tooltips.html#tooltips-tooltips-mobile > > > > > > How is this done in other parts of the code base? > > > > A fixed-height on text is generally unenforceable; most places end up > specifying > > wrap_content to avoid clipping the text. We roughly control the size of the > > TextView by adjusting the font size. > > We could specify padding in the tooltip background to achieve a similar effect. > On my device a fontsize of 14sp combined with setting a top and bottom padding > of 6.5dp comes close to a total height of 32dp, but 6.5dp still seems arbitrary > to me. Is there a commonly used vertical padding in other parts of the codebase? Setting padding like that is totally arbitrary and won't apply in all situations :/ You could try using https://developer.android.com/reference/android/view/View.html#setMinimumHeig... on the appropriate view so that the toast can expand as necessary. https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java (right): https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:204: private void prepareButtonView(ImageButton buttonView, String originUrl) { On 2016/12/14 16:04:01, jdoerrie wrote: > On 2016/12/13 23:14:30, dfalcantara (check my queue) wrote: > > This isn't really preparing a the buttonView; it's showing a tooltip. Can you > > make this more generic so it can be used elsewhere? > > > > 1) Call it showTooltip() > > 2) Take in a View instead of an ImageButton > > 3) Pass in a "message" instead of "originUrl", calling > > formatUrlForSecurityDisplay where this function is called from instead. > > 4) Pass in the layout ID to inflate > > 5) Maybe move it to UiUtils#showTooltip() > > How do you suggest we pass gravity, xOffset, yOffset and duration to the toast? > These will be probably different for each call site. I would like to avoid a > lengthy signature like > showTooltip(View view, String message, int layoutId, int gravity, int xOffset, > int yOffset, int duration), but I don't know how we can avoid this. Maybe we > should just return a Toast and the call site is responsible for showing it and > setting gravity and duration? Eh, passing the Toast back would defeat most of the abstraction I was aiming for. How about you keep the file in this place for now and I'll yank it out and adjust it later? Just generalize the function prototype as much as you think makes sense for now.
https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_dialog_item.xml (right): https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_dialog_item.xml:18: android:layout_centerVertical="true" On 2016/12/14 18:20:42, dfalcantara (check my queue) wrote: > On 2016/12/14 16:04:01, jdoerrie wrote: > > On 2016/12/13 23:14:29, dfalcantara (check my queue) wrote: > > > On 2016/12/13 17:24:48, jdoerrie wrote: > > > > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > > > > What happens if you just set android:gravity=center_vertical for the > > > > > RelativeLayout instead of setting it on each of the children > individually? > > > > > > > > This would not work unfortunately, android:gravity is not defined for > > > > RelativeLayout. The doc says android:layout_centerVertical centers the > child > > > > within it's parent, which is why we would need to repeat it for all > > elements: > > > > > > > > > > https://developer.android.com/reference/android/widget/RelativeLayout.LayoutP... > > > > > > > > This might be a moot point, since I reverted back to a LinearLayout in the > > > > latest patchset. > > > > > > Now that you've switched to using the linear layout, does my original > comment > > > about setting this on the parent apply? > > > > Unfortunately it does not apply, it still needs to specified on every child > > element. > > We're talking about android:gravity, right? That's the one that tells the > parent layout that its children should be placed however it wants? > > https://developer.android.com/reference/android/widget/LinearLayout.html#attr... > > We use it in download_item_view.xml, bookmark_widget_item.xml, and a bunch of > other places, so I'm not sure why your case wouldn't work. My bad, I was looking into android:layout_gravity. android:gravity works fine. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java (right): https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:171: R.dimen.psl_info_tooltip_height)); On 2016/12/14 18:20:42, dfalcantara (check my queue) wrote: > On 2016/12/14 16:04:01, jdoerrie wrote: > > On 2016/12/13 23:14:29, dfalcantara (check my queue) wrote: > > > On 2016/12/13 17:24:48, jdoerrie wrote: > > > > On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > > > > > Don't set the height of the TextView directly. People who have changed > > the > > > > > style or size of their fonts system-wide (for accessibility, e.g.) will > > bork > > > > > your UI. > > > > > > > > I would like to avoid this, however the official Material Design spec > > suggests > > > a > > > > fixed height of 32dp: > > > > > > > > > > https://material.google.com/components/tooltips.html#tooltips-tooltips-mobile > > > > > > > > How is this done in other parts of the code base? > > > > > > A fixed-height on text is generally unenforceable; most places end up > > specifying > > > wrap_content to avoid clipping the text. We roughly control the size of the > > > TextView by adjusting the font size. > > > > We could specify padding in the tooltip background to achieve a similar > effect. > > On my device a fontsize of 14sp combined with setting a top and bottom padding > > of 6.5dp comes close to a total height of 32dp, but 6.5dp still seems > arbitrary > > to me. Is there a commonly used vertical padding in other parts of the > codebase? > > Setting padding like that is totally arbitrary and won't apply in all situations > :/ You could try using > https://developer.android.com/reference/android/view/View.html#setMinimumHeig... > on the appropriate view so that the toast can expand as necessary. Done. https://codereview.chromium.org/2541693004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:187: textView.measure(0, 0); On 2016/12/08 01:32:44, dfalcantara (check my queue) wrote: > These should be MeasureSpecs instead of straight integer values. Use > MeasureSpec#makeMeasureSpec() so that it's obvious what you're trying to do. Acknowledged. https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/psl_info_tooltip.xml (right): https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/psl_info_tooltip.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2016/12/13 23:14:30, dfalcantara (check my queue) wrote: > Can you name this (and all related things) something generic, like > material_tooltip.xml, material_tooltip_background.xml, etc? Done. https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/psl_info_tooltip.xml:7: xmlns:android="http://schemas.android.com/apk/res/android" On 2016/12/13 23:14:30, dfalcantara (check my queue) wrote: > indent by 4s not 2s; check the XML file above Done. https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/psl_info_tooltip.xml:11: android:layout_width="fill_parent" On 2016/12/13 23:14:30, dfalcantara (check my queue) wrote: > fill_parent is deprecated; use match_parent Acknowledged. https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/psl_info_tooltip.xml:12: android:layout_height="fill_parent"> On 2016/12/13 23:14:30, dfalcantara (check my queue) wrote: > layout params at top Acknowledged. https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/psl_info_tooltip.xml:15: android:layout_height="@dimen/psl_info_tooltip_height" On 2016/12/13 23:14:30, dfalcantara (check my queue) wrote: > I don't think you need the outer LinearLayout. There's only one child here. Done. https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/psl_info_tooltip.xml:23: android:textColor="#fff" On 2016/12/13 23:14:30, dfalcantara (check my queue) wrote: > android:textColor="@android:color/white" Done. https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java (right): https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:204: private void prepareButtonView(ImageButton buttonView, String originUrl) { On 2016/12/14 18:20:42, dfalcantara (check my queue) wrote: > On 2016/12/14 16:04:01, jdoerrie wrote: > > On 2016/12/13 23:14:30, dfalcantara (check my queue) wrote: > > > This isn't really preparing a the buttonView; it's showing a tooltip. Can > you > > > make this more generic so it can be used elsewhere? > > > > > > 1) Call it showTooltip() > > > 2) Take in a View instead of an ImageButton > > > 3) Pass in a "message" instead of "originUrl", calling > > > formatUrlForSecurityDisplay where this function is called from instead. > > > 4) Pass in the layout ID to inflate > > > 5) Maybe move it to UiUtils#showTooltip() > > > > How do you suggest we pass gravity, xOffset, yOffset and duration to the > toast? > > These will be probably different for each call site. I would like to avoid a > > lengthy signature like > > showTooltip(View view, String message, int layoutId, int gravity, int xOffset, > > int yOffset, int duration), but I don't know how we can avoid this. Maybe we > > should just return a Toast and the call site is responsible for showing it and > > setting gravity and duration? > > Eh, passing the Toast back would defeat most of the abstraction I was aiming > for. How about you keep the file in this place for now and I'll yank it out and > adjust it later? Just generalize the function prototype as much as you think > makes sense for now. Done. https://codereview.chromium.org/2541693004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/password_manager/AccountChooserDialog.java:210: (ViewGroup) buttonView.findViewById(R.id.psl_info_tooltip)); On 2016/12/13 23:14:30, dfalcantara (check my queue) wrote: > 1) I'd avoid having a layout with the same name as the ID; gets confusing to > read it later. > > 2) Pretty sure it's just returning null when you do a search on buttonView for > that ID, anyway. You're telling the LayoutInflater to inflate the tooltip > LinearLayout as a child of itself before it's been inflated. > > 3) You actually want to pass in null, anyway -- the tooltip is set as the View > of the Toast below. Done.
lgtm https://codereview.chromium.org/2541693004/diff/100001/chrome/android/java/re... File chrome/android/java/res/drawable/material_tooltip_background.xml (right): https://codereview.chromium.org/2541693004/diff/100001/chrome/android/java/re... chrome/android/java/res/drawable/material_tooltip_background.xml:9: android:radius="2dp"/> nit: should be alright to keep XML tags with one attribute on one line instead of arbitrarily breaking it into two.
lgtm
The CQ bit was checked by jdoerrie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, vasilii@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2541693004/#ps120001 (title: "Addressed Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by jdoerrie@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481890570469830,
"parent_rev": "2eb3d6e07370ff078dd677a9f42b4a1d4c53054d", "commit_rev":
"f5bbac48ae6513656e2d3baafc04d6b6c674fc46"}
Message was sent while issue was closed.
Description was changed from ========== Add Information Tooltip for Public Suffix List Matches Similarly as done for Desktop this change adds an information tooltip for PSL matches. This tooltip shows the origin URL when tapped on. BUG=666340 R=bauerb@chromium.org,vasilii@chromium.org CC=vabr@chromium.org,melandory@chromium.org ========== to ========== Add Information Tooltip for Public Suffix List Matches Similarly as done for Desktop this change adds an information tooltip for PSL matches. This tooltip shows the origin URL when tapped on. BUG=666340 R=bauerb@chromium.org,vasilii@chromium.org CC=vabr@chromium.org,melandory@chromium.org Review-Url: https://codereview.chromium.org/2541693004 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add Information Tooltip for Public Suffix List Matches Similarly as done for Desktop this change adds an information tooltip for PSL matches. This tooltip shows the origin URL when tapped on. BUG=666340 R=bauerb@chromium.org,vasilii@chromium.org CC=vabr@chromium.org,melandory@chromium.org Review-Url: https://codereview.chromium.org/2541693004 ========== to ========== Add Information Tooltip for Public Suffix List Matches Similarly as done for Desktop this change adds an information tooltip for PSL matches. This tooltip shows the origin URL when tapped on. BUG=666340 R=bauerb@chromium.org,vasilii@chromium.org CC=vabr@chromium.org,melandory@chromium.org Committed: https://crrev.com/875df60ed3a8b3e1dd89043953b3f8aa00cf70d6 Cr-Commit-Position: refs/heads/master@{#439098} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/875df60ed3a8b3e1dd89043953b3f8aa00cf70d6 Cr-Commit-Position: refs/heads/master@{#439098} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
