Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1351303003:
Add the UI for the Interests Prototype. (Closed)
DescriptionAdd the UI for the Interests Prototype.
The interests are displayed in a 2 column grid. For each interest its name
and a representative image is displayed.
When an interest is clicked the user is taken to a news search about that
interest.
BUG=537145
Patch Set 1 #
Total comments: 51
Patch Set 2 : #Patch Set 3 : Fetch the oauth token from the native code. #
Total comments: 55
Depends on Patchset: Messages
Total messages: 13 (4 generated)
tache@google.com changed reviewers: + treib@chromium.org
First bunch of comments below. Mostly nits, but a few "real" ones as well. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:70: private DownloadImage mDownloadTask = null; Why is (only) this one explicitly initialized to null? https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:80: LruCache<String, Drawable> imageCache) { Weird line breaks? https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:86: mDrawingData = new DrawingData(mContext); Wait, when you use it like this, then nothing is actually shared, right? https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:110: */ Add @return please https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:133: // cache miss need to download the image and use a temporary image in the meantime. nit: Capitalize "Cache", also add a comma after "miss". https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:187: private class DownloadImage extends AsyncTask<String, Integer, Drawable> { DownloadImageTask? https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:214: protected void onPostExecute(Drawable image) { Should this have @Override? https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:233: } catch (Exception e) { Be more specific than just "Exception" please. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:54: public static InterestsPage buildPage(Context context, Tab tab, Any reason for not just calling the constructor directly? https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:82: List<Interest> interestList = Arrays.asList(interests); Any particular reason for switching from array to List here? https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:94: account, Does this handle a null account? https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:100: Account[] accounts = AccountManager.get(context).getAccounts(); This seems wrong - AccountManager is an Android thing, not a Chrome thing, so it might also return accounts that are not signed in to Chrome? Maybe even non-Google accounts? In native code, you'd use SigninManager[Base] to get the primary account. There should be a Java version of that as well. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:130: private static final String GOOGLE_SEARCH_URL = "https://www.google.com/search?tbm=nws&q=%s"; nit: too-long line also, .._URL_PATTERN? https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:131: private Dialog mDialog; The dialog thing is weird. Why would a DefaultListener have to know about it? Maybe just move the listener implementation to where it's actually used. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:144: String url = String.format(GOOGLE_SEARCH_URL, name); You need to URL encode the query string. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:176: mIsDestroyed = true; Should this method do anything? Also, mIsDestroyed isn't used. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:32: */ Either remove, or fill and move down to the method where it belongs. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:52: private GridView mInterestsList; List or Grid? https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:53: private InterestsListAdapter mAdapter; Move these up to the other member fields? https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:103: mImageCache); nit: line breaks are weird? https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:15: "https://www.googleapis.com/auth/googlenow"; Hm. Shouldn't the InterestsService itself (probably the native version) be responsible for fetching a properly-scoped access token, given a signed-in profile? https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:164: TabModelSelector tabModelSelector) { Unused param. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:1702: <message name="IDS_NTP_INTERESTS" desc="Title of the page where the user's topics of interests are shown"> nit: "topics of interest" (s/interests/interest/g) Also, this isn't actually used yet. I guess we don't have mocks on how exactly we want to surface this?
https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:70: private DownloadImage mDownloadTask = null; On 2015/09/18 13:26:03, Marc Treib wrote: > Why is (only) this one explicitly initialized to null? Done. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:80: LruCache<String, Drawable> imageCache) { On 2015/09/18 13:26:03, Marc Treib wrote: > Weird line breaks? Done. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:86: mDrawingData = new DrawingData(mContext); On 2015/09/18 13:26:03, Marc Treib wrote: > Wait, when you use it like this, then nothing is actually shared, right? Now it shared in the InterestPageView and past to the item via the constructor. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:110: */ On 2015/09/18 13:26:03, Marc Treib wrote: > Add @return please Removed the return value, since it was not used anywhere. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:133: // cache miss need to download the image and use a temporary image in the meantime. On 2015/09/18 13:26:03, Marc Treib wrote: > nit: Capitalize "Cache", also add a comma after "miss". Done. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:187: private class DownloadImage extends AsyncTask<String, Integer, Drawable> { On 2015/09/18 13:26:03, Marc Treib wrote: > DownloadImageTask? Done. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:214: protected void onPostExecute(Drawable image) { On 2015/09/18 13:26:03, Marc Treib wrote: > Should this have @Override? Done. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:233: } catch (Exception e) { On 2015/09/18 13:26:03, Marc Treib wrote: > Be more specific than just "Exception" please. Done. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:54: public static InterestsPage buildPage(Context context, Tab tab, On 2015/09/18 13:26:03, Marc Treib wrote: > Any reason for not just calling the constructor directly? Done. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:82: List<Interest> interestList = Arrays.asList(interests); On 2015/09/18 13:26:03, Marc Treib wrote: > Any particular reason for switching from array to List here? The native call returns the callback as an array. However most of the android code I've seen seems to use Lists instead of an array https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:94: account, On 2015/09/18 13:26:03, Marc Treib wrote: > Does this handle a null account? It should. I wrapped it in an if statement just to be sure https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:100: Account[] accounts = AccountManager.get(context).getAccounts(); On 2015/09/18 13:26:03, Marc Treib wrote: > This seems wrong - AccountManager is an Android thing, not a Chrome thing, so it > might also return accounts that are not signed in to Chrome? Maybe even > non-Google accounts? > In native code, you'd use SigninManager[Base] to get the primary account. There > should be a Java version of that as well. I couldn't find anything that returns me the primary account. The closest thing I found is the AccountManagerHelper which returns me the list of all google accounts. Switched to using AccountManagerHelper. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:130: private static final String GOOGLE_SEARCH_URL = "https://www.google.com/search?tbm=nws&q=%s"; On 2015/09/18 13:26:03, Marc Treib wrote: > nit: too-long line > also, .._URL_PATTERN? Done. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:131: private Dialog mDialog; On 2015/09/18 13:26:03, Marc Treib wrote: > The dialog thing is weird. Why would a DefaultListener have to know about it? > Maybe just move the listener implementation to where it's actually used. Moved this to the NewTabPage class similarly. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:144: String url = String.format(GOOGLE_SEARCH_URL, name); On 2015/09/18 13:26:03, Marc Treib wrote: > You need to URL encode the query string. Done. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:176: mIsDestroyed = true; On 2015/09/18 13:26:03, Marc Treib wrote: > Should this method do anything? > Also, mIsDestroyed isn't used. The InterestPage doesn't have any members that need to be destroyed explicitly. Removed mIsDestroyed. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:32: */ On 2015/09/18 13:26:03, Marc Treib wrote: > Either remove, or fill and move down to the method where it belongs. Done. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:52: private GridView mInterestsList; On 2015/09/18 13:26:03, Marc Treib wrote: > List or Grid? Done. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:53: private InterestsListAdapter mAdapter; On 2015/09/18 13:26:04, Marc Treib wrote: > Move these up to the other member fields? Done. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:103: mImageCache); On 2015/09/18 13:26:04, Marc Treib wrote: > nit: line breaks are weird? Done. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:15: "https://www.googleapis.com/auth/googlenow"; On 2015/09/18 13:26:04, Marc Treib wrote: > Hm. Shouldn't the InterestsService itself (probably the native version) be > responsible for fetching a properly-scoped access token, given a signed-in > profile? I don't think so. Looking at the OAuth2TokenService (java) it uses the android AccountManager. This allows it to show popups / notifications asking for the user's permission. I assume that how you get a valid token is platform specific and thus the InterestsFetcher shouldn't be responsible for it. I could move the AUTH_SCOPE to the native part and exposes through JNI to Java. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:164: TabModelSelector tabModelSelector) { On 2015/09/18 13:26:04, Marc Treib wrote: > Unused param. Done. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:1702: <message name="IDS_NTP_INTERESTS" desc="Title of the page where the user's topics of interests are shown"> On 2015/09/18 13:26:04, Marc Treib wrote: > nit: "topics of interest" (s/interests/interest/g) > Also, this isn't actually used yet. I guess we don't have mocks on how exactly > we want to surface this? This is used in the title of dialog in which the interests are currently shown.
BTW, do we have a bug (on crbug.com) yet for this feature? If not, you should create one and link it on all your CLs (by adding a BUG=12345 line in the description). Then you can point to any docs on bug, and also post screenshots. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:82: List<Interest> interestList = Arrays.asList(interests); On 2015/09/21 15:35:55, tache wrote: > On 2015/09/18 13:26:03, Marc Treib wrote: > > Any particular reason for switching from array to List here? > > The native call returns the callback as an array. However most of the android > code I've seen seems to use Lists instead of an array Acknowledged, fair enough. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:94: account, On 2015/09/21 15:35:55, tache wrote: > On 2015/09/18 13:26:03, Marc Treib wrote: > > Does this handle a null account? > > It should. I wrapped it in an if statement just to be sure Wouldn't it be great if OAuth2TokenService documented whether a null account is okay... but since another parameter is marked @Nullable and the account isn't, I guess checking for it here is the right call. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:100: Account[] accounts = AccountManager.get(context).getAccounts(); On 2015/09/21 15:35:55, tache wrote: > On 2015/09/18 13:26:03, Marc Treib wrote: > > This seems wrong - AccountManager is an Android thing, not a Chrome thing, so > it > > might also return accounts that are not signed in to Chrome? Maybe even > > non-Google accounts? > > In native code, you'd use SigninManager[Base] to get the primary account. > There > > should be a Java version of that as well. > > I couldn't find anything that returns me the primary account. The closest thing > I found is the AccountManagerHelper which returns me the list of all google > accounts. > > Switched to using AccountManagerHelper. ChromeSigninController.getSignedInUser()? https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:15: "https://www.googleapis.com/auth/googlenow"; On 2015/09/21 15:35:55, tache wrote: > On 2015/09/18 13:26:04, Marc Treib wrote: > > Hm. Shouldn't the InterestsService itself (probably the native version) be > > responsible for fetching a properly-scoped access token, given a signed-in > > profile? > > I don't think so. Looking at the OAuth2TokenService (java) it uses the android > AccountManager. This allows it to show popups / notifications asking for the > user's permission. The native ProfileOAuth2TokenService works on all platforms, it has some platform-specific implementation parts I think (which for Android probably forward to the Java one). You can look at PermissionRequestCreatorApiary for an example. > I assume that how you get a valid token is platform specific and thus the > InterestsFetcher shouldn't be responsible for it. > > I could move the AUTH_SCOPE to the native part and exposes through JNI to Java. Nah, if it's only used in Java, then that's where it should live. https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1351303003/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:1702: <message name="IDS_NTP_INTERESTS" desc="Title of the page where the user's topics of interests are shown"> On 2015/09/21 15:35:55, tache wrote: > On 2015/09/18 13:26:04, Marc Treib wrote: > > nit: "topics of interest" (s/interests/interest/g) > > Also, this isn't actually used yet. I guess we don't have mocks on how exactly > > we want to surface this? > > This is used in the title of dialog in which the interests are currently shown. Ah, right. Once again I prove my unfamiliarity with Java in Chrome :D
tache@google.com changed reviewers: + bauerb@chromium.org
bauerb@chromium.org changed reviewers: + newt@chromium.org
+Newt for NTP https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:186: private class DownloadImageTask extends AsyncTask<String, Integer, Drawable> { The progress type should be Void if it's unused. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:188: private InterestsItemView mView; If this is not a static class, it will have its outer object anyway. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:216: mCache.put(mUrl, image); I think you could simplify this a bit: Keep the cache out of this class and just always call a method in the outer class here, passing |this| along with the Drawable. That method updates the cache, and updates the view iff the passed task is the currently active one. That way you can get rid of the cache, URL and update flag in this class. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:32: private static final String TAG = "cr.browser.ntp"; Tags are not supposed to start with "cr." anymore; the logging system will automatically append it. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:94: * Releases internal resources. This must be called eventually, and the object should not used This comment is unnecessary. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:26: * The user's interests, displayed with a two collumn grid view. Nit: "column". https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:28: public class InterestsPageView extends LinearLayout { Just InterestsView? InterestsPageView implies something about how it's used. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:32: private InterestsListAdapter mAdapter; Make this final as well? https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:95: InterestsItemView view = (InterestsItemView) convertView; You could extract this variable and the return statement out of the if statement, as you assign a value in both branches and return it. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:14: public static final String AUTH_SCOPE = What's this for? https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:640: if (mDialog == null) { When would this happen?
https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/interests_page.xml (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/interests_page.xml:6: <org.chromium.chrome.browser.ntp.InterestsPageView InterestsPageView should be a FrameLayout (which is simpler) since it only has a single child. (and remove android:orientation too, sine that doesn't apply to FrameLayouts) https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/interests_page.xml:8: android:id="@+id/interests_content" nit: don't add an ID unless you're going to use it. Leaving out the ID makes it clear that the view isn't manipulated programmatically, which is helpful to know. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/interests_page.xml:16: <GridView xmlns:android="http://schemas.android.com/apk/res/android" remove redundant xmlns attribute (it's only needed on the root element in the file) https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/interests_page.xml:17: android:id="@+id/interests_list_view" indentation should look like this: <org.chromium.chrome.browser.ntp.InterestsPageView xmlns:android="http://schemas.android.com/apk/res/android" ... /> <GridView android:id="@+id/interests_list_view" ... /> </org.chromium.chrome.browser.ntp.InterestsPageView> https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:255: <dimen name="ntp_interest_item_image_text_size">20dp</dimen> Text sizes should always be specified in "sp" not "dp". This allows the text size to change when the user adjusts their system font size preferences. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:106: * Resets the view contents so that it can be reused in the list view. s/list/grid https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:115: mDownloadTask.setShouldUpdateView(false); How about mDownloadTask.cancel(), and then checking isCancelled() instead of mShouldUpdateView down below. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:121: if (mName != null && mName.equals(name) && mImageUrl != null You can use TextUtils.equals(name, mName) && TextUtils.equals(imageUrl, mImageUrl) https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:167: if (image == null) { Simply ignoring null images is surprising behavior. I'd just remove lines 167-169; all the callers check for null images before calling setImage() anyway. Also, make this method private. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:173: this.setCompoundDrawables(null, image, null, null); remove "this." https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:176: private void setImageBounds(Drawable image) { Why is this a separate method? I'd just inline it on line 171. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:186: private class DownloadImageTask extends AsyncTask<String, Integer, Drawable> { We should ideally use the Chrome network stack here (see url_fetcher.h). It provides automatic caching and allows for canceling in-progress downloads (which would be a good improvement here). In that case, you could move this code into the InterestsService, which already has a C++ side. Depending on the status of this "prototyping" you could make this a TODO for later. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:188: private InterestsItemView mView; On 2015/09/29 14:33:28, Bernhard Bauer wrote: > If this is not a static class, it will have its outer object anyway. Alternatively, make this a static class, which is better for inner classes anyway as it reduces scope of variables and allows for easier refactoring later if you want to pull out the inner class into a separate class. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:204: protected Drawable doInBackground(String... arg0) { s/arg0/args https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:26: public class InterestsPage implements NativePage { Will this ever be displayed as a webpage, or only as a dialog? If it's only ever a dialog, then there's no reason to implement NativePage. But perhaps on tablet, you want it to be a page instead of a dialog? https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:63: remove empty line https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:108: return "chrome-native://interests"; If this does need to be a NativePage, then add the URL and host to UrlConstants.java instead of hardcoding them here. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:59: private class InterestsListAdapter extends BaseAdapter { Extend ArrayAdapter instead of BaseAdapter. ArrayAdapter provides most of the methods below for you. (You also won't need to call notifyDataSetChanged() manually after switching to ArrayAdapter) https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:645: String url = String.format(GOOGLE_SEARCH_URL_PATTERN, I think you want TemplateUrlService.getInstance().getUrlForSearchQuery() here. Creating Google search URLs manually is fraught with peril, and you definitely shouldn't be doing that. For example, one immediate problem: this always searches google.com, never google.co.uk or bing.fr or whatever the user's default search engine is. (If you need to add custom query params to the URL, you might need to modify or add a new method to TemplateUrlService.) Also, how does this interact with non-Google search engines?
https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:14: public static final String AUTH_SCOPE = On 2015/09/29 14:33:28, Bernhard Bauer wrote: > What's this for? It was used in a previous revision, when getting the access token was done in Java. It can be removed now.
peconn@chromium.org changed reviewers: + peconn@chromium.org
This has been moved to https://codereview.chromium.org/1459593002/ . https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/interests_page.xml (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/interests_page.xml:6: <org.chromium.chrome.browser.ntp.InterestsPageView On 2015/09/29 17:33:21, newt wrote: > InterestsPageView should be a FrameLayout (which is simpler) since it only has a > single child. (and remove android:orientation too, sine that doesn't apply to > FrameLayouts) Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/interests_page.xml:8: android:id="@+id/interests_content" On 2015/09/29 17:33:21, newt wrote: > nit: don't add an ID unless you're going to use it. Leaving out the ID makes it > clear that the view isn't manipulated programmatically, which is helpful to > know. Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/interests_page.xml:16: <GridView xmlns:android="http://schemas.android.com/apk/res/android" On 2015/09/29 17:33:21, newt wrote: > remove redundant xmlns attribute (it's only needed on the root element in the > file) Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/interests_page.xml:17: android:id="@+id/interests_list_view" On 2015/09/29 17:33:21, newt wrote: > indentation should look like this: > > <org.chromium.chrome.browser.ntp.InterestsPageView > xmlns:android="http://schemas.android.com/apk/res/android" > ... /> > > <GridView > android:id="@+id/interests_list_view" > ... /> > > </org.chromium.chrome.browser.ntp.InterestsPageView> Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:255: <dimen name="ntp_interest_item_image_text_size">20dp</dimen> On 2015/09/29 17:33:21, newt wrote: > Text sizes should always be specified in "sp" not "dp". This allows the text > size to change when the user adjusts their system font size preferences. Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:106: * Resets the view contents so that it can be reused in the list view. On 2015/09/29 17:33:22, newt wrote: > s/list/grid Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:115: mDownloadTask.setShouldUpdateView(false); On 2015/09/29 17:33:22, newt wrote: > How about mDownloadTask.cancel(), and then checking isCancelled() instead of > mShouldUpdateView down below. This code was refactored out. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:121: if (mName != null && mName.equals(name) && mImageUrl != null On 2015/09/29 17:33:21, newt wrote: > You can use TextUtils.equals(name, mName) && TextUtils.equals(imageUrl, > mImageUrl) Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:167: if (image == null) { On 2015/09/29 17:33:22, newt wrote: > Simply ignoring null images is surprising behavior. I'd just remove lines > 167-169; all the callers check for null images before calling setImage() anyway. > Also, make this method private. Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:173: this.setCompoundDrawables(null, image, null, null); On 2015/09/29 17:33:21, newt wrote: > remove "this." Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:176: private void setImageBounds(Drawable image) { On 2015/09/29 17:33:22, newt wrote: > Why is this a separate method? I'd just inline it on line 171. Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:188: private InterestsItemView mView; On 2015/09/29 17:33:22, newt wrote: > On 2015/09/29 14:33:28, Bernhard Bauer wrote: > > If this is not a static class, it will have its outer object anyway. > > Alternatively, make this a static class, which is better for inner classes > anyway as it reduces scope of variables and allows for easier refactoring later > if you want to pull out the inner class into a separate class. The DownloadImageTask is pretty closely tied to the InterestsItemView. If you wanted to refactor it into a different class you'd have to edit this file anyway so I left this as a non-static class. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:204: protected Drawable doInBackground(String... arg0) { On 2015/09/29 17:33:22, newt wrote: > s/arg0/args Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsItemView.java:216: mCache.put(mUrl, image); On 2015/09/29 14:33:28, Bernhard Bauer wrote: > I think you could simplify this a bit: Keep the cache out of this class and just > always call a method in the outer class here, passing |this| along with the > Drawable. That method updates the cache, and updates the view iff the passed > task is the currently active one. That way you can get rid of the cache, URL and > update flag in this class. Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:32: private static final String TAG = "cr.browser.ntp"; On 2015/09/29 14:33:28, Bernhard Bauer wrote: > Tags are not supposed to start with "cr." anymore; the logging system will > automatically append it. Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:63: On 2015/09/29 17:33:22, newt wrote: > remove empty line Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPage.java:94: * Releases internal resources. This must be called eventually, and the object should not used On 2015/09/29 14:33:28, Bernhard Bauer wrote: > This comment is unnecessary. Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:26: * The user's interests, displayed with a two collumn grid view. On 2015/09/29 14:33:28, Bernhard Bauer wrote: > Nit: "column". Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:28: public class InterestsPageView extends LinearLayout { On 2015/09/29 14:33:28, Bernhard Bauer wrote: > Just InterestsView? InterestsPageView implies something about how it's used. Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:32: private InterestsListAdapter mAdapter; On 2015/09/29 14:33:28, Bernhard Bauer wrote: > Make this final as well? Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsPageView.java:95: InterestsItemView view = (InterestsItemView) convertView; On 2015/09/29 14:33:28, Bernhard Bauer wrote: > You could extract this variable and the return statement out of the if > statement, as you assign a value in both branches and return it. Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/InterestsService.java:14: public static final String AUTH_SCOPE = On 2015/10/06 14:12:33, Marc Treib wrote: > On 2015/09/29 14:33:28, Bernhard Bauer wrote: > > What's this for? > > It was used in a previous revision, when getting the access token was done in > Java. It can be removed now. Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:640: if (mDialog == null) { On 2015/09/29 14:33:28, Bernhard Bauer wrote: > When would this happen? Done. https://codereview.chromium.org/1351303003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:645: String url = String.format(GOOGLE_SEARCH_URL_PATTERN, On 2015/09/29 17:33:22, newt wrote: > I think you want TemplateUrlService.getInstance().getUrlForSearchQuery() here. > > Creating Google search URLs manually is fraught with peril, and you definitely > shouldn't be doing that. For example, one immediate problem: this always > searches http://google.com, never google.co.uk or bing.fr or whatever the user's > default search engine is. (If you need to add custom query params to the URL, > you might need to modify or add a new method to TemplateUrlService.) > > Also, how does this interact with non-Google search engines? Done. |
