Issue 1812293002: Add new NTP layout with snippet cards and hide it behind a flag

May

Description was changed from ========== Add new NTP layout with snippet cards and hide it ...

4 years, 9 months ago (2016-03-18 18:41:37 UTC) #1

May

Description was changed from ========== Add new NTP layout with snippet cards and hide it ...

4 years, 9 months ago (2016-03-18 18:56:09 UTC) #2

May

Description was changed from ========== Add new NTP layout with snippet cards and hide it ...

4 years, 9 months ago (2016-03-18 18:57:38 UTC) #4

May

newt@ -- not to put undue pressure on you, but just FYI that I will ...

4 years, 9 months ago (2016-03-21 10:19:17 UTC) #5

mcwilliams

Just a few nits https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/res/layout/new_tab_page_layout.xml File chrome/android/java/res/layout/new_tab_page_layout.xml (right): https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/res/layout/new_tab_page_layout.xml#newcode1 chrome/android/java/res/layout/new_tab_page_layout.xml:1: <?xml version="1.0" encoding="utf-8"?> Is there ...

4 years, 9 months ago (2016-03-21 12:45:50 UTC) #8

Bernhard Bauer

bauerb@chromium.org changed reviewers: + bauerb@chromium.org

4 years, 9 months ago (2016-03-21 15:13:56 UTC) #9

Bernhard Bauer

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/res/layout/new_tab_page_layout.xml File chrome/android/java/res/layout/new_tab_page_layout.xml (right): https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/res/layout/new_tab_page_layout.xml#newcode1 chrome/android/java/res/layout/new_tab_page_layout.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2016/03/21 12:45:50, mcwilliams wrote: > ...

4 years, 9 months ago (2016-03-21 15:13:56 UTC) #10

newt (away)

Off to a good start! I think this is a solid approach for allowing us ...

4 years, 9 months ago (2016-03-23 05:29:39 UTC) #11

Off to a good start! I think this is a solid approach for allowing us to
continue to improve the recycler view NTP, while minimizing the chances of
introducing bugs in the existing scroll view NTP.

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

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/new_tab_page_layout.xml:1: <?xml version="1.0"
encoding="utf-8"?>
On 2016/03/21 15:13:56, Bernhard Bauer wrote:
> On 2016/03/21 12:45:50, mcwilliams wrote:
> > Is there a way to preserve the history from new_page_tab.xml as this is a
copy
> > paste
> 
> You could try running `git cl upload` with the --similarity option. But even
if
> you don't, Git doesn't track file copying/renaming (it only keeps track of
file
> contents and uses heuristics when calculating diffs), so it only matters for
> purposes of this review.

Also, the change in indentation will probably thwart all efforts to get git to
recognize that this layout was moved from another file.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/new_tab_page_layout.xml:5:
<org.chromium.chrome.browser.ntp.NewTabPageLayout
were there any changes here, or was this a pure move?

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

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/new_tab_page_recyclerview.xml:7:
xmlns:chrome="http://schemas.android.com/apk/res-auto"
xmlns:chrome not needed

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

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/new_tab_page_scrollview.xml:8:
xmlns:chrome="http://schemas.android.com/apk/res-auto"
not needed

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:250: ||
ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS);
Is this check backwards? I'd expect the toolbar to be disabled when snippets are
enabled.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:17:
public class NewTabPageCardsManager implements Observer {
I'm not convinced this class should exist. Could everything here simply be
merged into NewTabPageAdapter? The constants and interface defined in this class
are core pieces of NewTabPageAdapter, so it makes sense to me to move them into
NewTabPageAdapter.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:31:
/** Base type for anything to add to the new tab page
fix wrapping, spacing, and indentation in comments throughout this file

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:44:
/** Represents the data for a header of a group of snippets
This comment doesn't seem right

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:59:
public NewTabPageCardsManager(SnippetsManager manager, NewTabRecyclerView
recyclerView,
javadoc

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:72:
if (data instanceof List) {
And if this instanceof check fails? Seems like a serious programming error (and
one that we'd want to find out about). I'd remove the instanceof check and just
use a cast here. Otherwise, a silent error might creep in where snippets aren't
being updated and no one notices for several releases.

Actually, see my comment later above removing use of Observer/Observable
altogether. This is just another reason to do so.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:85:
private NewTabRecyclerView mSnippetsView;
I'd put this next to mScrollView and add a comment that only one of these two
variables will be valid.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:262:
if (ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS)) {
Let's store this value in a member variable rather than rechecking it
everywhere.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:264:
mSnippetsView = (NewTabRecyclerView) stub.inflate();
mRecyclerView is probably a better name for this, since it holds not only the
snippets but also the rest of the NTP content.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:461:
// During startup the view may not be fully initialized, so we only calculate
the
fix wrapping and move this comment above the if-else statement so it isn't
duplicated.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:499:
int scrollY = getVerticalScroll();
why change this? You're using mScrollView explicitly down below, so I'd just
keep this as mScrollView.getScrollY()

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:666:
if (ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS)) {
Extract the common logic here, e.g.

    int scrollOffset = mAreSnippetsEnabled ? ... : ...;
    mContentView.setTranslationY(...);

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:712:
transformedBounds.offset(-getHorizontalScroll(), -getVerticalScroll());
I'd suggest always using the looping logic below -- not just for the no-snippets
case. It appears that the only adjustment you need to make is in computing the
scroll offset when handling the RecyclerView (use computeVerticalScrollOffset()
instead of getScrollY()). In other words, you could keep the old code, but
replace line 671 with this:

    if (view instanceof RecyclerView) {
        transformedBounds.offset(-view.computeHorizontalScrollOffset(),
-view.computeVerticalScrollOffset());
    } else {
        transformedBounds.offset(-view.getScrollX(), -view.getScrollY());
    }

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:717:
transformedBounds.offset(-getHorizontalScroll(), -getVerticalScroll());
This isn't right. We shouldn't be offsetting by the ScrollView's scroll each
time we go up a parent level; we should be offsetting by view.getScrollX() each
time (as in the old code).

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:776:
// Make the search box and logo the same width as the most visited tiles.
I'd just move all this measuring logic into NewTabPageLayout. That'll simplify
things, and remove the subtle timing dependency between the onMeasure() calls
and NewTabPageLayout.setMostVisitedWidth(). That's not the good type of subtle,
btw :P

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1081:
private int getHorizontalScroll() {
I don't think this method needs to exist (see comments above).

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java
(left):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:139:
public void focusableViewAvailable(View v) {
Are you sure it's OK to remove (i.e. not add) this? IIRC, this prevents "odd
jumps" when focusing the fakebox in landscape mode on a phone.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:70:
public void enableBottomShadow(int shadowColor) {
remove this (unless it's needed for some reason)

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:89:
} catch (IllegalArgumentException ex) {
This whole catch block can be removed. It's working around a bug that only
happens in ScrollView on certain old versions of Android.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:128:
if (mFadingShadow != null) {
We don't need this shadow, right? The shadow is used above the toolbar, but
there's no toolbar in the recycler view NTP world.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java:19:
* A class that represents the adapter backing the NTP RecyclerView. The first
element should always
Simpler: "The adapter backing the NTP RecyclerView."

"represents" is an overused word in class documentation. Most classes simply
*are* something, rather than *representing* something.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java:20:
* be the NTP above contents view, and subsequent elements are the cards to show
to the user.
What is "NTP above contents view"? Maybe: "the above-the-fold NTP contents
(logo, search box, most visited tiles)"

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java:23:
class NewTabPageAboveTheFoldViewHolder extends NewTabPageListItemViewHolder {
static

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java:34:
private List<NewTabPageListItem> mNewTabPageListItems;
might as well make these all final

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java:47:
mAboveTheFoldView = aboveTheFoldLayout;
Be consistent in your variable names. Unless there's a good reason to deviate,
the parameter name should match the member variable name. Variable names across
classes should be consistent too. I should be able to find all the places this
variable gets passed around between classes by grepping for "mAboveTheFoldView"
(or whatever you decide to call it).

You could even call this mNewTabPageLayout; then you don't need to create new
"aboveTheFold" terminology.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java:50:
public void setAboveTheFoldItem(AboveTheFoldListItem item) {
Why is this method even needed? We can simply do "mNewTabPageListItems.add(new
AboveTheFoldListItem())" in the constructor of this class.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java:61:
* Set the list of items to display in the snippets RecyclerView
nit: s/Set/Sets/

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageListItemViewHolder.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageListItemViewHolder.java:13:
* A class that represents an item we want to display in our list of card
snippets.
Update this comment, esp since the "main NTP content" is now included.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageListItemViewHolder.java:15:
abstract class NewTabPageListItemViewHolder extends RecyclerView.ViewHolder {
We could probably make this name a bit more concise. Maybe remove "List" or
"ListItem".

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageListItemViewHolder.java:19:
* Constructs a SnippetListItemViewHolder item used to display a component in the
list of
update comment

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageListItemViewHolder.java:43:
* @param article The SnippetListItem object that holds the data for this
ViewHolder
update javadoc and param name

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetCardItemViewHolder.java
(left):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetCardItemViewHolder.java:91:
String action = visibility == View.VISIBLE ? "MobileNTP.Snippets.ShowMore"
Do you need to mark this actions and histograms as deprecated?

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetCardItemViewHolder.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetCardItemViewHolder.java:81:
public void onBindViewHolder(NewTabPageListItem snippetItem) {
update param name

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:32:
public class SnippetsManager extends Observable {
Observable/Observer aren't commonly used in our codebase. In fact, they aren't
used at all, besides here! They have some pretty serious downsides as I see it:
 - The method names are so generic that the code the calls and implements those
methods is hard to understand at a glance
 - The observed quantity isn't documented anywhere. One has to examine the
implementation to understand what is really being observed. Even the *type* of
the observed object isn't documented (since these interfaces predate generics),
so one must make a blind cast from Object to the hopefully correct type.
 - The generic and reused method names thwart grep and even use Eclipse's "find
all callsites" feature

I'd suggest defining and implementing an explicit SnippetsChanged interface.
However, if you follow my suggestion below about combining the Manager classes,
you won't need to implement any observer interface at all.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:45:
private List<NewTabPageListItem> mListSnippetItems = new
ArrayList<NewTabPageListItem>();
nit: s/<NewTabPageListItem>/<>/

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:62:
public static class SnippetArticle implements NewTabPageListItem {
SnippetArticle contains more code than SnippetsManager, which is almost empty
besides listening to the SnippetsBridge and dealing with Observable. I'd suggest
moving SnippetArticle into its own separate file.

Also, it seems your Manager classes are having a minor identity crisis.
SnippetsManager, NewTabPageCardsManager, and NewTabPageAdapter don't have
clearly separated roles, AFAICT. SnippetsManager listens for updates to the list
of snippets, then notifies NewTabPageCardsManager, which does nothing but
forward the update to NewTabPageAdapter.

I'd suggest merging all three of these classes into a single class
(NewTabPageAdapter). Splitting code into separate classes is useful only insofar
as there's a clear division of responsibility. In this case, the division seems
arbitrary (caused by organic growth and change of code -- I understand :), so it
makes sense to remove it. (Alternatively, perhaps there is a logical division
between these classes, in which case you could re-divide responsibilities along
those lines.)

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:106:
mThumbnailView.setVisibility(View.VISIBLE);
We could fade in the images:

    mThumbnailView.setAlpha(0);
    mThumbnailView.animate().alpha(1).duration(...);

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:149:
mListSnippetItems = new ArrayList<NewTabPageListItem>();
Do you need to create a new ArrayList here? Seems like mListSnippetItems.clear()
would suffice.

May

FYI -- added background color to the new NTP as well since it was trivial ...

4 years, 9 months ago (2016-03-23 19:22:58 UTC) #12

FYI -- added background color to the new NTP as well since it was trivial to do
so.

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

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/new_tab_page_layout.xml:5:
<org.chromium.chrome.browser.ntp.NewTabPageLayout
On 2016/03/23 05:29:37, newt wrote:
> were there any changes here, or was this a pure move?

This was a pure move.

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

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/new_tab_page_recyclerview.xml:1: <?xml
version="1.0" encoding="utf-8"?>
On 2016/03/21 12:45:50, mcwilliams wrote:
> Should this file not be named ...recycler_view.xml

I'd prefer to keep it as is, since RecyclerView is an Android element.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/new_tab_page_recyclerview.xml:7:
xmlns:chrome="http://schemas.android.com/apk/res-auto"
On 2016/03/23 05:29:37, newt wrote:
> xmlns:chrome not needed

Done.

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

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/new_tab_page_scrollview.xml:1: <?xml
version="1.0" encoding="utf-8"?>
On 2016/03/21 12:45:50, mcwilliams wrote:
> Likewise here ...scroll_view.xml for naming conventions

Same, ScrollView is an Android element so prefer to keep that globbed together.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/new_tab_page_scrollview.xml:8:
xmlns:chrome="http://schemas.android.com/apk/res-auto"
On 2016/03/23 05:29:37, newt wrote:
> not needed

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:250: ||
ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS);
On 2016/03/23 05:29:37, newt wrote:
> Is this check backwards? I'd expect the toolbar to be disabled when snippets
are
> enabled.

Wait... this is weird. I must not have uploaded a newer change, it's the correct
way in my local copy ( &&
!ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS). Anyways, done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:17:
public class NewTabPageCardsManager implements Observer {
On 2016/03/23 05:29:38, newt wrote:
> I'm not convinced this class should exist. Could everything here simply be
> merged into NewTabPageAdapter? The constants and interface defined in this
class
> are core pieces of NewTabPageAdapter, so it makes sense to me to move them
into
> NewTabPageAdapter.

Yeah, I think NewTabPageAdapter could be combined with this. WRT your later
comments about NewTabPageCardsManager / NewTabPageAdapter / SnippetsManager
having blurry purposes, the intention is is for NewTabPageCardsManager (and
combined NewTabPageAdapter I suppose) to handle and merge all the cards that we
will show on the NTP. There's cards other than snippets that are planned in the
pipeline (but all still in the PRD stage).

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:31:
/** Base type for anything to add to the new tab page
On 2016/03/23 05:29:38, newt wrote:
> fix wrapping, spacing, and indentation in comments throughout this file

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:44:
/** Represents the data for a header of a group of snippets
On 2016/03/23 05:29:38, newt wrote:
> This comment doesn't seem right

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:59:
public NewTabPageCardsManager(SnippetsManager manager, NewTabRecyclerView
recyclerView,
On 2016/03/23 05:29:38, newt wrote:
> javadoc

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:72:
if (data instanceof List) {
On 2016/03/23 05:29:37, newt wrote:
> And if this instanceof check fails? Seems like a serious programming error
(and
> one that we'd want to find out about). I'd remove the instanceof check and
just
> use a cast here. Otherwise, a silent error might creep in where snippets
aren't
> being updated and no one notices for several releases.
> 
> Actually, see my comment later above removing use of Observer/Observable
> altogether. This is just another reason to do so.

Yeah, I was trying to avoid defining yet another new interface when there's
already nice support for it, but I'll remove it. It's more complex than what we
need anyway.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:85:
private NewTabRecyclerView mSnippetsView;
On 2016/03/23 05:29:38, newt wrote:
> I'd put this next to mScrollView and add a comment that only one of these two
> variables will be valid.

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:262:
if (ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS)) {
On 2016/03/23 05:29:38, newt wrote:
> Let's store this value in a member variable rather than rechecking it
> everywhere.

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:264:
mSnippetsView = (NewTabRecyclerView) stub.inflate();
On 2016/03/23 05:29:38, newt wrote:
> mRecyclerView is probably a better name for this, since it holds not only the
> snippets but also the rest of the NTP content.

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:461:
// During startup the view may not be fully initialized, so we only calculate
the
On 2016/03/23 05:29:38, newt wrote:
> fix wrapping and move this comment above the if-else statement so it isn't
> duplicated.

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:499:
int scrollY = getVerticalScroll();
On 2016/03/23 05:29:38, newt wrote:
> why change this? You're using mScrollView explicitly down below, so I'd just
> keep this as mScrollView.getScrollY()

Reverted.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:666:
if (ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS)) {
On 2016/03/23 05:29:38, newt wrote:
> Extract the common logic here, e.g.
> 
>     int scrollOffset = mAreSnippetsEnabled ? ... : ...;
>     mContentView.setTranslationY(...);

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:712:
transformedBounds.offset(-getHorizontalScroll(), -getVerticalScroll());
On 2016/03/23 05:29:38, newt wrote:
> I'd suggest always using the looping logic below -- not just for the
no-snippets
> case. It appears that the only adjustment you need to make is in computing the
> scroll offset when handling the RecyclerView (use
computeVerticalScrollOffset()
> instead of getScrollY()). In other words, you could keep the old code, but
> replace line 671 with this:
> 
>     if (view instanceof RecyclerView) {
>         transformedBounds.offset(-view.computeHorizontalScrollOffset(),
> -view.computeVerticalScrollOffset());
>     } else {
>         transformedBounds.offset(-view.getScrollX(), -view.getScrollY());
>     }

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:717:
transformedBounds.offset(-getHorizontalScroll(), -getVerticalScroll());
On 2016/03/23 05:29:38, newt wrote:
> This isn't right. We shouldn't be offsetting by the ScrollView's scroll each
> time we go up a parent level; we should be offsetting by view.getScrollX()
each
> time (as in the old code).

Good catch. Fixed.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:776:
// Make the search box and logo the same width as the most visited tiles.
On 2016/03/23 05:29:38, newt wrote:
> I'd just move all this measuring logic into NewTabPageLayout. That'll simplify
> things, and remove the subtle timing dependency between the onMeasure() calls
> and NewTabPageLayout.setMostVisitedWidth(). That's not the good type of
subtle,
> btw :P

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1081:
private int getHorizontalScroll() {
On 2016/03/23 05:29:38, newt wrote:
> I don't think this method needs to exist (see comments above).

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java
(left):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:139:
public void focusableViewAvailable(View v) {
On 2016/03/23 05:29:38, newt wrote:
> Are you sure it's OK to remove (i.e. not add) this? IIRC, this prevents "odd
> jumps" when focusing the fakebox in landscape mode on a phone.

Looks fine to me. See video:
https://drive.google.com/open?id=0B1flmW1uZoFBMUVRS3NkVkl1U2c

Is there something I should be looking out for?

(Ignore non-centered-ness of the logo and searchbox, I've filed a bug to fix
that later)

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:70:
public void enableBottomShadow(int shadowColor) {
On 2016/03/23 05:29:38, newt wrote:
> remove this (unless it's needed for some reason)

Removed. I think there's a bottom shadow on the mocks...? But I don't have spec
for what it should be, so removing for now.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:89:
} catch (IllegalArgumentException ex) {
On 2016/03/23 05:29:38, newt wrote:
> This whole catch block can be removed. It's working around a bug that only
> happens in ScrollView on certain old versions of Android.

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:128:
if (mFadingShadow != null) {
On 2016/03/23 05:29:38, newt wrote:
> We don't need this shadow, right? The shadow is used above the toolbar, but
> there's no toolbar in the recycler view NTP world.

Removed.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java:19:
* A class that represents the adapter backing the NTP RecyclerView. The first
element should always
On 2016/03/23 05:29:38, newt wrote:
> Simpler: "The adapter backing the NTP RecyclerView."
> 
> "represents" is an overused word in class documentation. Most classes simply
> *are* something, rather than *representing* something.

Acknowledged. Merged this into NewTabPageCardsManager.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java:20:
* be the NTP above contents view, and subsequent elements are the cards to show
to the user.
On 2016/03/23 05:29:39, newt wrote:
> What is "NTP above contents view"? Maybe: "the above-the-fold NTP contents
> (logo, search box, most visited tiles)"

Acknowledged. Merged this into NewTabPageCardsManager.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java:23:
class NewTabPageAboveTheFoldViewHolder extends NewTabPageListItemViewHolder {
On 2016/03/23 05:29:38, newt wrote:
> static

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java:34:
private List<NewTabPageListItem> mNewTabPageListItems;
On 2016/03/23 05:29:39, newt wrote:
> might as well make these all final

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java:47:
mAboveTheFoldView = aboveTheFoldLayout;
On 2016/03/23 05:29:38, newt wrote:
> Be consistent in your variable names. Unless there's a good reason to deviate,
> the parameter name should match the member variable name. Variable names
across
> classes should be consistent too. I should be able to find all the places this
> variable gets passed around between classes by grepping for
"mAboveTheFoldView"
> (or whatever you decide to call it).
> 
> You could even call this mNewTabPageLayout; then you don't need to create new
> "aboveTheFold" terminology.

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java:50:
public void setAboveTheFoldItem(AboveTheFoldListItem item) {
On 2016/03/23 05:29:38, newt wrote:
> Why is this method even needed? We can simply do "mNewTabPageListItems.add(new
> AboveTheFoldListItem())" in the constructor of this class.

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageAdapter.java:61:
* Set the list of items to display in the snippets RecyclerView
On 2016/03/23 05:29:39, newt wrote:
> nit: s/Set/Sets/

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageListItemViewHolder.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageListItemViewHolder.java:13:
* A class that represents an item we want to display in our list of card
snippets.
On 2016/03/23 05:29:39, newt wrote:
> Update this comment, esp since the "main NTP content" is now included.

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageListItemViewHolder.java:15:
abstract class NewTabPageListItemViewHolder extends RecyclerView.ViewHolder {
On 2016/03/23 05:29:39, newt wrote:
> We could probably make this name a bit more concise. Maybe remove "List" or
> "ListItem".

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageListItemViewHolder.java:19:
* Constructs a SnippetListItemViewHolder item used to display a component in the
list of
On 2016/03/23 05:29:39, newt wrote:
> update comment

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/NewTabPageListItemViewHolder.java:43:
* @param article The SnippetListItem object that holds the data for this
ViewHolder
On 2016/03/23 05:29:39, newt wrote:
> update javadoc and param name

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetCardItemViewHolder.java
(left):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetCardItemViewHolder.java:91:
String action = visibility == View.VISIBLE ? "MobileNTP.Snippets.ShowMore"
On 2016/03/23 05:29:39, newt wrote:
> Do you need to mark this actions and histograms as deprecated?

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetCardItemViewHolder.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetCardItemViewHolder.java:81:
public void onBindViewHolder(NewTabPageListItem snippetItem) {
On 2016/03/23 05:29:39, newt wrote:
> update param name

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:32:
public class SnippetsManager extends Observable {
On 2016/03/23 05:29:39, newt wrote:
> Observable/Observer aren't commonly used in our codebase. In fact, they aren't
> used at all, besides here! They have some pretty serious downsides as I see
it:
>  - The method names are so generic that the code the calls and implements
those
> methods is hard to understand at a glance
>  - The observed quantity isn't documented anywhere. One has to examine the
> implementation to understand what is really being observed. Even the *type* of
> the observed object isn't documented (since these interfaces predate
generics),
> so one must make a blind cast from Object to the hopefully correct type.
>  - The generic and reused method names thwart grep and even use Eclipse's
"find
> all callsites" feature
> 
> I'd suggest defining and implementing an explicit SnippetsChanged interface.
> However, if you follow my suggestion below about combining the Manager
classes,
> you won't need to implement any observer interface at all.

Changed to a custom implementation.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:45:
private List<NewTabPageListItem> mListSnippetItems = new
ArrayList<NewTabPageListItem>();
On 2016/03/23 05:29:39, newt wrote:
> nit: s/<NewTabPageListItem>/<>/

Done.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:62:
public static class SnippetArticle implements NewTabPageListItem {
On 2016/03/23 05:29:39, newt wrote:
> SnippetArticle contains more code than SnippetsManager, which is almost empty
> besides listening to the SnippetsBridge and dealing with Observable. I'd
suggest
> moving SnippetArticle into its own separate file.
> 
> Also, it seems your Manager classes are having a minor identity crisis.
> SnippetsManager, NewTabPageCardsManager, and NewTabPageAdapter don't have
> clearly separated roles, AFAICT. SnippetsManager listens for updates to the
list
> of snippets, then notifies NewTabPageCardsManager, which does nothing but
> forward the update to NewTabPageAdapter.
> 
> I'd suggest merging all three of these classes into a single class
> (NewTabPageAdapter). Splitting code into separate classes is useful only
insofar
> as there's a clear division of responsibility. In this case, the division
seems
> arbitrary (caused by organic growth and change of code -- I understand :), so
it
> makes sense to remove it. (Alternatively, perhaps there is a logical division
> between these classes, in which case you could re-divide responsibilities
along
> those lines.)

Moved SnippetArticle into its own file. We still need to do a favicon fetching
and thumbnail caching so it's going to get more complex than it currently is.

I decided not to merge this into the other classes for the reasons mentioned
earlier in this CR. SnippetManager is intended to handle all snippets-related
data. NewTabPageCardsManager is intended to handle data coming from all
providers. At the moment, this is just the above the fold stuff and snippets,
but later we will be adding more data providers.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:106:
mThumbnailView.setVisibility(View.VISIBLE);
On 2016/03/23 05:29:39, newt wrote:
> We could fade in the images:
> 
>     mThumbnailView.setAlpha(0);
>     mThumbnailView.animate().alpha(1).duration(...);

Fancy! :-D Added.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:149:
mListSnippetItems = new ArrayList<NewTabPageListItem>();
On 2016/03/23 05:29:39, newt wrote:
> Do you need to create a new ArrayList here? Seems like
mListSnippetItems.clear()
> would suffice.

Done.

May

maybelle@chromium.org changed reviewers: + rkaplow@chromium.org

4 years, 9 months ago (2016-03-23 19:25:13 UTC) #14

May

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java#newcode129 chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:129: int mostVisitedWidthSpec = To whoever takes over this CL: ...

4 years, 9 months ago (2016-03-24 07:21:20 UTC) #17

dgn

I got some layout issues when running this: https://goo.gl/photos/BxCkQs25vpABPkaW7 https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java (right): https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java#newcode20 chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:20: ...

4 years, 9 months ago (2016-03-24 13:56:26 UTC) #19

newt (away)

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java (left): https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java#oldcode139 chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:139: public void focusableViewAvailable(View v) { On 2016/03/23 19:22:57, May ...

4 years, 9 months ago (2016-03-24 19:47:39 UTC) #20

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java
(left):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:139:
public void focusableViewAvailable(View v) {
On 2016/03/23 19:22:57, May wrote:
> On 2016/03/23 05:29:38, newt wrote:
> > Are you sure it's OK to remove (i.e. not add) this? IIRC, this prevents "odd
> > jumps" when focusing the fakebox in landscape mode on a phone.
> 
> Looks fine to me. See video:
> https://drive.google.com/open?id=0B1flmW1uZoFBMUVRS3NkVkl1U2c
> 
> Is there something I should be looking out for?
> 
> (Ignore non-centered-ness of the logo and searchbox, I've filed a bug to fix
> that later)

This code is still needed.

See https://gerrit-int.chromium.org/44249 for why this was added in the first
place.

In general, don't remove a bug fix unless you understand why it was added and
are reasonably confident that it's no longer needed.

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java
(right):

https://codereview.chromium.org/1812293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:70:
public void enableBottomShadow(int shadowColor) {
On 2016/03/23 19:22:57, May wrote:
> On 2016/03/23 05:29:38, newt wrote:
> > remove this (unless it's needed for some reason)
> 
> Removed. I think there's a bottom shadow on the mocks...? But I don't have
spec
> for what it should be, so removing for now.

This is for the bottom shadow that appears above the toolbar. We might want an
overscroll effect when the user scrolls quickly to the very bottom of the
recycler view, but we probably don't want this type of shadow.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java
(right):

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:24:
public class NewTabPageCardsManager extends Adapter<NewTabPageViewHolder>
I think NewTabPageAdapter is a better name here. The presentation as cards is
secondary, and, in fact, some of the elements in this adapter (such as the logo,
fakebox, most visited sites, and the article headers) aren't cards at all.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:27:
* Describes the above the fold item
This comment is rather vague. Could you make it more useful? In particular, it'd
be helpful to mention that these values are "view types" returned by
ecyclerView.Adapter.getItemViewType().

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:29:
public static final int SNIPPET_ITEM_ABOVE_THE_FOLD = 1;
can these type constants be private?

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:57:
static class AboveTheFoldListItem implements NewTabPageListItem {
make this class and the class below private, if possible

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:66:
static class NewTabPageAboveTheFoldViewHolder extends NewTabPageViewHolder {
this name should parallel the class name above.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:80:
private List<NewTabPageListItem> mNewTabPageListItems;
might as well make this final too

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:93:
mNewTabPageAboveTheFoldItem = new AboveTheFoldListItem();
Likewise, this member variable name should match the class name
(AboveTheFoldListItem). In general, avoid creating multiple different
terminologies for the same item unless there's a good reason for it.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java
(right):

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:87:
public void setMostVisitedWidth(int width) {
As mentioned in NewTabPageView, this method should not exist.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:355:
if (!ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS)) {
better: if (mScrollView != null)

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:468:
if (mAreSnippetsEnabled) {
lines 468-478 need more de-duplication. The only difference between the two part
is "mRecyclerView.getHeight()" and "mScrollView.getHeight()"; extract that
difference and remove the duplication.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:770:
protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
This method should be deleted and the remainder of the logic in it should also
be moved to NewTabPageLayout. In particular, mContentView.setMostVisitedWidth()
should not exist; that's the subtle timing dependency that I mentioned should be
removed.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java
(right):

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:20:
public class NewTabRecyclerView extends RecyclerView {
On 2016/03/24 13:56:26, dgn wrote:
> s/NewTabRecyclerView/NewTabPageRecyclerView ? Is the [Page] missing on
purpose?

This name parallels NewTabScrollView, but is inconsistent with all the other
NewTabPage* classes. I'd suggest adding "Page" to this class name and to
NewTabScrollView, for consistency with everything else.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:57:
mGestureDetector.onTouchEvent(ev);
On 2016/03/24 13:56:26, dgn wrote:
> Q: Why is this done in onInterceptTouchEvent if we don't change the returned
> value? Wouldn't it be the same if calling the GestureDetector was done in
> onTouchEvent?

I'm not sure. Ted originally added this in https://gerrit-int.chromium.org/41926
. He might have an idea.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:27:
private static final int FADE_IN_ANIMATION_TIME_MS = 500;
300ms would be more standard

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java
(right):

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:35:
public void onBindViewHolder(NewTabPageListItem snippetItem) {}
why are we no longer setting the header text here?

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java
(right):

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java:19:
public class SnippetsManager {
I still don't understand why it's useful to keep this as a separate class,
rather than combining it into NewTabPageAdapter.

This class simply listens for updates from the SnippetsBridge and then forwards
those updates to NewTabPageAdapter. This adds extra indirection and complexity
and doesn't seem to add value. Is there some way that this class does add value
that I'm not seeing?

dgn

I'm taking over the CL since May is OOO for a while. I uploaded a ...

4 years, 9 months ago (2016-03-25 16:29:20 UTC) #21

dgn

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageViewHolder.java File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageViewHolder.java (right): https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageViewHolder.java#newcode37 chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageViewHolder.java:37: protected void loadUrl(String url) { On 2016/03/25 16:29:20, dgn ...

4 years, 9 months ago (2016-03-25 16:31:25 UTC) #22

dgn

4 years, 8 months ago (2016-03-28 21:33:02 UTC) #23

Addressed comments on https://codereview.chromium.org/1833763002

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:600:
mSnippetsManager = new SnippetsManager(mNewTabPageManager, mProfile);
How about checking here if the feature is enabled?
That would allow skipping the creation of the SnippetsManager (and Bridge, and
native stuff), and could allow loading a different layout directly, instead of
relying on a stub element in the new_tab_page layout.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java
(right):

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:24:
public class NewTabPageCardsManager extends Adapter<NewTabPageViewHolder>
On 2016/03/24 19:47:38, newt wrote:
> I think NewTabPageAdapter is a better name here. The presentation as cards is
> secondary, and, in fact, some of the elements in this adapter (such as the
logo,
> fakebox, most visited sites, and the article headers) aren't cards at all.

Done.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:27:
* Describes the above the fold item
On 2016/03/24 19:47:38, newt wrote:
> This comment is rather vague. Could you make it more useful? In particular,
it'd
> be helpful to mention that these values are "view types" returned by
> ecyclerView.Adapter.getItemViewType().

Done.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:29:
public static final int SNIPPET_ITEM_ABOVE_THE_FOLD = 1;
On 2016/03/24 19:47:38, newt wrote:
> can these type constants be private?

It needs to be used at least by NewTabPageListItem's subclasses. I could make it
protected in there, maybe? I don't think it changes much at that point though.
I'll revisit after the SnippetsManager refactoring.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:57:
static class AboveTheFoldListItem implements NewTabPageListItem {
On 2016/03/24 19:47:38, newt wrote:
> make this class and the class below private, if possible

Done.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:66:
static class NewTabPageAboveTheFoldViewHolder extends NewTabPageViewHolder {
On 2016/03/24 19:47:38, newt wrote:
> this name should parallel the class name above.

Done.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:80:
private List<NewTabPageListItem> mNewTabPageListItems;
On 2016/03/24 19:47:38, newt wrote:
> might as well make this final too

Done.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageCardsManager.java:93:
mNewTabPageAboveTheFoldItem = new AboveTheFoldListItem();
On 2016/03/24 19:47:38, newt wrote:
> Likewise, this member variable name should match the class name
> (AboveTheFoldListItem). In general, avoid creating multiple different
> terminologies for the same item unless there's a good reason for it.

Done.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java
(right):

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:87:
public void setMostVisitedWidth(int width) {
On 2016/03/24 19:47:39, newt wrote:
> As mentioned in NewTabPageView, this method should not exist.

Done.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:129:
int mostVisitedWidthSpec =
On 2016/03/24 07:21:20, May wrote:
> To whoever takes over this CL: This (line 129 - 136) should be an if
> (mMostVisitedWidth > 0) block. 

Done.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:355:
if (!ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS)) {
On 2016/03/24 19:47:39, newt wrote:
> better: if (mScrollView != null)

We have mAreSnippetsEnabled for that, right? Isn't it clearer to use it? We
probably want it to crash is somehow mScrollView turns out to be created with
enabledSnippets

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:468:
if (mAreSnippetsEnabled) {
On 2016/03/24 19:47:39, newt wrote:
> lines 468-478 need more de-duplication. The only difference between the two
part
> is "mRecyclerView.getHeight()" and "mScrollView.getHeight()"; extract that
> difference and remove the duplication.

Done.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:770:
protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
On 2016/03/24 19:47:39, newt wrote:
> This method should be deleted and the remainder of the logic in it should also
> be moved to NewTabPageLayout. In particular,
mContentView.setMostVisitedWidth()
> should not exist; that's the subtle timing dependency that I mentioned should
be
> removed.

Done. I'm not familiar with the layout mechanisms though. In which cases do we
run into those timing dependencies? I'd expect the measurements to happen on the
UI thread only and not have race conditions...

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java
(right):

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabRecyclerView.java:20:
public class NewTabRecyclerView extends RecyclerView {
On 2016/03/24 19:47:39, newt wrote:
> On 2016/03/24 13:56:26, dgn wrote:
> > s/NewTabRecyclerView/NewTabPageRecyclerView ? Is the [Page] missing on
> purpose?
> 
> This name parallels NewTabScrollView, but is inconsistent with all the other
> NewTabPage* classes. I'd suggest adding "Page" to this class name and to
> NewTabScrollView, for consistency with everything else.

Done.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:27:
private static final int FADE_IN_ANIMATION_TIME_MS = 500;
On 2016/03/24 19:47:39, newt wrote:
> 300ms would be more standard

Done.

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java
(right):

https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:35:
public void onBindViewHolder(NewTabPageListItem snippetItem) {}
On 2016/03/24 19:47:39, newt wrote:
> why are we no longer setting the header text here?

The string is now directly provided in new_tab_page_snippets_header.xml

PEConn

4 years, 8 months ago (2016-04-01 15:48:25 UTC) #24

Description was changed from

==========
Add new NTP layout with snippet cards and hide it behind a flag

Swapping out the ScrollView in the current NTP for a RecyclerView for the new
NTP with cards.
The current NTP's content view will be preserved and added as a View in the new
RecyclerView.
The swapping will happen via a ViewStub that gets inflated at runtime, depending
on the runtime switches.

Mocks are here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/160225-ChromeP...
Spec is here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/_UI%20Review#%...

Video demos:
Feature off: https://drive.google.com/open?id=0B1flmW1uZoFBWVZzVjZWRVotbjg
Feature on: https://drive.google.com/open?id=0B1flmW1uZoFBT0pNU2tFc3FlSDg

Known issues:
Missing favicon, publisher
Cards are not full width
NTP background is not the right gradient
Snap points are not yet implemented on new NTP
Cards are not shown as a hint yet
Search box jumps back down when scrolling between above the fold and the cards

BUG=584352
==========

to

==========
Add new NTP layout with snippet cards and hide it behind a flag

Swapping out the ScrollView in the current NTP for a RecyclerView for the new
NTP with cards.
The current NTP's content view will be preserved and added as a View in the new
RecyclerView.
The swapping will happen via a ViewStub that gets inflated at runtime, depending
on the runtime switches.

Mocks are here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/160225-ChromeP...
Spec is here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/_UI%20Review#%...

Video demos:
Feature off: https://drive.google.com/open?id=0B1flmW1uZoFBWVZzVjZWRVotbjg
Feature on: https://drive.google.com/open?id=0B1flmW1uZoFBT0pNU2tFc3FlSDg

Known issues:
Missing favicon, publisher
Cards are not full width
NTP background is not the right gradient
Snap points are not yet implemented on new NTP
Cards are not shown as a hint yet
Search box jumps back down when scrolling between above the fold and the cards

BUG=584352

This was continued in https://codereview.chromium.org/1833763002 .
==========

Issue 1812293002: Add new NTP layout with snippet cards and hide it behind a flag (Closed)

Description

Patch Set 1 #

Patch Set 2 : rebase + CR comments + added background color to new NTP #

Messages