Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815643006:
Fix location aspects of search engine settings UI Android UI (Closed)
DescriptionFix location aspects of search engine settings UI Android UI
The search engines UI had two bugs which are fixed in this CL:
- location is not supported for non-HTTPS search engines, but the 'location is blocked' message was shown, and was clickable, making it seem like it could be used
- when system location is off, a link should be shown for search engines that have location enabled or blocked to allow it to be turned on. This was only visible for HTTP search engines.
BUG=703516
Review-Url: https://codereview.chromium.org/2815643006
Cr-Commit-Position: refs/heads/master@{#465950}
Committed: https://chromium.googlesource.com/chromium/src/+/3040583e9e0ddd28ac850e615b8d99418cfa98a3
Patch Set 1 #Patch Set 2 : Handle not-set https engines #Patch Set 3 : Fix java noob mistake #Patch Set 4 : Only show 'turn on location' link if location is allowed #
Total comments: 2
Patch Set 5 : Rebase #Patch Set 6 : Show blocked when location is off and blocked #
Total comments: 4
Patch Set 7 : Remove confusing comment #Messages
Total messages: 35 (25 generated)
The CQ bit was checked by benwells@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.
The CQ bit was checked by benwells@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: 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 benwells@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...
Description was changed from ========== Fix location aspects of search engine settings UI Android UI The search engines UI had two bugs which are fixed in this CL: - the location setting is not available at all for non-HTTPS search engines, but was still being shown - when system location is off a link should be shown to allow it to be turned on. This was only visible for HTTP search engines. BUG=703516 ========== to ========== Fix location aspects of search engine settings UI Android UI The search engines UI had two bugs which are fixed in this CL: - location is not supported for non-HTTPS search engines, but the 'location is blocked' message was shown, and was clickable, making it seem like it could be used - when system location is off, a link should be shown for search engines that have location enabled or blocked to allow it to be turned on. This was only visible for HTTP search engines. BUG=703516 ==========
benwells@chromium.org changed reviewers: + twellington@chromium.org
After writing the commit description I think we should hide the 'click to turn on location' message when location is blocked, and only show it when location is allowed... WDYT?
The CQ bit was checked by benwells@chromium.org to run a CQ dry run
On 2017/04/13 01:48:11, benwells wrote: > After writing the commit description I think we should hide the 'click to turn > on location' message when location is blocked, and only show it when location is > allowed... > > WDYT? I just put up another patch set with that change, and added you to the bug for this.
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.
twellington@chromium.org changed reviewers: + finnur@chromium.org
+finnur@ - I think you're more familiar with this code than I am. Do you mind reviewing?
On 2017/04/13 02:46:49, Theresa wrote: > +finnur@ - I think you're more familiar with this code than I am. Do you mind > reviewing? Cool - Finnur this is why I was simplifying things with the last change you reviewed for me. Sorry for reviewer churn, I got the impression you weren't too familiar with this code either.
Sorry for the late response, Easter is a long holiday here. LGTM, one nit/question. https://codereview.chromium.org/2815643006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2815643006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:464: if (locationPermission != ContentSetting.ASK) return true; I forget... Are you guaranteed not to get ContentSetting.DEFAULT here?
The CQ bit was checked by benwells@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.
Finnur - PTAL, I've updated based on Kingston's advice on the bug. https://codereview.chromium.org/2815643006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2815643006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:464: if (locationPermission != ContentSetting.ASK) return true; On 2017/04/18 10:59:21, Finnur wrote: > I forget... Are you guaranteed not to get ContentSetting.DEFAULT here? Yep, that's right. The content settings system will resolve default to BLOCK, ALLOW or ASK, depending on what the default is.
Just one question and a nit. If the answer to the question is yes, then LGTM. https://codereview.chromium.org/2815643006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2815643006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:357: && !LocationUtils.getInstance().isSystemLocationSettingEnabled()) { I initially thought, wait a minute, but now talked myself into thinking this is better. This way, if my favorite search engine is blocked _and_ system location is off, I won't get the message that system location is off and think that enabling it is enough to enable location for my search engine also. Furhtermore, there's no ambiguity of what will happen if I enable system location for other reasons -- I won't have to check the UI again to see if my search engine has inadvertently been granted location also. Right? https://codereview.chromium.org/2815643006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:362: // Otherwise either location is allowed and enabled, or location is blocked. I think this comment just confuses matters... I would just remove it.
https://codereview.chromium.org/2815643006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2815643006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:357: && !LocationUtils.getInstance().isSystemLocationSettingEnabled()) { On 2017/04/19 14:28:56, Finnur wrote: > I initially thought, wait a minute, but now talked myself into thinking this is > better. This way, if my favorite search engine is blocked _and_ system location > is off, I won't get the message that system location is off and think that > enabling it is enough to enable location for my search engine also. Furhtermore, > there's no ambiguity of what will happen if I enable system location for other > reasons -- I won't have to check the UI again to see if my search engine has > inadvertently been granted location also. Right? Right :) https://codereview.chromium.org/2815643006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:362: // Otherwise either location is allowed and enabled, or location is blocked. On 2017/04/19 14:28:56, Finnur wrote: > I think this comment just confuses matters... I would just remove it. Done.
The CQ bit was checked by benwells@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org Link to the patchset: https://codereview.chromium.org/2815643006/#ps120001 (title: "Remove confusing comment")
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": 1492674900792810,
"parent_rev": "d8aa2f436c16b14c49320c78dca9ae64388ef3cd", "commit_rev":
"3040583e9e0ddd28ac850e615b8d99418cfa98a3"}
Message was sent while issue was closed.
Description was changed from ========== Fix location aspects of search engine settings UI Android UI The search engines UI had two bugs which are fixed in this CL: - location is not supported for non-HTTPS search engines, but the 'location is blocked' message was shown, and was clickable, making it seem like it could be used - when system location is off, a link should be shown for search engines that have location enabled or blocked to allow it to be turned on. This was only visible for HTTP search engines. BUG=703516 ========== to ========== Fix location aspects of search engine settings UI Android UI The search engines UI had two bugs which are fixed in this CL: - location is not supported for non-HTTPS search engines, but the 'location is blocked' message was shown, and was clickable, making it seem like it could be used - when system location is off, a link should be shown for search engines that have location enabled or blocked to allow it to be turned on. This was only visible for HTTP search engines. BUG=703516 Review-Url: https://codereview.chromium.org/2815643006 Cr-Commit-Position: refs/heads/master@{#465950} Committed: https://chromium.googlesource.com/chromium/src/+/3040583e9e0ddd28ac850e615b8d... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/3040583e9e0ddd28ac850e615b8d... |
