Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2791843002:
[subresource_filter] Add the toggle to the new experimental UI (Closed)
|
Created:
3 years, 8 months ago by Charlie Harrison Modified:
3 years, 8 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, dfalcantara+watch_chromium.org, srahim+watch_chromium.org, subresource-filter-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[subresource_filter] Add the toggle to the new experimental UI
This CL removes a button from the expanded infobar, and makes the
primary button have two settings, controlled by a toggle.
Screenshots with this change applied can be found in the linked bug.
BUG=689992
Review-Url: https://codereview.chromium.org/2791843002
Cr-Commit-Position: refs/heads/master@{#462192}
Committed: https://chromium.googlesource.com/chromium/src/+/957f0d0ec3423088198bfaf4db19ab4c60b5a5d9
Patch Set 1 #
Total comments: 6
Patch Set 2 : engedy review #
Total comments: 12
Patch Set 3 : dfalcantara review #
Total comments: 2
Patch Set 4 : add TODO and crbug link #
Messages
Total messages: 28 (16 generated)
The CQ bit was checked by csharrison@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.
Description was changed from ========== [subresource_filter] Add the toggle to the new experimental UI This CL removes a button from the expanded infobar, and makes the primary button have two settings, controlled by a toggle. BUG=689992 ========== to ========== [subresource_filter] Add the toggle to the new experimental UI This CL removes a button from the expanded infobar, and makes the primary button have two settings, controlled by a toggle. Screenshots with this change applied can be found in the linked bug. BUG=689992 ==========
csharrison@chromium.org changed reviewers: + dfalcantara@chromium.org
dfalcantara: PTAL?
csharrison@chromium.org changed reviewers: + engedy@chromium.org, shivanisha@chromium.org
+engedy: Would you also take a look? +shivanisha: PTAL as shadow reviewer.
LGTM % nits, but then again, I am not very familiar with Android UI. https://codereview.chromium.org/2791843002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java (right): https://codereview.chromium.org/2791843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:18: * After user proceed through Safe Browsing warning interstitials that are displayed when the site nit: Could you also do some wordsmithing on this first sentence? Something like: // This infobar appears when the user proceeds through a Safe Browsing ... https://codereview.chromium.org/2791843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:32: private boolean mIsChecked; nit: Let's name this in a way that is more informative. What do you think about mReloadIsToggled|mReloadIsChecked|mToggledReload. https://codereview.chromium.org/2791843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:64: 0, 0, mToggleText, R.id.subresource_filter_infobar_toggle, false); nit: Is it customary in these parts of codebase to add clarification comments like: /* iconResourceId */ ?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2791843002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java (right): https://codereview.chromium.org/2791843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:18: * After user proceed through Safe Browsing warning interstitials that are displayed when the site On 2017/04/04 12:49:01, engedy wrote: > nit: Could you also do some wordsmithing on this first sentence? Something like: > > // This infobar appears when the user proceeds through a Safe Browsing ... Done. https://codereview.chromium.org/2791843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:32: private boolean mIsChecked; On 2017/04/04 12:49:01, engedy wrote: > nit: Let's name this in a way that is more informative. What do you think about > mReloadIsToggled|mReloadIsChecked|mToggledReload. mReloadIsToggled sounds best to me. Done. https://codereview.chromium.org/2791843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:64: 0, 0, mToggleText, R.id.subresource_filter_infobar_toggle, false); On 2017/04/04 12:49:01, engedy wrote: > nit: Is it customary in these parts of codebase to add clarification comments > like: > > /* iconResourceId */ > > ? I don't think so (at least, I never saw this anywhere). I'm not so familiar with Android either so I will defer to experts. dfalcantara, WDYT?
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.
https://codereview.chromium.org/2791843002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java (right): https://codereview.chromium.org/2791843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:19: * displayed when the site ahead contains deceptive embedded content, the infobar appears, it "the infobar appears" now reads really weirdly. https://codereview.chromium.org/2791843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:68: mButton = layout.getPrimaryButton(); newline before this, move the comment from 69 above the mButton call to avoid comment sandwich. https://codereview.chromium.org/2791843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:70: mButton.setEms(Math.max(mOKButtonText.length(), mReloadButtonText.length())); I'm not sure this is the right call to be making; the button might be wider than it needs to be. Did you consider measuring the text manually using mButton.getPaint().measureText(), then using #setMinWidth() on the button? https://codereview.chromium.org/2791843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:90: public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) { In javaland, you don't need to say where it came from, but you do need to add @Override. https://codereview.chromium.org/2791843002/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2791843002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:12313: Reload There should already be an IDS_RELOAD somewhere for you to use.
Thanks for the review, I changed the current impl to use setMinEms, even though I understand it is wrong. https://codereview.chromium.org/2791843002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java (right): https://codereview.chromium.org/2791843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:19: * displayed when the site ahead contains deceptive embedded content, the infobar appears, it On 2017/04/04 20:39:43, dfalcantara (load balance plz) wrote: > "the infobar appears" now reads really weirdly. Oops, fixed it up. https://codereview.chromium.org/2791843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:68: mButton = layout.getPrimaryButton(); On 2017/04/04 20:39:43, dfalcantara (load balance plz) wrote: > newline before this, move the comment from 69 above the mButton call to avoid > comment sandwich. Done. https://codereview.chromium.org/2791843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:70: mButton.setEms(Math.max(mOKButtonText.length(), mReloadButtonText.length())); On 2017/04/04 20:39:44, dfalcantara (load balance plz) wrote: > I'm not sure this is the right call to be making; the button might be wider than > it needs to be. Did you consider measuring the text manually using > mButton.getPaint().measureText(), then using #setMinWidth() on the button? Thanks for the tip, but I tried to use measureText and it seems to not be wide enough for the button. Setting the minWidth based on the measured width doesn't make the button wide enough. A few other things I tried: - Using getTextBounds - Using measureText/getTextBounds in combination with various paddings. Is there something I'm missing? https://codereview.chromium.org/2791843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:90: public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) { On 2017/04/04 20:39:44, dfalcantara (load balance plz) wrote: > In javaland, you don't need to say where it came from, but you do need to add > @Override. Done. https://codereview.chromium.org/2791843002/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2791843002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:12313: Reload On 2017/04/04 20:39:44, dfalcantara (load balance plz) wrote: > There should already be an IDS_RELOAD somewhere for you to use. It looks like there is an IDS_RELOAD in android-only strings, is it possible to use those in C++ code? It looks like all the Reload strings in generated_resources.grd are custom (e.g. IDS_COLLECTED_COOKIES_INFOBAR_BUTTON). If we need to use a string in generated_resources.grd it seems more canonical to use our own.
lgtm % nit Not too familiar with Android UI. Mostly understanding this code as a shadow reviewer. https://codereview.chromium.org/2791843002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java (right): https://codereview.chromium.org/2791843002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:22: * ability to reload the page with the content we've blocked previously. Is this info bar specific to safe browsing sites only as the comment suggests?
https://codereview.chromium.org/2791843002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java (right): https://codereview.chromium.org/2791843002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:22: * ability to reload the page with the content we've blocked previously. On 2017/04/05 15:33:42, shivanisha wrote: > Is this info bar specific to safe browsing sites only as the comment suggests? Yes currently this feature is only enabled for sites navigated to after proceeding through a SB interstitial.
lgtm https://codereview.chromium.org/2791843002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java (right): https://codereview.chromium.org/2791843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:70: mButton.setEms(Math.max(mOKButtonText.length(), mReloadButtonText.length())); On 2017/04/05 13:31:31, Charlie Harrison wrote: > On 2017/04/04 20:39:44, dfalcantara (load balance plz) wrote: > > I'm not sure this is the right call to be making; the button might be wider > than > > it needs to be. Did you consider measuring the text manually using > > mButton.getPaint().measureText(), then using #setMinWidth() on the button? > > Thanks for the tip, but I tried to use measureText and it seems to not be wide > enough for the button. > > Setting the minWidth based on the measured width doesn't make the button wide > enough. > > A few other things I tried: > - Using getTextBounds > - Using measureText/getTextBounds in combination with various paddings. > > Is there something I'm missing? Sadly that's all I can think of. I'll revisit this soon™.
Thanks, let me land this for now and we can circle back on the bug I filed. https://codereview.chromium.org/2791843002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java (right): https://codereview.chromium.org/2791843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/SubresourceFilterExperimentalInfoBar.java:70: mButton.setEms(Math.max(mOKButtonText.length(), mReloadButtonText.length())); On 2017/04/05 18:33:29, dfalcantara (load balance plz) wrote: > On 2017/04/05 13:31:31, Charlie Harrison wrote: > > On 2017/04/04 20:39:44, dfalcantara (load balance plz) wrote: > > > I'm not sure this is the right call to be making; the button might be wider > > than > > > it needs to be. Did you consider measuring the text manually using > > > mButton.getPaint().measureText(), then using #setMinWidth() on the button? > > > > Thanks for the tip, but I tried to use measureText and it seems to not be wide > > enough for the button. > > > > Setting the minWidth based on the measured width doesn't make the button wide > > enough. > > > > A few other things I tried: > > - Using getTextBounds > > - Using measureText/getTextBounds in combination with various paddings. > > > > Is there something I'm missing? > > Sadly that's all I can think of. I'll revisit this soon™. Ok, let me add a TODO and open up a bug to track this.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org, shivanisha@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2791843002/#ps60001 (title: "add TODO and crbug link")
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": 60001, "attempt_start_ts": 1491419188726300,
"parent_rev": "2cfc514b3ee3bc1c29d7e853421fe96fb7a9d7ed", "commit_rev":
"4303887722744245a507ba1e20d56c27a673e763"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491419188726300,
"parent_rev": "2ed842497d2ab4c8dad580e756b84ab1cd8d2f0b", "commit_rev":
"957f0d0ec3423088198bfaf4db19ab4c60b5a5d9"}
Message was sent while issue was closed.
Description was changed from ========== [subresource_filter] Add the toggle to the new experimental UI This CL removes a button from the expanded infobar, and makes the primary button have two settings, controlled by a toggle. Screenshots with this change applied can be found in the linked bug. BUG=689992 ========== to ========== [subresource_filter] Add the toggle to the new experimental UI This CL removes a button from the expanded infobar, and makes the primary button have two settings, controlled by a toggle. Screenshots with this change applied can be found in the linked bug. BUG=689992 Review-Url: https://codereview.chromium.org/2791843002 Cr-Commit-Position: refs/heads/master@{#462192} Committed: https://chromium.googlesource.com/chromium/src/+/957f0d0ec3423088198bfaf4db19... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/957f0d0ec3423088198bfaf4db19... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
