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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 285233007: Remove built-in pages that don't work on Android from suggest. (Closed)

Created:
6 years, 7 months ago by Maria
Modified:
6 years, 7 months ago
Reviewers:
Peter Kasting, Mark P
CC:
chromium-reviews, James Su, newt (away), aurimas (slooooooooow)
Visibility:
Public.

Description

Remove built-in pages that don't work on Android from suggest. Also gets rid of Android dependency on quota-internals, which doesn't work on Android. BUG=373943 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271171

Patch Set 1 #

Patch Set 2 : Make fixes to include rules #

Total comments: 2

Patch Set 3 : Reorder constants #

Total comments: 2

Patch Set 4 : Switch OS order #

Patch Set 5 : Fixed builtin provider unit test #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+89 lines, -32 lines) Patch
M chrome/browser/autocomplete/builtin_provider.cc View 4 chunks +7 lines, -0 lines 0 comments Download
M chrome/browser/autocomplete/builtin_provider_unittest.cc View 1 2 3 4 2 chunks +49 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc View 1 4 chunks +3 lines, -3 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/url_constants.cc View 1 2 3 4 chunks +29 lines, -29 lines 2 comments Download

Messages

Total messages: 15 (0 generated)
Maria
6 years, 7 months ago (2014-05-15 22:50:25 UTC) #1
Peter Kasting
LGTM https://codereview.chromium.org/285233007/diff/20001/chrome/common/url_constants.cc File chrome/common/url_constants.cc (right): https://codereview.chromium.org/285233007/diff/20001/chrome/common/url_constants.cc#newcode563 chrome/common/url_constants.cc:563: kChromeUICacheHost, This section is a bit of a ...
6 years, 7 months ago (2014-05-15 22:54:17 UTC) #2
Maria
https://codereview.chromium.org/285233007/diff/20001/chrome/common/url_constants.cc File chrome/common/url_constants.cc (right): https://codereview.chromium.org/285233007/diff/20001/chrome/common/url_constants.cc#newcode563 chrome/common/url_constants.cc:563: kChromeUICacheHost, On 2014/05/15 22:54:18, Peter Kasting wrote: > This ...
6 years, 7 months ago (2014-05-15 23:00:22 UTC) #3
Peter Kasting
Still LGTM https://codereview.chromium.org/285233007/diff/40001/chrome/common/url_constants.cc File chrome/common/url_constants.cc (right): https://codereview.chromium.org/285233007/diff/40001/chrome/common/url_constants.cc#newcode597 chrome/common/url_constants.cc:597: #if defined(OS_ANDROID) || defined(OS_IOS) Tiny nit: Maybe ...
6 years, 7 months ago (2014-05-15 23:02:17 UTC) #4
Maria
https://codereview.chromium.org/285233007/diff/40001/chrome/common/url_constants.cc File chrome/common/url_constants.cc (right): https://codereview.chromium.org/285233007/diff/40001/chrome/common/url_constants.cc#newcode597 chrome/common/url_constants.cc:597: #if defined(OS_ANDROID) || defined(OS_IOS) On 2014/05/15 23:02:18, Peter Kasting ...
6 years, 7 months ago (2014-05-16 00:20:50 UTC) #5
Maria
The CQ bit was checked by mariakhomenko@chromium.org
6 years, 7 months ago (2014-05-16 00:20:55 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mariakhomenko@chromium.org/285233007/60001
6 years, 7 months ago (2014-05-16 00:22:27 UTC) #7
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are ...
6 years, 7 months ago (2014-05-16 02:46:33 UTC) #8
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 7 months ago (2014-05-16 04:03:36 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered_tests/builds/154375)
6 years, 7 months ago (2014-05-16 04:03:36 UTC) #10
Maria
The CQ bit was checked by mariakhomenko@chromium.org
6 years, 7 months ago (2014-05-16 18:35:46 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mariakhomenko@chromium.org/285233007/80001
6 years, 7 months ago (2014-05-16 18:36:49 UTC) #12
commit-bot: I haz the power
Change committed as 271171
6 years, 7 months ago (2014-05-17 05:13:56 UTC) #13
Mark P
https://codereview.chromium.org/285233007/diff/80001/chrome/common/url_constants.cc File chrome/common/url_constants.cc (right): https://codereview.chromium.org/285233007/diff/80001/chrome/common/url_constants.cc#newcode567 chrome/common/url_constants.cc:567: kChromeUICreditsHost, This is an array. Does reordering it change ...
6 years, 7 months ago (2014-05-19 18:30:19 UTC) #14
Peter Kasting
6 years, 7 months ago (2014-05-19 18:50:11 UTC) #15
Message was sent while issue was closed. 
https://codereview.chromium.org/285233007/diff/80001/chrome/common/url_consta...
File chrome/common/url_constants.cc (right):

https://codereview.chromium.org/285233007/diff/80001/chrome/common/url_consta...
chrome/common/url_constants.cc:567: kChromeUICreditsHost,
On 2014/05/19 18:30:20, Mark P wrote:
> This is an array.  Does reordering it change the order in which URLs appear in
> about:about?  If so, I'd suggest reverting this part of this change.  (Being
> alphabetized--regardless of platform--is handy.)

No.  about_ui.cc sorts the list itself.  Which is good, because trying to keep
this list ordered alphabetically by the values of these strings, instead of
their names, would be that much more confusing.

Powered by Google App Engine
This is Rietveld 408576698