User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:41.0) Gecko/20100101 Firefox/41.0 Build ID: 20150513191204 Steps to reproduce: We currently use the ICS/JB/KK pattern, the platform has moved on. Let's switch over to Material Design pattern.

This change seems reasonable to me. For an example on how this looks, see: https://www.google.com/design/spec/patterns/swipe-to-refresh.html#swipe-to-refresh-swipe-to-refresh

Sounds good. Would this work for our Panels Mike?

(In reply to Anthony Lam (:antlam) from comment #2) > Sounds good. Would this work for our Panels Mike? +1 from me. I'm a little surprised this isn't just on by default. Do we need to rev the support library version to get this styling? Do we need to opt-in somewhere in our code?

(In reply to Nick Alexander :nalexander (PTO July 17-27) from comment #3) > +1 from me. I'm a little surprised this isn't just on by default. Do we > need to rev the support library version to get this styling? Do we need to > opt-in somewhere in our code? Apparently we copied the source to GeckoSwipeToRefreshLayout and use that instead [1]. The reasons in the Javadoc are: 45 * GeckoSwipeRefreshLayout is mostly lifted from Android's support library (v4) with these 46 * modifications: 47 * - Removes elastic "rubber banding" effect when overscrolling the child view. 48 * - Changes the height of the progress bar to match the height of HomePager's page indicator. 49 * - Uses a rectangle rather than a circle for the SwipeProgressBar indicator. Rubber banding is no longer present, the height is no longer relevant, and I'm not sure what the rectangle vs. circle means, but it's probably not relevant. [1]: https://mxr.mozilla.org/mozilla-central/search?string=swiperefreshlayout&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Vivek said he'd handle this one on IRC.

Bug 1183588 - Material design swipe refresh pattern changes r?mcomella. * Replaced GeckoSwipeRefreshLayout with SwipeRefreshLayout from support library. * Handled refresh cancel in a UI Thread to avoid swipe refresh circle sticking around.

Sorry, Vivek - I'll get to this tomorrow.

Comment on attachment 8638853 [details] MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback r?nalexander. https://reviewboard.mozilla.org/r/14145/#review13101 ::: mobile/android/base/home/PanelRefreshLayout.java:74 (Diff revision 1) > - setRefreshing(false); > + setRefreshing(false); Why does this solve the problem of the refresh icon sticking around if the refresh is cancelled? You should probably either add comments to each invocation, or extend SwipeRefreshLayout and override setRefreshing so that it always calls itself in a UIThread.

Comment on attachment 8638853 [details] MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback r?nalexander. Bug 1183588 - Material design swipe refresh pattern changes r?mcomella. * Replaced GeckoSwipeRefreshLayout with SwipeRefreshLayout from support library. * Annotated the method with appropriate thread annotations. Support API SwipeRefreshLayout cancels the refresh view by scheduling a scale down animation. At the end of the animation, the view is set to Gone state. So, we must ensure that this view update happens in UiThread. The deprecated GeckoSwipeRefreshLayout got away this because the it cleared the progress bar with postInvalidate() which invalidates the view through event loop.

Hi Nick, As part of this bug I've added @WorkerThread annotation to onSyncStarted() and onSyncFinished(). I arrived at this conclusion through the logic of elimination that these callback are not run in MainThread. After digging through the code I find that requestSync is actually executed in a service [1]. Can you please correct me if I'm wrong with those annotations. 1 http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/5.1.1_r1/android/content/ContentResolver.java#1797

(In reply to Vivek Balakrishnan[:vivek] from comment #11) > Hi Nick, > > As part of this bug I've added @WorkerThread annotation to onSyncStarted() > and onSyncFinished(). I arrived at this conclusion through the logic of > elimination that these callback are not run in MainThread. After digging > through the code I find that requestSync is actually executed in a service > [1]. Can you please correct me if I'm wrong with those annotations. > > 1 > http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/ > android/5.1.1_r1/android/content/ContentResolver.java#1797 The callback is invoked here: http://androidxref.com/4.4.4_r1/xref/frameworks/base/services/java/com/android/server/content/SyncStorageEngine.java#547 Which is called all over the place, including in things like setBackoff(). It's not clear that setBackoff() couldn't be called from the main UI thread. Sadly, there's no documentation guarantee of anything. But we have a point we control: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/sync/FxAccountSyncStatusHelper.java. As a pre: patch, extract the interface from https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/FirefoxAccounts.java#72 to a new file; document that the methods are always called on a particular thread (which could be the UI thread, if you want that); and then make FxAccountSyncStatusHelper always invoke delegate methods in the right place. I should have documented the thread assumptions and enforced the threading model from the very beginning. That's my oversight.

Comment on attachment 8638853 [details] MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback r?nalexander. Clearing review – I think I'm waiting on the changes from comment 11 and comment 12.

Comment on attachment 8638853 [details] MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback r?nalexander. Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback r?nalexander.

Bug 1183588 - Material design swipe refresh pattern changes r?mcomella.

Comment on attachment 8650030 [details] MozReview Request: Bug 1183588 - Material design swipe refresh pattern changes r?mcomella. https://reviewboard.mozilla.org/r/16529/#review14963 Ship It!

Comment on attachment 8638853 [details] MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback r?nalexander. https://reviewboard.mozilla.org/r/14145/#review14965 If it works for you, it works for me: just nits. ::: mobile/android/base/fxa/SyncStatusListener.java:20 (Diff revision 3) > + * This is always called in UiThread. ...called on the UiThread. (Both places.) ::: mobile/android/base/widget/ActivityChooserModel.java:1336 (Diff revision 3) > - private class SyncStatusListener implements FirefoxAccounts.SyncStatusListener { > + private class SyncStatusDelegate implements SyncStatusListener { Consider InnerSyncStatusListener or something -- no sense using delegate and listener to refer to the same thing.

url: https://hg.mozilla.org/integration/fx-team/rev/53da6578fad4f05df702ff737bc11a141eae9e0a changeset: 53da6578fad4f05df702ff737bc11a141eae9e0a user: vivek <vivekb.balakrishnan@gmail.com> date: Wed Aug 19 21:16:10 2015 +0300 description: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback r=nalexander.

url: https://hg.mozilla.org/integration/fx-team/rev/720c7eff9ea0cafb08fe442311bb3d1c353ee977 changeset: 720c7eff9ea0cafb08fe442311bb3d1c353ee977 user: vivek <vivekb.balakrishnan@gmail.com> date: Wed Aug 19 21:31:50 2015 +0300 description: Bug 1183588 - Material design swipe refresh pattern changes r=mcomella.

https://hg.mozilla.org/mozilla-central/rev/53da6578fad4 https://hg.mozilla.org/mozilla-central/rev/720c7eff9ea0

Verified as fixed in build 43.0a1 2015-08-26; Devices: Asus Transformer Pad (Android 4.2.1). Google Nexus 9 (Android 5.1.1); Motorola Razr (Android 4.4.4).