Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(48)

Issue 2480153003: Add a TLD whitelist for Google Search URLs. (Closed)

Created:
4 years, 1 month ago by Maria
Modified:
4 years, 1 month ago
CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, agrieve+watch_chromium.org, kmadhusu+watch_chromium.org, Jered
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add a TLD whitelist for Google Search URLs. Also send an extra to Instant Apps when we have verified a page is SERP. BUG=656193, 658914 Committed: https://crrev.com/b5b2836070cadd1bea1d786593efeeb45893c342 Cr-Commit-Position: refs/heads/master@{#431670}

Patch Set 1 #

Patch Set 2 : Remove a log statement #

Patch Set 3 : Update tld list from domainlist. #

Patch Set 4 : Disable intent:// links to instant apps from non-SERP pages. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+76 lines, -4 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java View 1 2 3 2 chunks +14 lines, -2 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/instantapps/InstantAppsHandler.java View 2 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/android/url_utilities.cc View 1 2 2 chunks +58 lines, -1 line 2 comments Download

Messages

Total messages: 17 (9 generated)
Maria
4 years, 1 month ago (2016-11-07 21:52:44 UTC) #2
Ted C
lgtm
4 years, 1 month ago (2016-11-08 01:03:39 UTC) #4
Ted C
On 2016/11/08 01:03:39, Ted C wrote: > lgtm still lgtm
4 years, 1 month ago (2016-11-11 21:43:51 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2480153003/60001
4 years, 1 month ago (2016-11-11 22:12:41 UTC) #11
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 1 month ago (2016-11-11 22:28:56 UTC) #12
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/b5b2836070cadd1bea1d786593efeeb45893c342 Cr-Commit-Position: refs/heads/master@{#431670}
4 years, 1 month ago (2016-11-11 22:44:30 UTC) #14
battre (please use the other)
https://codereview.chromium.org/2480153003/diff/60001/chrome/browser/android/url_utilities.cc File chrome/browser/android/url_utilities.cc (right): https://codereview.chromium.org/2480153003/diff/60001/chrome/browser/android/url_utilities.cc#newcode29 chrome/browser/android/url_utilities.cc:29: // TODO(mariakhomenko): figure out how to keep this list ...
4 years, 1 month ago (2016-11-11 22:54:10 UTC) #16
Maria
4 years, 1 month ago (2016-11-11 22:57:13 UTC) #17
Message was sent while issue was closed.
On 2016/11/11 22:54:10, battre (please use the other) wrote:
>
https://codereview.chromium.org/2480153003/diff/60001/chrome/browser/android/...
> File chrome/browser/android/url_utilities.cc (right):
> 
>
https://codereview.chromium.org/2480153003/diff/60001/chrome/browser/android/...
> chrome/browser/android/url_utilities.cc:29: // TODO(mariakhomenko): figure out
> how to keep this list updated.
> IsCanonicalHostGoogleHostname in components/google/core/browser/google_util.cc
> could also benefit from this.
> 
>
https://codereview.chromium.org/2480153003/diff/60001/chrome/browser/android/...
> chrome/browser/android/url_utilities.cc:144: bool is_search =
> google_util::IsGoogleSearchUrl(gurl);
> Wouldn't it make sense to move the logic into this?

Yes, I will work on that next. I wanted the logic to be in android-specific
place because I am merging it into branch, so this limits the scope of any
potential regressions. I will move it to shared code on master for the future.

Powered by Google App Engine
This is Rietveld 408576698