|
|
Created:
3 years, 10 months ago by gogerald1 Modified:
3 years, 10 months ago CC:
chromium-reviews, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, gogerald+paymentswatch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UI elements to secure branding for payments
BUG=691104
Review-Url: https://codereview.chromium.org/2698703003
Cr-Commit-Position: refs/heads/master@{#451873}
Committed: https://chromium.googlesource.com/chromium/src/+/41f07f2348e5ccec788938057ff29fea5876ff57
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address comments, fix animation and improve efficiency #
Total comments: 14
Patch Set 3 : address comments and fit small device dynamically #
Total comments: 11
Patch Set 4 : format #Patch Set 5 : fix tests #Patch Set 6 : Measure bottom bar manually #Patch Set 7 : support rotation #
Total comments: 10
Patch Set 8 : rename #Patch Set 9 : address comments #
Total comments: 4
Patch Set 10 : nits #Messages
Total messages: 79 (53 generated)
Description was changed from ========== format secure brand payment request draft BUG= ========== to ========== Add UI elements to secure branding for payments BUG=691104 ==========
The CQ bit was checked by gogerald@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...
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gogerald@chromium.org changed reviewers: + dfalcantara@chromium.org, rouslan@chromium.org
Hi dfalcantara and rouslan@, PTAL, please refer the screenshots in the bug.
rouslan@chromium.org changed reviewers: + palmer@chromium.org
+palmer@: Would you be the right person to review security enamel related changes? https://codereview.chromium.org/2698703003/diff/60001/components/url_formatte... File components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java (right): https://codereview.chromium.org/2698703003/diff/60001/components/url_formatte... components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java:92: * @param uri The URI. Please align "The" together on these two lines. https://codereview.chromium.org/2698703003/diff/60001/components/url_formatte... components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java:97: public static CharSequence tintUrlSchemeForSecurityDisplay(String uri, int color) { Please pick either URL or URI and use that term throughout your patch. https://codereview.chromium.org/2698703003/diff/60001/components/url_formatte... components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java:97: public static CharSequence tintUrlSchemeForSecurityDisplay(String uri, int color) { How does the omnibox tint the URL scheme? It would be great to call into their function instead of constructing our own. https://codereview.chromium.org/2698703003/diff/60001/components/url_formatte... components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java:100: spannableUri.toString().indexOf(":"), Spannable.SPAN_EXCLUSIVE_EXCLUSIVE); Please tint only "https:" scheme. Although "file:" and "http://localhost" URLs allow usage of PaymentRequest for developers, these should not be tinted. IMHO.
https://codereview.chromium.org/2698703003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2698703003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:552: ApiCompatibilityUtils.setCompoundDrawablesRelativeWithIntrinsicBounds(hostName, Also, let's not show the lock icon for non-https. In general, let's follow the convention of omnibox. Best case scenario: re-use omnibox's code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
> +palmer@: Would you be the right person to review security enamel related changes? Not anymore. I'd suggest elawrence, estark, or lgarron.
rouslan@chromium.org changed reviewers: - palmer@chromium.org
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Hi rouslan@, PTAL https://codereview.chromium.org/2698703003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2698703003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:552: ApiCompatibilityUtils.setCompoundDrawablesRelativeWithIntrinsicBounds(hostName, On 2017/02/15 21:54:46, rouslan wrote: > Also, let's not show the lock icon for non-https. In general, let's follow the > convention of omnibox. Best case scenario: re-use omnibox's code. Done. Unfortunately, we can not simply use omnibox's code here from what I read in locationBarLayout. But I think we are good for now since we only add compound drawable and tint the scheme for https. And we should not bring up payment request UI if the website is dangerous, like not authenticated, omnibox tints the scheme to red in this case, https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr.... https://codereview.chromium.org/2698703003/diff/60001/components/url_formatte... File components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java (right): https://codereview.chromium.org/2698703003/diff/60001/components/url_formatte... components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java:92: * @param uri The URI. On 2017/02/15 21:45:02, rouslan wrote: > Please align "The" together on these two lines. Done. https://codereview.chromium.org/2698703003/diff/60001/components/url_formatte... components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java:97: public static CharSequence tintUrlSchemeForSecurityDisplay(String uri, int color) { On 2017/02/15 21:45:02, rouslan wrote: > Please pick either URL or URI and use that term throughout your patch. Done. https://codereview.chromium.org/2698703003/diff/60001/components/url_formatte... components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java:97: public static CharSequence tintUrlSchemeForSecurityDisplay(String uri, int color) { On 2017/02/15 21:45:02, rouslan wrote: > How does the omnibox tint the URL scheme? It would be great to call into their > function instead of constructing our own. They do the similar thing. https://codereview.chromium.org/2698703003/diff/60001/components/url_formatte... components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java:100: spannableUri.toString().indexOf(":"), Spannable.SPAN_EXCLUSIVE_EXCLUSIVE); On 2017/02/15 21:45:02, rouslan wrote: > Please tint only "https:" scheme. Although "file:" and "http://localhost" URLs > allow usage of PaymentRequest for developers, these should not be tinted. IMHO. Done.
https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/re... File chrome/android/java/res/layout/payment_request.xml (right): https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/re... chrome/android/java/res/layout/payment_request.xml:35: <LinearLayout Doing this means that the buttons will never stack on top of each other on small screens. Is that really intended? Your screenshot doesn't show anything except English; can you provide one in Greek? https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/re... chrome/android/java/res/layout/payment_request.xml:54: android:visibility="invisible"/> 1) Might as well just keep this visible. It doesn't draw anything either way. 2) Insert a space before />, to match the rest of the file. https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:558: null, null, null); newline here to avoid comment sandwich https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUiErrorView.java (right): https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUiErrorView.java:42: TextView hostName = (TextView) findViewById(R.id.hostname); This code is duplicated. Put it somewhere common. https://codereview.chromium.org/2698703003/diff/120001/components/url_formatt... File components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java (right): https://codereview.chromium.org/2698703003/diff/120001/components/url_formatt... components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java:94: * No newline on 94. https://codereview.chromium.org/2698703003/diff/120001/components/url_formatt... components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java:97: public static CharSequence tintUrlSchemeForSecurityDisplay(String uri, int color) { Find another place for this. The new code is Payments specific and in a class that is a "wrapper for utilities in url_formatter", a native class.
https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:550: if (origin.startsWith(UrlConstants.HTTPS_SCHEME)) { Let's use HTTPS_URL_PREFIX instead of HTTPS_SCHEME here, because that includes "://" after "https" as well.
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Hi dfalcantara@ and rouslan@, please take another look, https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/re... File chrome/android/java/res/layout/payment_request.xml (right): https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/re... chrome/android/java/res/layout/payment_request.xml:35: <LinearLayout On 2017/02/16 19:18:56, dfalcantara (load balance plz) wrote: > Doing this means that the buttons will never stack on top of each other on small > screens. Is that really intended? Your screenshot doesn't show anything except > English; can you provide one in Greek? Yes, to avoid another level of viewgroup. We have done this for "Sign in" and "cancel" buttons in the sign in page without seeing problem (https://cs.chromium.org/chromium/src/chrome/android/java/res/layout/account_s...). These two simple buttons must be able to fit in a single row for all devices. Tested on smallest emulated device. Refer the comments in the bug for details https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/re... chrome/android/java/res/layout/payment_request.xml:54: android:visibility="invisible"/> On 2017/02/16 19:18:56, dfalcantara (load balance plz) wrote: > 1) Might as well just keep this visible. It doesn't draw anything either way. > > 2) Insert a space before />, to match the rest of the file. Done. https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:550: if (origin.startsWith(UrlConstants.HTTPS_SCHEME)) { On 2017/02/16 19:41:51, rouslan wrote: > Let's use HTTPS_URL_PREFIX instead of HTTPS_SCHEME here, because that includes > "://" after "https" as well. Done. https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:558: null, null, null); On 2017/02/16 19:18:56, dfalcantara (load balance plz) wrote: > newline here to avoid comment sandwich Done. https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUiErrorView.java (right): https://codereview.chromium.org/2698703003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUiErrorView.java:42: TextView hostName = (TextView) findViewById(R.id.hostname); On 2017/02/16 19:18:57, dfalcantara (load balance plz) wrote: > This code is duplicated. Put it somewhere common. Done. https://codereview.chromium.org/2698703003/diff/120001/components/url_formatt... File components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java (right): https://codereview.chromium.org/2698703003/diff/120001/components/url_formatt... components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java:94: * On 2017/02/16 19:18:57, dfalcantara (load balance plz) wrote: > No newline on 94. Done. https://codereview.chromium.org/2698703003/diff/120001/components/url_formatt... components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java:97: public static CharSequence tintUrlSchemeForSecurityDisplay(String uri, int color) { On 2017/02/16 19:18:57, dfalcantara (load balance plz) wrote: > Find another place for this. The new code is Payments specific and in a class > that is a "wrapper for utilities in url_formatter", a native class. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/payment_request_bottom_bar.xml (right): https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/payment_request_bottom_bar.xml:17: <ImageView Indent by 4s https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/payment_request_header.xml (right): https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/payment_request_header.xml:59: android:ellipsize="end" Don't ellipsize at the end. You're supposed to ellipsize the beginning for security reasons (spoofing the beginning of long urls) https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java (right): https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:31: findViewById(R.id.logo).setVisibility(View.VISIBLE); This will end up causing another measure pass. Can you just setMeasuredDimension(0) on what you're setting to GONE and measure the logo with what space remains? https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestHeader.java (right): https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestHeader.java:71: spannableUri.toString().indexOf(":"), Spannable.SPAN_EXCLUSIVE_EXCLUSIVE); Shouldn't this be INCLUSIVE_EXCLUSIVE?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #4 (id:200001) has been deleted
The CQ bit was checked by gogerald@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...
The CQ bit was checked by gogerald@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...
Hi dfalcantara@, PTAL, https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/payment_request_bottom_bar.xml (right): https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/payment_request_bottom_bar.xml:17: <ImageView On 2017/02/17 01:53:54, dfalcantara (load balance plz) wrote: > Indent by 4s Done. https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/payment_request_header.xml (right): https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/payment_request_header.xml:59: android:ellipsize="end" On 2017/02/17 01:53:54, dfalcantara (load balance plz) wrote: > Don't ellipsize at the end. You're supposed to ellipsize the beginning for > security reasons (spoofing the beginning of long urls) Done. https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java (right): https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:31: findViewById(R.id.logo).setVisibility(View.VISIBLE); On 2017/02/17 01:53:54, dfalcantara (load balance plz) wrote: > This will end up causing another measure pass. Can you just > setMeasuredDimension(0) on what you're setting to GONE and measure the logo with > what space remains? Do not completely get your idea of setMeasuredDimension(0) to save one time of onMeasure. Do you mean measure each child independently and calculate the space to decide which one we should show? Anyway, we already did two times onMeasure here even without setting the logo gone. That might because of the linearlayout. Sets the logo gone does not increase number of onMeasure in this case. In addition, onMeasure should be very cheap on this flat linearlayout and set gone happens very rarely. Might be better to keep it like this for simplicity. https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestHeader.java (right): https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestHeader.java:71: spannableUri.toString().indexOf(":"), Spannable.SPAN_EXCLUSIVE_EXCLUSIVE); On 2017/02/17 01:53:54, dfalcantara (load balance plz) wrote: > Shouldn't this be INCLUSIVE_EXCLUSIVE? I think SPAN_EXCLUSIVE_EXCLUSIVE is better since we do not expect any changes. Tried to keep it consistent with what we did in ominibox https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java (right): https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:31: findViewById(R.id.logo).setVisibility(View.VISIBLE); On 2017/02/17 17:28:28, gogerald1 wrote: > On 2017/02/17 01:53:54, dfalcantara (load balance plz) wrote: > > This will end up causing another measure pass. Can you just > > setMeasuredDimension(0) on what you're setting to GONE and measure the logo > with > > what space remains? > > Do not completely get your idea of setMeasuredDimension(0) to save one time of > onMeasure. Do you mean measure each child independently and calculate the space > to decide which one we should show? Anyway, we already did two times onMeasure > here even without setting the logo gone. That might because of the linearlayout. > Sets the logo gone does not increase number of onMeasure in this case. In > addition, onMeasure should be very cheap on this flat linearlayout and set gone > happens very rarely. Might be better to keep it like this for simplicity. Simple doesn't always mean correct or efficient. LinearLayout always does two passes: one to figure out how big everything wants to be and one to make everything fit within the Layout (weights applied). You're saying that you checked that another onMeasure() pass didn't happen with you changing View visibility here? I find that extremely odd, given that visibility changes mean that children need to be re-measured (including the Space, which will need to expand to account for the now smaller logo). This code also only ever sets the logo to gone once; you don't account for the available space increasing, which can happen in split screen mode or if the user rotates their device.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java (right): https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:31: findViewById(R.id.logo).setVisibility(View.VISIBLE); On 2017/02/17 19:02:55, dfalcantara (load balance plz) wrote: > On 2017/02/17 17:28:28, gogerald1 wrote: > > On 2017/02/17 01:53:54, dfalcantara (load balance plz) wrote: > > > This will end up causing another measure pass. Can you just > > > setMeasuredDimension(0) on what you're setting to GONE and measure the logo > > with > > > what space remains? > > > > Do not completely get your idea of setMeasuredDimension(0) to save one time of > > onMeasure. Do you mean measure each child independently and calculate the > space > > to decide which one we should show? Anyway, we already did two times onMeasure > > here even without setting the logo gone. That might because of the > linearlayout. > > Sets the logo gone does not increase number of onMeasure in this case. In > > addition, onMeasure should be very cheap on this flat linearlayout and set > gone > > happens very rarely. Might be better to keep it like this for simplicity. > > Simple doesn't always mean correct or efficient. > > LinearLayout always does two passes: one to figure out how big everything wants > to be and one to make everything fit within the Layout (weights applied). > You're saying that you checked that another onMeasure() pass didn't happen with > you changing View visibility here? I find that extremely odd, given that > visibility changes mean that children need to be re-measured (including the > Space, which will need to expand to account for the now smaller logo). > > This code also only ever sets the logo to gone once; you don't account for the > available space increasing, which can happen in split screen mode or if the user > rotates their device. Might strange, but that's what I observed, it might could be optimized since the gone view has fixed width and height. Anyway, measured the bottom bar in onMeasure instead of using super.onMeasure. https://codereview.chromium.org/2698703003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:31: findViewById(R.id.logo).setVisibility(View.VISIBLE); On 2017/02/17 19:02:55, dfalcantara (load balance plz) wrote: > On 2017/02/17 17:28:28, gogerald1 wrote: > > On 2017/02/17 01:53:54, dfalcantara (load balance plz) wrote: > > > This will end up causing another measure pass. Can you just > > > setMeasuredDimension(0) on what you're setting to GONE and measure the logo > > with > > > what space remains? > > > > Do not completely get your idea of setMeasuredDimension(0) to save one time of > > onMeasure. Do you mean measure each child independently and calculate the > space > > to decide which one we should show? Anyway, we already did two times onMeasure > > here even without setting the logo gone. That might because of the > linearlayout. > > Sets the logo gone does not increase number of onMeasure in this case. In > > addition, onMeasure should be very cheap on this flat linearlayout and set > gone > > happens very rarely. Might be better to keep it like this for simplicity. > > Simple doesn't always mean correct or efficient. > > LinearLayout always does two passes: one to figure out how big everything wants > to be and one to make everything fit within the Layout (weights applied). > You're saying that you checked that another onMeasure() pass didn't happen with > you changing View visibility here? I find that extremely odd, given that > visibility changes mean that children need to be re-measured (including the > Space, which will need to expand to account for the now smaller logo). > > This code also only ever sets the logo to gone once; you don't account for the > available space increasing, which can happen in split screen mode or if the user > rotates their device. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Adds the support of rotation, ptal,
The CQ bit was checked by gogerald@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...
corrected the variable names, please review the patch 8,
https://codereview.chromium.org/2698703003/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java (right): https://codereview.chromium.org/2698703003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:26: View logoWithName = findViewById(R.id.logo_name); Cache the views in onFinishInflate() so you're not searching every single time the group is measured. https://codereview.chromium.org/2698703003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:43: int totolWidth = MeasureSpec.getSize(widthMeasureSpec); totalWidth https://codereview.chromium.org/2698703003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:45: primaryButtonWidth + secondaryButtonWidth + getPaddingLeft() + getPaddingRight(); This calculation doesn't account for the margins defined on the LayoutParams. Those aren't currently set in your layout, but you should try to properly handle them. https://codereview.chromium.org/2698703003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:59: : "Screen width is expected to fit the two buttons plus the logo at least."; 1) Asserting here isn't helpful; the fallback case of just showing the buttons is as good as you can get. 2) You're not checking if blankSpaceWidth is negative; I'm not sure what measureChild will do in that case. You should clamp blankSpaceWidth >= 0. https://codereview.chromium.org/2698703003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:70: + Math.max(logoWithName.getMeasuredHeight(), Pull out the max height calculation. The indentation makes it painful to read.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by gogerald@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...
Patchset #9 (id:320001) has been deleted
The CQ bit was checked by gogerald@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks, ptal, https://codereview.chromium.org/2698703003/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java (right): https://codereview.chromium.org/2698703003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:26: View logoWithName = findViewById(R.id.logo_name); On 2017/02/17 23:17:10, dfalcantara (load balance plz) wrote: > Cache the views in onFinishInflate() so you're not searching every single time > the group is measured. Done. https://codereview.chromium.org/2698703003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:43: int totolWidth = MeasureSpec.getSize(widthMeasureSpec); On 2017/02/17 23:17:11, dfalcantara (load balance plz) wrote: > totalWidth Done. https://codereview.chromium.org/2698703003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:45: primaryButtonWidth + secondaryButtonWidth + getPaddingLeft() + getPaddingRight(); On 2017/02/17 23:17:11, dfalcantara (load balance plz) wrote: > This calculation doesn't account for the margins defined on the LayoutParams. > Those aren't currently set in your layout, but you should try to properly handle > them. Done. https://codereview.chromium.org/2698703003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:59: : "Screen width is expected to fit the two buttons plus the logo at least."; On 2017/02/17 23:17:11, dfalcantara (load balance plz) wrote: > 1) Asserting here isn't helpful; the fallback case of just showing the buttons > is as good as you can get. > > 2) You're not checking if blankSpaceWidth is negative; I'm not sure what > measureChild will do in that case. You should clamp blankSpaceWidth >= 0. Done. Thought negative would be considered as zero. https://codereview.chromium.org/2698703003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:70: + Math.max(logoWithName.getMeasuredHeight(), On 2017/02/17 23:17:11, dfalcantara (load balance plz) wrote: > Pull out the max height calculation. The indentation makes it painful to read. Done.
The CQ bit was checked by gogerald@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
LGTM % nits By the way, the elide is green: https://drive.google.com/open?id=1nbDbfsKJSSipZ0v-lFSACS4LDJCSgCfleyPx3WD9kQo Is that fixable? Not necessary to do in this patch, because it's lower priority. https://codereview.chromium.org/2698703003/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java (right): https://codereview.chromium.org/2698703003/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:24: public PaymentRequestBottomBar(Context context, AttributeSet attrs) { nit: javadoc. https://codereview.chromium.org/2698703003/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestHeader.java (right): https://codereview.chromium.org/2698703003/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestHeader.java:26: public PaymentRequestHeader(Context context, AttributeSet attrs) { nit: javadoc.
Thanks, created a lower priority bug (https://bugs.chromium.org/p/chromium/issues/detail?id=693615) for the 'elide is green' as mentioned. https://codereview.chromium.org/2698703003/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java (right): https://codereview.chromium.org/2698703003/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestBottomBar.java:24: public PaymentRequestBottomBar(Context context, AttributeSet attrs) { On 2017/02/21 19:51:08, rouslan wrote: > nit: javadoc. Done. https://codereview.chromium.org/2698703003/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestHeader.java (right): https://codereview.chromium.org/2698703003/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestHeader.java:26: public PaymentRequestHeader(Context context, AttributeSet attrs) { On 2017/02/21 19:51:08, rouslan wrote: > nit: javadoc. Done.
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2698703003/#ps360001 (title: "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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by gogerald@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by gogerald@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": 360001, "attempt_start_ts": 1487724286217610, "parent_rev": "5ccaaeacdde0e1029f3f5e42d369f28ee9c1329c", "commit_rev": "41f07f2348e5ccec788938057ff29fea5876ff57"}
Message was sent while issue was closed.
Description was changed from ========== Add UI elements to secure branding for payments BUG=691104 ========== to ========== Add UI elements to secure branding for payments BUG=691104 Review-Url: https://codereview.chromium.org/2698703003 Cr-Commit-Position: refs/heads/master@{#451873} Committed: https://chromium.googlesource.com/chromium/src/+/41f07f2348e5ccec788938057ff2... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:360001) as https://chromium.googlesource.com/chromium/src/+/41f07f2348e5ccec788938057ff2... |