We should allow dragging the urlbar to open the tabs tray. It should follow your finger as you drag. This is a prototype implementation of this. Its messier than the one I just put up in bug 909427, mostly because our code to show/hide the tabs tray is kinda a mess and i was working around it to get the prototype running. i.e. all of the "boolean animated" stuff should go away and I think just calling TabsPanel.scrollTo (which should be renamed), should unhide the tray if its hidden. There's some performance issues as well, that seem to get worse the longer you play. I also did some hackery so you can slide the open tray closed, but only if you drag on the urlbar, not content. After playing with it for a weekend, I think that should probably be updated so that dragging any of it will drag the tray, but tapping will just close it.

Much cleaned up. Need to test on tablets, but if you've got feedback, would love hearing it.

What's the UX take on this?

From an email I sent to Wes: I love the pulling on the header to reveal the tab tray, that's a really nice feeling interaction. I agree we can refine the performance a bit but we should go for it I think.

The performance of this newer patch should be much better I think. I'm turning on hardware acceleration. Probably needs testing (disabling?) on lower end devices as well. But I also feel like we've got this same gesture detector in a few places now. Slide-to-close tabs, here, and in my other tab-indicator-drag patch. I think maybe we can factor out a lot of it into a shared class and just have the onDrag(dx, dy); onDragEnd(vx, vy); and onTap(); methods implemented for different callers. While its similar to Android's its (IMO) simpler to use and implement (you don't have to attach a touch listener which sends events to a GestureDetector, which sends them to a GestureListener, instead you just attach a single touch listener), and gives us a lot of power to override bits and pieces where we need to. I'd probably do that in a separate bug once we have these in though. Otherwise, its not useful.

Comment on attachment 795883 [details] [diff] [review] Patch v1 >diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java > public boolean onTouch(View view, MotionEvent event) { > if (mIsHidingTabs) { >+ mDragger.onTouch(view, event); > // Keep consuming events until the gesture finishes. Add a blank line before the comment >+ abstract class Draggable implements View.OnTouchListener { This looks like something we should break out. I think that's what you were talking about in your bug comment >diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java >- Tab tab = Tabs.getInstance().getSelectedTab(); >- if (tab != null) { >- if (!tab.isPrivate()) >- mActivity.showNormalTabs(); >- else >- mActivity.showPrivateTabs(); >- } >+ mActivity.showTabs(); This code made me wonder about something. Why are we treating mActivity like a GeckoApp and not a BrowserApp ? Why are we putting empty methods in GeckoApp that are only implemented in BrowserApp? I seems like we need to audit the code and make sure UI components, like BrowserToolbar, treat the activity like a BrowserApp. >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java > public void addPrivateTab() { } > > public void showNormalTabs() { } > > public void showPrivateTabs() { } > > public void showRemoteTabs() { } > >+ public void showTabs() { } >+ > private void showTabs(TabsPanel.Panel panel) { } > > public void hideTabs() { } Speak of the devil >diff --git a/mobile/android/base/TabsPanel.java b/mobile/android/base/TabsPanel.java > public TabsPanel(Context context, AttributeSet attrs) { > mActivity = (GeckoApp) context; Speak of the devil again. TabsPanel will never be used by anything but a BrowserApp. >+ public void show(Panel panel, boolean shouldResize) { >+ showPanel(panel); > >+ final boolean showAnimation = !mVisible; >+ prepareToShow(); It's a little weird that we call "showPanel" then call "prepareToShow". What am I missing here? >+ public void scrollTo(int dx, int dy) { >+ showPanel(panel); >+ prepareToShow(); >+ prepareTabsAnimation(null); Same This patch scares me because we already lived through something like this in XUL Fennec. It was a nightmare of regressions and tweaks. Why is this different? There is a shit-ton of scroll code in this patch, in many places. That scares me too. What kind of interaction nightmares are we jumping into?

I've asked Ian for an interaction mockup. I'm waiting for that before reviewing this and the other patch.

I don't think this really needs a mock... The only issue I saw with Wes's earlier implementation was that the dragging felt a little janky. The interaction was fine. The other thing we should think about is how this works on tablets. My thought would be to recreate what we had in XUL Fennec, where in landscape mode swiping from the edge opens the tab tray. In portrait mode, it would behave the same as phones since the tab tray is still on top.

My feedback to Wes was that I did not want a drag to start on content. If we want to use the URLBar as a drag target, that might work OK. It would be competing with the System Notification drag, but might be fine. For the sidebar on tablets, we used a drag that started on content, and it was a pain in the ass to get working without regressions. I'd rather limit any tablet sidebar gesture to use the "drag starts at edge" gesture supported on newer versions of Android.

Fair enough. Let's try it and see.

Comment on attachment 795883 [details] [diff] [review] Patch v1 Review of attachment 795883 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by. This is a pretty good opportunity to use a ViewDragHelper (in the support library now). It gives us a backwards-compatible API for dragging children in custom ViewGroups, which seems exactly what we're doing here. In theory, this would involve much less custom MotionEvent handling on our side. For more information, see: http://developer.android.com/reference/android/support/v4/widget/ViewDragHelper.html http://flavienlaurent.com/blog/2013/08/28/each-navigation-drawer-hides-a-viewdraghelper/ As mfinkle said, I'd avoid dragging-to-show-tabs starting from the web content area. ::: mobile/android/base/BrowserApp.java @@ +2294,5 @@ > + > + event.offsetLocation(0, event.getRawY()-event.getY()); > + tracker.addMovement(event); > + > + mPrevPoint = MotionEvent.obtain(event); This is creating a new object on each ACTION_MOVE event which is not ideal. Simply hold the arguments (dragX and dragY?) instead.

This patch uses viewdraghelper as suggested by lucas. ViewDragHelper was pretty tricky to use, but after reading its source code I managed to make it work (?). The basic idea here is to have the whole mainlayout dragged around. To do so, I had to implement an "outerLayout" which is the outest RelativeLayout in GeckoApp layout. It accepts drags from the toolbar, and from the left edge case of sidebar. Notes: - I had to accept drags only after gecko is ready. The draghelper moves the mainlayout by an offset, when gecko was finally ready the layout was automatically restored at offset 0. If this is an issue, I can keep working on it - I had to re-measure the tabs height and width AFTER they are made visible. Otherwise the getTabContainerHeight method of tabs panel gave wrong results. - Since the tabs button animation scrolls the layout, while the draghelper actually moves it by an offset, I had to restore the state under the hood in order to make it possible to close the tray after touching the main layout. Basically, right after the tray is opened by dragging, I set the whole state as if it was opened by clicking the button. It _seems_ to work, at least on my devices. I tried the horizontal/vertical ones on my n7, and the vertical one on my n4 before it committed suicide from the border of the table. Hope its sacrifice is not in vain.

Comment on attachment 8351524 [details] [diff] [review] This version uses viewdraghelper as suggested by lucasr. sriram switched jobs. but lucas should look at this :)

Federico, sorry for the delay here! I'll be having a look at your patch this week.

Comment on attachment 8351524 [details] [diff] [review] This version uses viewdraghelper as suggested by lucasr. Review of attachment 8351524 [details] [diff] [review]: ----------------------------------------------------------------- The code looks fine to me, and this approach feels pretty clean. A few questions/changes to the interaction. ::: mobile/android/base/OuterLayout.java @@ +16,5 @@ > + > +public class OuterLayout extends RelativeLayout { > + private final double AUTO_OPEN_SPEED_LIMIT = 800.0; > + > + public class DragHelperCallback extends ViewDragHelper.Callback { This class is big enough we should just put it in its own file. @@ +106,5 @@ > + public int clampViewPositionVertical(View child, int top, int dy) { > + if (mIsVertical) { > + final int topBound = getPaddingTop(); > + final int bottomBound = mVerticalRange; > + return Math.min(Math.max(top, topBound), bottomBound); I would almost rather we didn't do this. Sorta like how you can overpull notifications on tablets. @@ +133,5 @@ > + } else if (mDraggingBorder > rangeToCheck / 2 || speedToCheck > AUTO_OPEN_SPEED_LIMIT) { > + final int settleDestX = mIsVertical? 0 : mHorizontalRange; > + final int settleDestY = mIsVertical? mVerticalRange : 0; > + > + if(mDragHelper.settleCapturedViewAt(settleDestX, settleDestY)) { Could probably put these into variables and move this out of the if/else. i.e. int settleX = 0, settleY = 0; if (...) { settleX = ... settleY = ... } if (mDragHelper.settCaptureView(settleX, settle)Y) { .. } @@ +164,5 @@ > + private void setupTabsForDragging() { > + if (Tabs.getInstance().isShowingPrivateTab()) { > + mTabsPanel.prepareToShow(TabsPanel.Panel.PRIVATE_TABS); > + } else { > + mTabsPanel.prepareToShow(TabsPanel.Panel.NORMAL_TABS); Missing } @@ +166,5 @@ > + mTabsPanel.prepareToShow(TabsPanel.Panel.PRIVATE_TABS); > + } else { > + mTabsPanel.prepareToShow(TabsPanel.Panel.NORMAL_TABS); > + } > + mTabsPanel.showImmediately(); Gah. I hate this api... I wonder if we can do this in prepareToShow()? @@ +192,5 @@ > + if (Tabs.getInstance().getSelectedTab() == null) { // if no current tab is active. > + return false; > + } > + if (mIsOpen) { // while in open state we do not intercept touches. > + return false; My original stuff did let you drag to close (or just constantly flick it down a little and let it bounce back). I liked it. It gave my fingers something to do and felt nice. Can we put it back? @@ +204,5 @@ > + > + final int x = (int) MotionEventCompat.getX(event, 0); > + final int y = (int) MotionEventCompat.getY(event, 0); > + if (mDragHelper.isViewUnder(mToolbar, x, y)) { // intercepting only drags starting from the toolbar. > + return true; I'd just return mDragHelper.isViewUnder() ::: mobile/android/base/TabsPanel.java @@ +340,5 @@ > + > + public int getVerticalPanelHeight() { > + int actionBarHeight = mContext.getResources().getDimensionPixelSize(R.dimen.browser_toolbar_height); > + int height = actionBarHeight + getTabContainerHeight(mTabsContainer); > + return height; We should lazy load this. It will never change. i.e. if (mHeight == 0) { mHeight = getResources.getDimen() + getTabContainerHeight(); } return mHeight; @@ +442,5 @@ > mHeader.setLayerType(View.LAYER_TYPE_HARDWARE, null); > mTabsContainer.setLayerType(View.LAYER_TYPE_HARDWARE, null); > } > > + public void showImmediately() { This name is a little confusing, since it doesn't actually show anything. ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +730,3 @@ > Tab tab = Tabs.getInstance().getSelectedTab(); > if (tab != null) { > + if (!Tabs.getInstance().isShowingPrivateTab()) Why this change?

Comment on attachment 8351524 [details] [diff] [review] This version uses viewdraghelper as suggested by lucasr. Review of attachment 8351524 [details] [diff] [review]: ----------------------------------------------------------------- wesj has already given some good feedback. This is looking pretty good overall! My main concern here is getting a more robust way to manage the state of the UI. The OuterLayout as is knows a bit too much about the specifics of BrowserApp. I wonder if OuterLayout should simply use a well-defined set of BrowserApp APIs instead. This way we could centralize more of the UI state management in a single class (BrowserApp, in this case) and avoid having these multiple references to BrowserToolbar, TabsPanel, etc. ::: mobile/android/base/OuterLayout.java @@ +152,5 @@ > + private int mDraggingBorder; > + private int mVerticalRange; > + private int mHorizontalRange; > + private boolean mIsVertical; > + private boolean mIsOpen; nit: declare all member at the top of the class. @@ +216,5 @@ > + mMainLayout = (GeckoApp.MainLayout) findViewById(R.id.main_layout); > + mTabsPanel = (TabsPanel) findViewById(R.id.tabs_panel); > + mVerticalRange = mTabsPanel.getVerticalPanelHeight(); > + mHorizontalRange = mTabsPanel.getWidth(); > + mDragHelper = ViewDragHelper.create(this, 1.0f, new DragHelperCallback()); I'm a bit concerned with having two different references to the same views both in OuterLayout and BrowserApp. This is very prone to confusion. Maybe we should try to come up with a good interface that allows us to share these things across OuterLayout and BrowserApp?

Hey, does it make sense if I try to finish this? I've (re)started looking at making it work, but now I am afraid I might be working on something of no interest.

Hey Frederico, thanks for your continued interest in this! I don't see any reason why this wouldn't still be a helpful interaction, but let's ask Ian again to make sure.

Cool, sorry if I left it behind but I had a lot of stuff in between (including having a daughter). Hope I will be able to finish it :-)

Yeah, still feels like a thing that would be nice to have, time permitting. Also congratulations Federico! :)

Here is another attempt to make it work. A build can be find at https://dl.dropboxusercontent.com/u/3092639/fennec-34.0a1.en-US.android-arm.apk A couple of notes: - I guess this should be the starting point for this behaviour. I did not animate the tabs panel nor the menu / adds tab buttons - While in vertical mode, the toolbar can be targeted to be dragged (unless it's in edit mode). This means that the current "tap to close" behaviour works only if we tap somewhere else the toolbar location. We could then intercept the UP motionevent in order to understand if the user tapped the toolbar or he dragged it - The tabs counter button "flickers" a bit while closing. I am calling setAlpha(1.0), but it might take a bit too much. We could think about letting it appear smoothly, or letting it appear a little earlier when it's approaching to the rest position - While in tablet layout (horizontally), we can drag from the bezel, but at the moment it closes automatically as soon as the user taps on the right screen. This because we do not have a target to catch the gesture from. We could think about intercept events that are near to the border. - I was not sure where to catch the tabspanel size, I guess that part could be improved. Should I place a hook in tabspanel' onMeasure() ? -ViewDragHelper made me swear. A lot.

Fixed few nits I left here and there, such as spaces and a debug variable.

Thanks for the build Federico! Some initial comments from my end: - I am finding the drag-to-open interaction is pretty hard to trigger reliably. I can only make it work about half the time when i swipe from the title bar, and otherwise I end up accidentally triggering the URL bar or the tab or menu buttons. I don't really know what actionable thing to suggest here, I'm just finding it hard to do. It's nice when it works though! - There is an unfortunate overlap effect happening when you start dragging the bar down -- see screenshot http://cl.ly/image/3Y2z3b1b2s2v -- perhaps these icons shouldn't be showing right away?

Thanks for your feedback Ian.. (In reply to Ian Barlow (:ibarlow) from comment #22) > Thanks for the build Federico! Some initial comments from my end: > > - I am finding the drag-to-open interaction is pretty hard to trigger > reliably. I can only make it work about half the time when i swipe from the > title bar, and otherwise I end up accidentally triggering the URL bar or the > tab or menu buttons. I don't really know what actionable thing to suggest > here, I'm just finding it hard to do. It's nice when it works though! > There still must be some space to improve, I am afraid I am still missing something when you try to drag the first time fennec gets started and the drag does not get intercepted properly. What it looks like to me is that after you managed to drag once / opened by clicking the button, the interaction is much easier to trigger. Can you confirm this? I'll investigate anyway. > - There is an unfortunate overlap effect happening when you start dragging > the bar down -- see screenshot http://cl.ly/image/3Y2z3b1b2s2v -- perhaps > these icons shouldn't be showing right away? Ooops, did not notice them. Need to understand if it is possible to hide / set alpha on the dark band.

So, the previous implementation had to wait for gecko to be ready because it then forced the tabs to be drawn. Now I almost managed to make the drag work *immediately* after fennec is started, but I am getting a weird behaviour I am not able to sort out. I've been placing logs everywhere but I am afraid I got stuck and need some help. The thing is, if I start to drag immediately after a fresh fennec start (or after swiping it out from the recent apps), the drag works, but then something forces main layout to offset 0. I uploaded a video here https://www.youtube.com/watch?v=1OkNBgoKHgo , that "buonce" is the main layout brought back to offset 0, it feels like some event forces the main layout to be redrawn in place (?). It happens twice every time fennec starts from scratch. Once it starts, the dragging works. I trace mainLayout's top in the onTouchEvent callback, and what I get is something like 10 50 120 0 <- the top suddenly gets back to 0 Do you guys have any idea of what is going on? This attachment is the patch with the logs I placed, I am not asking for a feedback here (yet) but I am adding it in case could be of some help.

Comment on attachment 8464323 [details] [diff] [review] bug-909434-fix Review of attachment 8464323 [details] [diff] [review]: ----------------------------------------------------------------- Overall I'm pretty happy with this. I also saw the problem with it not triggering that ian mentioned. We should get some logging in to see why it bails sometimes and see if we can reduce those failures. I hope the problem isn't in ViewDragHelpers drag detection. I'm not sure what the jiggle is at the beginning. I'll try to play with it once I've gotten through my review queue. ::: mobile/android/base/BrowserApp.java @@ +1513,5 @@ > } > > @Override > public void onTabsLayoutChange(int width, int height) { > + mOuterLayout.onTabsLayoutChange(); This is a little odd. Can the outerLayout not listen to tabs itself? @@ +3006,5 @@ > previousSession); > } > + > + @Override > + public void showTabsPanel() { There is already a showTabs() method in BrowserApp. Can we use it here? It calls mTabsPanel.show(), so maybe its not what we want. @@ +3029,5 @@ > + > + @Override > + public boolean isVerticalLayout() { > + return !mTabsPanel.isSideBar(); > + } My gut feels like its strange for the BrowserApp to handle this instead of the tabs panel. @@ +3053,5 @@ > + > + @Override > + public boolean isToolbarEditing() { > + return mBrowserToolbar.isEditing(); > + } Similarly, it seems strange not to just get a reference to the toolbar and instead proxy these calls through BrowserApp. I guess that removes a "dependency" between the two, but the interface does that on some level as well.... Any reason that BrowserToolbar can't just be the ToolbarController instead? Also, BrowserApp just has too much junk in it, so I'd like to avoid adding more. ::: mobile/android/base/GeckoApp.java @@ +158,5 @@ > // Delay before running one-time "cleanup" tasks that may be needed > // after a version upgrade. > private static final int CLEANUP_DEFERRAL_SECONDS = 15; > > + protected OuterLayout mOuterLayout; Move to BrowserApp if its the only one who uses this. ::: mobile/android/base/OuterLayout.java @@ +28,5 @@ > + private ToolbarController mToolbarController; > + > + public static interface TabsController { > + public void showTabsPanel(); > + public void hideTabsPanel(); I think these methods should take a float 0 to 1 so that we can transition to in between states in showing/hiding the tabs tray. @@ +30,5 @@ > + public static interface TabsController { > + public void showTabsPanel(); > + public void hideTabsPanel(); > + public int getTabsHeight(); > + public int getTabsWidth(); Since this is a TabsController, I'd remove that word "Tab" from methods. It feels redudent. Same for "Toolbar" below. @@ +36,5 @@ > + } > + > + public static interface ToolbarController { > + public void hideToolbarControls(); > + public void showToolbarControls(); Same for these. @@ +74,5 @@ > + @Override > + public void onViewPositionChanged(View changedView, int left, int top, int dx, int dy) { > + if (mIsVertical) { > + mDraggingBorder = top; > + if (top == 0) { Can we transition the tabs tray in this? Follow up bug? @@ +87,5 @@ > + if (mIsVertical) { > + return mVerticalRange; > + } else { > + return super.getViewVerticalDragRange(child); > + } I still think we should let you drag as far as you want, but settle back to this point when you let go. @@ +229,5 @@ > + // because gecko restores the main layout to its original position. > + /*if (!GeckoThread.checkLaunchState(GeckoThread.LaunchState.GeckoRunning)) { > + return false; > + }*/ > + if (Tabs.getInstance().getSelectedTab() == null) { // if no current tab is active. When does this happen? ::: mobile/android/base/tabspanel/TabsPanel.java @@ +480,5 @@ > + } > + > + public int getVerticalPanelHeight() { > + int actionBarHeight = mContext.getResources().getDimensionPixelSize(R.dimen.browser_toolbar_height); > + int height = actionBarHeight + getTabContainerHeight(mTabsContainer); We're pretty big into making everything we can final nowadays (except methods/classes). @@ +560,5 @@ > } > > + public void restoreTabs() { > + if (!mIsSideBar) { > + ViewHelper.setTranslationY(mHeader, 0); This is the same in both cases, we could just move it out of here.

(In reply to Wesley Johnston (:wesj) from comment #25) > Comment on attachment 8464323 [details] [diff] [review] > bug-909434-fix > > Review of attachment 8464323 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall I'm pretty happy with this. I also saw the problem with it not > triggering that ian mentioned. We should get some logging in to see why it > bails sometimes and see if we can reduce those failures. I hope the problem > isn't in ViewDragHelpers drag detection. > > I'm not sure what the jiggle is at the beginning. I'll try to play with it > once I've gotten through my review queue. > From my understanding, viewdraghelper just works fine. One case when my code was not triggering, as per my previous comment, is from a fresh start. I kind of fixed it with patch https://bugzilla.mozilla.org/attachment.cgi?id=8468447 (which is pretty much the same stuff you feedbacked, except I don't wait geckodelayedstartup -> ensureTabsPanelExists to set the TabsController and thus activate the dragging). However, doing that brought up that jiggle issue. As I said in https://bugzilla.mozilla.org/show_bug.cgi?id=909434#c24 , it looks like mMainLayout is restored back to offset 0 for some reason. It only happens the first time fennec starts from scratch, if I am fast enough to drag the toolbar before waiting a couple of secs. After that no jiggle happens. Please note that https://bugzilla.mozilla.org/attachment.cgi?id=8468447 is a different patch from the one you just feedbacked, but I need some guidance in order to understand what is causing that jiggle, since I was not able to do that. > ::: mobile/android/base/BrowserApp.java > @@ +1513,5 @@ > > } > > > > @Override > > public void onTabsLayoutChange(int width, int height) { > > + mOuterLayout.onTabsLayoutChange(); > > This is a little odd. Can the outerLayout not listen to tabs itself? > > @@ +3006,5 @@ > > previousSession); > > } > > + > > + @Override > > + public void showTabsPanel() { > > There is already a showTabs() method in BrowserApp. Can we use it here? It > calls mTabsPanel.show(), so maybe its not what we want. mTabsPanel.show() triggers the animation, which is something we do not want (at least, we do not want an automatic animation). > > @@ +3029,5 @@ > > + > > + @Override > > + public boolean isVerticalLayout() { > > + return !mTabsPanel.isSideBar(); > > + } > > My gut feels like its strange for the BrowserApp to handle this instead of > the tabs panel. Maybe I misunderstood Lucas in https://bugzilla.mozilla.org/show_bug.cgi?id=909434#c15, but I thought he preferred to encapsulate all the calls in browserapp. > > @@ +3053,5 @@ > > + > > + @Override > > + public boolean isToolbarEditing() { > > + return mBrowserToolbar.isEditing(); > > + } > > Similarly, it seems strange not to just get a reference to the toolbar and > instead proxy these calls through BrowserApp. I guess that removes a > "dependency" between the two, but the interface does that on some level as > well.... Any reason that BrowserToolbar can't just be the ToolbarController > instead? > > Also, BrowserApp just has too much junk in it, so I'd like to avoid adding > more. See my comment above > > ::: mobile/android/base/GeckoApp.java > @@ +158,5 @@ > > // Delay before running one-time "cleanup" tasks that may be needed > > // after a version upgrade. > > private static final int CLEANUP_DEFERRAL_SECONDS = 15; > > > > + protected OuterLayout mOuterLayout; > > Move to BrowserApp if its the only one who uses this. Ok > > ::: mobile/android/base/OuterLayout.java > @@ +28,5 @@ > > + private ToolbarController mToolbarController; > > + > > + public static interface TabsController { > > + public void showTabsPanel(); > > + public void hideTabsPanel(); > > I think these methods should take a float 0 to 1 so that we can transition > to in between states in showing/hiding the tabs tray. > I guess that could be done once we introduce transition in showing the tabs tray (step 2 maybe?) > @@ +30,5 @@ > > + public static interface TabsController { > > + public void showTabsPanel(); > > + public void hideTabsPanel(); > > + public int getTabsHeight(); > > + public int getTabsWidth(); > > Since this is a TabsController, I'd remove that word "Tab" from methods. It > feels redudent. Same for "Toolbar" below. > > @@ +36,5 @@ > > + } > > + > > + public static interface ToolbarController { > > + public void hideToolbarControls(); > > + public void showToolbarControls(); > > Same for these. > Ok > @@ +74,5 @@ > > + @Override > > + public void onViewPositionChanged(View changedView, int left, int top, int dx, int dy) { > > + if (mIsVertical) { > > + mDraggingBorder = top; > > + if (top == 0) { > > Can we transition the tabs tray in this? Follow up bug? > Yeah, I'd focus to make this work first > @@ +87,5 @@ > > + if (mIsVertical) { > > + return mVerticalRange; > > + } else { > > + return super.getViewVerticalDragRange(child); > > + } > > I still think we should let you drag as far as you want, but settle back to > this point when you let go. > Step 2 ? > @@ +229,5 @@ > > + // because gecko restores the main layout to its original position. > > + /*if (!GeckoThread.checkLaunchState(GeckoThread.LaunchState.GeckoRunning)) { > > + return false; > > + }*/ > > + if (Tabs.getInstance().getSelectedTab() == null) { // if no current tab is active. > > When does this happen? > It should not, it's commented out and I should remove it. > ::: mobile/android/base/tabspanel/TabsPanel.java > @@ +480,5 @@ > > + } > > + > > + public int getVerticalPanelHeight() { > > + int actionBarHeight = mContext.getResources().getDimensionPixelSize(R.dimen.browser_toolbar_height); > > + int height = actionBarHeight + getTabContainerHeight(mTabsContainer); > > We're pretty big into making everything we can final nowadays (except > methods/classes). > Ok > @@ +560,5 @@ > > } > > > > + public void restoreTabs() { > > + if (!mIsSideBar) { > > + ViewHelper.setTranslationY(mHeader, 0); > > This is the same in both cases, we could just move it out of here. Ok

(In reply to Federico Paolinelli from comment #27) > Created attachment 8475297 [details] [diff] [review] > bug-909434-fix I forgot the comments: Here I: - fixed the weird startup thing. What was happening is that if the root view asks for a layout round, any offset is lost, so it was the one placed by the drag helper - fixed a crash that was happening if a rotation happened while dragging the bar - fixed a conflict between the toast and the toolbar. The toast was intercepting the down event in place of the toolbar - animated the tabs panel / toolbar button's alpha channel according to the progress of the dragging - enabled the drag from the bezel margin on tablets - allowed the drag to go further the rest position. Once the toolbar is released, it gets back .. and maybe something I am forgetting

and here is a fresh build https://dl.dropboxusercontent.com/u/3092639/fennec-34.0a1.en-US.android-arm.apk

Small update: - fixed a crash that happened if the user is fast enough to drag a settling view -setHWLayer returns on preHC devices

(In reply to Federico Paolinelli from comment #29) > and here is a fresh build > https://dl.dropboxusercontent.com/u/3092639/fennec-34.0a1.en-US.android-arm. > apk very interesting! 1. can I ask that tabs tray could also be shown when I swipe down _inside_ the page contents, when the page is scrolled up to the top? I uploaded a video clip of what I expected to https://bugzilla.mozilla.org/attachment.cgi?id=8476953 2. if I tap the url bar, I can no loger swipe down. implementing 1) would make this irrelevant.

Comment on attachment 8476046 [details] [diff] [review] bug-909434-fix Switching flag since I guess this needs a proper review :P

Comment on attachment 8476046 [details] [diff] [review] bug-909434-fix Review of attachment 8476046 [details] [diff] [review]: ----------------------------------------------------------------- These are mostly nits and an idea for a change I'd like for the interfaces. Curious what lucas says. ::: mobile/android/base/BrowserApp.java @@ +128,5 @@ > OnUrlOpenListener, > + ActionModeCompat.Presenter, > + OuterLayout.TabsController, > + OuterLayout.ToolbarController, > + OuterLayout.DragProgressListener { I would pull the TabsController and ToolbarConroller interfaces into classes more related to them or their own files. TabsTray.Controller and BrowserToolbar.Controller? @@ +496,5 @@ > Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.INTENT); > } > > + // if a layout round happens from the root, the offset placed by viewdraghelper gets forgotten and > + // main layout gets replaced to offset 0 Capitialize first word and period at the end of sentences. @@ +3064,5 @@ > + @Override > + public void onGlobalLayout() { > + mTabsPanel.getViewTreeObserver().removeGlobalOnLayoutListener(this); > + mTabsPanel.prepareToDrag(); > + mTabsPanel.setHWLayer(true); You could recursively call showPanel() here @@ +3120,5 @@ > + > + @Override > + public boolean isToastVisible() { > + return (mToast != null && mToast.isVisible()); > + } Its weird to have this in the tabs interface since it has nothing to do with tabs... ::: mobile/android/base/OuterLayout.java @@ +33,5 @@ > + public void hidePanel(); > + public int getHeight(); > + public int getWidth(); > + public void setHwLayer(boolean isHw); > + public boolean isToastVisible(); This toast bit seems unrelated to this interface @@ +36,5 @@ > + public void setHwLayer(boolean isHw); > + public boolean isToastVisible(); > + } > + > + public static interface ToolbarController { However, If we wanted to keep these interfaces in here, maybe we can make them a bit more generic. i.e. can we make the ToolbarController a "Draggable" interface that could encapsulate both the Toolbar and TabsTrays together: public static interface Draggable { public void getRange(); // I guess this would query the TabsTray public void startDrag(); // Right now hideControls just hides the keyboard. TabsController would do what is in showPanel. The setHWLayer code would also go in here? Maybe we need to pass in/out the start/end position of the drag. i.e. OPEN or CLOSED? public void endDrag(); // hidePanel goes in here. setHWLayer(false); public boolean canDrag(int x, int y); // Can we combine isInBounds and isEditing into something like this? public int getDragDir(); // I guess this would restrict things to only x/y dragging. I'm fine with that for now. We can reuse LinearLayout.HORIZONTAL/VERTICAL. I guess we could add BOTH at some point... public int getOrderedChildIndex(int index); // I don't understand the toast bit very well, but I'd just delegate to it here I guess... Would that work? } I'm sure Android uses the name Draggable somewhere, but as long as we're namespaced it should work here too. This could still "be" or live in BrowserApp, since its the main interface point that knows about these different widgets. What do you think? @@ +44,5 @@ > + public boolean isVerticalLayout(); > + } > + > + public static interface DragProgressListener { > + public void onDragProgress(float progress); This could go in the above interface too... @@ +55,5 @@ > + return; > + } > + if ((mDraggingState == ViewDragHelper.STATE_DRAGGING || mDraggingState == ViewDragHelper.STATE_SETTLING) && > + state == ViewDragHelper.STATE_IDLE) { > + // the view stopped moving. Change this to say something like // If we are moving from a moving view to a non-moving one. @@ +74,5 @@ > + } > + mTabsController.setHwLayer(false); > + } > + > + if (state == ViewDragHelper.STATE_DRAGGING && !isMoving()) { This is confusing. How are we dragging but not moving? @@ +88,5 @@ > + } > + > + @Override > + public void onViewPositionChanged(View changedView, int left, int top, int dx, int dy) { > + float progress = 0; Make this final and don't assign it till below. @@ +130,5 @@ > + @Override > + public int getOrderedChildIndex(int index) { > + // See ViewDragHelper's findTopChildUnder method. ViewDragHelper looks for the topmost view in z order > + // to understand what needs to be dragged. Here we are tampering Toast's index in case it's hidden, > + // otherwise draghelper would try to drag it. Nice detective work :) @@ +131,5 @@ > + public int getOrderedChildIndex(int index) { > + // See ViewDragHelper's findTopChildUnder method. ViewDragHelper looks for the topmost view in z order > + // to understand what needs to be dragged. Here we are tampering Toast's index in case it's hidden, > + // otherwise draghelper would try to drag it. > + int mainLayoutIndex = OuterLayout.this.indexOfChild(mMainLayout); Why the OuterLayout.this? @@ +144,5 @@ > + public boolean tryCaptureView(View view, int i) { > + if (mIsVertical || mIsOpen) { > + return (view.getId() == R.id.main_layout); > + } else { > + return false; Don't need the else here @@ +153,5 @@ > + public int clampViewPositionVertical(View child, int top, int dy) { > + if (mIsVertical) { > + return top; > + } else { > + return super.clampViewPositionVertical(child, top, dy); No else here either. @@ +162,5 @@ > + public int clampViewPositionHorizontal(View child, int left, int dx) { > + if (!mIsVertical) { > + return left; > + } else { > + return super.clampViewPositionHorizontal(child, left, dx); Or here. @@ +195,5 @@ > + final int settleDestX; > + final int settleDestY; > + if (settleToOpen) { > + settleDestX = mIsVertical? 0 : mHorizontalRange; > + settleDestY = mIsVertical? mVerticalRange : 0; Space before the ? in these throughput. Also, a blank line after most of these if-elses (unless it looks stupid). @@ +209,5 @@ > + } > + > + public OuterLayout(Context context, AttributeSet attrs) { > + super(context, attrs); > + mIsOpen = false; I'd just initialize this where its declared. @@ +253,5 @@ > + mTabsController.hidePanel(); > + } > + > + private boolean canInterceptTouchEvent(MotionEvent event) { > + if (Tabs.getInstance().getSelectedTab() == null) { // if no current tab is active. Is this possible? @@ +322,5 @@ > + } > + > + @Override > + protected void onFinishInflate() { > + mMainLayout = (GeckoApp.MainLayout) findViewById(R.id.main_layout); It sucks a bit that this has to know about the mainLayout. I wonder if it should be passed in here to make this a bit more generic.... @@ +354,5 @@ > + > + @Override > + protected void onSizeChanged(int w, int h, int oldw, int oldh) { > + if (mVerticalRange == 0) { > + mVerticalRange = h / 2; What's going on here? Can you add a comment? @@ +374,5 @@ > + * To be called when closing the tabs from outside (i.e. when touching the main layout). > + */ > + public void setClosed() { > + mIsOpen = false; > + mDragHelper.abort(); We should try to make it clear these aborts are in here. i.e. this will kill any current drags in progress, so you shouldn't use this internally. Only saying that because at first glance I was like "Why isn't he using this everywhere?" A little comment might help. ::: mobile/android/base/Tabs.java @@ +288,5 @@ > return tab != null && tab == mSelectedTab; > } > > + public boolean isShowingPrivateTab() { > + return (mSelectedTab != null && mSelectedTab.isPrivate()); I would probably not add this. getSelectedTab().isPrivate() is basically the same (although for you mSelectedTab can be null which sucks...) ::: mobile/android/base/tabs/TabsPanel.java @@ +397,5 @@ > + final boolean showAnimation = !mVisible; > + prepareToShow(panelToShow); > + if (isSideBar()) { > + if (showAnimation) > + dispatchLayoutChange(getWidth(), getHeight()); There is a case here where we won't dispatch a change. Do we want it? Also, don't forget braces. @@ +404,5 @@ > + dispatchLayoutChange(getWidth(), height); > + } > + } > + > + public void prepareToDrag() { Hmm.. I sorta wanted to avoid this having to know about dragging. Not sure how though :) Maybe it would be cleaner to just expose a show(Reason reasonToShow) which could then use the reason to determine if it should be 1.) Immediate 2.) Animated 3.) Dragging... I don't love that either though. Given I have no ideas, this is fine with me :) @@ +523,5 @@ > public Panel getCurrentPanel() { > return mCurrentPanel; > } > > + public void setHWLayer(boolean hw) { You might just override the setLayerType for TabsPanel here instead, so that this felt less like something tacked on. Is there some reason we can't just use setLayerType on this view?

(In reply to Wesley Johnston (:wesj) from comment #33) > Comment on attachment 8476046 [details] [diff] [review] > bug-909434-fix > > Review of attachment 8476046 [details] [diff] [review]: > ----------------------------------------------------------------- > > These are mostly nits and an idea for a change I'd like for the interfaces. > Curious what lucas says. > > ::: mobile/android/base/BrowserApp.java > @@ +128,5 @@ > > OnUrlOpenListener, > > + ActionModeCompat.Presenter, > > + OuterLayout.TabsController, > > + OuterLayout.ToolbarController, > > + OuterLayout.DragProgressListener { > > I would pull the TabsController and ToolbarConroller interfaces into classes > more related to them or their own files. TabsTray.Controller and > BrowserToolbar.Controller? > > @@ +496,5 @@ > > Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.INTENT); > > } > > > > + // if a layout round happens from the root, the offset placed by viewdraghelper gets forgotten and > > + // main layout gets replaced to offset 0 > > Capitialize first word and period at the end of sentences. > > @@ +3064,5 @@ > > + @Override > > + public void onGlobalLayout() { > > + mTabsPanel.getViewTreeObserver().removeGlobalOnLayoutListener(this); > > + mTabsPanel.prepareToDrag(); > > + mTabsPanel.setHWLayer(true); > > You could recursively call showPanel() here > How? > @@ +3120,5 @@ > > + > > + @Override > > + public boolean isToastVisible() { > > + return (mToast != null && mToast.isVisible()); > > + } > > Its weird to have this in the tabs interface since it has nothing to do with > tabs... > Yes that's true. I added it at last while was fixing that toast issue. I do agree that it is not its place. > ::: mobile/android/base/OuterLayout.java > @@ +33,5 @@ > > + public void hidePanel(); > > + public int getHeight(); > > + public int getWidth(); > > + public void setHwLayer(boolean isHw); > > + public boolean isToastVisible(); > > This toast bit seems unrelated to this interface > > @@ +36,5 @@ > > + public void setHwLayer(boolean isHw); > > + public boolean isToastVisible(); > > + } > > + > > + public static interface ToolbarController { > > However, If we wanted to keep these interfaces in here, maybe we can make > them a bit more generic. i.e. can we make the ToolbarController a > "Draggable" interface that could encapsulate both the Toolbar and TabsTrays > together: > > public static interface Draggable { > public void getRange(); // I guess this would query the TabsTray > > public void startDrag(); // Right now hideControls just hides the > keyboard. TabsController would do what is in showPanel. The setHWLayer code > would also go in here? Maybe we need to pass in/out the start/end position > of the drag. i.e. OPEN or CLOSED? > public void endDrag(); // hidePanel goes in here. setHWLayer(false); > public boolean canDrag(int x, int y); // Can we combine isInBounds and > isEditing into something like this? > public int getDragDir(); // I guess this would restrict things to only x/y > dragging. I'm fine with that for now. We can reuse > LinearLayout.HORIZONTAL/VERTICAL. I guess we could add BOTH at some point... > > public int getOrderedChildIndex(int index); // I don't understand the > toast bit very well, but I'd just delegate to it here I guess... Would that > work? > } > > I'm sure Android uses the name Draggable somewhere, but as long as we're > namespaced it should work here too. This could still "be" or live in > BrowserApp, since its the main interface point that knows about these > different widgets. What do you think? > I would not call it draggable since it lets me think to something that should be dragged. Here we are talking about methods that assist / rule the drag process (a bit like the Callback object passed to the drag helper, which has the *so lacking of imagination* name of "callback"). Despite the name, I think there are pros and cons of both solutions: - Having one big helper interface as you are suggesting raises the level of abstraction a little bit, maybe it will be easier to maintain the code (even if that specific interface will have only one implementation for this specific case). - Having the xxxController interfaces maybe ends up in making it easier to follow the code and to understand the behaviour we are trying to achieve (ie hideControls, showPanel, etc). > @@ +44,5 @@ > > + public boolean isVerticalLayout(); > > + } > > + > > + public static interface DragProgressListener { > > + public void onDragProgress(float progress); > > This could go in the above interface too... > > @@ +55,5 @@ > > + return; > > + } > > + if ((mDraggingState == ViewDragHelper.STATE_DRAGGING || mDraggingState == ViewDragHelper.STATE_SETTLING) && > > + state == ViewDragHelper.STATE_IDLE) { > > + // the view stopped moving. > > Change this to say something like // If we are moving from a moving view to > a non-moving one. Mmm, maybe it is my bad English, but what I meant is that the target view was moving and here we are handling the fact that it stopped moving (it went from dragging / settling state to idle state). Does it make sense? > > @@ +74,5 @@ > > + } > > + mTabsController.setHwLayer(false); > > + } > > + > > + if (state == ViewDragHelper.STATE_DRAGGING && !isMoving()) { > > This is confusing. How are we dragging but not moving? > Because state is the new state, isMoving() checks the old state. Since it was the same condition I thought it would have been better to refactor it. I can rename state to newState / add a comment. > @@ +88,5 @@ > > + } > > + > > + @Override > > + public void onViewPositionChanged(View changedView, int left, int top, int dx, int dy) { > > + float progress = 0; > > Make this final and don't assign it till below. > > @@ +130,5 @@ > > + @Override > > + public int getOrderedChildIndex(int index) { > > + // See ViewDragHelper's findTopChildUnder method. ViewDragHelper looks for the topmost view in z order > > + // to understand what needs to be dragged. Here we are tampering Toast's index in case it's hidden, > > + // otherwise draghelper would try to drag it. > > Nice detective work :) > > @@ +131,5 @@ > > + public int getOrderedChildIndex(int index) { > > + // See ViewDragHelper's findTopChildUnder method. ViewDragHelper looks for the topmost view in z order > > + // to understand what needs to be dragged. Here we are tampering Toast's index in case it's hidden, > > + // otherwise draghelper would try to drag it. > > + int mainLayoutIndex = OuterLayout.this.indexOfChild(mMainLayout); > > Why the OuterLayout.this? > Oops :-) > @@ +144,5 @@ > > + public boolean tryCaptureView(View view, int i) { > > + if (mIsVertical || mIsOpen) { > > + return (view.getId() == R.id.main_layout); > > + } else { > > + return false; > > Don't need the else here > Ok > @@ +153,5 @@ > > + public int clampViewPositionVertical(View child, int top, int dy) { > > + if (mIsVertical) { > > + return top; > > + } else { > > + return super.clampViewPositionVertical(child, top, dy); > > No else here either. > > @@ +162,5 @@ > > + public int clampViewPositionHorizontal(View child, int left, int dx) { > > + if (!mIsVertical) { > > + return left; > > + } else { > > + return super.clampViewPositionHorizontal(child, left, dx); > > Or here. > > @@ +195,5 @@ > > + final int settleDestX; > > + final int settleDestY; > > + if (settleToOpen) { > > + settleDestX = mIsVertical? 0 : mHorizontalRange; > > + settleDestY = mIsVertical? mVerticalRange : 0; > > Space before the ? in these throughput. Also, a blank line after most of > these if-elses (unless it looks stupid). > > @@ +209,5 @@ > > + } > > + > > + public OuterLayout(Context context, AttributeSet attrs) { > > + super(context, attrs); > > + mIsOpen = false; > > I'd just initialize this where its declared. > > @@ +253,5 @@ > > + mTabsController.hidePanel(); > > + } > > + > > + private boolean canInterceptTouchEvent(MotionEvent event) { > > + if (Tabs.getInstance().getSelectedTab() == null) { // if no current tab is active. > > Is this possible? > I guess. I found that same condition in a lot of other places (check BrowserApp for instance). There is also a comment in Tabs.java: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#871 So I figured out it was healthy to check > @@ +322,5 @@ > > + } > > + > > + @Override > > + protected void onFinishInflate() { > > + mMainLayout = (GeckoApp.MainLayout) findViewById(R.id.main_layout); > > It sucks a bit that this has to know about the mainLayout. I wonder if it > should be passed in here to make this a bit more generic.... > I left out mainLayout from the xxxController group because it is the target view. Since ViewDragHelper has to know it's id, I felt having a "getTargetViewId" method to be called in Callback's getTargetView() too redundant. > @@ +354,5 @@ > > + > > + @Override > > + protected void onSizeChanged(int w, int h, int oldw, int oldh) { > > + if (mVerticalRange == 0) { > > + mVerticalRange = h / 2; > > What's going on here? Can you add a comment? The first time fennec is started, tabs might not have been created while we drag (that made me sweat a lot). In that case we need to have an arbitrary range to start the dragging that will be updated as soon as the tabs are created. I'll add a comment. > > @@ +374,5 @@ > > + * To be called when closing the tabs from outside (i.e. when touching the main layout). > > + */ > > + public void setClosed() { > > + mIsOpen = false; > > + mDragHelper.abort(); > > We should try to make it clear these aborts are in here. i.e. this will kill > any current drags in progress, so you shouldn't use this internally. Only > saying that because at first glance I was like "Why isn't he using this > everywhere?" A little comment might help. > Not sure I understood. abort() kills any dragging, but more importantly, reset ViewDragHelper state so that it can intercept a new drag from Down / Move / Up. What do you mean by "use this internally"? Where else should I have used it? > ::: mobile/android/base/Tabs.java > @@ +288,5 @@ > > return tab != null && tab == mSelectedTab; > > } > > > > + public boolean isShowingPrivateTab() { > > + return (mSelectedTab != null && mSelectedTab.isPrivate()); > > I would probably not add this. getSelectedTab().isPrivate() is basically the > same (although for you mSelectedTab can be null which sucks...) Ok > > ::: mobile/android/base/tabs/TabsPanel.java > @@ +397,5 @@ > > + final boolean showAnimation = !mVisible; > > + prepareToShow(panelToShow); > > + if (isSideBar()) { > > + if (showAnimation) > > + dispatchLayoutChange(getWidth(), getHeight()); > > There is a case here where we won't dispatch a change. Do we want it? Also, > don't forget braces. I left the same code (including the lack of braces) as before the patch: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabs/TabsPanel.java#451 Not sure about the behaviour though. > > @@ +404,5 @@ > > + dispatchLayoutChange(getWidth(), height); > > + } > > + } > > + > > + public void prepareToDrag() { > > Hmm.. I sorta wanted to avoid this having to know about dragging. Not sure > how though :) Maybe it would be cleaner to just expose a show(Reason > reasonToShow) which could then use the reason to determine if it should be > > 1.) Immediate > 2.) Animated > 3.) Dragging... > > I don't love that either though. Given I have no ideas, this is fine with me > :) > > @@ +523,5 @@ > > public Panel getCurrentPanel() { > > return mCurrentPanel; > > } > > > > + public void setHWLayer(boolean hw) { > > You might just override the setLayerType for TabsPanel here instead, so that > this felt less like something tacked on. Is there some reason we can't just > use setLayerType on this view? Because setHWLayer calls "setLayerType" on header and tabs container, which I think is something different from calling setLayerType on TabsPanel and which is what the current animation does.

Comment on attachment 8476046 [details] [diff] [review] bug-909434-fix I think lucas is too busy to look at this. Lets run with my comments :) > > You could recursively call showPanel() here > How? public void onGlobalLayout() { mTabsPanel.getViewTreeObserver().removeGlobalOnLayoutListener(this); showPanel(); } Will that not work? > I would not call it draggable since it lets me think to something that > should be dragged. Here we are talking about methods that assist / rule the > drag process (a bit like the Callback object passed to the drag helper, > which has the *so lacking of imagination* name of "callback"). > Despite the name, I think there are pros and cons of both solutions: > - Having one big helper interface as you are suggesting raises the level of > abstraction a little bit, maybe it will be easier to maintain the code (even > if that specific interface will have only one implementation for this > specific case). > - Having the xxxController interfaces maybe ends up in making it easier to > follow the code and to understand the behaviour we are trying to achieve (ie > hideControls, showPanel, etc). I think I would rather encapsulate all this logic on one place. Since BrowserApp already knows about abut these widgets, it seems like a good candidate. > > Change this to say something like // If we are moving from a moving view to > > a non-moving one. > > Mmm, maybe it is my bad English, but what I meant is that the target view > was moving and here we are handling the fact that it stopped moving (it went > from dragging / settling state to idle state). Does it make sense? Yep! > I guess. I found that same condition in a lot of other places (check > BrowserApp for instance). There is also a comment in Tabs.java: > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs. > java#871 > So I figured out it was healthy to check Sounds good :) > I left out mainLayout from the xxxController group because it is the target > view. Since ViewDragHelper has to know it's id, I felt having a > "getTargetViewId" method to be called in Callback's getTargetView() too > redundant. I think I'd still rather pass something in, so that this felt a little more generic. You ok with that? > > We should try to make it clear these aborts are in here. i.e. this will kill > > any current drags in progress, so you shouldn't use this internally. Only > > saying that because at first glance I was like "Why isn't he using this > > everywhere?" A little comment might help. > > > > Not sure I understood. abort() kills any dragging, but more importantly, > reset ViewDragHelper state so that it can intercept a new drag from Down / > Move / Up. What do you mean by "use this internally"? Where else should I > have used it? There are other places inside OuterLayout where we set mIsClosed. I originally thought they should all call through here (i.e. they shold use the isClosed accessor instead of modifying it themselves to keep any other state consistent). But this doesn't just set the closed state, it also aborts panning, so that was a bad idea. > > There is a case here where we won't dispatch a change. Do we want it? Also, > > don't forget braces. > > I left the same code (including the lack of braces) as before the patch: > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabs/ > TabsPanel.java#451 Hmm.. Lets leave it then. Thanks :)

Sorry for the delay, I am pretty busy lately :( (In reply to Wesley Johnston (:wesj) from comment #35) > Comment on attachment 8476046 [details] [diff] [review] > bug-909434-fix > > I think lucas is too busy to look at this. Lets run with my comments :) > > > > You could recursively call showPanel() here > > How? > > public void onGlobalLayout() { > mTabsPanel.getViewTreeObserver().removeGlobalOnLayoutListener(this); > showPanel(); > } > > Will that not work? > Sure, I just misanderstood and thought you meant I was risking infinite recursion. Btw, I just "took inspiration" from showTabs(). To add a bit of consistency I can change that as well. > > I would not call it draggable since it lets me think to something that > > should be dragged. Here we are talking about methods that assist / rule the > > drag process (a bit like the Callback object passed to the drag helper, > > which has the *so lacking of imagination* name of "callback"). > > Despite the name, I think there are pros and cons of both solutions: > > - Having one big helper interface as you are suggesting raises the level of > > abstraction a little bit, maybe it will be easier to maintain the code (even > > if that specific interface will have only one implementation for this > > specific case). > > - Having the xxxController interfaces maybe ends up in making it easier to > > follow the code and to understand the behaviour we are trying to achieve (ie > > hideControls, showPanel, etc). > > I think I would rather encapsulate all this logic on one place. Since > BrowserApp already knows about abut these widgets, it seems like a good > candidate. > Ok I'll go that way. > > > Change this to say something like // If we are moving from a moving view to > > > a non-moving one. > > > > Mmm, maybe it is my bad English, but what I meant is that the target view > > was moving and here we are handling the fact that it stopped moving (it went > > from dragging / settling state to idle state). Does it make sense? > > Yep! > > > I guess. I found that same condition in a lot of other places (check > > BrowserApp for instance). There is also a comment in Tabs.java: > > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs. > > java#871 > > So I figured out it was healthy to check > > Sounds good :) > > > I left out mainLayout from the xxxController group because it is the target > > view. Since ViewDragHelper has to know it's id, I felt having a > > "getTargetViewId" method to be called in Callback's getTargetView() too > > redundant. > > I think I'd still rather pass something in, so that this felt a little more > generic. You ok with that? > I am ok, even though I am not sure this thing will be ever generalized. > > > We should try to make it clear these aborts are in here. i.e. this will kill > > > any current drags in progress, so you shouldn't use this internally. Only > > > saying that because at first glance I was like "Why isn't he using this > > > everywhere?" A little comment might help. > > > > > > > Not sure I understood. abort() kills any dragging, but more importantly, > > reset ViewDragHelper state so that it can intercept a new drag from Down / > > Move / Up. What do you mean by "use this internally"? Where else should I > > have used it? > > There are other places inside OuterLayout where we set mIsClosed. I > originally thought they should all call through here (i.e. they shold use > the isClosed accessor instead of modifying it themselves to keep any other > state consistent). But this doesn't just set the closed state, it also > aborts panning, so that was a bad idea. > > > > There is a case here where we won't dispatch a change. Do we want it? Also, > > > don't forget braces. > > > > I left the same code (including the lack of braces) as before the patch: > > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabs/ > > TabsPanel.java#451 > > Hmm.. Lets leave it then. Thanks :) Patch coming soon (or later)

Here we go

Comment on attachment 8488460 [details] [diff] [review] bug-909434-fix Review of attachment 8488460 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for delaying this. I'll try to look it over soon, but since lucas's rewrite landed, I have a feeling there's some bitrot here. I asked him to look, so this is a ping.

Comment on attachment 8488460 [details] [diff] [review] bug-909434-fix Review of attachment 8488460 [details] [diff] [review]: ----------------------------------------------------------------- I think this looks really good. I have some ideas/comments. Feel free to respond/argue back if you want. Also, like I said earlier, I'm sure this is bitrotted now. Thats my fault. Trying to adapt to more reviews lately, but still not good at it. Sorry :( I'm removing lucas again. I don't mind pinging him on this, but I"m not going to hold it up for his review. I'll try just setting a needinfo on the bug. ::: mobile/android/base/BrowserApp.java @@ +136,5 @@ > HomePager.OnNewTabsListener, > OnUrlOpenListener, > ActionModeCompat.Presenter, > + LayoutInflater.Factory, > + OuterLayout.DragCallback { So I really imagined this would be an inner non-static class in here. private class DragHelper implements DragCallback { } or something like that. @@ +239,5 @@ > > + private final int HORIZONTAL_DRAG_AREA = 256; // size in dip of horizontal draggable area. > + private int[] mToolbarLocation = new int[2]; // to avoid creation every time we need to check for toolbar location. > + private int mHorizontalDragLeftBound = 0; > + private int mHorizontalDragRightBound = 0; The we can at least encapsulate some of these things in that class. @@ +568,5 @@ > + v.setLeft(oldLeft); > + v.setRight(oldRight); > + } > + } > + }); But because its not a static class, we could do some of this stuff in its constructor. I don't think its worth making it non-static here (or trying to pull it out of BrowserApp) because it requires too much specific knowledge about our app.... @@ +671,5 @@ > setDynamicToolbarEnabled(enabled); > } > }); > > + mOuterLayout.setDraggable(this); setDragListener @@ +1298,5 @@ > }); > } > > + private boolean isSideBar() { > + return (HardwareUtils.isTablet() && getOrientation() == Configuration.ORIENTATION_LANDSCAPE); This probably needs a check for the new tablet ui in it. @@ +1609,5 @@ > + public void onLayoutChange(View v, int left, int top, int right, int bottom, int oldLeft, int oldTop, int oldRight, int oldBottom) { > + mOuterLayout.updateDragHelperParameters(); > + updateHorizontalBounds(); > + } > + }); This could go in the constructor too. @@ +3152,5 @@ > appLocale, > previousSession); > } > + > + public void showPanel() { Does this just get us ready to show the panel? If so, can we name it prepareToShowTabs() or something? @@ +3157,5 @@ > + if (ensureTabsPanelExists()) { > + // If we've just inflated the tabs panel, only show it once the current > + // layout pass is done to avoid displayed temporary UI states during > + // relayout. > + ViewTreeObserver vto = mTabsPanel.getViewTreeObserver(); Make things final if you can @@ +3196,5 @@ > + * restore the position of mainlayout as if it was opened by pressing the button. This allows the closing > + * mechanism to work. > + */ > + @Override > + public void onStartFromOpenPosition() { This naming confuses me. I'm also not sure if this needs to be part of the interface. i.e. can we put this in the onDragEnd listener somehow? @@ +3269,5 @@ > + } > + > + @Override > + public boolean canDrag(MotionEvent event) { > + if (Tabs.getInstance().getSelectedTab() == null) { // if no current tab is active. Put these comments on their own line. ::: mobile/android/base/OuterLayout.java @@ +11,5 @@ > +import android.view.View; > +import android.widget.LinearLayout; > +import android.widget.RelativeLayout; > + > +public class OuterLayout extends RelativeLayout { Add a really brief comment at the top here describing what this file does. @@ +26,5 @@ > + public static interface DragCallback { > + public void onStartFromOpenPosition(); > + public void onStartFromClosedPosition(); > + public void onStopToClosedPosition(); > + public void onStopToOpenPosition(); These four onStart/onStop<stuff> methods confuse me. This seems like startDrag/endDrag? I would rather that naming. Maybe we can just pass the state onStartDrag(State.OPEN); to them. On the other hand, does the outer layout need to know if its open or closed or can the DragCallback just keep track of that itself? @@ +114,5 @@ > + > + @Override > + public boolean tryCaptureView(View view, int i) { > + if (mIsVertical || mIsOpen) { > + return (view.getId() == R.id.main_layout); Add a comment about what's going on here. This is the one place outerlayout knows about a specific view, so it would be nice to write why :) @@ +143,5 @@ > + final float rangeToCheck = mIsVertical ? mVerticalRange : mHorizontalRange; > + final float speedToCheck = mIsVertical ? yvel : xvel; > + > + if (mDraggingBorder == 0) { > + mIsOpen = false; Do you need to call one of the onStopToClosed/OpenPosition functions here? This is probably a case where the DragCallback would think it was in the wrong state... unless it kept track of the progress passed to onDragProgress events. I'll leave it to you who knows what's "open" or "closed". @@ +212,5 @@ > + if (mDragHelper.shouldInterceptTouchEvent(event)) { > + return true; > + } > + } > + // Because while open the target layout is translated and draghelper does not catch it. Add a blank line before the comment (I try to add them after (and sometimes before) any block. @@ +254,5 @@ > + * To be called when closing the tabs from outside (i.e. when touching the main layout). > + */ > + public void setClosed() { > + mIsOpen = false; > + mDragHelper.abort(); Do you need to notify the dragCallback here? @@ +275,5 @@ > + public void reset() { > + updateOrientation(); > + if (isMoving()) { > + mDragHelper.abort(); > + mDragCallback.onStopToOpenPosition(); You might want to add null checks around these mDragCallback usages. ::: mobile/android/base/tabs/TabsPanel.java @@ +414,5 @@ > + if (showAnimation) > + dispatchLayoutChange(getWidth(), getHeight()); > + } else { > + int height = getVerticalPanelHeight(); > + dispatchLayoutChange(getWidth(), height); Not you, but I hate that we call dispatchLayoutChange to animate showing something... @@ +491,4 @@ > > + public void hideImmediately() { > + mVisible = false; > + setVisibility(View.INVISIBLE); Does this need to scroll things back? @@ +554,5 @@ > + } > + } > + > + public void prepareSidebarAnimation(int tabsPanelWidth) { > + if (mVisible) { Do you need the mVisible check in here? I would avoid it, since it will force things to depend on the order of operations. i.e. mVisible = true; prepareSidebarAnimation() works prepareSidebarAnimation(). mVisible = true; will fail... @@ +571,5 @@ > } > > if (mIsSideBar) { > final int tabsPanelWidth = getWidth(); > + prepareSidebarAnimation(tabsPanelWidth); But that probably would mean you need to leave the mVisibile check here.

Sorry for the delay.. (In reply to Wesley Johnston (:wesj) from comment #39) > Comment on attachment 8488460 [details] [diff] [review] > bug-909434-fix > > Review of attachment 8488460 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think this looks really good. I have some ideas/comments. Feel free to > respond/argue back if you want. Also, like I said earlier, I'm sure this is > bitrotted now. Thats my fault. Trying to adapt to more reviews lately, but > still not good at it. Sorry :( I'm removing lucas again. I don't mind > pinging him on this, but I"m not going to hold it up for his review. I'll > try just setting a needinfo on the bug. > > ::: mobile/android/base/BrowserApp.java > @@ +136,5 @@ > > HomePager.OnNewTabsListener, > > OnUrlOpenListener, > > ActionModeCompat.Presenter, > > + LayoutInflater.Factory, > > + OuterLayout.DragCallback { > > So I really imagined this would be an inner non-static class in here. > > private class DragHelper implements DragCallback { } > > or something like that. > Ok > @@ +3152,5 @@ > > appLocale, > > previousSession); > > } > > + > > + public void showPanel() { > > Does this just get us ready to show the panel? If so, can we name it > prepareToShowTabs() or something? > Done > @@ +3157,5 @@ > > + if (ensureTabsPanelExists()) { > > + // If we've just inflated the tabs panel, only show it once the current > > + // layout pass is done to avoid displayed temporary UI states during > > + // relayout. > > + ViewTreeObserver vto = mTabsPanel.getViewTreeObserver(); > > Make things final if you can > Ok > @@ +3196,5 @@ > > + * restore the position of mainlayout as if it was opened by pressing the button. This allows the closing > > + * mechanism to work. > > + */ > > + @Override > > + public void onStartFromOpenPosition() { > > This naming confuses me. I'm also not sure if this needs to be part of the > interface. i.e. can we put this in the onDragEnd listener somehow? > ok > @@ +3269,5 @@ > > + } > > + > > + @Override > > + public boolean canDrag(MotionEvent event) { > > + if (Tabs.getInstance().getSelectedTab() == null) { // if no current tab is active. > > Put these comments on their own line. > ok > @@ +26,5 @@ > > + public static interface DragCallback { > > + public void onStartFromOpenPosition(); > > + public void onStartFromClosedPosition(); > > + public void onStopToClosedPosition(); > > + public void onStopToOpenPosition(); > > These four onStart/onStop<stuff> methods confuse me. This seems like > startDrag/endDrag? I would rather that naming. Maybe we can just pass the > state onStartDrag(State.OPEN); to them. On the other hand, does the outer > layout need to know if its open or closed or can the DragCallback just keep > track of that itself? > I guess that would be hard (not impossible but hard) because outerlayout is the one aware of the location of its children. I'll rename those methods to onDragStart(booelan wasOpen) > @@ +114,5 @@ > > + > > + @Override > > + public boolean tryCaptureView(View view, int i) { > > + if (mIsVertical || mIsOpen) { > > + return (view.getId() == R.id.main_layout); > > Add a comment about what's going on here. This is the one place outerlayout > knows about a specific view, so it would be nice to write why :) > To be honest, that can be removed. > @@ +143,5 @@ > > + final float rangeToCheck = mIsVertical ? mVerticalRange : mHorizontalRange; > > + final float speedToCheck = mIsVertical ? yvel : xvel; > > + > > + if (mDraggingBorder == 0) { > > + mIsOpen = false; > > Do you need to call one of the onStopToClosed/OpenPosition functions here? > This is probably a case where the DragCallback would think it was in the > wrong state... unless it kept track of the progress passed to onDragProgress > events. I'll leave it to you who knows what's "open" or "closed". > the onStop / onStart class can be left as result of state change. This was a redundant / paranoid check, but you are right, it risks to pollute the state. > @@ +254,5 @@ > > + * To be called when closing the tabs from outside (i.e. when touching the main layout). > > + */ > > + public void setClosed() { > > + mIsOpen = false; > > + mDragHelper.abort(); > > Do you need to notify the dragCallback here? Nope. This has to be called whenever the MainLayout was closed by the old tap / back mechanism. > > @@ +275,5 @@ > > + public void reset() { > > + updateOrientation(); > > + if (isMoving()) { > > + mDragHelper.abort(); > > + mDragCallback.onStopToOpenPosition(); > > You might want to add null checks around these mDragCallback usages. > Done > ::: mobile/android/base/tabs/TabsPanel.java > @@ +414,5 @@ > > + if (showAnimation) > > + dispatchLayoutChange(getWidth(), getHeight()); > > + } else { > > + int height = getVerticalPanelHeight(); > > + dispatchLayoutChange(getWidth(), height); > > Not you, but I hate that we call dispatchLayoutChange to animate showing > something... > > @@ +491,4 @@ > > > > + public void hideImmediately() { > > + mVisible = false; > > + setVisibility(View.INVISIBLE); > > Does this need to scroll things back? > Nope, things were scrolled back as soon as drag started and they are already in place. > @@ +554,5 @@ > > + } > > + } > > + > > + public void prepareSidebarAnimation(int tabsPanelWidth) { > > + if (mVisible) { > > Do you need the mVisible check in here? I would avoid it, since it will > force things to depend on the order of operations. i.e. mVisible = true; > prepareSidebarAnimation() works prepareSidebarAnimation(). mVisible = true; > will fail... That's true, but I just maintaned the original behaviour. Maybe this can be handled in a different bug. > > @@ +571,5 @@ > > } > > > > if (mIsSideBar) { > > final int tabsPanelWidth = getWidth(); > > + prepareSidebarAnimation(tabsPanelWidth); > > But that probably would mean you need to leave the mVisibile check here. As before, I left the previous behaviour

Here we go. I _almost_ applied your requests, except for mTabsPanel.addOnLayoutChangeListener(new View.OnLayoutChangeListener() since mTabsPanel gets initialized lazily. I also had to take into account of the height of the systembar, since gecko_app layout has fitSystemWindow enabled. As a side effect of that, the toolbar context menu was being triggered while dragging and I had to explicitly enable / disable it. Finally, I handled the case of newTabletUi by enabling vertical dragging.

Comment on attachment 8502799 [details] [diff] [review] bug-909434-fix- new version Review of attachment 8502799 [details] [diff] [review]: ----------------------------------------------------------------- I think this looks great, but its missing OuterLayout.java :O ::: mobile/android/base/BrowserApp.java @@ +245,5 @@ > > + private DragHelper mDragHelper; > + > + private class DragHelper implements OuterLayout.DragCallback { > + private final int HORIZONTAL_DRAG_AREA = 256; // size in dip of horizontal draggable area. This should probably be a dimen. @@ +248,5 @@ > + private class DragHelper implements OuterLayout.DragCallback { > + private final int HORIZONTAL_DRAG_AREA = 256; // size in dip of horizontal draggable area. > + private int[] mToolbarLocation = new int[2]; // to avoid creation every time we need to check for toolbar location. > + private int mHorizontalDragLeftBound = 0; > + private int mHorizontalDragRightBound = 0; Some little comments about what these are would be nice. @@ +352,5 @@ > + private void updateHorizontalBounds() { > + if (getDragDirection() == LinearLayout.HORIZONTAL) { // no need otherwise. > + mHorizontalDragLeftBound = getHorizontalDragRange(); > + final float scale = getResources().getDisplayMetrics().density; > + mHorizontalDragRightBound = (int) (mHorizontalDragLeftBound + HORIZONTAL_DRAG_AREA * scale); If we made this a dimen, we could avoid some of this. I'm not sure if its faster or slower though.... Then again, this doesn't have to be fast. @@ +376,5 @@ > + if (getDragDirection() == LinearLayout.VERTICAL) { > + return isInToolbarBounds((int) event.getRawY()); > + } else { > + // in horizontal mode we can intercept always when closed, only in drag area while open. > + return (!mOuterLayout.isOpen()|| isInHorizontalDragBound(event)); space before the || @@ +392,5 @@ > + @Override > + public boolean canInterceptEventWhileOpen(MotionEvent event) { > + if (event.getActionMasked() != MotionEvent.ACTION_DOWN) { > + return false; > + } I've been putting blank lines between any blocks {} of code anymore. @@ +440,5 @@ > + private int getStatusBarHeight() { > + if (mStatusBarHeight != 0) { > + return mStatusBarHeight; > + } > + int resourceId = getResources().getIdentifier("status_bar_height", "dimen", "android"); This is neat. ::: mobile/android/base/tabs/TabsPanel.java @@ +426,5 @@ > + final boolean showAnimation = !mVisible; > + prepareToShow(panelToShow); > + if (isSideBar()) { > + if (showAnimation) > + dispatchLayoutChange(getWidth(), getHeight()); add { } @@ +555,5 @@ > public Panel getCurrentPanel() { > return mCurrentPanel; > } > > + public void setHWLayer(boolean hw) { setHWLayerEnabled(boolean enabled) ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +392,5 @@ > } > > + private int getStatusBarHeight() { > + int result = 0; > + int resourceId = getResources().getIdentifier("status_bar_height", "dimen", "android"); Oh hey, its duplicated. Can we put it somewhere common? Maybe HardwareUtils.java?.... @@ +954,5 @@ > + public void enableContextMenu() { > + contextMenuEnabled = true; > + } > + > + public void disableContextMenu() { I'd just put these in one setContextMenuEnabled(boolean enabled)

Sorry for forgetting OuterLayout, somebody must have stolen it from my patch. Here is the new patch, with the suggested changes AND OuterLayout

Comment on attachment 8505023 [details] [diff] [review] bug-909434-fix Review of attachment 8505023 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, and its early in this cycle. lets try this out. ::: mobile/android/base/BrowserApp.java @@ +757,5 @@ > mBrowserChrome = (ViewGroup) findViewById(R.id.browser_chrome); > mActionBarFlipper = (ViewFlipper) findViewById(R.id.browser_actionbar); > mActionBar = (ActionModeCompatView) findViewById(R.id.actionbar); > > + mOuterLayout = (OuterLayout) findViewById(R.id.outer_layout); GeckoApp also grabs this now: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1277 ::: mobile/android/base/resources/layout/gecko_app.xml @@ +7,5 @@ > xmlns:gecko="http://schemas.android.com/apk/res-auto" > android:layout_width="match_parent" > android:layout_height="match_parent" > + android:fitsSystemWindows="true" > + android:id="@+id/outer_layout"> There's an id on this now already. Flip yours to it, or remove it :) ::: mobile/android/base/tabs/TabsPanel.java @@ +556,5 @@ > public Panel getCurrentPanel() { > return mCurrentPanel; > } > > + public void setHWLayer(boolean enabled) { Update this name :) setHWLayerEnabled. ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +653,5 @@ > requestFocus(); > } > } > > + public void setToolBarButtonsAlpha(float alpha) { I think we should not do this when in the newTabletUI. Right now I see some strageness, but in general, I don't think we do anything in that case. @@ +941,5 @@ > setBackgroundResource(R.drawable.url_bar_bg); > editCancel.onLightweightThemeReset(); > } > + > + public void setContextMenuEnabled(boolean enabled) { You'll need to update browserApp to use this.

Fixed the last couple of nits

Comment on attachment 8505736 [details] [diff] [review] bug-909434-fix Review of attachment 8505736 [details] [diff] [review]: ----------------------------------------------------------------- Yay! I like it :) I pushed it to try as well: https://tbpl.mozilla.org/?tree=Try&rev=19fe8002c1b0

Ouch. Looks like we need to disable this on 2.3 or find a compat layer.

The problem is https://hg.mozilla.org/try/file/19fe8002c1b0/mobile/android/base/BrowserApp.java#l261: addOnLayoutChangeListener is available from api 11. I guess I can make it work on pre hc by overriding MainLayout's onLayout.

This last patch implement's poor man's onLayoutChangeListener which is supported only after version 11. Had to record the last position to restore main layout after a layout round.

Comment on attachment 8507488 [details] [diff] [review] bug-909434-fix This version relies on mainlayout's onLayout in order to intercept a layout round that happens while the layout is being dragged. I also had to use the dragged position recorder in OuterLayout since onLayout does not provide old values (as opposed to LayoutChangeListener

Comment on attachment 8507488 [details] [diff] [review] bug-909434-fix Review of attachment 8507488 [details] [diff] [review]: ----------------------------------------------------------------- Some brief comments that this is here for GB compatibility would be nice. TBH, if you wanted to do a quick pass through and throw some brief comments at the tops of classes about what they do/why they exist, I've been trying to do that more. i.e. just things like /* DragHelper receives callbacks from the ViewDragHelper and manages updating different parts of the UI during a drag. */ If you want to go full tilt and write java-docs for every method, no one will stop you, but we haven't started doing that much yet. If try looks good, feel free to upload a new patch or just mark this one. I'm out of town for a week (sorry I keep missing you on irc), so I'm just going to mark this checkin-needed. If you get to the comments changes and want to upload a new patch, go ahead :) https://tbpl.mozilla.org/?tree=Try&rev=6dc1e05d488b ::: mobile/android/base/GeckoApp.java @@ +2399,5 @@ > + @Override > + protected void onLayout(boolean changed, int left, int top, int right, int bottom) { > + super.onLayout(changed, left, top, right, bottom); > + if (mLayoutInterceptor != null) { > + mLayoutInterceptor.onLayout(); Add a comment here. I mostly want these documented so that we can remove them someday when Gingerbread dies. I wish there was a clean way to do this that used the new interfaces on ICS+ and the old ones on GB, but that's going to involve more code than I want I think. Lets do this for now, with some comments. ::: mobile/android/base/LayoutInterceptor.java @@ +5,5 @@ > + > + > +package org.mozilla.gecko; > + > +public interface LayoutInterceptor { Add a brief comment about what this file does. Something like: /** * Interface for receiving layout events from a View. Used because View.OnLayoutChangeListener * wasn't implemented in Gingerbread. **/

Grr. Bad unbitrot. Try again: https://tbpl.mozilla.org/?tree=Try&rev=00cb79e81115

(In reply to Wesley Johnston (:wesj) from comment #53) > Grr. Bad unbitrot. Try again: > Not sure what you meant by bad unbitrot \o/

Please wait until your Try run has actually succeeded before requesting checkin.

Still crashing on 2.3 , it looks like I've been abusing of onChangeLayoutListener.. need to sort that out too..

Another attempt. I had to remove the other reference to LayoutChangeListener, plus had to fix the rotate in the middle of a drag behaviour that broke.

Comment on attachment 8515122 [details] [diff] [review] bug-909434-fix Review of attachment 8515122 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I forgot this needed to run through try. (Please work!) Pushed: https://tbpl.mozilla.org/?tree=Try&rev=7e9fddd54b3a I think it looks good. We'll need to make bottom-edge dragging work when the tabs tray is open now (is that possible), but I want that in a separate bug.... ::: mobile/android/base/BrowserApp.java @@ +432,5 @@ > + } > + } > + > + public int getVerticalLowerLimit() { > + return getStatusBarHeight(); You probably only want this height on v19+ versions. ::: mobile/android/base/OuterLayout.java @@ +209,5 @@ > + > + @Override > + public boolean onInterceptTouchEvent(MotionEvent event) { > + if (mDragCallback.canDrag(event)) { > + if (mDragHelper.shouldInterceptTouchEvent(event)) { I get an NPE in this call occasionally on this 4.3 Nexus 7. I wonder if the event is null? or something else in shouldInterceptTouchEvent is failing :(

Some oranges on the 4.0 builds on try, but 2.3 looks good. Retriggered the oranges to see if they're real or not.

Month ago; status?

Sorry for the delay, I got distracted (plus fatherhood is pushing hard on me lately). New patch, where I (re-re-re) unbitrotted the code and disabled the behaviour for tablets (since it might clash with the new tablets redesign). The whole driver is the canDrag method of the DragHelper instance. Once everything is stable it should be quite easy to enable the dragging from the bottom bezel of the screen.

Heh. I'm also distracted. I pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=0460333b1984

Comment on attachment 8534053 [details] [diff] [review] bug-909434-fix Review of attachment 8534053 [details] [diff] [review]: ----------------------------------------------------------------- I'ma gonna land this :)

There are a hefty number of crashes related to this over the weekend, see dependencies. Unless they're all related to a simple fix, we should back this out.

Currently at work. I feel a bit more comfortable if we back this out since the crash are all inside ViewDragHelper and I need to figure out what I am doing wrong with that. Sorry for the inconvenience :(

Backed out: https://hg.mozilla.org/integration/fx-team/rev/7bbe830c1e88

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/7bbe830c1e88

Its pretty trivial to disable this feature. I'd like to reland and just turn it off if there are no complaints. We can tackle the NPE's in their own bugs then (and I won't have to re-review a huge patch that constantly bitrots).