Add bookmark search feature to enhanced bookmark

Add bookmark search feature to enhanced bookmark

Frontend:
1. The UI for bookmark search is implemented as a custom substitutable
View, which on phone takes the entire screen and behave as if an activity; on
tablet replaces the content RecyclerView and shows on the right of the screen.
2. The view tansitions are done by ViewSwitchers, which are placed as the root
of eb_main.xml.
3. EnhancedBookmarkSearchRow, a subclass of EnhancedBookmarkBookmarkRow, was
created to show bookmark results.

Backend:
1. We used the same backend that powers omnibox bookmark query. It
traverses the entire bookmark model and gets prefix matching resutls. A maximum
limit of 500 bookmarks is chosen in case of too large query that blocks the UI
thread.
2. Search history is saved in one single Android SharedPreference. Android does
not have a built-in support for saving list of strings; consequently JSONArray
is chosen to help serializing and deserializing.

The majority of the code has been previously reviewed in
https://chrome-internal-review.googlesource.com/#/c/180705/ and
https://chrome-internal-review.googlesource.com/#/c/216387/

BUG=515617
Committed: https://crrev.com/8178e1386b0005a8e7aec5fefc90cb6d183a1a13
Cr-Commit-Position: refs/heads/master@{#343105}

[Ian Wen, 2015-08-04 01:24:13]

ianwen@chromium.org changed reviewers:
+ newt@chromium.org

[Ian Wen, 2015-08-04 01:28:44]

newt@:

To facilitate your review, you could locally revert
https://codereview.chromium.org/1184613002, and download this patch as well.
Then do a git diff -d to compare the two patches to see the differences.

The major difference between this CL and a simple revert is:
1. BookmarkRow is used to show search results, instead of a custom view
constructed on the fly in the adapter.
2. History in sharedpreference was added.

[newt (away), 2015-08-04 16:38:38]

On 2015/08/04 01:28:44, Ian Wen wrote:
> newt@:
> 
> To facilitate your review, you could locally revert
> https://codereview.chromium.org/1184613002, and download this patch as well.
> Then do a git diff -d to compare the two patches to see the differences.

Could you actually do this revert locally and upload it as patch set 2, then
re-upload your original change? Then reviewers (like me!) can see the diff right
in codereview. Thanks :)

> The major difference between this CL and a simple revert is:
> 1. BookmarkRow is used to show search results, instead of a custom view
> constructed on the fly in the adapter.
> 2. History in sharedpreference was added.

Thanks for the overview. This is definitely helpful.

[Ian Wen, 2015-08-04 17:12:36]

On 2015/08/04 16:38:38, newt wrote:
> On 2015/08/04 01:28:44, Ian Wen wrote:
> > newt@:
> > 
> > To facilitate your review, you could locally revert
> > https://codereview.chromium.org/1184613002, and download this patch as well.
> > Then do a git diff -d to compare the two patches to see the differences.
> 
> Could you actually do this revert locally and upload it as patch set 2, then
> re-upload your original change? Then reviewers (like me!) can see the diff
right
> in codereview. Thanks :)
> 
> > The major difference between this CL and a simple revert is:
> > 1. BookmarkRow is used to show search results, instead of a custom view
> > constructed on the fly in the adapter.
> > 2. History in sharedpreference was added.
> 
> Thanks for the overview. This is definitely helpful.

Done. It was not as helpful as I thought, as some of files were in java_staging
file..

[newt (away), 2015-08-04 17:30:12]

On 2015/08/04 17:12:36, Ian Wen wrote:
> On 2015/08/04 16:38:38, newt wrote:
> > On 2015/08/04 01:28:44, Ian Wen wrote:
> > > newt@:
> > > 
> > > To facilitate your review, you could locally revert
> > > https://codereview.chromium.org/1184613002, and download this patch as
well.
> > > Then do a git diff -d to compare the two patches to see the differences.
> > 
> > Could you actually do this revert locally and upload it as patch set 2, then
> > re-upload your original change? Then reviewers (like me!) can see the diff
> right
> > in codereview. Thanks :)
> > 
> > > The major difference between this CL and a simple revert is:
> > > 1. BookmarkRow is used to show search results, instead of a custom view
> > > constructed on the fly in the adapter.
> > > 2. History in sharedpreference was added.
> > 
> > Thanks for the overview. This is definitely helpful.
> 
> Done. It was not as helpful as I thought, as some of files were in
java_staging
> file..

How about moving the files from java_staging to java?

[Ian Wen, 2015-08-04 18:31:40]

Done.

[newt (away), 2015-08-06 04:17:15]

newt@chromium.org changed reviewers:
+ kkimlabs@chromium.org

[newt (away), 2015-08-06 04:17:15]

Kibeom: could you take a pass at this? I've got a lot on my plate.

[Kibeom Kim (inactive), 2015-08-10 07:47:38]

I didn't go through all, but looks like many things, like list toggle button and
filter code are unintentionally included by reverting. At this state I think
maybe it's better to just copy-and paste the parts we need instead of reverting.

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout-sw720dp/eb_main.xml (right):

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout-sw720dp/eb_main.xml:19:
android:dividerHeight="0dp" />
Is 240dp and @color/default_primary_color intentional?

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/res...
File chrome/android/java/res/menu/eb_action_bar_menu.xml (right):

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/res...
chrome/android/java/res/menu/eb_action_bar_menu.xml:20:
chrome:showAsAction="ifRoom"/>
Is this a mistake?

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:388: */
Wasn't this async API before?

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java
(right):

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java:5:
package org.chromium.chrome.browser.enhanced_bookmarks;
s/enhanced_bookmarks/enhancedbookmarks ?

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkActionBar.java
(right):

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkActionBar.java:22:
import org.chromium.chrome.browser.enhanced_bookmarks.EnhancedBookmarksModel;
s/enhanced_bookmarks/enhancedbookmarks

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkActionBar.java:89:
return true;
added by mistake?

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkActionBar.java:104:
mDelegate.startDetailActivity(item.getId(), null);
I guess the previous one is correct if we use have imported the correct
EnhancedBookmarksModel.

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkActionBar.java:193:
getMenu().findItem(R.id.list_toggle_menu_id).setVisible(false);
mistake?

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkActionBar.java:203:
getMenu().findItem(R.id.list_toggle_menu_id).setVisible(true);
mistake?

https://codereview.chromium.org/1270693004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkActionBar.java:295:
}
Mistake block?

[Kibeom Kim (inactive), 2015-08-10 09:02:47]

Seems like patchset1 is the current one? reviewed patchset 1.

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/eb_search_core.xml (right):

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/eb_search_core.xml:8: <LinearLayout
Let's use standard android.support.v7.widget.Toolbar

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/eb_search_core.xml:13:
android:descendantFocusability="afterDescendants" >
Q: what do we need this for?

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:388: */
Q: wasn't this an async API before?

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchRow.java
(right):

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchRow.java:17:
public class EnhancedBookmarkSearchRow extends EnhancedBookmarkBookmarkRow {
EnhancedBookmarkBookmarkRow class assumes that selection mode and the more icon
is used. Could you make it flexible instead of band-aiding here?

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java
(right):

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:52:
* {@link ViewSwitcher}, it contains a {@link TextView} and a {@link ListView}.
In general I think it's good to comment purpose or why. The assumptions
mentioned here are internal detail for history&result toggle.

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:76:
// If currently showing history, toggle.
I think the code is simple enough that // If.. comments are not needed here and
below.

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:189:
if (!mSearchText.getText().toString().isEmpty()) mSearchText.setText("");
Don't need to convert string with TextUtils.isEmpty()

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:194:
if (TextUtils.isEmpty(currentText)) return;
Shouldn't this be an assert?

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:198:
if (results != null) populateResultListView(results);
Q: When results can be null?

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:282:
assert false : "Only history list should have onItemClickListener.";
How about

assert parent == mHistoryList && view instanceof TextView : "..." ?

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:391:
final EnhancedBookmarkSearchRow row  = (EnhancedBookmarkSearchRow) convertView;
nit s/row  =/row =

[Ian Wen, 2015-08-12 00:09:26]

ptal. All addressed.

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/eb_search_core.xml (right):

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/eb_search_core.xml:8: <LinearLayout
On 2015/08/10 09:02:47, Kibeom Kim wrote:
> Let's use standard android.support.v7.widget.Toolbar

Done. Thanks for the advice. It indeed looks nicer with toolbar (such as the
back button now has a round ripple).

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/eb_search_core.xml:13:
android:descendantFocusability="afterDescendants" >
On 2015/08/10 09:02:47, Kibeom Kim wrote:
> Q: what do we need this for?

This is needed for letting the edittext get focus, as part of the effort to show
the keyboard when the search view is launched.

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:388: */
On 2015/08/10 09:02:47, Kibeom Kim wrote:
> Q: wasn't this an async API before?

This particular API has always been async. It uses local bookmark model and does
a tree traversal to find the matching bookmarks. The async one was the server
search, and was removed.

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchRow.java
(right):

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchRow.java:17:
public class EnhancedBookmarkSearchRow extends EnhancedBookmarkBookmarkRow {
On 2015/08/10 09:02:47, Kibeom Kim wrote:
> EnhancedBookmarkBookmarkRow class assumes that selection mode and the more
icon
> is used. Could you make it flexible instead of band-aiding here?

I added setSelectable() in parent class and removed setOnClickListener(null)
below.

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java
(right):

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:52:
* {@link ViewSwitcher}, it contains a {@link TextView} and a {@link ListView}.
On 2015/08/10 09:02:47, Kibeom Kim wrote:
> In general I think it's good to comment purpose or why. The assumptions
> mentioned here are internal detail for history&result toggle.

Done.

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:76:
// If currently showing history, toggle.
On 2015/08/10 09:02:47, Kibeom Kim wrote:
> I think the code is simple enough that // If.. comments are not needed here
and
> below.

Done.

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:189:
if (!mSearchText.getText().toString().isEmpty()) mSearchText.setText("");
On 2015/08/10 09:02:47, Kibeom Kim wrote:
> Don't need to convert string with TextUtils.isEmpty()

Done.

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:194:
if (TextUtils.isEmpty(currentText)) return;
On 2015/08/10 09:02:47, Kibeom Kim wrote:
> Shouldn't this be an assert?

Ah no it can't be an assert here. The user can type nothing while press the
search button on keyboard. We should let them crash.

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:198:
if (results != null) populateResultListView(results);
On 2015/08/10 09:02:47, Kibeom Kim wrote:
> Q: When results can be null?

It will never be null. Removed the check. This is a legacy for server search.

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:282:
assert false : "Only history list should have onItemClickListener.";
On 2015/08/10 09:02:47, Kibeom Kim wrote:
> How about
> 
> assert parent == mHistoryList && view instanceof TextView : "..." ?

Done.

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:391:
final EnhancedBookmarkSearchRow row  = (EnhancedBookmarkSearchRow) convertView;
On 2015/08/10 09:02:47, Kibeom Kim wrote:
> nit s/row  =/row =

Done.

[Kibeom Kim (inactive), 2015-08-12 04:26:30]

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java
(right):

https://codereview.chromium.org/1270693004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:194:
if (TextUtils.isEmpty(currentText)) return;
On 2015/08/12 00:09:26, Ian Wen wrote:
> On 2015/08/10 09:02:47, Kibeom Kim wrote:
> > Shouldn't this be an assert?
> 
> Ah no it can't be an assert here. The user can type nothing while press the
> search button on keyboard. We should let them crash.

Hmm.. we check isEmpty and call resetUI already in in afterTextChanged #161. But
in the observer, we just call sendSearchQuery(). Is this correct?

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/res...
File chrome/android/java/res/layout/eb_search_core.xml (right):

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/eb_search_core.xml:11:
android:layout_height="@dimen/toolbar_height_no_shadow"
?attr/actionBarSize (suggested by go/appcompat-material-doc and also matches
other toolbars we have)

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/eb_search_core.xml:12:
android:orientation="horizontal"
I suppose orientation is not needed?

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/eb_search_core.xml:13:
android:descendantFocusability="afterDescendants" >
Is this still needed with Toolbar?

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkRow.java
(right):

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkRow.java:46:
private boolean mIsSelectable = true;
I think we can structure without this member variable.

1. Create abstract boolean isSelectable() and return true or false in the
subclasses.
2. Inside onFinishInflate(),

if (isSelectable()) {
    mMoreIcon....
    setOnClickListener(this);
    setOnLongClickListener(this);
}

Then need to check mMoreIcon == null but no need to call isSelectable() again.

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkRow.java:73:
protected abstract void onClick();
Let's remove this, and we can call EnhancedBookmarkSearchRow#setOnClickListener
directly from EnhancedBookmarkSearchView.

For other rows, we can override setOnClickListener in this class and save it,
and call at #210.

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchRow.java
(right):

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchRow.java:38:
mMoreIcon.setVisibility(View.GONE);
remove?

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java
(right):

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:97:
private Toolbar mSearchBar;
Since this is only used in onFinishInflate(), doesn't need to be a member
variable.

[Ian Wen, 2015-08-12 18:32:41]

Thanks for the comments. All addressed.

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/res...
File chrome/android/java/res/layout/eb_search_core.xml (right):

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/eb_search_core.xml:11:
android:layout_height="@dimen/toolbar_height_no_shadow"
On 2015/08/12 04:26:29, Kibeom Kim wrote:
> ?attr/actionBarSize (suggested by go/appcompat-material-doc and also matches
> other toolbars we have)

Done. On nexus 9 this is 56dp instead of 48.

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/eb_search_core.xml:12:
android:orientation="horizontal"
On 2015/08/12 04:26:29, Kibeom Kim wrote:
> I suppose orientation is not needed?

Done.

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/eb_search_core.xml:13:
android:descendantFocusability="afterDescendants" >
On 2015/08/12 04:26:29, Kibeom Kim wrote:
> Is this still needed with Toolbar?

No. Removed.

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkRow.java
(right):

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkRow.java:73:
protected abstract void onClick();
On 2015/08/12 04:26:29, Kibeom Kim wrote:
> Let's remove this, and we can call
EnhancedBookmarkSearchRow#setOnClickListener
> directly from EnhancedBookmarkSearchView.
> 
> For other rows, we can override setOnClickListener in this class and save it,
> and call at #210.

Talked offline. We still keep this while making a new name for search row so
that they don't mess up.

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchRow.java
(right):

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchRow.java:38:
mMoreIcon.setVisibility(View.GONE);
On 2015/08/12 04:26:30, Kibeom Kim wrote:
> remove?

Done.

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java
(right):

https://codereview.chromium.org/1270693004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:97:
private Toolbar mSearchBar;
On 2015/08/12 04:26:30, Kibeom Kim wrote:
> Since this is only used in onFinishInflate(), doesn't need to be a member
> variable.

Done.

[Kibeom Kim (inactive), 2015-08-12 20:35:52]

lgtm

https://codereview.chromium.org/1270693004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkRow.java
(right):

https://codereview.chromium.org/1270693004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkRow.java:91:
boolean isSelectable() {
nit: protected

https://codereview.chromium.org/1270693004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkRow.java:184:
}
let's do

if (isSelectable()) {
    mMoreIcon = (TintedImageButton) findViewById(R.id.more);
    mMoreIcon.setColorFilterMode(PorterDuff.Mode.MULTIPLY);
    mMoreIcon.setOnClickListener(new OnClickListener() {
        @Override
        public void onClick(View view) {
            showMenu(view);
        }
    });
}

I think this is cleaner since in this way we don't do an unnecessary work if
it's not needed, and also prevents accidentally using mMoreIcon.

https://codereview.chromium.org/1270693004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchRow.java
(right):

https://codereview.chromium.org/1270693004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchRow.java:51:
public boolean isSelectable() {
Doesn't need to be public.

[newt (away), 2015-08-12 21:18:32]

lgtm after comment

https://codereview.chromium.org/1270693004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java
(right):

https://codereview.chromium.org/1270693004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:308:
} catch (Exception e) {
don't catch EXCEPTION. Catch JSONException

[Ian Wen, 2015-08-12 21:24:53]

https://codereview.chromium.org/1270693004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkRow.java
(right):

https://codereview.chromium.org/1270693004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkRow.java:184:
}
On 2015/08/12 20:35:52, Kibeom Kim wrote:
> let's do
> 
> if (isSelectable()) {
>     mMoreIcon = (TintedImageButton) findViewById(R.id.more);
>     mMoreIcon.setColorFilterMode(PorterDuff.Mode.MULTIPLY);
>     mMoreIcon.setOnClickListener(new OnClickListener() {
>         @Override
>         public void onClick(View view) {
>             showMenu(view);
>         }
>     });
> }
> 
> I think this is cleaner since in this way we don't do an unnecessary work if
> it's not needed, and also prevents accidentally using mMoreIcon.

Done.

https://codereview.chromium.org/1270693004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchRow.java
(right):

https://codereview.chromium.org/1270693004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchRow.java:51:
public boolean isSelectable() {
On 2015/08/12 20:35:52, Kibeom Kim wrote:
> Doesn't need to be public.

Done.

https://codereview.chromium.org/1270693004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java
(right):

https://codereview.chromium.org/1270693004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSearchView.java:308:
} catch (Exception e) {
On 2015/08/12 21:18:32, newt wrote:
> don't catch EXCEPTION. Catch JSONException

Done.

[Ian Wen, 2015-08-12 21:25:01]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2015-08-12 21:25:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkimlabs@chromium.org, newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1270693004/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-08-12 21:25:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1270693004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1270693004/120001

[commit-bot: I haz the power, 2015-08-12 22:06:19]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-08-12 22:06:53]

Patchset 7 (id:??) landed as
https://crrev.com/8178e1386b0005a8e7aec5fefc90cb6d183a1a13
Cr-Commit-Position: refs/heads/master@{#343105}