Replace enhanced bookmarks salient images with favicons

Replace enhanced bookmarks salient images with favicons

Use the LargeIconBridge instead of salient images; remove android code for retrieving salient images. Also, remove url from the list view.

BUG=499415
Committed: https://crrev.com/7842457a77384f4f3433789f5a6519f596bb8e9d
Cr-Commit-Position: refs/heads/master@{#335932}

[Theresa, 2015-06-23 20:35:06]

twellington@chromium.org changed reviewers:
+ kkimlabs@chromium.org, newt@chromium.org, sky@chromium.org

[Theresa, 2015-06-23 20:35:06]

ptal - trying to wrap this up by branch point

kkimlabs@ for overall review & OWNER of
  chrome/browser/enhanced_bookmarks/
  components/resources/enhanced_bookmarks/

newt@ for chrome/android OWNERS review/approval

sky@ for
  chrome/BUILD.gn
  chrome/browser/BUILD.gn
  chrome/browser/ui/tab_helpers.cc
  chrome/browser/bookmarks/enhanced_bookmarks_features.*

[Kibeom Kim (inactive), 2015-06-23 21:02:50]

lgtm with nits

https://codereview.chromium.org/1202713002/diff/40001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItem.java
(right):

https://codereview.chromium.org/1202713002/diff/40001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItem.java:212:
if (mLargeIconBridge == null) mLargeIconBridge = new LargeIconBridge();
since we have to use mLargeIconBridge at least once, I'd just put it in the
constructor and skip null check.

https://codereview.chromium.org/1202713002/diff/40001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/1202713002/diff/40001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:636: sources +=
rebase_path(gypi_values.chrome_browser_bookmark_android_sources,
Do we want to remove this one?

Previously added
https://codereview.chromium.org/1153283005/diff/160001/chrome/browser/BUILD.gn

[Theresa, 2015-06-23 21:47:42]

twellington@chromium.org changed reviewers:
+ jochen@chromium.org

[Theresa, 2015-06-23 21:47:44]

+jochen@ for removal of stuff in components/

https://codereview.chromium.org/1202713002/diff/40001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItem.java
(right):

https://codereview.chromium.org/1202713002/diff/40001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItem.java:212:
if (mLargeIconBridge == null) mLargeIconBridge = new LargeIconBridge();
On 2015/06/23 21:02:50, Kibeom Kim wrote:
> since we have to use mLargeIconBridge at least once, I'd just put it in the
> constructor and skip null check.

Done.

https://codereview.chromium.org/1202713002/diff/40001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/1202713002/diff/40001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:636: sources +=
rebase_path(gypi_values.chrome_browser_bookmark_android_sources,
On 2015/06/23 21:02:50, Kibeom Kim wrote:
> Do we want to remove this one?
> 
> Previously added
> https://codereview.chromium.org/1153283005/diff/160001/chrome/browser/BUILD.gn

Currently it's still needed because I split the bookmark specific resources from
chrome_browser_android_sources (in the previous patchset you linked) so that
they could be included separately (builds were failing on windows irc). With
this current patchset, only enhanced_bookmarks_bridge.cc and
enhanced_bookmarks_bridge.h are left in that source set, so I could merge it
back into chrome_browser_android_sources and remove this line if that seems
better.

[sky, 2015-06-23 21:58:18]

LGTM

[newt (away), 2015-06-23 22:09:40]

lgtm after nits

https://codereview.chromium.org/1202713002/diff/60001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItem.java
(left):

https://codereview.chromium.org/1202713002/diff/60001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItem.java:65:
mUrlView = (TextView) findViewById(R.id.url);
Please update the CL description to mention that you're removing the URL too

https://codereview.chromium.org/1202713002/diff/60001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItem.java
(right):

https://codereview.chromium.org/1202713002/diff/60001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItem.java:67:
mCornerRadius =
style nit: I think it's more readable (and common in our codebase) to wrap after
"("

    mCornerRadius = getResources().getDimensionPixelSize(
            R.dimen.enhanced_bookmark_item_corner_radius);
    etc

[Kibeom Kim (inactive), 2015-06-23 22:09:58]

(just a note: btw, we should also delete the local image database file)

[Theresa, 2015-06-23 22:29:48]

On 2015/06/23 22:09:58, Kibeom Kim wrote:
> (just a note: btw, we should also delete the local image database file)

Discussed offline, agreed, we need to delete the local image database file. This
will be done in a follow-on CL.

[Theresa, 2015-06-23 22:33:52]

https://codereview.chromium.org/1202713002/diff/60001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItem.java
(left):

https://codereview.chromium.org/1202713002/diff/60001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItem.java:65:
mUrlView = (TextView) findViewById(R.id.url);
On 2015/06/23 22:09:40, newt wrote:
> Please update the CL description to mention that you're removing the URL too

Done.

https://codereview.chromium.org/1202713002/diff/60001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItem.java
(right):

https://codereview.chromium.org/1202713002/diff/60001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItem.java:67:
mCornerRadius =
On 2015/06/23 22:09:40, newt wrote:
> style nit: I think it's more readable (and common in our codebase) to wrap
after
> "("
> 
>     mCornerRadius = getResources().getDimensionPixelSize(
>             R.dimen.enhanced_bookmark_item_corner_radius);
>     etc

Done.

[jochen (gone - plz use gerrit), 2015-06-24 14:47:02]

lgtm

[Theresa, 2015-06-24 14:55:00]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2015-06-24 14:55:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkimlabs@chromium.org, newt@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1202713002/#ps80001
(title: "Line wraps in EnhanacedBookmarkItem.java")

[commit-bot: I haz the power, 2015-06-24 14:55:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1202713002/80001

[commit-bot: I haz the power, 2015-06-24 16:19:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-24 16:20:37]

Patchset 5 (id:??) landed as
https://crrev.com/7842457a77384f4f3433789f5a6519f596bb8e9d
Cr-Commit-Position: refs/heads/master@{#335932}