… including about:home. (Bug 977161)

AS this bug is relevant to 977167, I would like to patch this as well

They're actually unrelated, so let's finish up Bug 977167 first.

I would like to work on this bug. Can you please assign this to me

I would suggest that they actually not be filtered -- that feels a little too "clever -- but that when a user tabs a synced tab that is also open locally, it should switch to that already open tab.

This is the 1/2 patch containing changes related to bug 977167

This is 2/2 patch specific to bug 977164. A new testcase is added to test the filtering while retrieving the tabs from remote devices.

Comment on attachment 8416820 [details] [diff] [review] patch 1 : changes related to bug 977167 No need to attach the same patch to multiple bugs; we have "Depends on" in Bugzilla for that.

Comment on attachment 8416822 [details] [diff] [review] patch 2 : changes specific to 977164 Review of attachment 8416822 [details] [diff] [review]: ----------------------------------------------------------------- I think you might have missed the point of this bug. The original goal here was to not display in the Remote Tabs tray any tabs that were already open on the local device. Rather than filtering out tabs to ignore based on a regex, we would dynamically filter out tabs to ignore based on the set of open URLs. Ian's current opinion is that we should still display them (perhaps with a subtle styling change), but tapping those should switch to the already-open tab, rather than opening a new tab. This patch isn't necessary, because all clients will already filter out any tabs that they don't think are worth sharing.

Morphing description to make a little more sense. Vivek, assuming you're still interested in addressing this bug: take a look at onChildClick in mobile/android/base/tabspanel/RemoteTabsList.java.

yeah sure, I will look into this.

> Ian's current opinion is that we should still display them (perhaps with a > subtle styling change), but tapping those should switch to the already-open > tab, rather than opening a new tab. I'd like to see this subtle styling change happen, following https://bugzilla.mozilla.org/show_bug.cgi?id=1003608#c4. That is, use TwoLinePageView (suitably generalized?) to unify the "switch to tab" behaviour.

(In reply to vivek from comment #10) > yeah sure, I will look into this. vivek, are you working on this bug? If not then I would like to take over.

Zorro, I'm still working on a patch for this bug and hope to complete it.

New patch that has changes related to TwoLinePageRow. @rnewman: Can you give me inputs on writing test for this.

Comment on attachment 8440282 [details] [diff] [review] 977164.patch Nick, could you review this? Most of the patch is related to your styling comment :D

Comment on attachment 8440282 [details] [diff] [review] 977164.patch Review of attachment 8440282 [details] [diff] [review]: ----------------------------------------------------------------- This will need rework to use the OnOpenUrlListener mechanics of the home panels; let me know if that's complicated. (Home panels are created and initialized rather differently than tabs tray panels.) The styling on tablet needs work, see the attached screenshot. I'd like to see this without favicons before getting UX involved. Overall, good exploration. Sorry for the long delay. ::: mobile/android/base/home/TwoLinePageRow.java @@ +231,5 @@ > + public void populateChildViews(final String title, final String url) { > + // Update the title and url apperances. > + mTitle.setTextAppearance(getContext(), R.style.TabRowTextAppearance); > + mUrl.setTextAppearance(getContext(), R.style.TabRowTextAppearance_Url); > + setShowIcons(true); I'd rather not show icons at all, at least at first. Is this the favicon, or the little bookmark star, or what? ::: mobile/android/base/resources/layout-xlarge-v11/remote_tabs_child.xml @@ +8,5 @@ > + android:gravity="center_vertical" > + android:layout_width="fill_parent" > + android:layout_height="@dimen/remote_tab_child_row_height" > + android:orientation="vertical" > + android:paddingLeft="4dp" nit: get rid of padding entirely. This looks pretty bad on tablets. We need to hide the icon entirely for tablets. See: https://www.dropbox.com/s/itxdd77mz5v4okd/Two.Line.Page.Row.in.Tabs.Tray.on.Tablet.png ::: mobile/android/base/resources/layout/remote_tabs_child.xml @@ +8,5 @@ > + android:gravity="center_vertical" > + android:layout_width="fill_parent" > + android:layout_height="@dimen/remote_tab_child_row_height" > + android:orientation="vertical" > + android:paddingLeft="4dp" nit: get rid of padding. Shouldn't need orientation, either. This and the other layout file are identical; get rid of the other one. ::: mobile/android/base/tabspanel/RemoteTabsList.java @@ +74,5 @@ > > Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.NONE, "remote"); > > + // Switch to the already open tab if exists. > + int tabIndex = Tabs.getInstance().isOpen(tab.get("url")); There's a mechanism for doing this based on OnUrlOpenListener. See http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BookmarksPanel.java#92 for an example of how to use it. Then there's a flag to use, see http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BookmarksListView.java#98 @@ +129,5 @@ > CLIENT_RESOURCE, > tabsList, > R.layout.remote_tabs_child, > TAB_KEY, > + null) { Do we need this? Can we not just set the title/url using the ids from two_line_page_row.xml? @@ +137,5 @@ > + if (convertView == null) { > + convertView = new TwoLinePageRow(getContext()); > + } > + > + String title = (String) ((Map<String,Object>)getChild(groupPosition, childPosition)).get("title"); nit: extract the map to a temporary, and final everywhere. (If we need this.)

New patch that uses OnOpenUrlListener. Further, the icon in TwoLinePageRow is hidden.

An Alternate patch that uses two_line_page_row.xml (In reply to Nick Alexander :nalexander from comment #16) > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/ > BookmarksListView.java#98 > > @@ +129,5 @@ > > CLIENT_RESOURCE, > > tabsList, > > R.layout.remote_tabs_child, > > TAB_KEY, > > + null) { > > Do we need this? Can we not just set the title/url using the ids from > two_line_page_row.xml? two_line_page_row.xml is a merge layout and can't be used directly as child layout for SimpleExpandableListAdapter. So I've wrapped it inside a Linear layout. Can you suggest which of these two pathces is correct?

Comment on attachment 8449806 [details] [diff] [review] 977164_V2.patch Review of attachment 8449806 [details] [diff] [review]: ----------------------------------------------------------------- Ah, I see what you mean. remote_tabs_child.xml should be a duplicate of http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/home_item_row.xml. ::: mobile/android/base/tabspanel/RemoteTabsList.java @@ +73,5 @@ > } > > Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.NONE, "remote"); > > + // Switch to the already open tab if exists. // Open the selected tab, switching to an existing tab if possible.

Comment on attachment 8449805 [details] [diff] [review] 977164.patch Review of attachment 8449805 [details] [diff] [review]: ----------------------------------------------------------------- I think this first approach is the correct one, but I have some nits and some questions. Looking better, though! ::: mobile/android/base/home/TwoLinePageRow.java @@ +229,5 @@ > + /** > + * Populates the Title, Url text view with passed in parameters. > + */ > + public void populateChildViews(final String title, final String url) { > + // Update the title and url apperances. nit: ... and url appearance. But I still don't understand why this is needed. ::: mobile/android/base/resources/layout/remote_tabs_child.xml @@ +6,5 @@ > +<org.mozilla.gecko.home.TwoLinePageRow xmlns:android="http://schemas.android.com/apk/res/android" > + android:background="@android:color/transparent" > + android:gravity="center_vertical" > + android:layout_width="fill_parent" > + android:layout_height="@dimen/remote_tab_child_row_height" /> \ No newline at end of file This seems right, although you should be able to drop a few things (like the background), and you should be able to use the page_row_height. Also, I think we're using match_parent instead of fill_parent now. ::: mobile/android/base/tabspanel/RemoteTabsList.java @@ +75,5 @@ > > Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.NONE, "remote"); > > + // Switch to the already open tab if exists. > + ((OnUrlOpenListener) context).onUrlOpen(tab.get("url"), EnumSet.of(OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB)); This is a little dangerous. Do like the home panels do and save a member variable, catching class cast exceptions the same way. (This can be in the constructor, which takes a context.) We're moving Remote Tabs towards a Home Panel; we want to do everything like home panels do. @@ +129,5 @@ > + null) { > + @SuppressWarnings("unchecked") > + @Override > + public View getChildView(int groupPosition, int childPosition, boolean isLastChild, View convertView, ViewGroup parent) { > + if (convertView == null) { Extract a TwoLinePageRow temporary above here. @@ +136,5 @@ > + > + final Map<String,String> dataMap = (Map<String, String>) getChild(groupPosition, childPosition); > + final String title = dataMap.get("title"); > + final String url = dataMap.get("url"); > + ((TwoLinePageRow) convertView).populateChildViews(title, url); There's a setUrl and a setTitle method on your TwoLinePageRow. I don't understand why we can't use the setter methods. Why do we need this new populateChildViews method?

Quick drive-by comment ... existing behaviour for remoteTabsPanel is that if you tap / select three different listItems you wind up with three tabs opened. While this patch won't open a new tab where one is already open, it also doesn't open a new tab where one isn't already open. That is, new tabs are openned by navigating forward in history in the current tab.

(In reply to Mark Capella [:capella] from comment #21) > Quick drive-by comment ... existing behaviour for remoteTabsPanel is that if > you tap / select three different listItems you wind up with three tabs > opened. > > While this patch won't open a new tab where one is already open, it also > doesn't open a new tab where one isn't already open. That is, new tabs are > openned by navigating forward in history in the current tab. Wow, is that true? That is not what I expected from the onUrlOpen call. We should check this does the right thing -- switch to tab or open in new tab as appropriate.

This patch extracts a non-Cursor update method and uses it (when possible) in the adapter that backs the Remote Tabs home panel. This updates the "switch-to-tab" text and button when appropriate. This part is uncontroversial, I think.

This both expands the set of TabEvents we respond to, and should do less work. It starts a fresh favicon load task in response to a few new tab events; this is working well locally. And it filters tab events by URL earlier, which should reduce the amount of events we respond to. margaret: this is more controversial. There could be perf implications from reloading favicons, although I see none locally. mfinkle: you seem to be the source for TabEvents knowledge. Does this look reasonable?

Comment on attachment 8483796 [details] [diff] [review] Part 1: Update TwoLinePageRow in Remote Tabs home panel. r=margaret Review of attachment 8483796 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/RemoteTabsExpandableListAdapter.java @@ +153,5 @@ > final RemoteTab tab = client.tabs.get(childPosition); > > + // The view is a TwoLinePageRow only for some of our child views: it's > + // present for the home panel children and not for the tabs tray > + // children. Therefore, we must handle one case manually. Is there a bug filed for removing the panel in the tabs tray? Perhaps we should file a bug to remove this logic when it becomes unnecessary. ::: mobile/android/base/home/TwoLinePageRow.java @@ +187,2 @@ > > + public void update(String title, String url, long bookmarkId) { This method could be private, since it's only used in here, and I don't really see a consumer passing in a bookmark id but not a cursor.

Comment on attachment 8483797 [details] [diff] [review] Part 2: Update TwoLinePageRow favicons in response to (some) TabEvents. r=margaret Just looking at the approach here. I am a little worried about using LINK_FAVICON and PAGE_SHOW in addition to FAVICON. Won't all three get triggered? If so, I think we are forcing updates too frequently.

Comment on attachment 8483797 [details] [diff] [review] Part 2: Update TwoLinePageRow favicons in response to (some) TabEvents. r=margaret Review of attachment 8483797 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine to me, I just wonder why we need to handle PAGE_SHOW and LINK_FAVICON in addition to the FAVICON event. ::: mobile/android/base/home/TwoLinePageRow.java @@ +113,5 @@ > @Override > public void onTabChanged(final Tab tab, final Tabs.TabEvents msg, final Object data) { > + if (mPageUrl == null || !mPageUrl.equals(tab.getURL())) { > + return; > + } Good call, this should have already been here. I just looked to see who added this logic, and of course I did! http://hg.mozilla.org/mozilla-central/rev/847f396b0dde @@ +126,5 @@ > + case PAGE_SHOW: > + case LINK_FAVICON: > + // FAVICON is clear. PAGE_SHOW and LINK_FAVICON are optimistic. > + // There's precedent: they're handled by BrowserApp. > + updateDisplayedFavicon(); It would be nice if there was a way to know when a new favicon was actually downloaded and stored. Briefly looking through the tree, it seems like we should only need to be listening for the FAVICON event here, since that should happen after PAGE_SHOW and LINK_FAVICON. What's the extra benefit of trying with these two other events? @@ -155,5 @@ > - * Stores the page URL, so that we can use it to replace "Switch to tab" if the open > - * tab changes or is closed. > - */ > - private void updateDisplayedUrl(String url) { > - mPageUrl = url; Why did I even add this method signature instead of just setting mPageUrl below? In fact, that's what my original patch even did! https://bugzilla.mozilla.org/attachment.cgi?id=784582&action=diff I blame wesj and his review comments :)

Haha, for the record, I wrote my review before seeing mfinkle's comment :)

These patches have r+, but I want to make sure that the threading and synchronization on the url/title is rock solid. I just saw a race locally :(

I elected to land just the "switch-to-tab" UI improvements. I'll file a new ticket to make TwoLinePageRow handle Favicon messages better; there are threading and perf implications that need to be considered carefully. https://hg.mozilla.org/integration/fx-team/rev/214e4036c0e3

Verified as fixed in Build: Firefox for Android 35.0a1 (2014-09-21) Device: Motorola Razr (Android 4.1.2) and Samsung S3 (Android 4.3)