Closed Bug 958889 Opened 11 years ago Closed 11 years ago

Make Fennec "Synced Tabs" tray display entry point to Firefox Account + Sync when user has no existing accounts

Categories

(Firefox for Android Graveyard :: General, defect, P1)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect

Tracking

(relnote-firefox 32+, fennec32+)

Status:
VERIFIED FIXED
Milestone:
Firefox 32
Tracking Flags:
Tracking Status
relnote-firefox --- 32+
fennec 32+ ---

People

(Reporter: nalexander, Assigned: mcomella)

References

Details

(Whiteboard: [qa+][parallel])

Attachments

(15 files, 43 obsolete files)

Part 1: Move tabspanel files to new tabspanel package.
24.62 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Part 2: Miscellaneous code clean up.
14.45 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
(Part 3) Screenshot v2
68.95 KB, image/png
antlam
: feedback+
Details
Part 2.5 - Add branding strings and get upgrade URL method (github)
57 bytes, text/x-github-pull-request
nalexander
: review+
Details | Review
(Part 4) Screenshot v2
67.91 KB, image/png
antlam
: feedback+
Details
Part 2.6: Remove unused PanelView.getLayout code.
2.54 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
Part 2.7: Add getFirefoxAccountState method. (github)
57 bytes, text/x-github-pull-request
nalexander
: review+
Details | Review
(part 3) Screenshot v3 H
73.08 KB, image/png
Details
(part 4) Screenshot H v3 - center/center
68.88 KB, image/png
antlam
: feedback+
Details
Part 6: Refresh remote tabs panel on activity resume.
4.52 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Part 5: Make RemoteTabsContainer.show/hide idempotent.
2.31 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Part 2.8: Add RemoteTabsPanel.
20.83 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Part 3: Add RemoteTabsSetupPanel for phones.
13.59 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Part 4: Add remote tabs verification screen for phones.
11.93 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Part 7: Prevent tablets from showing new remote tabs panel states.
2.84 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
UX has suggested that the Fennec "Synced Tabs" tray could be an entry point to Firefox Account + Sync for users that have no existing accounts. This ticket will track: * initial UX design; * landing any assets required for the UX; * implementing an account check at the appropriate times (or an appropriate accounts changed listener); * launching the sign up/sign in activity as appropriate.
Hmm, I used tray when I really wanted "TabsPanel". No matter. The code to modify is here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/RemoteTabs.java mfinkle, this would be a great ticket for a front-end dev to pick up. Can you assign to an appropriate person?
Flags: needinfo?(mark.finkle)
ibarlow: I saw mocks for what this might look like but I can't find them, and in any case, we need you to integrate this with Fennec's look and feel. What I saw was essentially the content of the status activity that we intend to show when a user has no account; that is, the first screen the user sees after clicking Fennec Settings > Sync. It was just graphics and a "Get started" button. Can you wrangle the UX side of this?
Flags: needinfo?(ibarlow)
I'm not sure this is an Fx 29 thing. It might be part of future upsell. kar?
Flags: needinfo?(krudnitski)
Whiteboard: [qa?]
I believe this is not Fx29 work. That said, we have wanted to add this to the Tabs Panel, and there might already be an existing bug. Ian might have some existing mockups.
Flags: needinfo?(mark.finkle)
This is a wireframe of the snippet and tab tray entry points. Over to Anthony for finalized visual design. http://cl.ly/image/0v3l331A3U1R
Flags: needinfo?(ibarlow)
Although a great entry point, I don't think it's as visible or imperative as the other entry points in creating FxA in time for Fx29. If it can make it, then that's icing sugar. If we are too resource constrained (!), push this to the next release.
Flags: needinfo?(krudnitski)
Assignee: nobody → alam
Status: NEW → ASSIGNED
Whiteboard: [qa?] → [qa?][needs visual design]
Designs for these screens can be found here: https://www.dropbox.com/s/uqwcyiiiqrw0j8u/FXA%20-%20Sync%20snippet.png https://www.dropbox.com/s/mpg9ilftm5qomhs/FXA%20-%20Sync%20Tab%20V.png https://www.dropbox.com/s/psnelkk95u43ye2/FXA%20-%20Sync%20Tab%20H.png Let me know if these links work for you guys. Likewise, other screens to the whole Sync process can be found there too.
Looks good, thanks Anthony!
Assignee: alam → nobody
Status: ASSIGNED → NEW
Whiteboard: [qa?][needs visual design] → [qa?]
tracking-fennec: --- → 30+
Component: Android Sync → General
Product: Android Background Services → Firefox for Android
Whiteboard: [qa?] → [qa+][parallel]
Priority: -- → P1
Assigning to mfinkle to get an assignee for this, or argue that it should be pushed back to 31.
Assignee: nobody → mark.finkle
Status: NEW → ASSIGNED
mfinkle is not going to be around much between mwc and then pto.
Yup, I know. If he doesn't respond in a suitable timeframe, I'll wear my mfinkle hat: http://www.freeoboi.ru/images/842700363.jpg
Pushing to 31. This is not imperative.
tracking-fennec: 30+ → 31+
Assignee: mark.finkle → michael.l.comella
tracking-fennec: 31+ → 32+
Anthony, the links in comment 8 don't appear to work anymore - do you mind reuploading? (unless someone else has a copy) Additionally, can you link to/upload any required assets?
Flags: needinfo?(alam)
Flags: needinfo?(alam)
Giving these pre-bug cleanup reviews to Brian because he doesn't get enough. :)
Attachment #8410571 - Flags: review?(bnicholson)
Oh, yeah, motivations! * I moved the files to the tabspanel package so I can add some more files for the different configurations specified in comment 15 without polluting base. This gives the benefit of being able to hide classes which do not need to be public to base * I renamed RemoteTabs -> RemoteTabsList so I can have a series of these files (e.g. RemoteTabsFactory, RemoteTabsUnconfigured, or similar). * I would have done my whitespace changes in a separate patch, but hg was having a fit.
Tablet: forever the ugly duckling in my memory. :(
Attachment #8410629 - Flags: review?(bnicholson)
Attachment #8410571 - Attachment is obsolete: true
Attachment #8410571 - Flags: review?(bnicholson)
Sorry, missed a spot.
Attachment #8410649 - Flags: review?(bnicholson)
Attachment #8410629 - Attachment is obsolete: true
Attachment #8410629 - Flags: review?(bnicholson)
Attachment #8410649 - Flags: review?(bnicholson) → review+
Comment on attachment 8410572 [details] [diff] [review] Part 2: Remove member variable prefixes. Review of attachment 8410572 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tabspanel/TabsTray.java @@ +195,5 @@ > > private void refreshTabsData() { > // Store a different copy of the tabs, so that we don't have to worry about > // accidentally updating it on the wrong thread. > + this.tabs = new ArrayList<Tab>(); "this" is unnecessary here. @@ +200,5 @@ > > + Iterable<Tab> tabsList = Tabs.getInstance().getTabsInOrder(); > + for (Tab tab : tabsList) { > + if (tab.isPrivate() == isPrivate) > + this.tabs.add(tab); And here. @@ +435,5 @@ > > + private float swipeStartX; > + private float swipeStartY; > + private boolean swiping; > + private boolean isEnabled; Nit: s/isEnabled/enabled/ for consistency with swiping.
Attachment #8410572 - Flags: review?(bnicholson) → review+
Comment on attachment 8410572 [details] [diff] [review] Part 2: Remove member variable prefixes. Review of attachment 8410572 [details] [diff] [review]: ----------------------------------------------------------------- Today's convention is that existing code should keep its style.
Attachment #8410572 - Flags: review+ → review-
Comment on attachment 8410573 [details] [diff] [review] Part 3: Miscellaneous code clean up. Review of attachment 8410573 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tabspanel/RemoteTabsList.java @@ +24,5 @@ > implements TabsPanel.PanelView, > ExpandableListView.OnGroupClickListener, > ExpandableListView.OnChildClickListener, > TabsAccessor.OnQueryTabsCompleteListener { > + private static final String LOGTAG = RemoteTabsList.class.getSimpleName(); It might be a good idea to get these log tags programmatically, but we don't want to lose the "Gecko" prefix. All of our LOGTAGs (should) start with "Gecko" to make it easy to grep for Fennec-specific messages. ::: mobile/android/base/tabspanel/TabsPanel.java @@ +32,5 @@ > > public class TabsPanel extends LinearLayout > implements LightweightTheme.OnChangeListener, > IconTabWidget.OnTabChangedListener { > + private static final String LOGTAG = TabsPanel.class.getSimpleName(); Same here. ::: mobile/android/base/tabspanel/TabsTray.java @@ +38,5 @@ > import android.widget.TextView; > > class TabsTray extends TwoWayView > implements TabsPanel.PanelView { > + private static final String LOGTAG = TabsTray.class.getSimpleName(); And here. @@ +554,5 @@ > animateClose(swipeView, dismissTranslation); > else > animateCancel(swipeView); > > + velocityTracker.recycle(); Good catch -- this pool-based system seems pretty fragile... You'll also want this above with the `if (!mSwiping)` condition, right?
Attachment #8410573 - Flags: review?(bnicholson) → review-
Comment on attachment 8410572 [details] [diff] [review] Part 2: Remove member variable prefixes. comment 26 references https://mail.mozilla.org/pipermail/mobile-firefox-dev/2014-April/000622.html - i.e. no changing of conventions in pre-existing files!
Attachment #8410572 - Attachment is obsolete: true
Fixed the mentioned issues and rebased.
Attachment #8413063 - Flags: review?(bnicholson)
Comment on attachment 8413062 [details] [diff] [review] Part 1: Move tabspanel files to new tabspanel package. Move r+. Pretty sure I did the rebase correctly, but feel free to re-review if you want.
Attachment #8413062 - Flags: review+
Attachment #8413063 - Flags: review?(bnicholson) → review+
This patch adds the RemoteTabsSetup screen in the vertical orientation for phones. Note that it is currently unused by the remainder of the code. The intent is to add one more View for the verification screen and swap them (and the remote tabs list) out depending on the current account state. I left a few TODOs in where I was unsure of how to handle edge cases.
Attachment #8414130 - Flags: review?(nalexander)
re part 3's patch (comment 32): RemoteTabsSetup.initialize exists to be called from TabsPanel's constructor. When RemoteTabsSetup is constructed, `findViewById` returns null, so I called `initialize` in TabPanel's constructor instead.
Anthony, how does this look to you? ^ comment 13
Flags: needinfo?(alam)
Hey guys, preview looks good but the font size on the copy for "Sign in to sync your tabs,..." and "Using an older version of Sync?" seems a bit larger than on the design. If we could bring the font size down one step it should be good. The title text "Welcome to Sync" and "Get started" is designed to be larger so it shouldn't be the same font size as the body copy.
Flags: needinfo?(alam)
Landing parts 1 and 2 to prevent myself from having to do anymore awful rebasing: remote: https://hg.mozilla.org/integration/fx-team/rev/e68c18a91f3d remote: https://hg.mozilla.org/integration/fx-team/rev/c29c34aea5b0
Keywords: leave-open
Attached image (Part 3) Screenshot v2 —
Anthony, I updated the build with your suggestions from comment 36 - what do you think?
Attachment #8414131 - Attachment is obsolete: true
Attachment #8414762 - Flags: feedback?(alam)
Attached image (Part 5) Screenshot (obsolete) —
Anthony, additionally, what do you think of this? I noticed the "Confirm your account" font looks slightly different, but I'm not quite sure what's going on there.
(In reply to Michael Comella (:mcomella) from comment #39) > Created attachment 8414762 [details] > (Part 3) Screenshot v2 > > Anthony, I updated the build with your suggestions from comment 36 - what do > you think? Looks good! (In reply to Michael Comella (:mcomella) from comment #42) > Created attachment 8414868 [details] > (Part 5) Screenshot > > Anthony, additionally, what do you think of this? > > I noticed the "Confirm your account" font looks slightly different, but I'm > not quite sure what's going on there. The font size on the "Confirm your account" needs to be the same font size as the "Welcome to Sync". I think that's why it's looking slightly different. :)
Attached image (Part 4) Screenshot v2 —
Anthony, what do you think? I corrected the font size of "Confirm your account".
Attachment #8414868 - Attachment is obsolete: true
Attachment #8415497 - Flags: feedback?(alam)
Updated text sizes in accordance with comment 43.
Attachment #8415499 - Flags: review?(nalexander)
Attachment #8414130 - Attachment is obsolete: true
Attachment #8414130 - Flags: review?(nalexander)
(In reply to Michael Comella (:mcomella) from comment #44) > Created attachment 8415497 [details] > (Part 5) Screenshot v2 > > Anthony, what do you think? I corrected the font size of "Confirm your > account". Looks good. Thanks Michael.
Attachment #8415497 - Flags: feedback?(alam) → feedback+
I'm getting caught up on how exactly I should integrate these panels (e.g. how TabsPanel should show them) so I'm just going to request feedback, move on to that section, and come back. I left in the TODOs where applicable.
I overwrite some previous work I did but it's probably not worth the trouble to rebase.
Attachment #8416212 - Flags: review?(nalexander)
Attached image (Part 6) Screenshot (obsolete) —
Anthony, what do you think?
Attachment #8416214 - Flags: feedback?(alam)
Comment on attachment 8415499 [details] [diff] [review] Part 3: Add RemoteTabsSetup vertical for phones. Review of attachment 8415499 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tabspanel/RemoteTabsSetup.java @@ +15,5 @@ > +import android.view.View; > +import android.view.ViewGroup; > +import android.widget.Button; > +import android.widget.LinearLayout; > + class comment! @@ +16,5 @@ > +import android.view.ViewGroup; > +import android.widget.Button; > +import android.widget.LinearLayout; > + > +public class RemoteTabsSetup extends LinearLayout implements PanelView { This really has nothing to do with remote tabs. I'll keep reading, but I might want to unify these names away from remote tabs. @@ +29,5 @@ > + getStartedButton.setOnClickListener(new OnClickListener() { > + @Override > + public void onClick(final View v) { > + // TODO: If the account is locked out, we can't show this. > + final Context context = RemoteTabsSetup.this.getContext(); nit: you can just call getContext(), so no need for the awkward Class.this. So, create account is really sign up. But of course, the user might want to sign in. I always intended an activity like this to go to the "get started" activity; I guess you're showing the get started activity here, so we don't want to show it twice. In any case, your TODO should be handled: it should say something like, the FxAccount*Activity will re-direct to an appropriate activity if the user is locked out, or in the rare cases when we have a race condition. @@ +42,5 @@ > + public void onClick(final View v) { > + String VERSION = AppConstants.MOZ_APP_VERSION; > + String OS = AppConstants.OS_TARGET; > + // TODO: Locale switching? > + String LOCALE = Utils.getLanguageTag(Locale.getDefault()); Again, shouldn't this be okay? The URL is generated at click time, so the locale is up to date. I wonder about extracting this into helper method on FirefoxAccounts (that takes the Locale, say). @@ +47,5 @@ > + final String url = getResources().getString( > + R.string.fxaccount_link_old_firefox, VERSION, OS, LOCALE); > + > + Tabs.getInstance().loadUrlInTab(url); > + tabsPanel.autoHidePanel(); Can you explain what this does? Closes the tabs panel? @@ +74,5 @@ > + } > + > + @Override > + public boolean shouldExpand() { > + return true; In other implementors, this depends on if we are vertical. Perhaps your future patch updates this?
Attachment #8415499 - Flags: review?(nalexander) → feedback+
Comment on attachment 8416212 [details] [diff] [review] Part 6: Create RemoteTabsSetup for horizontal orientation on phones. Review of attachment 8416212 [details] [diff] [review]: ----------------------------------------------------------------- This patch should be folded into the previous. I'm going to redirect to lucasr (who might redirect elsewhere). There's nothing obviously wrong, here, but I really don't like hard-coded padding and margins, etc. I'd prefer a linear layout with center gravity, so that everything gets vertically centered. And a horizontal layout with two vertical linear layouts, each the same weight (so they share the space), and then right and left gravity inside the vertical layouts. We have lots of screen sizes to consider. ::: mobile/android/base/tabspanel/RemoteTabsSetup.java @@ +76,5 @@ > > @Override > public boolean shouldExpand() { > + // TODO: Is this the appropriate way to get the orientation? > + return getResources().getConfiguration().orientation == Configuration.ORIENTATION_PORTRAIT; You're inheriting from LinearLayout, which (in the case of TabsTray) seems to arrange this change in orientation automatically. I think you can extract the orientation from your super class, but I'm not seeing how. I wager lucasr will know.
Attachment #8416212 - Flags: review?(nalexander) → review?(lucasr.at.mozilla)
(In reply to Nick Alexander :nalexander from comment #51) > Comment on attachment 8416212 [details] [diff] [review] > Part 6: Create RemoteTabsSetup for horizontal orientation on phones. > > Review of attachment 8416212 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch should be folded into the previous. Previous being logically previous, namely Part 3.
Comment on attachment 8415499 [details] [diff] [review] Part 3: Add RemoteTabsSetup vertical for phones. Review of attachment 8415499 [details] [diff] [review]: ----------------------------------------------------------------- The sync stuff here looks fine. I'm not sure if the UI stuff is best practice; you might request from lucasr or bnicholson. ::: mobile/android/base/tabspanel/RemoteTabsSetup.java @@ +31,5 @@ > + public void onClick(final View v) { > + // TODO: If the account is locked out, we can't show this. > + final Context context = RemoteTabsSetup.this.getContext(); > + Intent intent = new Intent(context, FxAccountCreateAccountActivity.class); > + context.startActivity(intent); Add the new task flag, like in SyncPreference, etc.
Comment on attachment 8416014 [details] [diff] [review] (WIP) Part 5: Add RemoteTabsConfirm screen for phones. Review of attachment 8416014 [details] [diff] [review]: ----------------------------------------------------------------- This will need work, but only because you're pushing the boundaries (in the best ways). I want to keep the Fennec interface solely through the FirefoxAccounts class. That will mean building out some functions to query the Account state. However, I'm really not convinced that I want to expose this "verification needed" state in Fennec in this way. There's going to be tricky asynchronous interactions with any code that tries to do what this code does. I held a hard line where only FxAccountStatusActivity would display and dispatch on these states; I'd like to absorb what ramifications this patch has for the system before reviewing it. Let me sleep on it, and maybe talk to our UX guys. ::: mobile/android/base/resources/layout/tabs_panel.xml @@ +77,5 @@ > > + <org.mozilla.gecko.tabspanel.RemoteTabsConfirm android:id="@+id/remote_tabs_confirm" > + android:layout_width="match_parent" > + android:layout_height="match_parent" > + android:paddingTop="48dp" dimens for the padding? And is it really needed?
Attachment #8416014 - Flags: feedback?(nalexander) → feedback+
Comment on attachment 8414819 [details] [review] Part 2.5 - Add branding strings and get upgrade URL method (github) This looks fine. UX gets the call here :)
Attachment #8414819 - Flags: review?(nalexander) → review+
(In reply to Michael Comella (:mcomella) from comment #49) > Created attachment 8416214 [details] > (Part 6) Screenshot > > Anthony, what do you think? Hey Michael, The copy that reads "Using an old version of Sync?" and "Sign in to sync your tabs bookmarks and more" should have their bottoms align. You might also want to shrink the gap between the header "Welcome to Sync" and the first line of copy on the left there, the gap is a bit large it seems. Thanks Michael!
Attachment #8414762 - Flags: feedback?(alam) → feedback+
Comment on attachment 8416212 [details] [diff] [review] Part 6: Create RemoteTabsSetup for horizontal orientation on phones. Review of attachment 8416212 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall but, looking at the screenshots for portrait and landscape, it seems we could have a single layout file and only vary the orientation of the RemoteTabsSetup's toplevel container to achieve the same results? The layout would what you have now as layout-land/remote_tabs_setup.xml. Generally speaking, it's better to vary layout via styles instead of having almost duplicate variations of the same layout files because styles are much cheaper/easier to maintain.
Attachment #8416212 - Flags: review?(lucasr.at.mozilla) → feedback+
Attachment #8416214 - Flags: feedback?(alam) → feedback-
Comment on attachment 8414819 [details] [review] Part 2.5 - Add branding strings and get upgrade URL method (github) This now needs to go before part 3 and has some additional components to review. (In reply to Nick Alexander :nalexander from comment #50) > I wonder about extracting this into helper method on FirefoxAccounts (that > takes the Locale, say). I added that here.
Attachment #8414819 - Attachment description: Part 4 - Add "Firefox Accounts" branding strings (github) → Part 2.5 - Add branding strings and get upgrade URL method (github)
Attachment #8414819 - Flags: review+ → review?(nalexander)
Fixed for nalexander's comments - still looking at Lucas', but it shouldn't affect what nalexander has to review (much).
Attachment #8418464 - Flags: review?(nalexander)
Comment on attachment 8416212 [details] [diff] [review] Part 6: Create RemoteTabsSetup for horizontal orientation on phones. Merged into part 3.
Attachment #8416212 - Attachment is obsolete: true
(In reply to Lucas Rocha (:lucasr) from comment #57) > Looks good overall but, looking at the screenshots for portrait and > landscape, it seems we could have a single layout file and only vary the > orientation of the RemoteTabsSetup's toplevel container to achieve the same > results? The layout would what you have now as > layout-land/remote_tabs_setup.xml. Makes sense but how can I change the attributes (i.e. "android:style="@style/...") on orientation change?
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Michael Comella (:mcomella) from comment #61) > (In reply to Lucas Rocha (:lucasr) from comment #57) > > Looks good overall but, looking at the screenshots for portrait and > > landscape, it seems we could have a single layout file and only vary the > > orientation of the RemoteTabsSetup's toplevel container to achieve the same > > results? The layout would what you have now as > > layout-land/remote_tabs_setup.xml. > > Makes sense but how can I change the attributes (i.e. > "android:style="@style/...") on orientation change? You have to re-inflate the view to update styles on the fly (it's a limitation Android has).
Flags: needinfo?(lucasr.at.mozilla)
Blocks: 1007360
(In reply to Anthony Lam from comment #56) > The copy that reads "Using an old version of Sync?" and "Sign in to sync > your tabs bookmarks and more" should have their bottoms align. I'm not quite sure how to do this, particularly while dealing with l10n. Do you mind if I save this for a followup?
Flags: needinfo?(alam)
Updated for lucas' comments. Instead of using style to set orientation, I just set the orientation directly (I couldn't find any way to set the style dynamically).
Attachment #8419000 - Flags: review?(nalexander)
Attachment #8418464 - Attachment is obsolete: true
Attachment #8418464 - Flags: review?(nalexander)
Attachment #8419000 - Flags: review?(lucasr.at.mozilla)
Attached image (part 3) Screenshot v3 H (obsolete) —
Anthony, what do you think? I reduced the separation between the components.
Attachment #8416214 - Attachment is obsolete: true
Attachment #8419004 - Flags: feedback?(alam)
Comment on attachment 8419004 [details] (part 3) Screenshot v3 H It's looking good. I would focus on the alignment of the baseline of "Using an older version of Sync?" and "...Bookmarks, passwords & more."
Attachment #8419004 - Flags: feedback?(alam) → feedback-
Flags: needinfo?(alam)
Comment on attachment 8419000 [details] [diff] [review] Part 3: Add RemoteTabsSetup for phones. Nevermind, portrait needs layout_weight=0 on the inner LinearLayouts while landscape needs layout_weight=1. I can set this dynamically, but at that point I should really be using styles as Lucas mentioned. Lucas, when I reinflate the layout, the style doesn't seem to be updated - do you have any code samples I can see so I can figure out what's going wrong here?
Attachment #8419000 - Flags: review?(lucasr.at.mozilla)
Flags: needinfo?(lucasr.at.mozilla)
Anything else to add on comment 54?
Flags: needinfo?(nalexander)
(In reply to Michael Comella (:mcomella) from comment #67) > Comment on attachment 8419000 [details] [diff] [review] > Part 3: Add RemoteTabsSetup for phones. > > Nevermind, portrait needs layout_weight=0 on the inner LinearLayouts while > landscape needs layout_weight=1. I can set this dynamically, but at that > point I should really be using styles as Lucas mentioned. > > Lucas, when I reinflate the layout, the style doesn't seem to be updated - > do you have any code samples I can see so I can figure out what's going > wrong here? You need to define the same style in different configurations. You patch only defines the styles in values/style.xml. You'd need to define the same style in, say, values-land/styles.xml so that the changes take effect when you rotate the device.
Flags: needinfo?(lucasr.at.mozilla)
Gotcha. I wasn't sure if it was preferable to use @dimen here over stuffing the values in layouts (I opted for the latter to avoid having to manage four files rather than two).
Attachment #8419766 - Flags: review?(lucasr.at.mozilla)
Attachment #8419000 - Attachment is obsolete: true
Attachment #8419000 - Flags: review?(nalexander)
And if you're curious which direction I'm going in terms of showing the panels, see here. To summarize, this patch adds the RemoteTabsPanelFactory class which manages swapping out all View states of the RemoteTabsPanel. TabsPanel has minimal changes from before, and the Factory should handle initialization and all major bouts with FxAccount state (beyond asserting state when buttons are pressed). Note that some changes I've updated from part 4 (formerly part 5) are present here so the patches are not yet continuous. In a part 6, all the new RemoteTabs* classes can end up in a tabspanel.remotetabs package, with the factory being the only public class. I hate to do it this way, but it's probably not worth my time to deal with rebasing the old patches.
Comment on attachment 8419766 [details] [diff] [review] Part 3: Add RemoteTabsSetup for phones. Review of attachment 8419766 [details] [diff] [review]: ----------------------------------------------------------------- Looking overall but needs some cleanups. ::: mobile/android/base/resources/drawable/remote_tabs_setup_button_background.xml @@ +5,5 @@ > + > +<selector xmlns:android="http://schemas.android.com/apk/res/android"> > + <item android:state_pressed="true" > + android:drawable="@drawable/remote_tabs_setup_button_background_pressed" /> > + <item android:drawable="@drawable/remote_tabs_setup_button_background_enabled" /> Unless you're planning to use the drawable for each state in a different context, you should simply define the shapes inline here instead of defining each of them in a separate file. IIRC, you can do this: <item android:state_pressed="true"> <shape>...</shape> </item> ::: mobile/android/base/resources/layout/remote_tabs_setup.xml @@ +4,5 @@ > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<org.mozilla.gecko.tabspanel.PanelViewLinearLayout > + xmlns:android="http://schemas.android.com/apk/res/android" > + xmlns:gecko="http://schemas.android.com/apk/res-auto" You're not using the gecko namespace anywhere in this file. Remove. @@ +10,5 @@ > + style="@style/RemoteTabsSetup" > + android:layout_width="match_parent" > + android:layout_height="match_parent"> > + > + <LinearLayout style="@style/RemoteTabsColumn" Given that this is going to behave more like a 'row' when in portrait orientation, maybe this style should be named RemoteTabsSection or something. ::: mobile/android/base/resources/values/styles.xml @@ +493,5 @@ > + <item name="android:paddingLeft">72dp</item> > + <item name="android:paddingRight">72dp</item> > + </style> > + > + <style name="RemoteTabsItem.TextAppearance"> This base TextAppearance doesn't seem to have enough value. Maybe just define the specific styles for Header and Linkified instead? @@ +496,5 @@ > + > + <style name="RemoteTabsItem.TextAppearance"> > + <item name="android:textColor">#FFC0C9D0</item> > + <item name="android:textSize">16sp</item> > + <item name="android:background">@android:color/transparent</item> Why is setting the background necessary here? @@ +499,5 @@ > + <item name="android:textSize">16sp</item> > + <item name="android:background">@android:color/transparent</item> > + </style> > + > + <style name="RemoteTabsItem.TextAppearance.Header"> I realize we're not very consistent with styles names right now, but it seems we've been mostly naming TextAppearance styles more like TextAppearance.RemoteTabsItem.Header. Not a big deal though. ::: mobile/android/base/tabspanel/PanelViewLinearLayout.java @@ +11,5 @@ > + > +/** > + * A basic implementation of the {@link PanelView} interface on a {@link LinearLayout}. > + */ > +public class PanelViewLinearLayout extends LinearLayout implements PanelView { If this is only used in RemoteTabsSetup, maybe it should be reflect that in the name? No need to have this generic name. @@ +38,5 @@ > + } > + > + @Override > + public boolean shouldExpand() { > + return getResources().getConfiguration().orientation == Configuration.ORIENTATION_PORTRAIT; Maybe you should use the LinearLayout orientation instead? IIUC, you only want to expand if the layout is vertical. So, just do this instead: return (getOrientation() == LinearLayout.VERTICAL);
Attachment #8419766 - Flags: review?(lucasr.at.mozilla) → feedback+
mcomella: I'm going to have to leave this for the weekend. So sorry to make you wait again :(
(In reply to Nick Alexander :nalexander from comment #55) > Comment on attachment 8414819 [details] [review] > Part 2.5 - Add branding strings and get upgrade URL method (github) > > This looks fine. UX gets the call here :) Looks like this r+ got lost. This could land indep. of everything else as a part 0, to shorten your mq.
Flags: needinfo?(nalexander)
Comment on attachment 8414819 [details] [review] Part 2.5 - Add branding strings and get upgrade URL method (github) Ship it!
Attachment #8414819 - Flags: review?(nalexander) → review+
Comment on attachment 8419851 [details] [diff] [review] Part 5: Add RemoteTabsPanelFactory. Review of attachment 8419851 [details] [diff] [review]: ----------------------------------------------------------------- I made some small comments, but I really don't think this PanelView-in-a-PanelView is the right approach. We should talk. ::: mobile/android/base/tabspanel/RemoteTabsPanelFactory.java @@ +1,1 @@ > +package org.mozilla.gecko.tabspanel; nit: license. @@ +18,5 @@ > +import android.view.View.OnClickListener; > +import android.view.ViewGroup; > +import android.widget.Button; > + > +public class RemoteTabsPanelFactory implements PanelView { nit: class comment, always a class comment. And this thing isn't a factory: that is, it's not a re-ified constructor passed into some consumer that needs to make things. It's a living, breathing view in the UI (that happens to have sub-views, and switch between them). Really, it's closer to a view switcher than a factory. @@ +30,5 @@ > + > + public RemoteTabsPanelFactory(final Context context, final View containingView) { > + // TODO: Stubs. > + setupPanel = > + (PanelViewLinearLayout) containingView.findViewById(R.id.remote_tabs_setup); Man, this is hard to read. But you know Fennec style better than me. @@ +41,5 @@ > + } > + > + @Override > + public ViewGroup getLayout() { > + return currentPanel.getLayout(); Funny story: Eclipse thinks this method is not used. Which is good, 'cuz I wanted to understand how dynamic this could be; and my gut tells me this should be constant, and only the inner views should change. Kill it as a pre? @@ +79,5 @@ > + setupGetStartedButton.setOnClickListener(new OnClickListener() { > + @Override > + public void onClick(final View v) { > + final Intent intent; > + if (FxAccountAgeLockoutHelper.isLockedOut(SystemClock.elapsedRealtime())) { You don't need to do this. All the FxAccount* activities check the status and lock the user out during |onResume|.
Attachment #8419851 - Flags: review-
(In reply to Lucas Rocha (:lucasr) from comment #72) > Comment on attachment 8419766 [details] [diff] [review] > Part 3: Add RemoteTabsSetup for phones. > > Review of attachment 8419766 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looking overall but needs some cleanups. > > ::: > mobile/android/base/resources/drawable/remote_tabs_setup_button_background. > xml > @@ +5,5 @@ > > + > > +<selector xmlns:android="http://schemas.android.com/apk/res/android"> > > + <item android:state_pressed="true" > > + android:drawable="@drawable/remote_tabs_setup_button_background_pressed" /> > > + <item android:drawable="@drawable/remote_tabs_setup_button_background_enabled" /> > > Unless you're planning to use the drawable for each state in a different > context, you should simply define the shapes inline here instead of defining > each of them in a separate file. IIRC, you can do this: > > <item android:state_pressed="true"> > <shape>...</shape> > </item> Ooh, thanks for teaching me this: we should do this for the FxA buttons in Background Services.
(In reply to Michael Comella (:mcomella) from comment #68) > Anything else to add on comment 54? Capturing our conversation from earlier today: the two things I want to maintain are that 1) Fennec accesses FxA stuff through the FirefoxAccounts "contract" class; 2) Fennec's interface to FxA stuff is essentially read-only. In particular, AndroidFxAccount is not a good interface and should not leak to Fennec; and the control flow of FxA state between Sync and the FxA*Activity classes is not robust; we should try not to a third point that modifies FxA state.
Comment on attachment 8419766 [details] [diff] [review] Part 3: Add RemoteTabsSetup for phones. Review of attachment 8419766 [details] [diff] [review]: ----------------------------------------------------------------- This approach seems fine, but let's wait at least one more iteration for final review. ::: mobile/android/base/resources/values/colors.xml @@ +97,5 @@ > <color name="swipe_refresh_white">#FFFFFFFF</color> > <color name="swipe_refresh_orange_dark">#FF9500</color> > + > + <!-- Remote tabs setup --> > + <color name="remote_tabs_setup_button_background">#FFE66000</color> Again, maybe we should lift the FxA colors up to Fennec? I don't feel too strongly about the duplication, and it's possible these are different, so this could be ignored or follow-up. ::: mobile/android/base/resources/values/dimens.xml @@ +111,5 @@ > <!-- ArticleItemView dimensions --> > <dimen name="article_item_view_padding">15dp</dimen> > + > + <!-- Remote tabs setup --> > + <dimen name="remote_tabs_setup_corner_radius">5dp</dimen> I see we have a corner radius for FxA already; presumably, these should agree. I'd be happy to lift this dimen into the set of Fennec dimens. http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/fxaccount_dimens.xml#8 ::: mobile/android/base/tabspanel/PanelViewLinearLayout.java @@ +1,1 @@ > +package org.mozilla.gecko.tabspanel; nit: license. This file looks like boilerplate for the later patch; fold it into that one? ::: mobile/android/base/tabspanel/TabsPanel.java @@ +151,5 @@ > + public void onClick(final View v) { > + final Context context = getContext(); > + final Intent intent; > + if (FxAccountAgeLockoutHelper.isLockedOut(SystemClock.elapsedRealtime())) { > + intent = new Intent(context, FxAccountCreateAccountNotAllowedActivity.class); I commented on a later patch, but you don't need this check, *and* you don't need to handle delegating to StatusActivity. Just fire the CreateAccount intent and forget.
Attachment #8419766 - Flags: review?(nalexander) → feedback+
Comment on attachment 8422109 [details] [diff] [review] Part 2.6: Remove unused PanelView.getLayout code. If it compiles (which it did locally!), wfm.
Attachment #8422109 - Flags: review?(nalexander) → review+
(In reply to Anthony Lam from comment #66) > Comment on attachment 8419004 [details] > (part 3) Screenshot v3 H > > It's looking good. I would focus on the alignment of the baseline of "Using > an older version of Sync?" and "...Bookmarks, passwords & more." Where did we land on this alignment issue?
(In reply to Anthony Lam from comment #82) > Where did we land on this alignment issue? I'm currently working on the coding aspects, but I will do the best I can to make it align - I'll post a screenshot when I'm done.
Attached patch Part 2.8: Add RemoteTabsPanel. (obsolete) — — Splinter Review
A second attempt at the remote tabs panel switcher.
Comment on attachment 8422766 [details] [diff] [review] Part 2.8: Add RemoteTabsPanel. Review of attachment 8422766 [details] [diff] [review]: ----------------------------------------------------------------- This approach is what I was expecting. I imagine you'll continue by adding the other view types to |RemotePanelType|, and then you'll be better placed to see how it feels. Roll on! ::: mobile/android/base/resources/layout/remote_tabs_list.xml @@ +1,1 @@ > +<?xml version="1.0" encoding="utf-8"?> nit: the file name doesn't appear to agree with the contents.
Attachment #8422766 - Flags: feedback?(nalexander) → feedback+
Comment on attachment 8422766 [details] [diff] [review] Part 2.8: Add RemoteTabsPanel. Sorry, this is getting ridiculous. x_x
Attachment #8422766 - Attachment description: Part 2.7: Add RemoteTabsPanel. → Part 2.8: Add RemoteTabsPanel.
Comment on attachment 8419851 [details] [diff] [review] Part 5: Add RemoteTabsPanelFactory. Obsolete, in favor of part 2.7.
Attachment #8419851 - Attachment is obsolete: true
Attached patch Part 2.8: Add RemoteTabsPanel. (obsolete) — — Splinter Review
Fairly similar to the last patch, but fixed for tablet.
Attachment #8422821 - Flags: review?(nalexander)
By the way, in part 2.8, I'm not sure what to do with the NeedsPassword Action in RemoteTabsPanel.getPanelTypeFromAccountState.
How's this, Anthony?
Attachment #8419004 - Attachment is obsolete: true
Flags: needinfo?(alam)
I think addressed all of Lucas' and Nick's comments. If not, feel free to point me back. In reply to Nick Alexander :nalexander from comment #79) > Again, maybe we should lift the FxA colors up to Fennec? I don't feel too > strongly about the duplication, and it's possible these are different, so > this could be ignored or follow-up. These are different (orange, rather than blue). (In reply to Lucas Rocha (:lucasr) from comment #72) > ::: mobile/android/base/resources/values/styles.xml > @@ +493,5 @@ > > + <item name="android:paddingLeft">72dp</item> > > + <item name="android:paddingRight">72dp</item> > > + </style> > > + > > + <style name="RemoteTabsItem.TextAppearance"> > > This base TextAppearance doesn't seem to have enough value. Maybe just > define the specific styles for Header and Linkified instead? Why doesn't it have value? Even if it's not used directly, it's inherited by both Header and Linkified. > Why is setting the background necessary here? It's not - deleted. > I realize we're not very consistent with styles names right now, but it > seems we've been mostly naming TextAppearance styles more like > TextAppearance.RemoteTabsItem.Header. Not a big deal though. The dots imply inheritance and since we're working with a TextAppearence particular to the RemoteTabsItem, we don't care much for the base TextAppearance style but want to inherit the RemoteTabsItem style - hence the difference in naming convention.
Attachment #8422864 - Flags: review?(nalexander)
Attachment #8422864 - Flags: review?(lucasr.at.mozilla)
(In reply to Michael Comella (:mcomella) from comment #92) > Created attachment 8422860 [details] > (part 3) Screenshot v3 H > > How's this, Anthony? Looks good Michael. As a final touch can we move both elements away from the top (vertically) by 5 dp? (if this file is at MDPI)
Flags: needinfo?(alam)
(In reply to Anthony Lam (:antlam) from comment #94) > Looks good Michael. As a final touch can we move both elements away from the > top (vertically) by 5 dp? (if this file is at MDPI) xhdpi - do you still want 5dp?
Flags: needinfo?(alam)
(In reply to Michael Comella (:mcomella) from comment #95) > (In reply to Anthony Lam (:antlam) from comment #94) > > Looks good Michael. As a final touch can we move both elements away from the > > top (vertically) by 5 dp? (if this file is at MDPI) > > xhdpi - do you still want 5dp? At XHDPI does Android not double this by itself? The conversion should be x2 from MDPI >> HHDPI if I'm not mistaken so let's go with that. Thanks!
Flags: needinfo?(alam)
Nick for fxa, lucas for UI. I left a TODO that could result in infinite loops - I'll fix it in a later patch. Other TODO: * Tablet * Refresh the View when the Activity is resumed * UI nits (e.g. pixel pushing - probably a followup)
Attachment #8423548 - Flags: review?(nalexander)
Attachment #8423548 - Flags: review?(lucasr.at.mozilla)
Bad stuff happens when you refresh without this one. :P
Attachment #8423549 - Flags: review?(nalexander)
Attachment #8415497 - Attachment description: (Part 5) Screenshot v2 → (Part 4) Screenshot v2
Attached image (part 4) Screenshot H (obsolete) —
I think I'm going to do the nitty UI changes in a followup bug, just so I can get the functionality landed, but does this otherwise look okay, Anthony?
Attachment #8423551 - Flags: feedback?(alam)
Comment on attachment 8422864 [details] [diff] [review] Part 3: Add RemoteTabsSetup for phones. Review of attachment 8422864 [details] [diff] [review]: ----------------------------------------------------------------- The style bits look fine to me with the suggested changes. I think I don't enough context to fully understand the changes to RemoteTabsPanel though. I'm counting on nalexander to do the real review here :-) ::: mobile/android/base/resources/values/colors.xml @@ +97,5 @@ > <color name="swipe_refresh_white">#FFFFFFFF</color> > <color name="swipe_refresh_orange_dark">#FF9500</color> > + > + <!-- Remote tabs setup --> > + <color name="remote_tabs_setup_button_background">#FFE66000</color> If you don't care about the alpha channel, just use #E66000. Same for all other colors defined in this patch. ::: mobile/android/base/resources/values/styles.xml @@ +469,5 @@ > + </style> > + > + <style name="RemoteTabsItemBase"> > + <item name="android:layout_width">match_parent</item> > + <item name="android:layout_height">wrap_content</item> I tend to prefer defining layout_width/layout_height directly in the layout files to keep enough context in-place as you read them. Only define them in the styles if you actually intend to vary them on different configurations (which doesn't seem to be the case here). ::: mobile/android/base/tabspanel/RemoteTabsPanel.java @@ +38,5 @@ > + } > + > + @Override > + public boolean shouldExpand() { > + return getOrientation() == LinearLayout.VERTICAL; nit: enclose expression with parens?
Attachment #8422864 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8423548 [details] [diff] [review] Part 4: Add remote tabs verification screen for phones. Review of attachment 8423548 [details] [diff] [review]: ----------------------------------------------------------------- I don't have enough context to review this patch. I think nalexander is a more appropriate reviewer for this patch. ::: mobile/android/base/resources/values-land/styles.xml @@ +17,5 @@ > <item name="android:nextFocusUp">@+id/info</item> > </style> > > + <!-- Remote tabs panel --> > + <style name="RemoteTabsPanelChild" parent="RemoteTabsPanelChildBase"> Any reason for not using the final style name in the previous patch?
Attachment #8423548 - Flags: review?(lucasr.at.mozilla)
(In reply to Michael Comella (:mcomella) from comment #100) > Created attachment 8423551 [details] > (part 4) Screenshot H > > I think I'm going to do the nitty UI changes in a followup bug, just so I > can get the functionality landed, but does this otherwise look okay, Anthony? I would like to place the "Resend email" link on the right hand side like it is now but move the "A verification link has been sent to..." copy back to the left hand side (like the other horizontal mode design). Spacing wise we could probably remove the "Firefox Accounts" copy to allow more space. This keeps it more consistent with the other horizontal mode. Then its just a matter of vertically aligning the "Resend email" copy. I've cc'd Ryan here to see if he has any views on removing that copy.
Flags: needinfo?(rfeeley)
(In reply to Lucas Rocha (:lucasr) from comment #102) > Any reason for not using the final style name in the previous patch? Wanting to spend less time rebasing and more time coding, and maybe a little bit of lazy. I'll move it into part 3.
Updated for Lucas' comments.
Attachment #8424082 - Flags: review?(nalexander)
Attachment #8422864 - Attachment is obsolete: true
Attachment #8422864 - Flags: review?(nalexander)
Addressing lucas' comments.
Attachment #8424092 - Flags: review?(nalexander)
Attachment #8423548 - Attachment is obsolete: true
Attachment #8423548 - Flags: review?(nalexander)
The listener gets removed and re-added on rotation, which I don't really like, but I'm not sure what else to do.
Attachment #8424252 - Flags: review?(nalexander)
Attached image (part 4) Screenshot H v2 - top (obsolete) —
Attachment #8423551 - Attachment is obsolete: true
Attachment #8423551 - Flags: feedback?(alam)
Attachment #8425105 - Flags: feedback?
Attached image (part 4) Screenshot H v2 - center (obsolete) —
Anthony, what do you think? See also comment 108 for comparison.
Attachment #8425106 - Flags: feedback?(alam)
Also, to get this functionality in faster, how would you feel if this landed without tablet support? We might be able to get it into the same version, if I work fast enough, in a followup bug.
Flags: needinfo?(alam)
(In reply to Michael Comella (:mcomella) from comment #109) > Created attachment 8425106 [details] > (part 4) Screenshot H v2 - center > > Anthony, what do you think? See also comment 108 for comparison. Can we center align the text that reads "Resend email"? Alignment on the vertical and horizontal axis both look good.
Attachment #8425106 - Flags: feedback?(alam) → feedback-
Flags: needinfo?(alam)
(In reply to Michael Comella (:mcomella) from comment #110) > Also, to get this functionality in faster, how would you feel if this landed > without tablet support? We might be able to get it into the same version, if > I work fast enough, in a followup bug. Without tablet support how will this appear? or will it not appear at all? I think in terms of a UI for tablet, we just have to make sure we extend the space accordingly. Since Firefox on Android for tablets currently has a rather small tray and both orientations seem to lend itself better to the vertical tray design we have here, I was originally intending to use the vertical design for tablets. To create additional space, we'd have to move the "current tab" down/to the right. Thoughts?
Flags: needinfo?(rfeeley)
(In reply to Michael Comella (:mcomella) from comment #110) > Also, to get this functionality in faster, how would you feel if this landed > without tablet support? We might be able to get it into the same version, if > I work fast enough, in a followup bug. +1 from me. Land early and often. (In reply to Anthony Lam (:antlam) from comment #112) > Without tablet support how will this appear? or will it not appear at all? Unless mcomella is some kind of wizard, it'll just not appear at all -- no change from current.
I'm confused why the screenshots are showing notification UI in obscure places and not using standard Android notifications.
Comment on attachment 8425677 [details] (part 4) Screenshot H v3 - center/center I think that's good for now. Thanks Michael!
Attachment #8425677 - Flags: feedback?(alam) → feedback+
Attachment #8424092 - Attachment is obsolete: true
Attachment #8424092 - Flags: review?(nalexander)
Attachment #8425800 - Flags: review?(lucasr.at.mozilla)
Note to mcomella: I see in RTC.canChildScrollUp an (expensive) check for FxA.firefoxAccountsExist. That can go away: it should depend only on the (cheaper, pre-computed) state, right?
Note to antlam: I find the "Firefox Accounts/Confirm your account" double header looks odd. I'd expect either "Confirm your Firefox Account" or the Firefox Accounts header to be larger.
(In reply to Nick Alexander :nalexander from comment #119) > Note to antlam: I find the "Firefox Accounts/Confirm your account" double > header looks odd. I'd expect either "Confirm your Firefox Account" or the > Firefox Accounts header to be larger. I would agree - hence the removal of it :)
Attached patch Rollup patch (obsolete) — — Splinter Review
Attachment #8425105 - Attachment is obsolete: true
Attachment #8425105 - Flags: feedback?
Attached patch Part 2.8: Add RemoteTabsPanel. (obsolete) — — Splinter Review
Panel-in-panel, as discussed on IRC.
Attachment #8426690 - Flags: review?(nalexander)
Attachment #8422821 - Attachment is obsolete: true
Attachment #8422821 - Flags: review?(nalexander)
Panel-in-panel setup screen.
Attachment #8427194 - Flags: review?(nalexander)
Attachment #8424082 - Attachment is obsolete: true
Attachment #8424082 - Flags: review?(nalexander)
Attachment #8425800 - Attachment is obsolete: true
Attachment #8425800 - Flags: review?(nalexander)
Attachment #8425800 - Flags: review?(lucasr.at.mozilla)
Changed access level on RemoteTabsSetupPanel to package protected.
Attachment #8427201 - Flags: review?(nalexander)
Attachment #8427194 - Attachment is obsolete: true
Attachment #8427194 - Flags: review?(nalexander)
Attachment #8423549 - Attachment is obsolete: true
Attachment #8423549 - Flags: review?(nalexander)
Attachment #8424252 - Attachment is obsolete: true
Attachment #8424252 - Flags: review?(nalexander)
Attachment #8425835 - Attachment is obsolete: true
Attachment #8425835 - Flags: review?(nalexander)
Comment on attachment 8427204 [details] [diff] [review] Part 6: Refresh remote tabs panel on activity resume. Review of attachment 8427204 [details] [diff] [review]: ----------------------------------------------------------------- While you're here, remove the unused: -import android.view.ViewGroup; This can land now, before anything else. ::: mobile/android/base/tabspanel/TabsPanel.java @@ +60,5 @@ > private TabsListContainer mTabsContainer; > private PanelView mPanel; > private PanelView mPanelNormal; > private PanelView mPanelPrivate; > + private RemoteTabsPanel mPanelRemote; This can stay a PanelView. @@ +121,5 @@ > > mPanelPrivate = (PanelView) findViewById(R.id.private_tabs); > mPanelPrivate.setTabsPanel(this); > > + mPanelRemote = (RemoteTabsPanel) findViewById(R.id.remote_tabs); This doesn't need to change.
Attachment #8427204 - Flags: review?(nalexander) → review+
Comment on attachment 8427202 [details] [diff] [review] Part 5: Make RemoteTabsContainer.show/hide idempotent. Review of attachment 8427202 [details] [diff] [review]: ----------------------------------------------------------------- Per our IRC discussion, fold this into the onResume patch and it's good to go. ::: mobile/android/base/tabspanel/RemoteTabsContainer.java @@ +71,5 @@ > @Override > public void show() { > + if (!isListening) { > + isListening = true; > + TabsAccessor.getTabs(context, list); Don't guard this. We always want the tab list to be refreshed, no matter what, so just guard the addSync... stuff.
Attachment #8427202 - Flags: review?(nalexander) → review+
Comment on attachment 8426690 [details] [diff] [review] Part 2.8: Add RemoteTabsPanel. Review of attachment 8426690 [details] [diff] [review]: ----------------------------------------------------------------- ::: 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/. --> > + > +<org.mozilla.gecko.tabspanel.RemoteTabsContainer The top-level element in this file is not TabsList, it's TabsConatiner, so call it a remote_tabs_container. ::: mobile/android/base/tabspanel/RemoteTabsContainer.java @@ -66,5 @@ > } > > @Override > public void show() { > - setVisibility(VISIBLE); Why are you getting rid of these things? This panel no longer acts like a regular panel -- it'll only do sensible things when the enclosing panel does the right thing, right? In general, I want the panel-of-panel to assume nothing about the child panels, and the child panels to work like regular panels. ::: mobile/android/base/tabspanel/RemoteTabsPanel.java @@ +15,5 @@ > +import android.view.View; > +import android.widget.FrameLayout; > + > +/** > + * The panel which manages and displays the appropriate View in the This comment needs to be updated to reflect the panel-of-panels approach. Being clear about when and how panels are created/inflated is the key point. @@ +22,5 @@ > +class RemoteTabsPanel extends FrameLayout implements PanelView { > + private enum RemotePanelType { > + SETUP, > + VERIFICATION, > + TABS_LIST TABS_LIST is not symmetric, call it CONTAINER. All the resources are remote_tabs_{setup,verification,container}.
Attachment #8426690 - Flags: review?(nalexander) → review+
Comment on attachment 8427201 [details] [diff] [review] Part 3: Add RemoteTabsSetup for phones. Review of attachment 8427201 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tabspanel/RemoteTabsSetupPanel.java @@ +13,5 @@ > +import android.util.AttributeSet; > +import android.view.View; > +import android.widget.LinearLayout; > + > +class RemoteTabsSetupPanel extends LinearLayout implements PanelView { Class comment, please, with emphasis on the fact that this is a sub-panel at present. @@ +57,5 @@ > + tabsPanel = panel; > + } > + > + @Override > + public void show() { /* Do nothing */ } Can we make this do the right thing when it's *not* a sub-panel?
Attachment #8427201 - Flags: review?(nalexander) → review+
Comment on attachment 8427200 [details] [diff] [review] Part 4: Add remote tabs verification screen for phones. Review of attachment 8427200 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tabspanel/RemoteTabsVerificationPanel.java @@ +1,1 @@ > +package org.mozilla.gecko.tabspanel; nit: license. @@ +9,5 @@ > +import android.util.AttributeSet; > +import android.view.View; > +import android.widget.LinearLayout; > +import android.widget.TextView; > + nit: class comment. @@ +43,5 @@ > + final TextView verificationView = > + (TextView) findViewById(R.id.remote_tabs_confirm_verification); > + final String email = FirefoxAccounts.getFirefoxAccountEmail(getContext()); > + if (email == null) { > + // We're in an unexpected state - let's update the View. Log an error, and ignore. There's no sense looping, and there's nothing to be done. You could hide the bad link? Or close the tabsPanel entirely? @@ +60,5 @@ > + // We don't need the reference. > + } > + > + @Override > + public void show() { nit: include Android visibility like other panels.
Attachment #8427200 - Flags: review?(nalexander) → review+
Comment on attachment 8427268 [details] [diff] [review] Part 7: Prevent tablets from showing new remote tabs panel states. Review of attachment 8427268 [details] [diff] [review]: ----------------------------------------------------------------- lgtm.
Attachment #8427268 - Flags: review?(nalexander) → review+
Comment on attachment 8422812 [details] [review] Part 2.7: Add getFirefoxAccountState method. (github) Minor nits on github.
Attachment #8422812 - Flags: review?(nalexander) → review+
Made RemoteTabsContainerPanel GONE by default.
Because of the above changes, we can just return when on tablet, rather than setting the visibility everytime.
Comment on attachment 8427455 [details] [diff] [review] Part 7: Prevent tablets from showing new remote tabs panel states. Oi, logistics.
Attachment #8427455 - Flags: review+
Thought this was broken for my Nexus 7 (bug 1016989) and then I scoured this bug for tablet mention. What's the plan here for supporting tablets?
Status: RESOLVED → VERIFIED
(In reply to Aaron Train [:aaronmt] from comment #151) > What's the plan here for supporting tablets? Followup bug 1014999, hopefully to be in the same release channel as the phone implementation.
Depends on: 1017142
QA Contact: catalin.suciu
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: