Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1459593002:
Added a UI for the Interests Prototype. (Closed)
DescriptionAdded a UI for the Interests Prototype.
It is added as a third (middle) option to the new tab page. When an interest
is clicked, the user is taken to a news search.
This is a continuation of https://codereview.chromium.org/1351303003/ .
To get this to work, use the flags:
--enable-ntp-interests --interests-url=http://localhost:8788/v1/interests
You must also be signed in.
BUG=537145
Committed: https://crrev.com/7397a0cb47eeac9e92181ff8faf85bcb74f00f16
Cr-Commit-Position: refs/heads/master@{#365264}
Patch Set 1 #Patch Set 2 : #
Total comments: 26
Patch Set 3 : #Patch Set 4 : #
Total comments: 17
Patch Set 5 : #
Total comments: 74
Patch Set 6 : #
Total comments: 12
Patch Set 7 : #
Total comments: 4
Patch Set 8 : #Patch Set 9 : #Messages
Total messages: 48 (19 generated)
peconn@chromium.org changed reviewers: + bauerb@chromium.org, newt@chromium.org, treib@chromium.org
Some small comments. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:80: * @param imageUrl The URL for the interet's image. name and imageUrl don't exist; listener and drawingData don't have a comment https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:123: return; Hm, I'd still set mInterest = interest here https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:182: // the image. Do we still want to cache the image? If not, this check could be merged into the early-out check above. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java (right): https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:27: * List the interests of the user. When an interest is clicked the user is redirect to a google s/redirect/redirected https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:34: private final Profile mProfile; This member seems unnecessary https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:85: public interface InterestsListener { I find this name a bit confusing, I read it as "this will tell me when interests are available"... "InterestClickListener"? https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java (right): https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:40: public void setListener(InterestsListener listener) { Comment that this must be called before setInterests? (That *is* the case, right?) https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:57: private Context mContext = null; Do you need this? |getContext()| should give you the same thing, no? https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:69: mDrawingData = new InterestsItemView.DrawingData(getContext()); Can you just initialize this in the InterestsView ctor?
https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/interests_page.xml (right): https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/interests_page.xml:12: android:focusable="true" Sort these alphabetically? https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page.xml (right): https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page.xml:192: <FrameLayout We probably need to move this behind a command line flag or something. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java (right): https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:65: Log.w(TAG, "Interests Callback returned null."); This seems kind of unnecessary if you should a user-visible toast. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:682: private static class InterestDialogSelectedListener implements InterestsListener { You could probably even make this an anonymous inner class, so you don't need the members and constructor.
https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/interests_page.xml (right): https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/interests_page.xml:12: android:focusable="true" On 2015/11/19 17:15:15, Bernhard Bauer wrote: > Sort these alphabetically? I did this for all elements except verticalSpacing (which I put directly after horizontalSpacing). It's ugly, but I think they're related so should be next to each other. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page.xml (right): https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page.xml:192: <FrameLayout On 2015/11/19 17:15:15, Bernhard Bauer wrote: > We probably need to move this behind a command line flag or something. Done. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:80: * @param imageUrl The URL for the interet's image. On 2015/11/19 12:42:18, Marc Treib wrote: > name and imageUrl don't exist; listener and drawingData don't have a comment Done. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:123: return; On 2015/11/19 12:42:18, Marc Treib wrote: > Hm, I'd still set mInterest = interest here Done. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:182: // the image. On 2015/11/19 12:42:18, Marc Treib wrote: > Do we still want to cache the image? If not, this check could be merged into the > early-out check above. We've gone to the effort of loading it, so we may as well cache it. The use-case I see is a user opening the tab then immediately scrolling down so the images for the first few elements are loaded, cached and not displayed. Then the cached images are likely to be used. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java (right): https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:27: * List the interests of the user. When an interest is clicked the user is redirect to a google On 2015/11/19 12:42:18, Marc Treib wrote: > s/redirect/redirected Done. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:34: private final Profile mProfile; On 2015/11/19 12:42:18, Marc Treib wrote: > This member seems unnecessary Done. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:65: Log.w(TAG, "Interests Callback returned null."); On 2015/11/19 17:15:15, Bernhard Bauer wrote: > This seems kind of unnecessary if you should a user-visible toast. Done. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:85: public interface InterestsListener { On 2015/11/19 12:42:19, Marc Treib wrote: > I find this name a bit confusing, I read it as "this will tell me when interests > are available"... "InterestClickListener"? Done. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java (right): https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:40: public void setListener(InterestsListener listener) { On 2015/11/19 12:42:19, Marc Treib wrote: > Comment that this must be called before setInterests? (That *is* the case, > right?) Done. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:57: private Context mContext = null; On 2015/11/19 12:42:19, Marc Treib wrote: > Do you need this? |getContext()| should give you the same thing, no? Done. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:69: mDrawingData = new InterestsItemView.DrawingData(getContext()); On 2015/11/19 12:42:19, Marc Treib wrote: > Can you just initialize this in the InterestsView ctor? Done. https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1459593002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:682: private static class InterestDialogSelectedListener implements InterestsListener { On 2015/11/19 17:15:15, Bernhard Bauer wrote: > You could probably even make this an anonymous inner class, so you don't need > the members and constructor. Done.
knn@chromium.org changed reviewers: + knn@chromium.org
Unrelated to your current patch but since you are touching this already :) https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java (right): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:75: GetInterestsCallback wrappedCallback = new GetInterestsCallback() { This doesn't seem to do anything. Moreover since the C++ object is controlled from java and the callback itself is controlled by the C++ object (weak pointer) we don't need anything here. Remove?
https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:133: // Cache miss, download the image and use a temporary in the meantime. Hm... if we are called for the same interest several times in a row, we will start multiple downloads. Could we collapse those somehow? https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java (right): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:42: * This must be called before setInterests. You could wrap setInterests in a {@link} block.
https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:133: // Cache miss, download the image and use a temporary in the meantime. On 2015/12/02 14:09:47, Bernhard Bauer wrote: > Hm... if we are called for the same interest several times in a row, we will > start multiple downloads. Could we collapse those somehow? (I suspect this will eventually boil down to something equivalent to promises. I really wonder why we don't have that yet...)
Some nits. https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (left): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:1: <?xml version="1.0" encoding="utf-8"?> Unintentional? https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:40: private static final String TAG = "interests_item_view"; CamelCase https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java (right): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:62: "Could not retrieve interests.", Toast.LENGTH_SHORT); Make this an i18n resource? https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:341: RecordUserAction.record("MobileNTPSwitchToInterests"); Please add an actions.xml entry. https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:339: if (CommandLine.getInstance().hasSwitch(ChromeSwitches.ENABLE_NTP_INTERESTS)) { Can we check if user is signed in here?
Description was changed from ========== Added a UI for the Interests Prototype. It is added as a third (middle) option to the new tab page. When an interest is clicked, the user is taken to a news search. This is a continuation of https://codereview.chromium.org/1351303003/ . BUG=537145 ========== to ========== Added a UI for the Interests Prototype. It is added as a third (middle) option to the new tab page. When an interest is clicked, the user is taken to a news search. This is a continuation of https://codereview.chromium.org/1351303003/ . To get this to work, use the flags: --enable-ntp-interests --interests-url=http://localhost:8788/v1/interests BUG=537145 ==========
Description was changed from ========== Added a UI for the Interests Prototype. It is added as a third (middle) option to the new tab page. When an interest is clicked, the user is taken to a news search. This is a continuation of https://codereview.chromium.org/1351303003/ . To get this to work, use the flags: --enable-ntp-interests --interests-url=http://localhost:8788/v1/interests BUG=537145 ========== to ========== Added a UI for the Interests Prototype. It is added as a third (middle) option to the new tab page. When an interest is clicked, the user is taken to a news search. This is a continuation of https://codereview.chromium.org/1351303003/ . To get this to work, use the flags: --enable-ntp-interests --interests-url=http://localhost:8788/v1/interests You must also be signed in. BUG=537145 ==========
https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (left): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2015/12/02 14:18:59, knn wrote: > Unintentional? Acknowledged. https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:40: private static final String TAG = "interests_item_view"; On 2015/12/02 14:18:59, knn wrote: > CamelCase Done. https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:133: // Cache miss, download the image and use a temporary in the meantime. On 2015/12/02 14:10:39, Bernhard Bauer wrote: > On 2015/12/02 14:09:47, Bernhard Bauer wrote: > > Hm... if we are called for the same interest several times in a row, we will > > start multiple downloads. Could we collapse those somehow? > > (I suspect this will eventually boil down to something equivalent to promises. I > really wonder why we don't have that yet...) Done. https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java (right): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:62: "Could not retrieve interests.", Toast.LENGTH_SHORT); On 2015/12/02 14:18:59, knn wrote: > Make this an i18n resource? Done. https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java (right): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:75: GetInterestsCallback wrappedCallback = new GetInterestsCallback() { On 2015/12/02 13:30:43, knn wrote: > This doesn't seem to do anything. Moreover since the C++ object is controlled > from java and the callback itself is controlled by the C++ object (weak pointer) > we don't need anything here. Remove? Done. https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java (right): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:42: * This must be called before setInterests. On 2015/12/02 14:09:47, Bernhard Bauer wrote: > You could wrap setInterests in a {@link} block. Done. https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:341: RecordUserAction.record("MobileNTPSwitchToInterests"); On 2015/12/02 14:18:59, knn wrote: > Please add an actions.xml entry. Done. https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1459593002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:339: if (CommandLine.getInstance().hasSwitch(ChromeSwitches.ENABLE_NTP_INTERESTS)) { On 2015/12/02 14:18:59, knn wrote: > Can we check if user is signed in here? Done.
Thanks lgtm! I missed this the first time but you could put all the interests specific code inside [chrome/.../browser/ntp/interests] perhaps at the end of review so that the diffs don't become hard to review.
peconn@chromium.org changed reviewers: + asvitkine@chromium.org
@asvitkine, could you please look over actions.xml
lgtm
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1459593002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1459593002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Lots of (mostly) small comments, but generally this looks good :) One bigger comment, that can be addressed in this CL or in a later CL: I think it makes sense to move the downloading logic out of the InterestsItemView. Moving it out of InterestsItemView will allow easier mocking of the download logic during tests, and I think will make the code clearer and simpler to maintain in the long run. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/interests_page.xml (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/interests_page.xml:12: android:orientation="vertical" remove this; orientation isn't a valid attribute on FrameLayouts https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page.xml (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page.xml:195: android:id="@+id/interests_button" Put id first, then layout_width, the layout_height, then other layout_* attributes, then all other attributes. (This is the order used throughout Chrome code and the same order used by the Android Studio auto-formatter) layout_* attributes are grouped together since they're all instructions for how the view will be arranged within its parent. Also, please update interests_page.xml. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/res... File chrome/android/java/res/values/colors.xml (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/res... chrome/android/java/res/values/colors.xml:176: <array name="letter_tile_colors"> It'd be fine to just put these colors in Java. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:105: public static final String ENABLE_NTP_INTERESTS = "enable-ntp-interests"; You could remove "ntp" from this flag. --enable-interests seems fine and easier to type. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:50: static final class DrawingData { make this private https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:102: mIconGenerator = new RoundedIconGenerator( It'd be worth reusing the RoundedIconGenerator (e.g. by moving it into DrawingData) since it creates a Paint object, which is fairly expensive. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:125: && TextUtils.equals(interest.getName(), mInterest.getName()) Seems like you should add an equals() method interest and use it here, or just use == if you only care about identity. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:149: // Display a letter tile in the meantime. It might look a bit jumpy to generate default icons and then replace them with real images. Alternatively, we could just fade in the images when they're downloaded and not show a placeholder (unless, perhaps, the download failed). There's no need to change this behavior in this CL; just a thought for the future. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:190: private static class ImageDownloadTask extends AsyncTask<Void, Void, Drawable> { This class doesn't logically belong inside InterestItemView. I'm OK with keeping it here for now, though, since there's already a TODO to "Replace this with something from the C++ Chrome stack". https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:234: public void onCallback(Drawable image, String url) { I'd call this "onImageDownloaded()" https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:251: static class ImagePlaceholder { How about calling this "ImageHolder"? "ImagePlaceholder" sounds it's a default image (i.e. an image generated by RoundedIconGenerator). https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:252: private final List<ImageDownloadedCallback> mListeners = new LinkedList<>(); "Callback" or "Listener"? Pick one and use it consistently :) Also, you can use ObserverList (in base/), though its fancy features (such as removing a listener during iteration) may not be needed here. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:253: private Drawable mImage = null; no need for "= null" in a member variable declaration; that's the default in java https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:261: for (ImageDownloadedCallback listener : mListeners) { Shouldn't you clear mListeners after calling them? Otherwise, you have the potential for memory leaks: the ImageCache holds references to all the ImagePlaceholders, which in turn hold references to every IterestsItemView that's ever been created with that ImageCache. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:274: public Boolean isFilled() { s/Boolean/boolean/ (Never use Boolean if you can avoid it.) Though I'm not convinced you need this method. The caller could just use get(). https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:278: public Drawable get() { s/get/getImageDrawable/ ? https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:26: * List the interests of the user. When an interest is clicked the user is redirected to a search How about: "Displays a list of topics that we think the user is interested in." https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:29: public class InterestsPage implements NativePage { With the current setup, this doesn't need to be a NativePage since we never actually navigate a tab to chrome-native://interests. Likewise, you don't need to add UrlConstants.INTERESTS_URL. On the other hand, bookmarks and recent tabs are both dialogs on phones, but are loaded inside a tab on tablets. That's why they implement NativePage. We should consider doing the same for the interests page. (That could certainly be a separate CL.) https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:39: * Creates a InterestsPage to be shown in document mode. only in document mode?? If so, why? https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:44: * @param activity The activity from which the interests page is loaded. You don't need the "activity" param. The context is sufficient for getting resources. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:54: mPageView = (InterestsView) inflater.inflate(R.layout.interests_page, null); You can use the View.inflate() convenience method: mPageView = (InterestsView) View.inflate(context, R.layout.interests_page, null); https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:59: public void onInterestsAvailableCallback(Interest[] interests) { style nit: remove "Callback" from this method name https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:82: * Called when a interest is selected. s/a interest/an interest/ (and elsewhere too) https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:115: return "interests"; If you're going to use a NativePage, this constant should be moved into UrlConstants. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:25: * The user's interests, displayed with a two column grid view. Can you perhaps explain "interests" using some standard terminology that will be understood by someone who's not familiar with Interests. E.g. "A list of topics that the user may be interested in, displayed with a two column grid view" https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:27: public class InterestsView extends FrameLayout { can this be package private? https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:37: mImageCache = new LruCache<>(30); Might as well move mImageCache and mDrawingData inside the InterestsListAdapter. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:49: public void setInterests(List<Interest> interests) { javadoc https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:184: dialog.dismiss(); style nit: there's a few too many newlines here IMO. Use newlines to separate logical sections of code, but don't separate every single line. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:342: launchInterestsDialog(mActivity, mTab); what happens on tablets? A fullscreen dialog? https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:690: remove this https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:138: /** Opens the recent tabs page in the current tab. */ fix javadoc https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:341: && ChromeSigninController.get(getContext()).isSignedIn()) { This class (NewTabPageView) shouldn't interact directly with other parts of Chrome, such as the ChromeSigninController. That interaction should go through NewTabPageManager. Also, hiding/showing this button depending on the signed-in state probably isn't a suitable long-term plan. Rather, we'd probably want a UI where the interests page shows a sign-in promo if the user isn't already signed in. For now, this is fine though. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1689: <message name="IDS_NTP_INTERESTS" desc="Text for interests button [CHAR-LIMIT=20]"> Translators probably need more context here. What is the "interests button"? You should also contact the l10n team to make sure they've added "Interests" to their translation glossary. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1692: <message name="IDS_NTP_NO_INTERESTS_TOAST" desc="Toast notification displayed when interests retrieval fails [CHAR-LIMIT=40]"> Don't need CHAR-LIMIT. CHAR-LIMITs should only be used if a long translation will be truncated or break the UI. See the top of this file for more details on CHAR-LIMITS. https://codereview.chromium.org/1459593002/diff/80001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1459593002/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:8097: <description>User opens the interests dialog.</description> s/opens/opened/
https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/interests_page.xml (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/interests_page.xml:12: android:orientation="vertical" On 2015/12/12 00:30:30, newt wrote: > remove this; orientation isn't a valid attribute on FrameLayouts Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page.xml (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page.xml:195: android:id="@+id/interests_button" On 2015/12/12 00:30:30, newt wrote: > Put id first, then layout_width, the layout_height, then other layout_* > attributes, then all other attributes. (This is the order used throughout Chrome > code and the same order used by the Android Studio auto-formatter) > > layout_* attributes are grouped together since they're all instructions for how > the view will be arranged within its parent. > > Also, please update interests_page.xml. Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/res... File chrome/android/java/res/values/colors.xml (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/res... chrome/android/java/res/values/colors.xml:176: <array name="letter_tile_colors"> On 2015/12/12 00:30:30, newt wrote: > It'd be fine to just put these colors in Java. Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:105: public static final String ENABLE_NTP_INTERESTS = "enable-ntp-interests"; On 2015/12/12 00:30:30, newt wrote: > You could remove "ntp" from this flag. --enable-interests seems fine and easier > to type. Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:50: static final class DrawingData { On 2015/12/12 00:30:30, newt wrote: > make this private The DrawingData instance is created and held by the InterestsView. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:102: mIconGenerator = new RoundedIconGenerator( On 2015/12/12 00:30:30, newt wrote: > It'd be worth reusing the RoundedIconGenerator (e.g. by moving it into > DrawingData) since it creates a Paint object, which is fairly expensive. Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:125: && TextUtils.equals(interest.getName(), mInterest.getName()) On 2015/12/12 00:30:31, newt wrote: > Seems like you should add an equals() method interest and use it here, or just > use == if you only care about identity. An interest contains a name, an image url and a relevance score. In this case only the name and the image url matter to the ItemView - there's no need to update if two different Interests share an image url and a name and I don't feel an equals() method would be appropriate here since it ignores the relevance. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:149: // Display a letter tile in the meantime. On 2015/12/12 00:30:31, newt wrote: > It might look a bit jumpy to generate default icons and then replace them with > real images. > > Alternatively, we could just fade in the images when they're downloaded and not > show a placeholder (unless, perhaps, the download failed). > > There's no need to change this behavior in this CL; just a thought for the > future. Acknowledged. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:190: private static class ImageDownloadTask extends AsyncTask<Void, Void, Drawable> { On 2015/12/12 00:30:31, newt wrote: > This class doesn't logically belong inside InterestItemView. I'm OK with keeping > it here for now, though, since there's already a TODO to "Replace this with > something from the C++ Chrome stack". Acknowledged. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:234: public void onCallback(Drawable image, String url) { On 2015/12/12 00:30:31, newt wrote: > I'd call this "onImageDownloaded()" Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:251: static class ImagePlaceholder { On 2015/12/12 00:30:30, newt wrote: > How about calling this "ImageHolder"? "ImagePlaceholder" sounds it's a default > image (i.e. an image generated by RoundedIconGenerator). Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:252: private final List<ImageDownloadedCallback> mListeners = new LinkedList<>(); On 2015/12/12 00:30:31, newt wrote: > "Callback" or "Listener"? Pick one and use it consistently :) > > Also, you can use ObserverList (in base/), though its fancy features (such as > removing a listener during iteration) may not be needed here. Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:253: private Drawable mImage = null; On 2015/12/12 00:30:31, newt wrote: > no need for "= null" in a member variable declaration; that's the default in > java Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:261: for (ImageDownloadedCallback listener : mListeners) { On 2015/12/12 00:30:31, newt wrote: > Shouldn't you clear mListeners after calling them? Otherwise, you have the > potential for memory leaks: the ImageCache holds references to all the > ImagePlaceholders, which in turn hold references to every IterestsItemView > that's ever been created with that ImageCache. Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:274: public Boolean isFilled() { On 2015/12/12 00:30:31, newt wrote: > s/Boolean/boolean/ (Never use Boolean if you can avoid it.) > > Though I'm not convinced you need this method. The caller could just use get(). Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:278: public Drawable get() { On 2015/12/12 00:30:30, newt wrote: > s/get/getImageDrawable/ ? Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:26: * List the interests of the user. When an interest is clicked the user is redirected to a search On 2015/12/12 00:30:31, newt wrote: > How about: "Displays a list of topics that we think the user is interested in." Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:29: public class InterestsPage implements NativePage { On 2015/12/12 00:30:31, newt wrote: > With the current setup, this doesn't need to be a NativePage since we never > actually navigate a tab to chrome-native://interests. Likewise, you don't need > to add UrlConstants.INTERESTS_URL. > > On the other hand, bookmarks and recent tabs are both dialogs on phones, but are > loaded inside a tab on tablets. That's why they implement NativePage. We should > consider doing the same for the interests page. (That could certainly be a > separate CL.) Acknowledged. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:39: * Creates a InterestsPage to be shown in document mode. On 2015/12/12 00:30:31, newt wrote: > only in document mode?? If so, why? Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:44: * @param activity The activity from which the interests page is loaded. On 2015/12/12 00:30:31, newt wrote: > You don't need the "activity" param. The context is sufficient for getting > resources. Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:54: mPageView = (InterestsView) inflater.inflate(R.layout.interests_page, null); On 2015/12/12 00:30:31, newt wrote: > You can use the View.inflate() convenience method: > > mPageView = (InterestsView) View.inflate(context, R.layout.interests_page, > null); Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:59: public void onInterestsAvailableCallback(Interest[] interests) { On 2015/12/12 00:30:31, newt wrote: > style nit: remove "Callback" from this method name Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:82: * Called when a interest is selected. On 2015/12/12 00:30:31, newt wrote: > s/a interest/an interest/ (and elsewhere too) Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:115: return "interests"; On 2015/12/12 00:30:31, newt wrote: > If you're going to use a NativePage, this constant should be moved into > UrlConstants. Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:25: * The user's interests, displayed with a two column grid view. On 2015/12/12 00:30:31, newt wrote: > Can you perhaps explain "interests" using some standard terminology that will be > understood by someone who's not familiar with Interests. E.g. "A list of topics > that the user may be interested in, displayed with a two column grid view" Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:27: public class InterestsView extends FrameLayout { On 2015/12/12 00:30:31, newt wrote: > can this be package private? I don't think so (trying to compile with this package private gives an error about InterestView being not instantiatable). Don't all UI components have to be instantiatable? https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:37: mImageCache = new LruCache<>(30); On 2015/12/12 00:30:31, newt wrote: > Might as well move mImageCache and mDrawingData inside the InterestsListAdapter. Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:49: public void setInterests(List<Interest> interests) { On 2015/12/12 00:30:31, newt wrote: > javadoc Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:184: dialog.dismiss(); On 2015/12/12 00:30:31, newt wrote: > style nit: there's a few too many newlines here IMO. Use newlines to separate > logical sections of code, but don't separate every single line. Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:342: launchInterestsDialog(mActivity, mTab); On 2015/12/12 00:30:31, newt wrote: > what happens on tablets? A fullscreen dialog? Yes, for the moment. As you suggested in InterestsPage.java, this will eventually bring up a native page, but for now it is a full page dialog. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:690: On 2015/12/12 00:30:31, newt wrote: > remove this Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:138: /** Opens the recent tabs page in the current tab. */ On 2015/12/12 00:30:31, newt wrote: > fix javadoc Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:341: && ChromeSigninController.get(getContext()).isSignedIn()) { On 2015/12/12 00:30:31, newt wrote: > This class (NewTabPageView) shouldn't interact directly with other parts of > Chrome, such as the ChromeSigninController. That interaction should go through > NewTabPageManager. > > Also, hiding/showing this button depending on the signed-in state probably isn't > a suitable long-term plan. Rather, we'd probably want a UI where the interests > page shows a sign-in promo if the user isn't already signed in. For now, this is > fine though. Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1689: <message name="IDS_NTP_INTERESTS" desc="Text for interests button [CHAR-LIMIT=20]"> On 2015/12/12 00:30:31, newt wrote: > Translators probably need more context here. What is the "interests button"? You > should also contact the l10n team to make sure they've added "Interests" to > their translation glossary. Done. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1692: <message name="IDS_NTP_NO_INTERESTS_TOAST" desc="Toast notification displayed when interests retrieval fails [CHAR-LIMIT=40]"> On 2015/12/12 00:30:31, newt wrote: > Don't need CHAR-LIMIT. CHAR-LIMITs should only be used if a long translation > will be truncated or break the UI. See the top of this file for more details on > CHAR-LIMITS. Done. https://codereview.chromium.org/1459593002/diff/80001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1459593002/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:8097: <description>User opens the interests dialog.</description> On 2015/12/12 00:30:32, newt wrote: > s/opens/opened/ Done.
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1459593002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1459593002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1459593002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1459593002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
lgtm after final comments https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:125: && TextUtils.equals(interest.getName(), mInterest.getName()) On 2015/12/14 17:05:15, PEConn1 wrote: > On 2015/12/12 00:30:31, newt wrote: > > Seems like you should add an equals() method interest and use it here, or just > > use == if you only care about identity. > > An interest contains a name, an image url and a relevance score. In this case > only the name and the image url matter to the ItemView - there's no need to > update if two different Interests share an image url and a name and I don't feel > an equals() method would be appropriate here since it ignores the relevance. Gotcha. Thanks for explaining. https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java (right): https://codereview.chromium.org/1459593002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsView.java:27: public class InterestsView extends FrameLayout { On 2015/12/14 17:05:15, PEConn1 wrote: > On 2015/12/12 00:30:31, newt wrote: > > can this be package private? > > I don't think so (trying to compile with this package private gives an error > about InterestView being not instantiatable). Don't all UI components have to be > instantiatable? Fair point. Android guidelines recommend that all Views used in xml layouts are public. In practice, it's OK to make the class package private, since the view framework uses reflection to access the class and can access non-public classes. I prefer limiting visibility where possible and logical, but making this public does follow the Android guidelines, so that's fine. https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/re... File chrome/android/java/res/layout/new_tab_page.xml (right): https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/new_tab_page.xml:194: android:layout_weight="1" nit: layout_width, layout_height, then other layout_* attributes https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:47: private static final String[] COLOR_CODES = {"#f16364", "#f58559", "#f9a43e", "#e4c62e", Just use an int array: private static final int[] COLORS = {0xfff16364, ... }; and you can skip the call to Color.parseColor() down below https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:134: ImageHolder placeholder = mImageCache.get(mInterest.getImageUrl()); s/placeholder/holder/ https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:191: private final ImageHolder mImagePlaceholder; s/Placeholder/Holder/ https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:194: public ImageDownloadTask(String url, ImageHolder placeholder, Resources resources) { s/placeholder/holder/ https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:358: if (CommandLine.getInstance().hasSwitch(ChromeSwitches.ENABLE_NTP_INTERESTS) Better: add a method isInterestsEnabled() in NewTabPage manager, and put both the signed in and command line check into the method. https://codereview.chromium.org/1459593002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (right): https://codereview.chromium.org/1459593002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:97: * Enable content snippets on the NTP fix javadoc https://codereview.chromium.org/1459593002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:99: public static final String ENABLE_NTP_INTERESTS = "enable-interests"; s/ENABLE_NTP_INTERESTS/ENABLE_INTERESTS/
https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/re... File chrome/android/java/res/layout/new_tab_page.xml (right): https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/new_tab_page.xml:194: android:layout_weight="1" On 2015/12/14 21:18:39, newt wrote: > nit: layout_width, layout_height, then other layout_* attributes Done. https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:47: private static final String[] COLOR_CODES = {"#f16364", "#f58559", "#f9a43e", "#e4c62e", On 2015/12/14 21:18:39, newt wrote: > Just use an int array: > > private static final int[] COLORS = {0xfff16364, ... }; > > and you can skip the call to Color.parseColor() down below Done. https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:134: ImageHolder placeholder = mImageCache.get(mInterest.getImageUrl()); On 2015/12/14 21:18:39, newt wrote: > s/placeholder/holder/ Done. https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:191: private final ImageHolder mImagePlaceholder; On 2015/12/14 21:18:39, newt wrote: > s/Placeholder/Holder/ Done. https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:194: public ImageDownloadTask(String url, ImageHolder placeholder, Resources resources) { On 2015/12/14 21:18:39, newt wrote: > s/placeholder/holder/ Done. https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1459593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:358: if (CommandLine.getInstance().hasSwitch(ChromeSwitches.ENABLE_NTP_INTERESTS) On 2015/12/14 21:18:39, newt wrote: > Better: add a method isInterestsEnabled() in NewTabPage manager, and put both > the signed in and command line check into the method. Done. https://codereview.chromium.org/1459593002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (right): https://codereview.chromium.org/1459593002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:97: * Enable content snippets on the NTP On 2015/12/14 21:18:39, newt wrote: > fix javadoc Done. https://codereview.chromium.org/1459593002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:99: public static final String ENABLE_NTP_INTERESTS = "enable-interests"; On 2015/12/14 21:18:39, newt wrote: > s/ENABLE_NTP_INTERESTS/ENABLE_INTERESTS/ Done.
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1459593002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1459593002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1459593002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1459593002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by peconn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, knn@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1459593002/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1459593002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1459593002/160001
Message was sent while issue was closed.
Description was changed from ========== Added a UI for the Interests Prototype. It is added as a third (middle) option to the new tab page. When an interest is clicked, the user is taken to a news search. This is a continuation of https://codereview.chromium.org/1351303003/ . To get this to work, use the flags: --enable-ntp-interests --interests-url=http://localhost:8788/v1/interests You must also be signed in. BUG=537145 ========== to ========== Added a UI for the Interests Prototype. It is added as a third (middle) option to the new tab page. When an interest is clicked, the user is taken to a news search. This is a continuation of https://codereview.chromium.org/1351303003/ . To get this to work, use the flags: --enable-ntp-interests --interests-url=http://localhost:8788/v1/interests You must also be signed in. BUG=537145 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Added a UI for the Interests Prototype. It is added as a third (middle) option to the new tab page. When an interest is clicked, the user is taken to a news search. This is a continuation of https://codereview.chromium.org/1351303003/ . To get this to work, use the flags: --enable-ntp-interests --interests-url=http://localhost:8788/v1/interests You must also be signed in. BUG=537145 ========== to ========== Added a UI for the Interests Prototype. It is added as a third (middle) option to the new tab page. When an interest is clicked, the user is taken to a news search. This is a continuation of https://codereview.chromium.org/1351303003/ . To get this to work, use the flags: --enable-ntp-interests --interests-url=http://localhost:8788/v1/interests You must also be signed in. BUG=537145 Committed: https://crrev.com/7397a0cb47eeac9e92181ff8faf85bcb74f00f16 Cr-Commit-Position: refs/heads/master@{#365264} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/7397a0cb47eeac9e92181ff8faf85bcb74f00f16 Cr-Commit-Position: refs/heads/master@{#365264} |
