[Android] Hide "Recently visited" text if there is not custom search engine

[Android] Hide "Recently visited" text if there is not custom search engine

Currently even there is no custom search engine to show (such as clean
start), "Recently visited" header will still be displayed. This
is confusing for users.

To fix this, there is no custom search engine to show, remove the "Recently
visited" header.

BUG=679608
Review-Url: https://codereview.chromium.org/2623113002
Cr-Commit-Position: refs/heads/master@{#443476}
Committed: https://chromium.googlesource.com/chromium/src/+/1b87f94692daf34c6525f0beaf798aab1657380a

[ltian, 2017-01-11 02:08:54]

The CQ bit was checked by ltian@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 02:09:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 03:56:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 03:56:36]

Dry run: This issue passed the CQ dry run.

[ltian, 2017-01-11 04:59:34]

ltian@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[ltian, 2017-01-11 04:59:34]

dfalcantara@chromium.org: can you take a look of my change in this CL?

Thanks!

[gone, 2017-01-11 18:11:47]

Also your title is kind of weird.  "is no custom search engine" maybe?

https://codereview.chromium.org/2623113002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
(right):

https://codereview.chromium.org/2623113002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:158:
return mPrepopulatedSearchEngines == null ? 0
Any time it's this complicated, use an if block.  The indentation here makes it
really hard to understand what you're actually doing and makes it look like the
second level belongs to the first because it's indented less than the condition
it actually belongs to.

https://codereview.chromium.org/2623113002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:378:
if (mRecentSearchEngines.size() > 0) {
You're effectively returning 0 or adding one to the true size, right?  Add a
comment explaining why here.  Makes it so that the next person looking at this
code doesn't have to look up your commit message to understand why.

[gone, 2017-01-11 18:12:05]

On 2017/01/11 18:11:47, dfalcantara (check queue) wrote:
> Also your title is kind of weird.  "is no custom search engine" maybe?
> 
>
https://codereview.chromium.org/2623113002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
> (right):
> 
>
https://codereview.chromium.org/2623113002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:158:
> return mPrepopulatedSearchEngines == null ? 0
> Any time it's this complicated, use an if block.  The indentation here makes
it
> really hard to understand what you're actually doing and makes it look like
the
> second level belongs to the first because it's indented less than the
condition
> it actually belongs to.
> 
>
https://codereview.chromium.org/2623113002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:378:
> if (mRecentSearchEngines.size() > 0) {
> You're effectively returning 0 or adding one to the true size, right?  Add a
> comment explaining why here.  Makes it so that the next person looking at this
> code doesn't have to look up your commit message to understand why.

Or maybe "no custom search engine set"?

[ltian, 2017-01-12 19:45:12]

The CQ bit was checked by ltian@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 19:45:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 20:17:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 20:17:37]

Dry run: This issue passed the CQ dry run.

[ltian, 2017-01-12 21:18:54]

https://codereview.chromium.org/2623113002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
(right):

https://codereview.chromium.org/2623113002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:158:
return mPrepopulatedSearchEngines == null ? 0
On 2017/01/11 18:11:47, dfalcantara (check queue) wrote:
> Any time it's this complicated, use an if block.  The indentation here makes
it
> really hard to understand what you're actually doing and makes it look like
the
> second level belongs to the first because it's indented less than the
condition
> it actually belongs to.

Done.

https://codereview.chromium.org/2623113002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:378:
if (mRecentSearchEngines.size() > 0) {
On 2017/01/11 18:11:47, dfalcantara (check queue) wrote:
> You're effectively returning 0 or adding one to the true size, right?  Add a
> comment explaining why here.  Makes it so that the next person looking at this
> code doesn't have to look up your commit message to understand why.

Done.

[gone, 2017-01-12 21:26:06]

https://codereview.chromium.org/2623113002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
(right):

https://codereview.chromium.org/2623113002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:163:
return 0;
Was trying to get rid of the ternary operators entirely and make it easier to
understand what was going on here.  How about this:

{
    int size = 0;

    if (mPrepopulatedSearchEngines != null) {
        size += mPrepopulatedEngines.size();
    }

    if (mRecentSearchEngines.size() != null && mRecentSearchEngines.size() != 0)
{
        // Account for the header by adding one to the size.
        size = mRecentSearchEngines.size() + 1;
    }

    return size;
}

[ltian, 2017-01-13 00:09:42]

https://codereview.chromium.org/2623113002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
(right):

https://codereview.chromium.org/2623113002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:163:
return 0;
On 2017/01/12 21:26:05, dfalcantara (check queue) wrote:
> Was trying to get rid of the ternary operators entirely and make it easier to
> understand what was going on here.  How about this:
> 
> {
>     int size = 0;
> 
>     if (mPrepopulatedSearchEngines != null) {
>         size += mPrepopulatedEngines.size();
>     }
> 
>     if (mRecentSearchEngines.size() != null && mRecentSearchEngines.size() !=
0)
> {
>         // Account for the header by adding one to the size.
>         size = mRecentSearchEngines.size() + 1;
>     }
> 
>     return size;
> }

Done.

[gone, 2017-01-13 01:04:14]

lgtm

[ltian, 2017-01-13 02:09:50]

The CQ bit was checked by ltian@chromium.org

[commit-bot: I haz the power, 2017-01-13 02:10:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 03:40:28]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484273390540450,
"parent_rev": "5cc4c469882d169f8efb789ff82235252f40f270", "commit_rev":
"1b87f94692daf34c6525f0beaf798aab1657380a"}

[commit-bot: I haz the power, 2017-01-13 03:41:04]

Description was changed from

==========
[Android] Hide "Recently visited" text if there is not custom search engine

Currently even there is no custom search engine to show (such as clean
start), "Recently visited" header will still be displayed. This
is confusing for users.

To fix this, there is no custom search engine to show, remove the "Recently
visited" header.

BUG=679608
==========

to

==========
[Android] Hide "Recently visited" text if there is not custom search engine

Currently even there is no custom search engine to show (such as clean
start), "Recently visited" header will still be displayed. This
is confusing for users.

To fix this, there is no custom search engine to show, remove the "Recently
visited" header.

BUG=679608

Review-Url: https://codereview.chromium.org/2623113002
Cr-Commit-Position: refs/heads/master@{#443476}
Committed:
https://chromium.googlesource.com/chromium/src/+/1b87f94692daf34c6525f0beaf79...
==========

[commit-bot: I haz the power, 2017-01-13 03:41:05]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/1b87f94692daf34c6525f0beaf79...