Enable pull-to-refresh on Sync'd tabs page

VERIFIED FIXED in Firefox 31

Status

()

Firefox for Android
Awesomescreen
--
enhancement
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: Paul [pwd], Assigned: jdover)

Tracking

(Depends on: 1 bug, {feature})

Trunk
Firefox 31
All
Android
feature
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap ?

Firefox Tracking Flags

(firefox31 verified, firefox32 verified, relnote-firefox 31+, fennec+)

Details

Attachments

(4 attachments, 9 obsolete attachments)

14.50 KB, patch
jdover
: review+
jdover
: feedback+
Details | Diff | Splinter Review
1.36 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
19.70 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
5.36 KB, patch
jdover
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:22.0) Gecko/20130312 Firefox/22.0
Build ID: 20130312031046

Steps to reproduce:

Send Tab to Device extension (desktop) is broken and the closest users can get is the ability to visit a page, click the sync button on then open sync'd tabs tab on their mobile device. However an overwhelming majority of the time, this hasn't been caught by the mobile browser and updating sync information is an incredibly long task, requiring uses to leave the app completely. Enabling Pull to Refresh as a means of updating these tabs is a huge win for the User Experience in regards to sync'd tabs and being able to present timely usable data.
(Reporter)

Updated

5 years ago
OS: Windows 7 → Android
Hardware: x86 → ARM

Updated

5 years ago
Component: General → Android Sync
Product: Firefox for Android → Mozilla Services
Version: Trunk → unspecified

Updated

5 years ago
Severity: normal → enhancement
Not a WONTFIX, but I don't know what our approach is to refreshable UI. Ian?
Status: UNCONFIRMED → NEW
Ever confirmed: true
I agree that this might be a win for the user experience.  I think that we alreday implement the following similar behaviour: tapping the "Synced Tabs" tab should trigger a tab sync (and correctly handle the asynchronous updates), but I might be wrong.  CCing ibarlow for UI thoughts.
I've certainly caught myself wishing for a manual refresh button somewhere in cases where a synced tab didn't make it through. Pull-to-refresh might be a nice thing to add. Even if we auto-sync when you tap on the synced tabs section, giving a user more obvious/direct control with pull to refresh would be nice too.
Component: Android Sync → Android Sync
Product: Mozilla Services → Android Background Services
Let's do it.
tracking-fennec: --- → ?
Hardware: ARM → All
tracking-fennec: ? → +
Any input needed from you, Ian, before someone knocks this out?
Component: Android Sync → Awesomescreen
Flags: needinfo?(ibarlow)
Keywords: uiwanted
Product: Android Background Services → Firefox for Android
Version: unspecified → Trunk
I'm a bit swamped so I don't want designs to block this. Feel free to start and I'll comment as we go. Basically it should look something like this when you pull down:

[downward arrow] Pull down to refresh…

and then

[upward arrow] Release to refresh…
Flags: needinfo?(ibarlow)
(Reporter)

Comment 7

4 years ago
(In reply to Ian Barlow (:ibarlow) from comment #6)
> I'm a bit swamped so I don't want designs to block this. Feel free to start
> and I'll comment as we go. Basically it should look something like this when
> you pull down:
> 
> [downward arrow] Pull down to refresh…
> 
> and then
> 
> [upward arrow] Release to refresh…

Android is moving toward the non-bouncy pull-to-refresh that's featured in Gmail and Google+ whereby as a user pulls down, the action bar displays "Pull to Refresh" and then you get the dashed line.

A library implementing this method is featured here: https://github.com/chrisbanes/ActionBar-PullToRefresh

We could use an orange dashed line?
I would also take that as an implementation option, yes.
Thanks, folks.

Shovel-ready!
Keywords: uiwanted
jdover: this might be a simpler starting point for your pull-to-refresh work.
Assignee: nobody → jdover
Status: NEW → ASSIGNED
(Assignee)

Comment 11

4 years ago
Created attachment 8401584 [details] [diff] [review]
patch

Added the SwipeRefreshLayout to the synced tabs tray and it fires TabAccessor.getTabs(), however the content provider query doesn't seem to be returning updated data? This may just have to do with how infrequent synced tabs are posted from browsers to FxSync.
Attachment #8401584 - Flags: feedback?(rnewman)
(Assignee)

Updated

4 years ago
Depends on: 990161, 990166
Comment on attachment 8401584 [details] [diff] [review]
patch

Review of attachment 8401584 [details] [diff] [review]:
-----------------------------------------------------------------

Could we get a Part 0 that just does the splitting of RemoteTabs, then a Part 1 that implements the change? It's hard to see what's new.

nalexander and margaret can continue the reviewing from here on in -- I'll be on the road. I'll check in when I can.

::: mobile/android/base/RemoteTabsContainer.java
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Should be

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

@@ +69,5 @@
> +    }
> +
> +    @Override
> +    public void onRefresh() {
> +        TabsAccessor.getTabs(context, this);

You need to trigger a sync here, rather than just re-running the query.

Waiting on https://github.com/mozilla-services/android-sync/pull/424 , but:

Something like:

  if (SyncAccounts.syncAccountsExist(getContext())) {
    // TODO: figure this out. It won't be as neat as the FxA version.
  } else {
    Account account = FirefoxAccounts.getFirefoxAccount(getContext()));
    if (account == null) {
      // They don't use Sync.
      return;
    }
    EnumSet hints = EnumSet.of(FirefoxAccounts.SyncHint.NOW, FirefoxAccounts.SyncHint.IGNORE_LOCAL_RATE_LIMIT);
    String[] engines = new String[] {"tabs"};
    new AndroidFxAccount(getContext(), account).requestSync(hints, engines);
  }
Attachment #8401584 - Flags: feedback?(rnewman)
(Assignee)

Comment 13

4 years ago
Created attachment 8402072 [details] [diff] [review]
01 - Split RemoteTabs into container and list
Attachment #8401584 - Attachment is obsolete: true
Attachment #8402072 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 14

4 years ago
Created attachment 8402076 [details] [diff] [review]
02 - Add swipe to refresh to sync tab list

Flagging nalexander for feedback on the sync code.
Attachment #8402076 - Flags: feedback?(nalexander)
Comment on attachment 8402076 [details] [diff] [review]
02 - Add swipe to refresh to sync tab list

Review of attachment 8402076 [details] [diff] [review]:
-----------------------------------------------------------------

This is really great, and will guide the interface we land for requesting and listening to sync results.  In general, we want Fennec to only interact with Sync via the two classes SyncAccounts and FirefoxAccounts; you'll see that the PR you reference (which has landed) exposes FirefoxAccoutns.requestSync.  What we haven't done is expose the SyncStatus helper yet.  The requestSync call will be easy to update; the status observer stuff requires a little more thought on both of our parts.  I'm going to ni myself on this ticket so I don't lose track.

Sorry for the delayed reviews, busy times.

::: mobile/android/base/RemoteTabsContainer.java
@@ +42,5 @@
> +        setColorScheme(R.color.swipe_refresh_orange, R.color.background_tabs,
> +                       R.color.swipe_refresh_orange, R.color.background_tabs);
> +
> +        setOnRefreshListener(new RefreshListener());
> +        ContentResolver.addStatusChangeListener(ContentResolver.SYNC_OBSERVER_TYPE_ACTIVE,

The status change listener definitely needs to be added in onResume and removed in onPause, because the inner SyncObserver will keep the outer RemoteTabsContainer alive forever.
Attachment #8402076 - Flags: feedback?(nalexander) → feedback+
Flags: needinfo?(nalexander)

Comment 16

4 years ago
Comment on attachment 8402072 [details] [diff] [review]
01 - Split RemoteTabs into container and list

Review of attachment 8402072 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good, I just think we can improve the layout files.

As a for-the-future comment, this might have been easier to review if you did an hg rename from RemoteTabs->RemoteTabsList in one patch, then made changes on top of that in another patch. But I can't judge you too harshly because I basically did exactly what you did in another bug earlier today :)

::: mobile/android/base/RemoteTabsContainer.java
@@ +24,5 @@
> +import java.util.List;
> +
> +public class RemoteTabsContainer extends FrameLayout
> +                                 implements TabsPanel.PanelView {
> +    private static final String LOGTAG = "GeckoRemoteTabs";

Get rid of this, it's unused.

@@ +33,5 @@
> +    public RemoteTabsContainer(Context context, AttributeSet attrs) {
> +        super(context, attrs);
> +        this.context = context;
> +
> +        LayoutInflater.from(context).inflate(R.layout.remote_tabs_list, this);

Instead of inflating this here and making a new layout file, why not just include the list in tabs_panel.xml?

::: mobile/android/base/RemoteTabsList.java
@@ +15,5 @@
> +import java.util.ArrayList;
> +import java.util.HashMap;
> +import java.util.List;
> +
> +public class RemoteTabsList extends ExpandableListView

This could be package access, since it should only be used by RemoteTabsContainer. Also, this is a good opportunity to add some class comments to both this class and RemoteTabsContainer to explain what they do (one sentence would be fine).

@@ +19,5 @@
> +public class RemoteTabsList extends ExpandableListView
> +                            implements ExpandableListView.OnGroupClickListener,
> +                                       ExpandableListView.OnChildClickListener,
> +                                       TabsAccessor.OnQueryTabsCompleteListener {
> +    private static final String LOGTAG = "GeckoRemoteTabs";

This is also unused.

@@ +21,5 @@
> +                                       ExpandableListView.OnChildClickListener,
> +                                       TabsAccessor.OnQueryTabsCompleteListener {
> +    private static final String LOGTAG = "GeckoRemoteTabs";
> +
> +    private Context mContext;

I'm assuming you kept these m prefixes because this logic is copied over from RemoteTabs. However, I would r+ a follow-up patch that removes the prefixes for consistency with RemoteTabsContainer.

::: mobile/android/base/resources/layout/remote_tabs_list.xml
@@ +2,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<merge xmlns:android="http://schemas.android.com/apk/res/android">

Why do you need this merge? It only has one child, so I think we can get rid of it.
Attachment #8402072 - Flags: review?(margaret.leibovic) → feedback+
(Assignee)

Comment 17

4 years ago
(In reply to Nick Alexander :nalexander from comment #15)
> The status change listener definitely needs to be added in onResume and
> removed in onPause, because the inner SyncObserver will keep the outer
> RemoteTabsContainer alive forever.

Also to note that this listener doesn't seem to fire at the right time. When it fires,
account.isSyncing() is still returning true and then it isn't firing again after the sync
is complete. Maybe FxA needs its own status listening interface? It's also possible I'm using
SyncObserver incorrectly, but it seems like what I have should be working, provided that
FxASyncProvider uses the content provider normally.
(In reply to Josh Dover [:jdover] from comment #17)
> (In reply to Nick Alexander :nalexander from comment #15)
> > The status change listener definitely needs to be added in onResume and
> > removed in onPause, because the inner SyncObserver will keep the outer
> > RemoteTabsContainer alive forever.
> 
> Also to note that this listener doesn't seem to fire at the right time. When
> it fires,
> account.isSyncing() is still returning true and then it isn't firing again
> after the sync
> is complete. Maybe FxA needs its own status listening interface? It's also
> possible I'm using
> SyncObserver incorrectly, but it seems like what I have should be working,
> provided that
> FxASyncProvider uses the content provider normally.

Your SyncObserver code isn't right.  (I saw it, and I should have pointed that out in my initial pass.)  We'll end up using some code I wrote recently [1] to smooth this Android wart; the ni on myself was to come back to this and expose the listening interface through the FirefoxAccounts interface.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/sync/FxAccountSyncStatusHelper.java
Flags: needinfo?(nalexander)
Flags: needinfo?(nalexander)
(Assignee)

Comment 19

4 years ago
Created attachment 8405117 [details] [diff] [review]
01 - Split RemoteTabs into container and list
Attachment #8402072 - Attachment is obsolete: true
Attachment #8405117 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 20

4 years ago
Created attachment 8405118 [details] [diff] [review]
02 - Add swipe to refresh to sync tab list

Note: this only supports new sync. In IRC, nalexander and I decided that old sync was not worth the extra effort of setting up the SyncObserver if it is going to be deprecated as early as Fx 32.
Attachment #8402076 - Attachment is obsolete: true
Attachment #8405118 - Flags: review?(nalexander)
(In reply to Josh Dover [:jdover] from comment #20)

> Note: this only supports new sync. In IRC, nalexander and I decided that old
> sync was not worth the extra effort of setting up the SyncObserver if it is
> going to be deprecated as early as Fx 32.

I think that is the right call. Even if we don't migrate until Fx33 or Fx34. New Sync is where we should be expending effort.

Comment 22

4 years ago
Comment on attachment 8405117 [details] [diff] [review]
01 - Split RemoteTabs into container and list

Review of attachment 8405117 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: mobile/android/base/RemoteTabsList.java
@@ +23,5 @@
> +class RemoteTabsList extends ExpandableListView
> +                            implements ExpandableListView.OnGroupClickListener,
> +                                       ExpandableListView.OnChildClickListener,
> +                                       TabsAccessor.OnQueryTabsCompleteListener {
> +    private Context context;

This could be final.

@@ +26,5 @@
> +                                       TabsAccessor.OnQueryTabsCompleteListener {
> +    private Context context;
> +    private TabsPanel tabsPanel;
> +
> +    private static ArrayList <ArrayList <HashMap <String, String>>> mTabsList;

Please write a follow-up patch to drop this prefix for consistency.
Attachment #8405117 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8405118 [details] [diff] [review]
02 - Add swipe to refresh to sync tab list

Review of attachment 8405118 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking great, but I want to do one more pass after reworking the android-sync code a little to make this cleaner.  I'm going to do that now, and will post a pre- patch for you to review.  So r+, but please don't land until we go one more round.

::: mobile/android/base/RemoteTabsContainer.java
@@ +18,3 @@
>  
>  /**
>   * Serves as container to wrap the list of synced tabs and provide swipe-to-refresh support. The

nit: provides, or implements.

@@ +24,2 @@
>                                   implements TabsPanel.PanelView {
> +    private static final String[] STAGES = new String[] { "tabs" };

nit: descriptive names, always.  STAGES_TO_SYNC_ON_REFRESH, or similar.

@@ +30,5 @@
>  
>      public RemoteTabsContainer(Context context, AttributeSet attrs) {
>          super(context, attrs);
>          this.context = context;
> +        syncObserver = new SyncObserver();

nit: (and a matter of opinion): I generally prefer descriptive names, so that searching globally is easier.  That is, RemoteTabsSyncObserver instead of SyncObserver; RemoteTabsRefreshListener instead of RefreshListener.

This is a very personal preference; do what feels right to you, and keep consistent with Fennec style as much as possible.

@@ +49,5 @@
> +
> +
> +    @Override
> +    public boolean canChildScrollUp() {
> +        // We are not supporting swipe-to-refresh for old sync. This disabled the swipe gesture if

nit: disables.
(Assignee)

Comment 24

4 years ago
Created attachment 8406425 [details] [diff] [review]
01 - Split RemoteTabs into container and list

(In reply to :Margaret Leibovic from comment #22)
> This could be final.

Done.

> Please write a follow-up patch to drop this prefix for consistency.

Done.
Attachment #8405117 - Attachment is obsolete: true
Attachment #8406425 - Flags: review+
(Assignee)

Comment 25

4 years ago
Created attachment 8406428 [details] [diff] [review]
02 - Add swipe to refresh to sync tab list

(In reply to Nick Alexander :nalexander from comment #23)
> This is looking great, but I want to do one more pass after reworking the
> android-sync code a little to make this cleaner.  I'm going to do that now,
> and will post a pre- patch for you to review.  So r+, but please don't land
> until we go one more round.

Fixed nits, just waiting on android-sync code to finish this. Can you add the
dependent bug?

This also now depends on the custom GeckoSwipeRefreshLayout in bug 970707 to
address UX concerns.
Attachment #8405118 - Attachment is obsolete: true
Attachment #8405118 - Flags: review?(nalexander)
Attachment #8406428 - Flags: review?(nalexander)
(Assignee)

Updated

4 years ago
Depends on: 970707
Created attachment 8406638 [details] [diff] [review]
Part 00 - Refactor FirefoxAccounts

rnewman: this does a few small things, all in service of keeping all
FxAccount + Sync access via the FirefoxAccounts interface.

1) expose sync status observer stuff via FirefoxAccounts;

2) don't expose AndroidFxAccount in sync status observer;

3) requestSync on background thread.  Sigh.
Attachment #8406638 - Flags: review?(rnewman)
Attachment #8406638 - Flags: feedback?(jdover)
Created attachment 8406640 [details] [diff] [review]
Add swipe to refresh to sync tab list

jdover: Updated to build on refactored FirefoxAccounts.  I see I left
a little logging in; it needs to go.  But it reveals an issue: the
fragment lasts a lot longer than we want, so it's continually
refreshing the tabs, etc, etc, on background syncs.  Can we start and
stop the observer based on visibility events?
Attachment #8406640 - Flags: feedback?(jdover)
Comment on attachment 8406638 [details] [diff] [review]
Part 00 - Refactor FirefoxAccounts

Review of attachment 8406638 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/fxa/FirefoxAccounts.java
@@ +231,5 @@
>      Logger.info(LOG_TAG, "Requesting sync.");
>      logSyncHints(syncHints);
>  
> +    // We get strict mode warnings on some devices, so make the request on a
> +    // background thread.

Can we just disable the strict mode warnings instead?

::: mobile/android/base/fxa/activities/FxAccountConfirmAccountActivity.java
@@ +117,5 @@
> +    }
> +
> +    @Override
> +    public Account getAccount() {
> +      return fxAccount.getAndroidAccount();

Insanity edge case: what happens if you delete the account from Android Settings mid-way through using this?

::: mobile/android/base/fxa/sync/FxAccountSyncStatusHelper.java
@@ +35,5 @@
>    // Used to unregister this as a listener.
>    protected Object handle = null;
>  
>    // Maps delegates to whether their underlying Android account was syncing the
>    // last time we observed a status change.

// Access to this map should be synchronized.
Attachment #8406638 - Flags: review?(rnewman) → review+
(Assignee)

Comment 29

4 years ago
Comment on attachment 8406638 [details] [diff] [review]
Part 00 - Refactor FirefoxAccounts

Review of attachment 8406638 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/fxa/FirefoxAccounts.java
@@ +249,5 @@
> +   * Only a weak reference to <code>syncObserver</code> is held.
> +   *
> +   * @param syncObserver to start notifying.
> +   */
> +  public static void startObservingSyncStatus(SyncStatusDelegate syncObserver) {

What's the reasoning for using the 'SyncStatusDelegate' and 'startObservingSyncStatus' naming convention rather than some more Android, like 'SyncStatusListener' and 'addSyncStatusListener'?
Attachment #8406638 - Flags: review?(rnewman)
Attachment #8406638 - Flags: review+
Attachment #8406638 - Flags: feedback?(jdover)
Attachment #8406638 - Flags: feedback+
(Assignee)

Comment 30

4 years ago
Comment on attachment 8406638 [details] [diff] [review]
Part 00 - Refactor FirefoxAccounts

Somehow rnewman's r+ got removed...
Attachment #8406638 - Flags: review?(rnewman) → review+
(Assignee)

Comment 31

4 years ago
Comment on attachment 8406640 [details] [diff] [review]
Add swipe to refresh to sync tab list

Review of attachment 8406640 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/RemoteTabsContainer.java
@@ +60,5 @@
> +
> +    @Override
> +    public void onAttachedToWindow() {
> +        super.onAttachedToWindow();
> +        FirefoxAccounts.startObservingSyncStatus(syncObserver);

We could move the add/remove to #show and #hide to avoid leaking. #hide is always called on this panel when the tab tray is closed.
Attachment #8406640 - Flags: feedback?(jdover) → feedback+
(In reply to Josh Dover [:jdover] from comment #31)
> Comment on attachment 8406640 [details] [diff] [review]
> Add swipe to refresh to sync tab list
> 
> Review of attachment 8406640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/RemoteTabsContainer.java
> @@ +60,5 @@
> > +
> > +    @Override
> > +    public void onAttachedToWindow() {
> > +        super.onAttachedToWindow();
> > +        FirefoxAccounts.startObservingSyncStatus(syncObserver);
> 
> We could move the add/remove to #show and #hide to avoid leaking. #hide is
> always called on this panel when the tab tray is closed.

wfm.
Flags: needinfo?(nalexander)
(In reply to Josh Dover [:jdover] from comment #29)
> Comment on attachment 8406638 [details] [diff] [review]
> Part 00 - Refactor FirefoxAccounts
> 
> Review of attachment 8406638 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/fxa/FirefoxAccounts.java
> @@ +249,5 @@
> > +   * Only a weak reference to <code>syncObserver</code> is held.
> > +   *
> > +   * @param syncObserver to start notifying.
> > +   */
> > +  public static void startObservingSyncStatus(SyncStatusDelegate syncObserver) {
> 
> What's the reasoning for using the 'SyncStatusDelegate' and
> 'startObservingSyncStatus' naming convention rather than some more Android,
> like 'SyncStatusListener' and 'addSyncStatusListener'?

This is just an android-sync vs. Fennec distinction.  Back in the mists of time, rnewman preferred delegate for callbacks of this sort, and we've been pretty consistent.  Of course, this API is for Fennec's consumption, so I'll take your point and alter the names.
Landed part 0:

https://hg.mozilla.org/integration/fx-team/rev/f2e5bc417f96

Merge viking, please keep this ticket open so the other parts can land.
Keywords: leave-open
Comment on attachment 8406428 [details] [diff] [review]
02 - Add swipe to refresh to sync tab list

Review of attachment 8406428 [details] [diff] [review]:
-----------------------------------------------------------------

This does need to be re-worked, since I renamed following your suggestions before landing Part 0, but:

r=nalexander with those mechanical changes and with (un-)registering the listener in (hide/)show.
Attachment #8406428 - Flags: review?(nalexander) → review+
(Assignee)

Comment 36

4 years ago
Created attachment 8409124 [details] [diff] [review]
02 - Add swipe to refresh to sync tab list

Updated for naming changes.
Attachment #8406428 - Attachment is obsolete: true
Attachment #8409124 - Flags: review+
(Assignee)

Comment 37

4 years ago
Created attachment 8409130 [details] [diff] [review]
03 - Call hide() on TabPanel after tab tray is hidden

hide() isn't being called when the tabs tray is closed, leading to the sync listener still being subscribed to sync events. This patch fixes that.
Attachment #8409130 - Flags: review?(michael.l.comella)
Comment on attachment 8409130 [details] [diff] [review]
03 - Call hide() on TabPanel after tab tray is hidden

lgtm
Attachment #8409130 - Flags: review?(michael.l.comella) → review+
(Assignee)

Updated

4 years ago
Keywords: leave-open → checkin-needed
(Assignee)

Updated

4 years ago
Attachment #8406640 - Attachment is obsolete: true
(Assignee)

Comment 39

4 years ago
Note to sheriff: Part 00 has already been landed.
https://hg.mozilla.org/mozilla-central/rev/f2e5bc417f96
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
(Reporter)

Updated

4 years ago
Depends on: 998700
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #40)
> https://hg.mozilla.org/mozilla-central/rev/f2e5bc417f96

This is just part 0.  Unfortunately, our flags got lost.  I will land the remaining pieces now.
Backed out last two pieces (which had bad commit messages, sorry!) for rc* failures.

https://hg.mozilla.org/integration/fx-team/rev/62bedf537dfd

jdover, this looks a lot like XML inflation didn't get updated.  See log at

https://tbpl.mozilla.org/php/getParsedLog.php?id=38149837&tree=Fx-Team&full=1#error0

Probably wants to hit try before re-landing.
(Assignee)

Comment 45

4 years ago
Created attachment 8409945 [details] [diff] [review]
01 - Split RemoteTabs into container and list

Forgot to make changes to tablet layout.

Only patches 1 and 2 need to be landed now.
Attachment #8406425 - Attachment is obsolete: true
Attachment #8409945 - Flags: review?(nalexander)
(Assignee)

Comment 46

4 years ago
Created attachment 8409946 [details] [diff] [review]
02 - Add swipe to refresh to sync tab list
Attachment #8409124 - Attachment is obsolete: true
Attachment #8409946 - Flags: review+
Comment on attachment 8409945 [details] [diff] [review]
01 - Split RemoteTabs into container and list

Review of attachment 8409945 [details] [diff] [review]:
-----------------------------------------------------------------

Rubber stamp -- I'm sure the changes are reasonable, and try is green.  Ship it!
Attachment #8409945 - Flags: review?(nalexander) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/57c3be479931
https://hg.mozilla.org/mozilla-central/rev/6fa987a53e37
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]

Updated

4 years ago
Flags: in-moztrap?(fennec)
Keywords: feature, verifyme

Comment 51

4 years ago
I found a small bug in the current Nightly (2014-05-01).

If you pull down only a bit and release your finger before the orange line reaches the left and right ends of the screen, the refresh doesn't start and the orange line 'shrinks back' to the middle until it disappears. This works fully as intended.

But, if you instead pull down (also only a bit), then move your finger up a bit (so that the line shrinks a bit but not fully disappears) and then release your finger, the line will never go away.

Summarized: If you release your finger while moving down it works and the line goes away. But if you instead release your finger while moving up again the line will never go away by itself.

(PS: Thanks for implementing this great feature!)

Updated

4 years ago
Depends on: 1005072
I filed a new bug based on this (bug 1005072). The interaction still seems to be a work-in-progress as far as I can tell.
(Reporter)

Comment 53

4 years ago
Should I file a separate bug for the fact that P2R doesn't fetch sent tabs?
(In reply to Paul [pwd] from comment #53)
> Should I file a separate bug for the fact that P2R doesn't fetch sent tabs?

Yes. In it, please provide a descriptive observation with steps to reproduce if possible. First I've heard of this.
(Reporter)

Updated

4 years ago
Depends on: 1014150
Depends on: 1014335
relnote-firefox: --- → 31+

Updated

4 years ago
Depends on: 1021123
Removing the relnote-firefox 31+ flag since this will be disabled in 31 via bug 1021123
relnote-firefox: 31+ → ---

Comment 56

4 years ago
(In reply to Aaron Train [:aaronmt] from comment #55)
> Removing the relnote-firefox 31+ flag since this will be disabled in 31 via
> bug 1021123

Change of plans! Bug 1021123 is now going to make the colors a bit better, better enough that UX is okay with us shipping this.
relnote-firefox: --- → ?

Updated

4 years ago
Status: RESOLVED → VERIFIED
status-firefox32: --- → verified
Keywords: verifyme
Aaron, it seems that is going to ship in 31 too. Can someone check this in 31?

Added the release notes with Margaret proposed wording:
"Added ability to refresh synced tabs on demand"
status-firefox31: --- → affected
relnote-firefox: ? → 31+
Keywords: verifyme

Updated

4 years ago
status-firefox31: affected → verified
Keywords: verifyme
Depends on: 1058106
You need to log in before you can comment on or make changes to this bug.