I don't expect we'll need any major change in the code itself (at least initially). We might end up needing more fundamental changes in the code *if* the behavior of the toolbar in the new tablet UI changes a bit more drastically.

Passing the ball to mcomella. We have discussed a few ways to approach this.

Comment on attachment 8467801 [details] [diff] [review] Use different layout in toolbar when new tablet UI is enabled (r=mcomella) Here's the hack I have to apply a different toolbar layout when the new tablet UI is enabled. This should give you an idea about some of the spots where we need to adapt the UI. Forward button is kinda broken.

Forgot to mention one important aspect of this patch: I'm prefixing all resources related to the new tablet UI with NewTablet/new_tablet. This will make it a lot easier to see where we're changing the UI for the new tablet UX (i.e. dxr-friendly) and easier to override the old tablet UI with the new resources once we're ready to ship.

It seems you are missing your changes to attrs.xml: /home/mcomella/dev/trees/larch/mobile/android/base/resources/layout-large-v11/new_tablet_browser_toolbar.xml:75: error: No resource identifier found for attribute 'layout' in package 'org.mozilla.fennec_mcomella' I'll fix it locally and carry on.

Ah, I see - patches are in the blocking bugs.

I decided that the proper way to do this would be to have an abstract BrowserToolbar base class, with a subclass for each device type. However, to do this, we need to stub our views so BrowserToolbarStub can be declared in the xml. I figured this wasn't worth the time, so I opted to take the middle ground and call device-specific code via a BrowserToolbarDevice interface from the BrowserToolbar code. Note that the BrowserToolbarDevice interface may not be final - it may change as I do the other device types.

Comment on attachment 8474026 [details] [diff] [review] Part 1: Abstract BrowserToolbar phone layout Review of attachment 8474026 [details] [diff] [review]: ----------------------------------------------------------------- Looks like a pretty good start. I like the fact that you started small here. Some questions for further consideration: - How could we define this interface in such a way that can avoid null checks for elements that don't exist in all UI flavours? For example, back/forward buttons and actionBarButtons are only used on tablets, translating edge is only needed on phones, etc. - We're most likely replacing the dynamic tab counter with a simpler tabs button in the new tablet UI. How can we define this interface in a way that we can vary this? ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +171,5 @@ > private static final Interpolator buttonsInterpolator = new AccelerateInterpolator(); > > private static final int FORWARD_ANIMATION_DURATION = 450; > > + private final BrowserToolbarDevice device; nit: 'Device' doesn't seem to match what you're doing here. BrowserToolbarUI? BrowserToolbarImpl?

From your initial patch (thus not sure if you should review this).

(In reply to Lucas Rocha (:lucasr) from comment #8) > - How could we define this interface in such a way that can avoid null > checks for elements that don't exist in all UI flavours? For example, > back/forward buttons and actionBarButtons are only used on tablets, > translating edge is only needed on phones, etc. I'm making each phone/tablet implementation inherit from a shared base class that takes care of this initialization. However, because we need to do this after Inflation, I moved inflation into the BrowserToolbarUI implementors, which is a little gross because the inflation is hidden, but works better than "postInflationInitialize()" and "getLayoutID()" methods in the interface. > - We're most likely replacing the dynamic tab counter with a simpler tabs > button in the new tablet UI. How can we define this interface in a way that > we can vary this? As in we need to be able to update dynamically on tablet, but not on phone? I think I will just utilize BrowserToolbarUI, but I will do this after I complete the tablet patch so I can better understand the whole picture and avoid doing too much at once. > nit: 'Device' doesn't seem to match what you're doing here. > BrowserToolbarUI? BrowserToolbarImpl? I went with BrowserToolbarUI since all of the split code should be exclusively UI related.

Did some renaming.

Comment on attachment 8474831 [details] [diff] [review] Part 0: Add basic new_tablet BrowserToolbar resources Review of attachment 8474831 [details] [diff] [review]: ----------------------------------------------------------------- Looks good without the shadow view. ::: mobile/android/base/resources/layout-large-v11/new_tablet_browser_toolbar.xml @@ +35,5 @@ > + android:layout_centerVertical="true" > + android:padding="13dp" > + android:src="@drawable/ic_menu_back" > + android:contentDescription="@string/back" > + android:background="@drawable/url_bar_nav_button"/> With the exception of the back/forward buttons, I think the new toolbar layout will be a lot more linear with no overlapping children. It we could change the new tablet toolbar to be a simpler LinearLayout instead. No need to do this in this patch though. Please file a follow-up to investigate. @@ +102,5 @@ > + android:layout_width="match_parent" > + android:layout_height="2dp" > + android:layout_alignParentBottom="true" > + android:background="@color/url_bar_shadow" > + android:contentDescription="@null"/> I've just merged m-c into larch. This view can go away. @@ +112,5 @@ > + style="@style/UrlBar.ImageButton.Icon" > + android:layout_alignParentRight="true" > + android:src="@drawable/close_edit_mode_selector" > + android:paddingLeft="2dp" > + android:paddingRight="2dp" Not specific to this patch but I wonder if this padding is actually necessary. Please file a follow-up to remove it from all *browser_toolbar.xml if it's not.

Comment on attachment 8474875 [details] [diff] [review] Part 1: Abstract BrowserToolbar phone layout Review of attachment 8474875 [details] [diff] [review]: ----------------------------------------------------------------- To be honest, I'm slightly mixed about this approach after seeing how the BrowserToolbarUI implementations interact with BrowserToolbar because the responsibilities are kinda of vague i.e. BrowserToolbar delegates only "part" of the UI implementation to BrowserToolbarUI which is a bit weird. And the view getters look a bit leaky for something that is supposed to just be composed together. Just as an exercise, let's see at how this would look if we took the subclassing path: 1. BrowserToolbar would be abstract and have abstract methods that match your current BrowserToolbarUI (startEditing, stopEditing, getFocusOrder). 2. BrowserToolbar would have protected constructor like BrowserToolbar(Context context, int layoutId). Subclasses would just call super(context, R.layout.THEIR_LAYOUT_ID) in their public *BrowserToolbar(Context context) constructor. This protected constructor will go away once the new tablet UI actually replaces the current one in Nightly i.e. we'll always use R.layout.browser_toolbar. 3. The child views references would become protected members instead of private. 4. The BrowserToolbar subclasses would look almost the same as your current BrowserToolbarUI implementations. You'd have a BrowserToolbarPhoneBase and BrowserToolbarTabletBase inheriting from BrowserToolbar. And then the concrete implementation subclasses. 5. You'd create a simple stub that would look like: // Not tested at all btw :-) public class BrowserToolbarStub extends View { public BrowserToolbatStub(Context context, AttributeSet attrs, int defStyle) { super(context, attrs, defStyle); } @Override protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) { // Always empty. setMeasuredDimension(0, 0); } @Override public void draw(Canvas canvas) { // Draw nothing. } // maybe init() or create() or ... public BrowserToolbar addAndReturn() { final BrowserToolbar toolbar; if (NewTabletUI.isEnabled()) { toolbar = new NewTabletBrowserToolbar(getContext()); } else if (HardwardUtils.isTablet()) { toolbar = new TabletBrowserToolbar(getContext()); } else if (Build.VERSION.SDK_INT < 11) { toolbar = new GBPhoneBrowserToolbar(getContext()); } else { toolbar = new PhoneBrowserToolbar(getContext()); } final ViewGroup parent = (ViewGroup) getParent(); final int index = parent.indexOfChild(this); parent.removeViewInLayout(this); final LayoutParams lp = getLayoutParams(); if (lp != null) { parent.addView(toolbar, index, lp); } else { parent.addView(toolbar, index); } toolbar.setClickable(true); toolbar.setFocusable(true); toolbar.setBackgroundDrawable(getBackground()); return toolbar; } } Or can simply use stock ViewStub and setLayoutResource() dynamically following the same logic. This would involve creating a separate XML file for each toolbar flavour which is kinda annoying but I'd be fine with that too. Then use the stub in gecko_app.xml and call addAndReturn() is BrowserApp's constructor. Subclassing feels slightly saner to me here because it becomes clear that each subclass is a variation of the same type of UI component. Thoughts? ps: In terms of patch structure, I'd first implement the variations between tablet and phones. Then make the new tablet UI changes on top, in a separate patch. Not a big deal though. ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +142,5 @@ > private ThemedImageButton menuButton; > private ThemedImageView menuIcon; > private LinearLayout actionItemBar; > private MenuPopup menuPopup; > + private final List<View> focusOrder; No change? @@ +188,5 @@ > + if (HardwareUtils.isTablet()) { > + final int layoutID = (isNewTablet) ? > + R.layout.new_tablet_browser_toolbar : R.layout.browser_toolbar; > + LayoutInflater.from(context).inflate(layoutID, this); > + toolbarUI = null; If you plan to work on the BrowserToolbarUI implementation for tablets in a follow-up, there's no need to include BrowserToolbarTabletBase in this patch. @@ +649,5 @@ > } > } > } > > + protected void updateTabCountAndAnimate(final int count) { The way java treats scoping for inner protected interfaces is so *not* intuitive :-) @@ +835,5 @@ > public void setOnStopEditingListener(OnStopEditingListener listener) { > stopEditingListener = listener; > } > > + protected void showUrlEditLayout() { Same question about protected vs package access. ::: mobile/android/base/toolbar/BrowserToolbarTabletBase.java @@ +1,1 @@ > +package org.mozilla.gecko.toolbar; No need to include this in this patch as you're not actually implementing tablet UI just yet. ::: mobile/android/base/toolbar/BrowserToolbarUIPhone.java @@ +55,5 @@ > + } > + > + @Override > + public void startEditing(final PropertyAnimator animator) { > + if (toolbar.isAnimatingEntry()) { Is there any value in leaving the notion of isAnimatingEntry() in BrowserToolbar? It seems very phone-specific now.

Actually, a cleaner approach would be to dynamically resolve the class name from the layout file using a LayoutInflater.Factory instead of using ViewStubs. BrowserApp is a FragmentActivity which is already a LayoutInflater.Factory itself. You can just override this method and resolve org.mozilla.gecko.toolbar.BrowserToolbar with something like: View onCreateView(String name, Context context, AttributeSet attrs) { final View view; if (name.equals(BrowserToolbar.class.getName())) { view = BrowserToolbar.create(context, attrs); } else { view = super.onCreateView(name, context, attrs); } return v; }

(In reply to Lucas Rocha (:lucasr) from comment #13) > ::: mobile/android/base/toolbar/BrowserToolbar.java > @@ +142,5 @@ > > private ThemedImageButton menuButton; > > private ThemedImageView menuIcon; > > private LinearLayout actionItemBar; > > private MenuPopup menuPopup; > > + private final List<View> focusOrder; > > No change? What do you mean here? > @@ +649,5 @@ > > + protected void updateTabCountAndAnimate(final int count) { > > The way java treats scoping for inner protected interfaces is so *not* > intuitive :-) > > @@ +835,5 @@ > > + protected void showUrlEditLayout() { > > Same question about protected vs package access. I don't understand what you mean here - what is the issue?

Will update the part numbers.

Comment on attachment 8477099 [details] [diff] [review] Part 1: Implement LayoutInflater.Factory in BrowserApp to inflate device specific BrowserToolbars Review of attachment 8477099 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Double-check if we actually need to explicitly set factory. ::: mobile/android/base/BrowserApp.java @@ +470,5 @@ > } > > @Override > public void onCreate(Bundle savedInstanceState) { > + LayoutInflater.from(getContext()).setFactory(this); // Must be called before super.onCreate. Are you sure we need this? FragmentActivity does that for us. See: https://github.com/android/platform_frameworks_support/blob/master/v4/java/android/support/v4/app/FragmentActivity.java#L201

(In reply to Michael Comella (:mcomella) from comment #15) > (In reply to Lucas Rocha (:lucasr) from comment #13) > > ::: mobile/android/base/toolbar/BrowserToolbar.java > > @@ +142,5 @@ > > > private ThemedImageButton menuButton; > > > private ThemedImageView menuIcon; > > > private LinearLayout actionItemBar; > > > private MenuPopup menuPopup; > > > + private final List<View> focusOrder; > > > > No change? > > What do you mean here? Splinter fail apparently. It displayed this as if you were removing and adding same code. > > @@ +649,5 @@ > > > + protected void updateTabCountAndAnimate(final int count) { > > > > The way java treats scoping for inner protected interfaces is so *not* > > intuitive :-) > > > > @@ +835,5 @@ > > > + protected void showUrlEditLayout() { > > > > Same question about protected vs package access. > > I don't understand what you mean here - what is the issue? Ignore it. I got confused with the scopes of the interfaces and other classes.

Comment 17 is correct - we don't need to set it explicitly. Updated this patch accordingly.

I could have split this up better so that I created the boilerplate files in one patch, then made the phone implementation in another, but it was too much work.

Addressed Lucas' comments on the removal of "isAnimatingEntry" from the base class by adding an abstract isAnimating method.

Addressed comments.

Rebase.

Rebase.

Moved the new phone-specific roundCorner* code into the phone classes.

Sorry for the churn - added licenses to the tops of all of the new files and class comments for tablets.

Rebase.

I changed a bit of functionality in actionItemBar, which is why I left it out of part 3. The things I have so concern over: * Using the view ID to update the focus order on actionItemBar * Changing the behavior of addActionItem on phones, where it now returns false.

Final part.

Try, parts 0-5: https://tbpl.mozilla.org/?tree=Try&rev=6a22c41e8781

Comment on attachment 8478498 [details] [diff] [review] Part 2: Add boilerplate BrowserToolbar subclasses and phone implementation Review of attachment 8478498 [details] [diff] [review]: ----------------------------------------------------------------- Looks like a pretty good start. ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +171,5 @@ > > + public abstract boolean isAnimating(); > + > + protected abstract void transitionForStartEditing(PropertyAnimator animator); > + protected abstract void transitionForStopEditing(); nit: These method names look a bit weird. Maybe triggerStartEditingTransition/triggerStopEditingTransition? @@ +198,5 @@ > // BrowserToolbar is attached to BrowserApp only. > activity = (BrowserApp) context; > > // Inflate the content. > + // TODO: Remove the branch when new tablet becomes old tablet. Please add this to the "Enabling the new UI in Nightly" section in the tablet-refresh etherpad. @@ +202,5 @@ > + // TODO: Remove the branch when new tablet becomes old tablet. > + if (!isNewTablet) { > + LayoutInflater.from(context).inflate(R.layout.browser_toolbar, this); > + } else { > + LayoutInflater.from(context).inflate(R.layout.new_tablet_browser_toolbar, this); It would be slightly nicer if the constructor took this as an argument instead. Not a big deal though. File a follow-up if you think this is worth doing.

Comment on attachment 8478502 [details] [diff] [review] Part 3: Create BrowserToolbar tablet and new_tablet implementations Review of attachment 8478502 [details] [diff] [review]: ----------------------------------------------------------------- Awesome. ::: mobile/android/base/toolbar/BrowserToolbarTabletBase.java @@ +40,5 @@ > + backButton = (BackButton) findViewById(R.id.back); > + setButtonEnabled(backButton, false); > + forwardButton = (ForwardButton) findViewById(R.id.forward); > + setButtonEnabled(forwardButton, false); > + initButtonListeners(); Initialize these in onAttachedToWindow() for consistency with the BrowserToolbar parent class. @@ +47,5 @@ > + focusOrder.addAll(urlDisplayLayout.getFocusOrder()); > + focusOrder.addAll(Arrays.asList(actionItemBar, menuButton)); > + } > + > + private void initButtonListeners() { This should become your onAttachedToWindow().

Comment on attachment 8478512 [details] [diff] [review] Part 4: Remove unused views and move actionItemBar to TabletBase Review of attachment 8478512 [details] [diff] [review]: ----------------------------------------------------------------- Less useless views, win.

Comment on attachment 8478585 [details] [diff] [review] Part 5: Remove final isTablet invocations in BrowserToolbar Review of attachment 8478585 [details] [diff] [review]: ----------------------------------------------------------------- We'll probably have different implementations of transitionForTabsTray in the new tablet IO but this is fine for now. ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +149,5 @@ > protected abstract void updateNavigationButtons(Tab tab); > > protected abstract void transitionForStartEditing(PropertyAnimator animator); > protected abstract void transitionForStopEditing(); > + public abstract void transitionForTabsTray(PropertyAnimator animator, boolean areTabsShown); triggerTabsPanelTransition instead?

I believe the try run in comment 31 has failures because the forward button is not currently implemented - I'll do a basic implementation in part 6.

(In reply to Lucas Rocha (:lucasr) from comment #32) > It would be slightly nicer if the constructor took this as an argument > instead. Not a big deal though. File a follow-up if you think this is worth > doing. Since we're only holding onto this until the new tablet is the default, I don't think it's a big deal.

(In reply to Lucas Rocha (:lucasr) from comment #33) > ::: mobile/android/base/toolbar/BrowserToolbarTabletBase.java > @@ +40,5 @@ > > + backButton = (BackButton) findViewById(R.id.back); > > + setButtonEnabled(backButton, false); > > + forwardButton = (ForwardButton) findViewById(R.id.forward); > > + setButtonEnabled(forwardButton, false); > > + initButtonListeners(); > > Initialize these in onAttachedToWindow() for consistency with the > BrowserToolbar parent class. Why? What is the benefit? I can see this being useful for two things: 1) Lazy initialization, though I'm skeptical about the benefits 2) To free memory by nulling our listeners in onDetachedFromWindow. However, we don't do this so we end up doing redundant work and forcing GC every time this View is attached to the window.

Review comments.

Review comments.

Comment on attachment 8479325 [details] [diff] [review] Part 6: Copy-pasta rough BrowserToolbarNewTablet forward button behavior Looking at the failures in comment 42 locally, the forward button may be overlapped by the back button, so the back button gets hit instead - I'll update my patch shortly.

Copy-pasta with a hint of increasing the animation with from width * .5 to width * .75. try run in comment 44.

Comment on attachment 8480168 [details] [diff] [review] Part 6: Copy-pasta rough BrowserToolbarNewTablet forward button behavior Rubber stamp to help get tests passing. As I understand it, this will be refined in bug 1058322.

For question in comment 38. I'm going to land, so we can file a followup if this is an issue.

https://hg.mozilla.org/projects/larch/rev/17ec4d74742f https://hg.mozilla.org/projects/larch/rev/30a930761cb1 https://hg.mozilla.org/projects/larch/rev/387272b14ffb https://hg.mozilla.org/projects/larch/rev/ce933bcd3da1 https://hg.mozilla.org/projects/larch/rev/010efd1be249 https://hg.mozilla.org/projects/larch/rev/14ad06c823ed https://hg.mozilla.org/projects/larch/rev/7160edf098c7

(In reply to Michael Comella (:mcomella) from comment #47) > For question in comment 38. I'm going to land, so we can file a followup if > this is an issue. Not a big issue, it's just to keep some consistency across the toolbar classes.

https://hg.mozilla.org/mozilla-central/rev/17ec4d74742f https://hg.mozilla.org/mozilla-central/rev/30a930761cb1 https://hg.mozilla.org/mozilla-central/rev/387272b14ffb https://hg.mozilla.org/mozilla-central/rev/ce933bcd3da1 https://hg.mozilla.org/mozilla-central/rev/010efd1be249 https://hg.mozilla.org/mozilla-central/rev/14ad06c823ed https://hg.mozilla.org/mozilla-central/rev/7160edf098c7