Currently, we don't have mocks for this, but this should look just like the Highlights section. Pocket stories should be its own section inside activity stream and should appear above Highlights. ADDITIONALLY it should include the "from Pocket" branding. When fetching pocket stories make sure that it doesn't slow down the rest of the UI. Pocket stories should be fetched async from highlights and Topsites and inserted without reloading the entire table. There should also be a section header similar to desktop (attached). Each pocket story should also have a description saying it is a trending story in the place where it would say "Visited" or "Bookmarked", including the "Trending" lightning bolt branding. All existing functionality related to the context menu such as bookmarking/opening in new tab should work, although "delete from history" should be removed.

Hi Aaron, we added this into the current sprint, though there's one more bug that needs to happen before this, but that should be done early this week (hopefully). Can you upload the mocks to this bug when they're done, thanks!

Here's the InVision mockup for Pocket stories on New Tab: https://mozilla.invisionapp.com/share/B5CVCBOP3#/screens/246811554_New_Tab_-_Pocket_Stories

Still waiting on Pocket build requirements to settle (bug 1387553, bug 1386906), but that doesn't block starting development on adding Pocket to the new tab page so I'm picking this up.

need-info-ing bbell to add the "trending story" icon to this bug.

Aaron, I have a question about what goes in the context menu for a Pocket item? I was thinking: Open in new tab Open in private tab --- Dismiss --- Share Copy address Any others? The other possible ones in the last section could also be: Bookmark, Add to Home Screen, neither of which seem relevant.

Those look good, though we could include Bookmark and Save to Pocket (which would hand off to the Pocket app). Bookmarking is a little odd because you haven't viewed the article but you could use that flow as a reminder, which is why we included it on desktop. Bookmark Save to Pocket* --- Open in new tab Open in private tab --- Dismiss --- Share Copy address * optional for now if this is problematic

Yeah, I think "save to Pocket" would require some other flows, because there's no guarantee that people are logged into Pocket or even have a Pocket account. And even when adding that, it might make more sense to first check if user is logged into pocket, but that will be out of the scope of this bug. I'll file though, and flag it for triage. Thanks for the quick response!

Comment on attachment 8898009 [details] Bug 1380808 - Use a single data model, stop using resource ids as viewTypes. This is the enum approach I was asking you about earlier. It's messy but consolidates a lot of the hard-coding of positions into a single ugly enum, rather than scattering the hard-coding all over the place.

Trending icon.

Reflagging need-info for the lightning bolt icon instead of jagged arrow.

Updated.

Aaron, what happens if people dismiss all 3 Pocket items (and go through the queue). I'm not sure I'll implement that in this bug, but what happens in the case where the Pocket Stories is empty? Does the whole section go away?

On desktop we have a zero-state that has a icon and filler text. On mobile, however, real estate is a little more precious so it should probably hide the section altogether until new stories come in. Will NI Bryan, too, and get his thoughts.

Comment on attachment 8898009 [details] Bug 1380808 - Use a single data model, stop using resource ids as viewTypes. https://reviewboard.mozilla.org/r/169316/#review176012 Cool solution! It's great how everything is in the same data structure and you wrap the values for the fixed types. ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:34 (Diff revision 2) > import java.util.EnumSet; > +import java.util.LinkedList; > import java.util.List; > > +/** > + * StreamRecycleAdapter is the adapter for the Activity Stream panel. nit: -> `StreamRecyclerAdapter`. fwiw, I usually don't name the thing directly in my comments to avoid it going out of date (rather, I describe its function in my comments and since the name should describe its function, this works well enough). ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:38 (Diff revision 2) > +/** > + * StreamRecycleAdapter is the adapter for the Activity Stream panel. > + * > + * Every item is in a single adapter: Top Sites, Welcome panel, Highlights. > + */ > + nit: ws ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:54 (Diff revision 2) > private HomePager.OnUrlOpenInBackgroundListener onUrlOpenInBackgroundListener; > > private int tiles; > private int tilesSize; > > - private List<Highlight> highlights; > + // Item types created with (viewType, stable id) nit: This comment is redundant to the constructor so I'm concerned it could go out of date (but I do find it helpful for this to come next to the enum definitions, whereas the constructor, by convention, is awkwardly placed). ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:61 (Diff revision 2) > + TOP_PANEL (0, -2), // RecyclerView.NO_ID is -1, so start hard-coded stableIds at -2. > + WELCOME (1, -3), > + HIGHLIGHTS_TITLE (2, -4), > + HIGHLIGHT_ITEM (3, -1); // There can be multiple Highlight Items so caller should handle as a special case. > + > + public final int viewType; Rather than using `viewType` as incremental values from 0, you could remove the field and use [`Enum.ordinal()`](https://developer.android.com/reference/java/lang/Enum.html#ordinal()). Since it isn't as readable, I think you should be able to wrap it in a function like `getViewType() { return this.ordinal() }` ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:64 (Diff revision 2) > + HIGHLIGHT_ITEM (3, -1); // There can be multiple Highlight Items so caller should handle as a special case. > + > + public final int viewType; > + public final int stableId; > + > + RowItemType(int val, int id) { nit: it'd be a little clearer if the arguments were the same names as the fields ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:129 (Diff revision 2) > @Override > public void onBindViewHolder(StreamItem holder, int position) { > int type = getItemViewType(position); > - > - if (type == HighlightItem.LAYOUT_ID) { > - final int actualPosition = translatePositionToCursor(position); > + if (type == RowItemType.HIGHLIGHT_ITEM.viewType) { > + final Highlight highlight = (Highlight) recyclerViewModel.get(position); > + ((HighlightItem) holder).bind(highlight, position - HIGHLIGHTS_OFFSET, tilesSize); nit: if we put `position - HIGHLIGHTS_OFFSET` in a function like, `getPositionInHighlightsFromRecyclerModelPosition`, we're less likely to get the math wrong and it makes it easy to clean up this code when we add Pocket items. `position - offset` is used in other places too so if you add the method, change those too! ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:200 (Diff revision 2) > - final Highlight highlight = highlights.get(translatePositionToCursor(position)); > - > - // Highlights are always picked from recent history - So using the history id should > + // Highlights are always picked from recent history - So using the history id should > - // give us a unique (positive) id. > + // give us a unique (positive) id. > + final Highlight highlight = (Highlight) recyclerViewModel.get(position); > + final long id = highlight.getHistoryId(); unused ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/RowItem.java:10 (Diff revision 2) > + > +package org.mozilla.gecko.activitystream.homepanel.model; > + > +import org.mozilla.gecko.activitystream.homepanel.StreamRecyclerAdapter.RowItemType; > + > +public interface RowItem { nit: I'd declare this inside StreamRecyclerAdapter so that it's clear where this is intended to be used - `RowItem` is very unspecific. Alternatively, you could rename it `StreamRowItem` or similar.

Comment on attachment 8898075 [details] Bug 1380808 - Rename some "Item" classes to be more descriptive. https://reviewboard.mozilla.org/r/169382/#review176028 Thank goodness someone is fixing these names. :) Notes: * When refactoring, if you select a generic word to refactor (in this case, "Item"), it's good to check out the non-code refactorings IJ tries to do because it will rename unrelated strings (as happened here). You can highlight them in the list and hit backspace to avoid making those specific refacorings. Sometimes I find IJ even trying to refactor platform Android code - yikes! * For future reference, it's easier to review renames when each rename is in a separate patch ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:54 (Diff revision 1) > private HomePager.OnUrlOpenInBackgroundListener onUrlOpenInBackgroundListener; > > private int tiles; > private int tilesSize; > > - // Item types created with (viewType, stable id) > + // UrlItem types created with (viewType, stable id) -> `RowItem` ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/UrlItem.java:13 (Diff revision 1) > import android.support.annotation.Nullable; > > /** > - * Shared interface for activity stream item models. > + * Shared interface for activity stream items that model a url/link item. > */ > -public interface Item { > +public interface UrlItem { nit: Maybe `PageStreamItem`? I like having Stream because it tells you which part of the code base this is used in and "Url" initially confused me a bit because I wasn't sure what "Url" represented – is it *just* a url? ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/WelcomePanel.java:50 (Diff revision 1) > @Override > public void onClick(View v) { > // To animate between item changes, RecyclerView keeps around the old version of the view, > // and creates a new equivalent item (which is bound using the new data) - followed by > // animating between those two versions. Hence we just need to make sure that > - // any future calls to onCreateViewHolder create a version of the Header Item > + // any future calls to onCreateViewHolder create a version of the Header UrlItem nit: -> `StreamViewHolder`? ::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:220 (Diff revision 1) > DownloadContent content = existingContent.buildUpon() > .updateFromKinto(object) > .build(); > > if (existingContent.getLastModified() >= content.getLastModified()) { > - Log.d(LOGTAG, "Item has not changed: " + content); > + Log.d(LOGTAG, "UrlItem has not changed: " + content); nit: -> `Item` ::: mobile/android/base/java/org/mozilla/gecko/home/HomeContextMenuInfo.java:33 (Diff revision 1) > public int bookmarkId = -1; > public RemoveItemType itemType = null; > > /* package-private */ @Nullable Boolean isAsPinned; > > - // Item type to be handled with "Remove" selection. > + // UrlItem type to be handled with "Remove" selection. nit: -> `RemoveItemType` ::: mobile/android/base/java/org/mozilla/gecko/reader/SavedReaderViewHelper.java:144 (Diff revision 1) > assertItemsLoaded(); > > try { > mItems.put(pageURL, makeItem(path, size)); > } catch (JSONException e) { > - Log.w(LOG_TAG, "Item insertion failed:", e); > + Log.w(LOG_TAG, "UrlItem insertion failed:", e); nit: -> `Item` ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/TabStripComponent.java:139 (Diff revision 1) > public boolean isSatisfied() { > return childView[0] != null; > } > }); > > - fAssertNotNull("Item at index " + index + " exists", childView[0]); > + fAssertNotNull("UrlItem at index " + index + " exists", childView[0]); nit: -> `Item`

Comment on attachment 8900001 [details] Bug 1380808 - HighlightItem -> HighlightItemRow. https://reviewboard.mozilla.org/r/171318/#review176542

Comment on attachment 8900002 [details] Bug 1380808 - HighlightsTitle -> HighlightsTitleRow. https://reviewboard.mozilla.org/r/171320/#review176544

Comment on attachment 8900003 [details] Bug 1380808 - TopPanel -> TopPanelRow. https://reviewboard.mozilla.org/r/171322/#review176546

Comment on attachment 8900004 [details] Bug 1380808 - WelcomePanel -> WelcomePanelRow. https://reviewboard.mozilla.org/r/171324/#review176548 Cool renames. :)

Maria, the Pocket API gives us two links for each article: - The link to the actual article - A redirect link hosted by Pocket I assume that the redirect link is so that Pocket can see how their trending stories perform. However, there are some downsides to using the Pocket link vs the actual link: - There is a redirect - Since we visit the redirect link, it gets added to our history (and I bet users don't care about it) - If you copy the link, you get the Pocket link (not the actual link) [we can change this in the future, to have a different "copy link" and a "open link" in these cases] Can you follow up with Pocket to figure out what benefits they get from us using the redirect link? And then decide which link we should use in AS Pocket :) (If they don't care, I'll just use the actual link, but atm I'm using the redirect link.) iOS should do the same thing that you decide, but it's trivial to change.

From Farhan on what iOS does: "When displaying the url show the actual url But when clicking the url redirect through pocket servers." "The share menu uses the actual url. If the user does an action that opens the URL right away and they wont see the redirect I show them http://get.pocket.com, otherwise show them the actual url"

That link is less about tracking and more about making sure the publisher sees a referer for the link. (Otherwise publishers have no idea we're sending them traffic). On Desktop we're using the original link (not the pocket link) but adding in the referrer ourselves. This allows us to still solve the referer problem but remove the extra network jump and the other issues noted above. The ticket and work (and presumably code on desktop if it's helpful to see) is tracked here: https://github.com/mozilla/activity-stream/issues/2940 Essentially we just need to add a "Referer" header to the URL request we load the browser with. The Referer to use is "https://getpocket.com/recommendations" In our own iOS app, we just load the web view with something like the following: NSString *referrer = @"https://getpocket.com/recommendations"; NSMutableURLRequest *mutableRequest = [NSMutableURLRequest requestWithURL:urlToLoad]; [mutableRequest setValue:referrer forHTTPHeaderField: @"Referer"]; [webView loadRequest:request];

Comment on attachment 8900840 [details] Bug 1380808 - Add Pocket to new tab with placeholders. https://reviewboard.mozilla.org/r/172288/#review177696 ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsDividerItemDecoration.java:40 (Diff revision 2) > divider = a.getDrawable(0); > a.recycle(); > } > > @Override > + @SuppressWarnings("rawtypes") nit: I don't see any warnings on this method locally - why is this necessary? ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:51 (Diff revision 2) > > private static final String LOGTAG = StringUtils.safeSubstring("Gecko" + StreamRecyclerAdapter.class.getSimpleName(), 0, 23); > > private Cursor topSitesCursor; > private List<RowModel> recyclerViewModel; // List of item types backing this RecyclerView. > + private List<TopStory> topStories; In your previous change, you moved all the highlights into recyclerViewModel to simplify but now we've added topStories, which duplicates some contents of `recyclerViewModel` – do you think it's worth the duplication? ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:127 (Diff revision 2) > final LayoutInflater inflater = LayoutInflater.from(parent.getContext()); > > if (type == RowItemType.TOP_PANEL.getViewType()) { > return new TopPanelRow(inflater.inflate(TopPanelRow.LAYOUT_ID, parent, false), onUrlOpenListener, onUrlOpenInBackgroundListener); > + } else if (type == RowItemType.TOP_STORIES_TITLE.getViewType()) { > + return new HighlightsTitleRow(inflater.inflate(HighlightsTitleRow.LAYOUT_ID, parent, false)); nit: if we're reusing this, we should rename it ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:129 (Diff revision 2) > if (type == RowItemType.TOP_PANEL.getViewType()) { > return new TopPanelRow(inflater.inflate(TopPanelRow.LAYOUT_ID, parent, false), onUrlOpenListener, onUrlOpenInBackgroundListener); > + } else if (type == RowItemType.TOP_STORIES_TITLE.getViewType()) { > + return new HighlightsTitleRow(inflater.inflate(HighlightsTitleRow.LAYOUT_ID, parent, false)); > + } else if (type == RowItemType.TOP_STORIES_ITEM.getViewType()) { > + return new HighlightItemRow(inflater.inflate(HighlightItemRow.LAYOUT_ID, parent, false), this); nit: same ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:141 (Diff revision 2) > } else { > throw new IllegalStateException("Missing inflation for ViewType " + type); > } > } > > - private int getHighlightsOffsetFromRVPosition(int position) { > + private int getHighlightsPositionFromRVPosition(int position) { nit: `getHighlightsPositionFromRecyclerViewPosition` Took me a second to figure out what RV was and Java convention is to be as verbose as possible anyway. :P ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:142 (Diff revision 2) > throw new IllegalStateException("Missing inflation for ViewType " + type); > } > } > > - private int getHighlightsOffsetFromRVPosition(int position) { > - return position - HIGHLIGHTS_OFFSET; > + private int getHighlightsPositionFromRVPosition(int position) { > + return position - indexOfType(RowItemType.HIGHLIGHTS_TITLE, recyclerViewModel) - 1; Why `- 1`? We click the item at position 3 in the array (the first top story) and get indexOfType (should be 3 since it's the first match) so I'd think `3 - 3 == 0` is the desired index, but -1 would get -1. ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:145 (Diff revision 2) > > - private int getHighlightsOffsetFromRVPosition(int position) { > - return position - HIGHLIGHTS_OFFSET; > + private int getHighlightsPositionFromRVPosition(int position) { > + return position - indexOfType(RowItemType.HIGHLIGHTS_TITLE, recyclerViewModel) - 1; > } > > + private int getTopStoriesPositionFromRVPosition(int position) { nit: same ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:146 (Diff revision 2) > - private int getHighlightsOffsetFromRVPosition(int position) { > - return position - HIGHLIGHTS_OFFSET; > + private int getHighlightsPositionFromRVPosition(int position) { > + return position - indexOfType(RowItemType.HIGHLIGHTS_TITLE, recyclerViewModel) - 1; > } > > + private int getTopStoriesPositionFromRVPosition(int position) { > + return position - indexOfType(RowItemType.TOP_STORIES_TITLE, recyclerViewModel) - 1; Again, why -1? ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:179 (Diff revision 2) > + final int size; > + final int viewType = getItemViewType(position); > + if (viewType == RowItemType.HIGHLIGHT_ITEM.getViewType()) { > + type = ActivityStreamTelemetry.Contract.TYPE_HIGHLIGHTS; > + actionPosition = getHighlightsPositionFromRVPosition(position); > + size = recyclerViewModel.size() - FIXED_ROWS.length - topStories.size(); This seems really fragile, e.g. if someone moves the sort of the items or adds a new item type, this will break. Is there a better way to calculate? e.g., maybe we should do `indexOfType(HighlightItem)` and count the number of consecutive items of type HighlightItem. ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:215 (Diff revision 2) > openContextMenu(highlightItem, position, ActivityStreamTelemetry.Contract.INTERACTION_LONG_CLICK); > return true; > } > > - private boolean onItemClickIsValidHighlightItem(final int position) { > - if (getItemViewType(position) != RowItemType.HIGHLIGHT_ITEM.getViewType()) { > + private boolean onItemClickIsValidRowItem(final int position) { > + final int viewType = getItemViewType(position); nit: indentation ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:246 (Diff revision 2) > public void openContextMenu(final HighlightItemRow highlightItem, final int position, @NonNull final String interactionExtra) { > - final Highlight highlight = (Highlight) recyclerViewModel.get(position); > + final WebpageRowModel model = (WebpageRowModel) recyclerViewModel.get(position); > > ActivityStreamTelemetry.Extras.Builder extras = ActivityStreamTelemetry.Extras.builder() > .set(ActivityStreamTelemetry.Contract.SOURCE_TYPE, ActivityStreamTelemetry.Contract.TYPE_HIGHLIGHTS) > - .set(ActivityStreamTelemetry.Contract.ACTION_POSITION, position - HIGHLIGHTS_OFFSET) > + .set(ActivityStreamTelemetry.Contract.ACTION_POSITION, position - FIXED_ROWS.length - MAX_TOP_STORIES) This should be topStories.size(), not MAX_TOP_STORIES, right? In any case, I think this should calculate the position in a less fragile way, like my previous comment to that effect. ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:272 (Diff revision 2) > public int getItemCount() { > return recyclerViewModel.size(); > } > > public void swapHighlights(List<Highlight> highlights) { > - recyclerViewModel = recyclerViewModel.subList(0, HIGHLIGHTS_OFFSET); > + recyclerViewModel = recyclerViewModel.subList(0, FIXED_ROWS.length + topStories.size()); This calculation also seems really fragile (e.g. if item order is swapped, will this code get updated?). We could get `indexOfType` and swap based on the last consecutive location, maybe? But maybe I'm being too careful (because the existing code had this assumption when it creates a sublist and calls `addAll`). Since we've been talking about it, I guess we're seeing that one difficulty of putting everything in the same data structure is that it becomes more difficult to get various metadata about one type in that data structure. ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:282 (Diff revision 2) > + private void loadTopStories() { > + List<TopStory> newStories = makePlaceholderStories(); > + topStories = newStories; > + > + final int insertionIndex = indexOfType(RowItemType.TOP_STORIES_TITLE, recyclerViewModel) + 1; > + for (int i = 0; i < Math.min(MAX_TOP_STORIES, newStories.size()); i++) { nit: this loop can be replaced with `List.addAll` (and it'd be slightly more efficient too!). ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:288 (Diff revision 2) > + recyclerViewModel.add(insertionIndex + i, newStories.get(i)); > + } > + } > + > + private static int indexOfType(RowItemType type, List<RowModel> rowModelList) { > + for (int i = 0; i < rowModelList.size(); i++) { nit: why `for` over iterator? Add a comment. (to me, a traditional for loop is an exception vs. the iterator and should be commented). ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:299 (Diff revision 2) > + } > + > + private List<TopStory> makePlaceholderStories() { > + final List<TopStory> stories = new LinkedList<>(); > + final String[] TITLES = { "Placeholder 1", "Placeholder 2", "Placeholder 3"}; > + for (String title: TITLES) { nit: `final String title : TITLES` ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:306 (Diff revision 2) > + } > + return stories; > + } > + > + private TopStory makeStory(final String title, final String url) { > + return new TopStory() { Why not make `TopStory` non-abstract and pass the title & url in as constructor parameters? You can call the constructor directly in `makePlaceholderStories` instead of wrapping with `makeStory` too. ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/TopStory.java:11 (Diff revision 2) > +package org.mozilla.gecko.activitystream.homepanel.model; > + > +import org.mozilla.gecko.activitystream.Utils; > +import org.mozilla.gecko.activitystream.homepanel.StreamRecyclerAdapter; > + > +public abstract class TopStory implements WebpageRowModel { nit: class comment ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/WebpageRowModel.java:13 (Diff revision 2) > +import org.mozilla.gecko.activitystream.Utils; > + > +public interface WebpageRowModel extends WebpageModel, RowModel { > + Utils.HighlightSource getSource(); > + long getUniqueId(); > + String getImageUrl(); nit: I think this is redundant to my change (that added `getImageUrl` to Item which `WebpageModel`? ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/HighlightItemRow.java:42 (Diff revision 2) > > private final StreamOverridablePageIconLayout pageIconLayout; > - private final TextView pageTitleView; > - private final TextView pageSourceView; > private final TextView pageDomainView; > + private final TextView pageTitleView; nit: unnecessary churn, here and below ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/HighlightsTitleRow.java:20 (Diff revision 2) > > public HighlightsTitleRow(final View itemView) { > super(itemView); > } > + > + public void setTitle(final int titleResId) { nit: add the `@StringRes` annotation (or whatever) ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/HighlightsTitleRow.java:20 (Diff revision 2) > > public HighlightsTitleRow(final View itemView) { > super(itemView); > } > + > + public void setTitle(final int titleResId) { It's a code smell that `HighlightsTitleRow`'s layout contains a default String (for the highlights title) and we override that default title here – why don't we take the title as part of the constructor? ::: mobile/android/base/locales/en-US/android_strings.dtd:837 (Diff revision 2) > <!ENTITY helper_triple_readerview_open_title "Available offline"> > <!ENTITY helper_triple_readerview_open_message "Bookmark Reader View items to read them offline."> > <!ENTITY helper_triple_readerview_open_button "Add to Bookmarks"> > > <!ENTITY activity_stream_topsites "Top Sites"> > +<!-- LOCALIZATION NOTE (activity_stream_topstories): &brandPocket is the brand of a company that is being used to provide suggestions for articles. --> nit: maybe add, "In the US, this is Pocket." As a localizer, I might think, "Is there another company providing article suggestions here? I'm not sure!" but maybe they'd recognize Pocket and that'd clear it up ::: mobile/android/base/strings.xml.in:27 (Diff revision 2) > <resources> > <string name="moz_app_displayname">@MOZ_APP_DISPLAYNAME@</string> > <string name="content_authority_db_browser">@ANDROID_PACKAGE_NAME@.db.browser</string> > <string name="moz_android_shared_fxaccount_type">@ANDROID_PACKAGE_NAME@_fxaccount</string> > <string name="android_package_name_for_ui">@ANDROID_PACKAGE_NAME@</string> > + <string name="brand_pocket">Pocket</string> &brandPocket (in between the open and close tag)?

Comment on attachment 8900841 [details] Bug 1380808 - Add live Pocket content. https://reviewboard.mozilla.org/r/172290/#review177716 I'm concerned that the Loader code isn't working as intended but otherwise, lgtm. If you want to land this series quickly (so the other bugs are unblocked, so we waste less time with conflicts, etc.), I don't think it'll break anything if you land it directly and fix the nits in a follow-up. Caveat: if the loader isn't working correctly and is working like I'm afraid it might be, we might hit the Pocket API each time top sites is requested, which wouldn't be good with the size of the Nightly population. ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java:30 (Diff revision 2) > import org.mozilla.gecko.db.BrowserDB; > import org.mozilla.gecko.fxa.FirefoxAccounts; > import org.mozilla.gecko.home.HomePager; > import org.mozilla.gecko.activitystream.homepanel.model.Highlight; > import org.mozilla.gecko.activitystream.homepanel.topsites.TopSitesPagerAdapter; > +import org.mozilla.gecko.widget.DoorhangerConfig; lol, you couldn't get enough of those doorhangers, huh? :P Unused. :) ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:285 (Diff revision 2) > + recyclerViewModel.remove(insertionIndex); > + numOldStories--; > + } > + > topStories = newStories; > + for (int i = 0; i < getNumTopStoriesShown(); i++) { nit: It's confusing that we're called a method, `getNumTopStoriesShown` when at this moment shown most likely means, 1) the old stories still shown to the user or 2) no stories, since you remove the stories from the model. Two approaches I'd prefer are: 1. `getTopStoriesToShow` dynamically returns a list from topStories 2. We cache `shownTopStories` as a separate data structure (a subset of `topStories` - maybe rename to `allTopStories`) and use that directly. ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:34 (Diff revision 2) > +import java.util.LinkedList; > +import java.util.List; > import java.util.Locale; > import java.util.concurrent.TimeUnit; > > +import ch.boye.httpclientandroidlib.util.TextUtils; Use Android TextUtils, not httpclient lib ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:95 (Diff revision 2) > // changing clock times, but this is not a time-sensitive task. > final long previousTime = sharedPreferences.getLong(CACHE_TIMESTAMP_MILLIS_PREFIX + localeLang, 0); > if (System.currentTimeMillis() - previousTime > REFRESH_INTERVAL_MILLIS) { > forceLoad(); > } else { > - deliverResult(sharedPreferences.getString(STORIES_CACHE_PREFIX + localeLang, null)); > + deliverResult(jsonStringToTopStories(sharedPreferences.getString(STORIES_CACHE_PREFIX + localeLang, null))); Does calling `forceLoad` and `deliverResult` from `onStartLoading` make a difference in how the loader runs? I can't find anything in the docs but this code is notoriously under-documented. Looking at the source, onStartLoading [1] doesn't do anything and startLoading [2] (which calls onStartLoading) sets some mutable state to say its loading so if we call the right methods to reset the state, this might work. However, deliverResult [3] doesn't seem to change the state of the loader (though I didn't follow the call to the listener). Did you verify it wasn't making the network request each time? I wonder if it was actually doing, "loader starts; return cached; loadInBackground; return from network". [1]: http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/content/Loader.java#298 [2]: http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/content/Loader.java#286 [3]: http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/content/Loader.java#142 ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:149 (Diff revision 2) > connection.disconnect(); > } > } > } > > + private static List<TopStory> jsonStringToTopStories(String jsonResponse) { nit: I find parsing server output is usually a good place for a test. ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:166 (Diff revision 2) > + final String title = item.getString("title"); > + final String url = item.getString("dedupe_url"); > + final String imageUrl = item.getString("image_src"); > + topStories.add(makeStory(title, url, imageUrl)); > + } > + } catch (JSONException e) { nit: if we also catch inside the loop, we could drop invalid items but still recover to potentially show more items ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:182 (Diff revision 2) > + } > + return stories; > + } > + > + private static TopStory makeStory(final String title, final String url, final String imageUrl) { > + return new TopStory() { From previous commit: use constructor ::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java:195 (Diff revision 2) > + return url; > + } > + > + @Override > + public String getImageUrl() { > + if (!TextUtils.isEmpty(imageUrl)) { nit: -> ternary operator

Comment on attachment 8900840 [details] Bug 1380808 - Add Pocket to new tab with placeholders. https://reviewboard.mozilla.org/r/172288/#review177696 > In your previous change, you moved all the highlights into recyclerViewModel to simplify but now we've added topStories, which duplicates some contents of `recyclerViewModel` – do you think it's worth the duplication? This mainly is so that we can have a list of top stories to pull from as people dismiss items - currently there's no dismiss functionality but eventually we'll need to have a buffer of top stories, that is bigger than the stories displayed. I wouldn't think of this as a duplication but rather a source of stories. > Why `- 1`? We click the item at position 3 in the array (the first top story) and get indexOfType (should be 3 since it's the first match) so I'd think `3 - 3 == 0` is the desired index, but -1 would get -1. The indexOfType is to find the index of the *title* object, which will be one less than the items. So say we want to find the relative topstories index of item index 4, and the topstories title is index 2 (that is, the 3rd item). So the expected result would be that this topstories item would be the second top story item, so index 1. The math from this would be (position - title_pos - 1) = (4 - 2 - 1) = 1, which is the index of the top story. But maybe that is confusing - I agree that the extra -1 makes things more complicated, so I'll just change it to indexof the *item* object. (The original thought was that it's unambiguous for the title because there is only 1). > This should be topStories.size(), not MAX_TOP_STORIES, right? > > In any case, I think this should calculate the position in a less fragile way, like my previous comment to that effect. Good call - I'll add a method, but I'll use it in the next patch because that's where I actually handle top stories telemetry. > nit: why `for` over iterator? Add a comment. (to me, a traditional for loop is an exception vs. the iterator and should be commented). This is setting up for having a queue of top stories, to show when people dismiss stories from the new tab page. So in this case, I used a for-loop because that way we can be dynamic about the number of stories that we add. Since that's not clear and this has come up multiple times in this review, I'll rename topStories to be topStoriesQueue.

Comment on attachment 8900841 [details] Bug 1380808 - Add live Pocket content. https://reviewboard.mozilla.org/r/172290/#review177716 > Does calling `forceLoad` and `deliverResult` from `onStartLoading` make a difference in how the loader runs? I can't find anything in the docs but this code is notoriously under-documented. > > Looking at the source, onStartLoading [1] doesn't do anything and startLoading [2] (which calls onStartLoading) sets some mutable state to say its loading so if we call the right methods to reset the state, this might work. However, deliverResult [3] doesn't seem to change the state of the loader (though I didn't follow the call to the listener). > > Did you verify it wasn't making the network request each time? I wonder if it was actually doing, "loader starts; return cached; loadInBackground; return from network". > > [1]: http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/content/Loader.java#298 > [2]: http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/content/Loader.java#286 > [3]: http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/java/android/content/Loader.java#142 The documentation says for startLoading: "If a previous load has been completed and is still valid the result may be passed to the callbacks immediately." This suggests to me that calling deliverResult will not kick off another load. I put in a couple of breakpoints (in onStartLoading, loadInBackground) and verified that if we're within the caching window, we hit deliverResult and skip forceLoad, and loadInBackground is never called. This seems consistent with the docs to me! > nit: I find parsing server output is usually a good place for a test. Good point. I really want to land this and get it into the Nightly and unblock other bugs, so I filed bug 1393700. It's assigned to me and a P1, so I'll write a test and send it back to you for review.

Pushed by cliu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2c3ddfeeb6cb Use a single data model, stop using resource ids as viewTypes. r=mcomella https://hg.mozilla.org/integration/autoland/rev/79ab1ccac4e3 Rename some "Item" classes to be more descriptive. r=mcomella https://hg.mozilla.org/integration/autoland/rev/4c654dc4134c HighlightItem -> HighlightItemRow. r=mcomella https://hg.mozilla.org/integration/autoland/rev/f3b302498dba HighlightsTitle -> HighlightsTitleRow. r=mcomella https://hg.mozilla.org/integration/autoland/rev/8391c1d74298 TopPanel -> TopPanelRow. r=mcomella https://hg.mozilla.org/integration/autoland/rev/362e3f571156 WelcomePanel -> WelcomePanelRow. r=mcomella https://hg.mozilla.org/integration/autoland/rev/84125714ab8a Add Pocket to new tab with placeholders. r=mcomella https://hg.mozilla.org/integration/autoland/rev/4017a1fe2924 Add live Pocket content. r=mcomella

https://hg.mozilla.org/mozilla-central/rev/2c3ddfeeb6cb https://hg.mozilla.org/mozilla-central/rev/79ab1ccac4e3 https://hg.mozilla.org/mozilla-central/rev/4c654dc4134c https://hg.mozilla.org/mozilla-central/rev/f3b302498dba https://hg.mozilla.org/mozilla-central/rev/8391c1d74298 https://hg.mozilla.org/mozilla-central/rev/362e3f571156 https://hg.mozilla.org/mozilla-central/rev/84125714ab8a https://hg.mozilla.org/mozilla-central/rev/4017a1fe2924

Pocket stories have been added and work without causing the device any kind of noticeable slowdown. Marking as verified.