759041 - Add swipe between screens in AwesomeScreen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Swiping between "tabs" is an ICS design pattern. We should support it!

Comment 1

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

Mostly working WIP. Imports Android's compatibility layer to make this work (works for me at least, but may need some tweaking for other machines?).

Curious if lucasr had any advice/ideas on a good way to organize this stuff now that we know a bit more about what we're doing....

Comment 2

[Sriram Ramasubramanian [:sriram]]

(In reply to Wesley Johnston (:wesj) from comment #1)
> Created attachment 627624 [details] [diff] [review]
> WIP Patch
> 
> Mostly working WIP. Imports Android's compatibility layer to make this work
> (works for me at least, but may need some tweaking for other machines?).
> 
> Curious if lucasr had any advice/ideas on a good way to organize this stuff
> now that we know a bit more about what we're doing....

I don't understand the reason behind hard-coded "id" values for all the tab contents. Basically this might interfere with other id's. It's better to refer id's as R.id.something.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: WIP 1/5 - Interface

I want to refactor this stuff because I hate it. So here goes. This creates an AwesomeBarTab interface that each tab can implement/extend.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: WIP 2/5 - Move AllPages to new interface

This moves the all pages tab over

Comment 6

[Wesley Johnston (:wesj)]

Attachment: WIP 3/5 - Move bookmarks tab

Comment 7

[Wesley Johnston (:wesj)]

Attachment: WIP 4/5 - Move history tab

Comment 8

[Wesley Johnston (:wesj)]

Attachment: WIP 5/5 - Cleanup AwesomebarTabs

This is just removing a bunch of stuff that isn't needed anymore at this point.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Wip 6/5 - Move click and back actions into tabs

This moves a bunch of click and back actions into the tabs... This is the part I most wanted to put up and ask if you had any feedback on Lucas. There's two more pieces I need to finish. One moves contextmenu actions over to their respective tabs and the last actually fixes this bug and implements swipe.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: WIP 1/5 - Interface

So halfway through uploading, I realized I was looking at the wrong patch set... Reuploading the first four guys here. Sorry for the bugspam....

Comment 11

[Wesley Johnston (:wesj)]

Attachment: WIP 2/5 - Move AllPages to new interface

Comment 12

[Wesley Johnston (:wesj)]

Attachment: WIP 3/5 - Move bookmarks tab

Comment 13

[Wesley Johnston (:wesj)]

Attachment: WIP 4/5 - Move history tab

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

This bug has the patches. Duping bug 715447 to this one.

Comment 16

[Peter Retzer (:pretzer)]

(In reply to Mark Finkle (:mfinkle) from comment #14)
> This bug has the patches. Duping bug 715447 to this one.

Woops sorry! I didn't realize there was a patch attached.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch 1/8

Get the ball rolling with reviews here too. This is the initial interface. It changes slightly before I'm done, but this is a good enough base for most of the initial refactoring.

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Path 2/8 - Move All tabs page

And now we move each tab to use this... one by one.

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Path 3/8 - Move bookmarks tab

Comment 20

[Wesley Johnston (:wesj)]

Attachment: Patch 4/8 - Move history tab

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Path 5/8 - Some cleanup

This is some cleanup we can do now that everything is out of AwesomeBarTabs. We can actually remove AwesomeBarTabs entirely, but I didn't do that here in this bug.

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch 6/8 - Move actions into tabs

This moves most of the click, back, and context menu handling stuff into the tabs. There is still some code to handle the final context menu commands in AwesomeBar. Most of it just ends up duplicated somewhere else if we try to move it.

Comment 23

[Wesley Johnston (:wesj)]

Attachment: Patch 7/8 - Add compatbility librarires to the build

This builds Fennec with the Android compatibility libraries. I assume I did this wrong, so I'm kinda looking for advice on the right way.

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Patch 8/8 - Use a ViewPager

This actually uses a view pager to swipe between tabs. I also modified how click handling was being done because we no longer use the inflater with this.

Comment 25

[Wesley Johnston (:wesj)]

Several things I think can also be done.

1.) Kill AwesomeBarTabs including removing the TabHost from our layout code. We don't need it anymore.

2.) All the search service stuff should just be built into AllPagesTab I think. Maybe we can do that in a way that's more general so that its easy for other services to send things to the awesome screen....

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 629414 [details] [diff] [review]
Patch 7/8 - Add compatbility librarires to the build

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

looks basically right, some nits

::: config/android-common.mk
@@ +22,5 @@
>  endif
>  
>  # For Android, this defaults to $(ANDROID_SDK)/android.jar
>  ifndef JAVA_BOOTCLASSPATH
> +  JAVA_BOOTCLASSPATH = $(ANDROID_SDK)/android.jar:$(ANDROID_SDK)/../../extras/android/support/v4/android-support-v4.jar

I think you need to add this to JAVA_BOOTCLASSPATH even if its defined externally. Also, add a variable for the android support path. So:

ifndef JAVA_BOOTCLASSPATH
JAVA_BOOTCLASSPATH = $(ANDROID_SDK)/android.jar
endif

ifndef JAVA_SUPPORT_LIB_PATH
JAVA_SUPPORT_LIB_PATH = $(ANDROID_SDK)/../../extras/android/support/v4/android-support-v4.jar
endif

JAVA_BOOTCLASSPATH+=":$(JAVA_SUPPORT_LIB_PATH)"

::: mobile/android/base/Makefile.in
@@ +891,5 @@
>        $(JAVAC) $(JAVAC_FLAGS) $(javac-xlint) \
>          -d classes -classpath classes \
>          $(filter-out $(classes-dex-preqs-excl),$?)\
>      ))
> +	$(DX) --dex --output=$@ classes $(ANDROID_SDK)/../../extras/android/support/v4/android-support-v4.jar

use JAVA_SUPPORT_LIB_PATH here

Comment 27

[Lucas Rocha (:lucasr)]

Comment on attachment 629407 [details] [diff] [review]
Patch 1/8

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

I'd like to see a new patch with the new view holder bits from Brian's changes to AwesomeBarTabs.java. But looks generally good.

::: mobile/android/base/awesomebar/AwesomeBarTab.java
@@ +14,5 @@
> +    abstract public int       getTitleStringId();
> +    abstract public String    getTabName();
> +    abstract public View      getView();
> +    abstract public void      destroy();
> +    abstract public void      filter(String searchTerm);

nit: I generally prefer to not align members like this because it doesn't scale well for long type names later. No big deal.

@@ +24,5 @@
> +    public AwesomeBarTab(Context context) {
> +        mContext = context;
> +    }
> +
> +    protected class ViewHolder {

I guess you'll have to rebase to use the new view holder bits that Brian added as part of the search suggestions bits.

Comment 28

[Lucas Rocha (:lucasr)]

Comment on attachment 629408 [details] [diff] [review]
Path 2/8 - Move All tabs page

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

Commit message should probably be something like "Factor out All pages tab from AwesomeBarTabs" for clarity. Patch looks really nice. Just want to see a new rebased version for a final review.

::: mobile/android/base/AwesomeBar.java
@@ +201,5 @@
>                  }
>              }
>          });
>  
> +        registerForContextMenu(mAwesomeTabs.mAllPagesTab.getView());

Why not just add a getAllPagesTab() public method that returns mAllPagesTab.getView() instead?

::: mobile/android/base/AwesomeBarTabs.java
@@ +72,5 @@
>      private BookmarksQueryTask mBookmarksQueryTask;
>      private HistoryQueryTask mHistoryQueryTask;
> +
> +    // XXX - this should not be public when we are done!
> +    public AllPagesTab mAllPagesTab;

See my previous comment.

@@ +657,5 @@
> +    private TabSpec addAwesomeTab(String id, int titleId, final AwesomeBarTab awesomeTab) {
> +        TabSpec tab = getTabSpec(id, titleId);
> +        tab.setContent(new TabHost.TabContentFactory() {
> +            public View createTabContent(String tag) {
> +                final ListView list = (ListView)awesomeTab.getView();

Given that getView will always be a ListView anyway, why not just change the interface to be getListView()? It would make the API more specific and explicit.

nit: space between the cast and the method call.

@@ +709,2 @@
>  
> +        addAwesomeTab(mAllPagesTab.getTabName(),

Now that I see this call in context, maybe it should just be called getId() (instead of getName()) as it better matches what its semantics.

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +40,5 @@
> +
> +public class AllPagesTab extends AwesomeBarTab {
> +    public static final String LOGTAG = "ALL_PAGES";
> +    public int getTitleStringId() { return R.string.awesomebar_all_pages_title; }
> +    public String getTabName() { return "allPages"; }

I personally don't like this kind of "inline" methods in Java code. Just indent and place those methods just like any other below.

@@ +47,5 @@
> +    public AllPagesTab(Context context) {
> +        super(context);
> +    }
> +
> +    private ListView mView = null;

Declare all properties together at the top.

@@ +77,5 @@
> +    public boolean onBackPressed() {
> +        return false;
> +    }
> +
> +    private AwesomeBarCursorAdapter mCursorAdapter = null;

Ditto: move all property declarations to the top of the class.

@@ +160,5 @@
> +                viewHolder.starView = (ImageView) convertView.findViewById(R.id.bookmark_star);
> +
> +                convertView.setTag(viewHolder);
> +            } else {
> +                viewHolder = (ViewHolder) convertView.getTag();

I think you'll need to rebase to account for the changes from Brian here too.

::: mobile/android/base/awesomebar/AwesomeBarTab.java
@@ +23,3 @@
>  abstract public class AwesomeBarTab {
>      abstract public int       getTitleStringId();
>      abstract public String    getTabName();

This method could probably be just getName(). The "tab" in the name is redundant.

@@ +41,5 @@
>          public ImageView faviconView;
>          public ImageView starView;
>      }
> +
> +    private LayoutInflater mInflater = null;

Same here. Declare at the top of the class.

@@ +49,5 @@
> +        }
> +        return mInflater;
> +    }
> +
> +    private ContentResolver mContentResolver = null;

Declare all properties at the top of the class.

@@ +57,5 @@
> +        }
> +        return mContentResolver;
> +    }
> +
> +    private Resources mResources;

Ditto.

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 629409 [details] [diff] [review]
Path 3/8 - Move bookmarks tab

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

This will probably need a rebase to account for the reading list changes. Needs a few small fixes. But looks great! Giving r- to see a rebased/cleaned up version for a final review.

::: mobile/android/base/AwesomeBar.java
@@ +463,5 @@
>  
>              Cursor cursor = (Cursor) selectedItem;
>  
>              // Don't show the context menu for folders
> +            if (!(list == mAwesomeTabs.mBookmarksTab.getView() &&

Maybe create a local variable holding the bookmarks tab view and use it everywhere here? Just to avoid calling getView multiple times and making the code a bit cleaner.

::: mobile/android/base/AwesomeBarTabs.java
@@ +447,2 @@
>          TabSpec tab = getTabSpec(id, titleId);
> +        tab.setContent(factory);

You're going back and forth a bit with this addAwesomeTab method. Change it once with the final code in the first "all pages tab" patch and keep it unchanged in the following patches.

@@ +502,5 @@
>      private void addBookmarksTab() {
>          Log.d(LOGTAG, "Creating Bookmarks tab");
>  
> +        if (mBookmarksTab == null) {
> +            mBookmarksTab = new BookmarksTab(mContext);

Why do you need to handle null here? Isn't addBookmarksTab called only once anyway?

@@ +518,5 @@
> +                                   }
> +                               });
> +                               return list;
> +                           }
> +                      });

Just checking: the lazy loading of tabs will now rely on the factory only creating the tab content once the respective tab is selected, right? Make sure we're not creating all tabs on awesome screen startup. We don't want 3 queries running at the same time here.

@@ +548,5 @@
>                  (Map<String,Object>) mHistoryAdapter.getChild(groupPosition, childPosition);
>  
>          String url = (String) historyItem.get(URLColumns.URL);
>  
> +        if (!TextUtils.isEmpty(url) && mUrlOpenListener != null)

This change looks unrelated to this patch. Move it to another patch.

::: mobile/android/base/awesomebar/BookmarksTab.java
@@ +40,5 @@
> +    public BookmarksTab(Context context) {
> +        super(context);
> +    }
> +
> +    private ListView mView = null;

Move all property declarations to the top of the class.

@@ +65,5 @@
> +        if (cursor != null)
> +            cursor.close();
> +    }
> +
> +    public void filter(String searchTerm) {

Do you really need this here? filter() method is only called in the AllPagesTab. No need to be part of the common interface.

@@ +73,5 @@
> +    public boolean onBackPressed() {
> +        return false;
> +    }
> +
> +    private BookmarksListAdapter mCursorAdapter = null;

Ditto for property declarations.

@@ +108,5 @@
> +        }
> +        return mCursorAdapter;
> +    }
> +
> +    private BookmarksQueryTask mQueryTask = null;

Ditto.

Comment 30

[Lucas Rocha (:lucasr)]

Comment on attachment 629410 [details] [diff] [review]
Patch 4/8 - Move history tab

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

Looks great. Giving r- just to see a rebased version for a final review.

::: mobile/android/base/AwesomeBarTabs.java
@@ +113,1 @@
>          mInflater = (LayoutInflater) mContext.getSystemService(Context.LAYOUT_INFLATER_SERVICE);

Is mInflater still used anywhere after this change?

@@ -416,5 @@
>          addAllPagesTab();
>          addBookmarksTab();
>          addHistoryTab();
>  
> -        setOnTabChangedListener(new TabHost.OnTabChangeListener() {

If you remove that we won't dismiss the keyboard when you switch to Bookmarks and History tabs which is something we want to keep, no?

@@ +224,3 @@
>  
> +        addAwesomeTab(mHistoryTab.getTabName(),
> +                        mHistoryTab.getTitleStringId(),

Indentation is slightly off here.

::: mobile/android/base/awesomebar/HistoryTab.java
@@ +47,5 @@
> +        mContentObserver = null;
> +        mContentResolver = context.getContentResolver();
> +    }
> +
> +    private ExpandableListView mView = null;

Move all property declarations to the top of the class.

@@ +65,5 @@
> +        if (mContentObserver != null)
> +            BrowserDB.unregisterContentObserver(mContentResolver, mContentObserver);
> +    }
> +
> +    public void filter(String searchTerm) {

As I said, no need to keep this in the common tab interface.

@@ +73,5 @@
> +    public boolean onBackPressed() {
> +        return false;
> +    }
> +
> +    private HistoryListAdapter mCursorAdapter = null;

Move to the top.

@@ +79,5 @@
> +        Log.i(LOGTAG, "xxx getCursorAdapter ");
> +        return mCursorAdapter;
> +    }
> +
> +    private HistoryQueryTask mQueryTask = null;

Move to the top.

@@ +80,5 @@
> +        return mCursorAdapter;
> +    }
> +
> +    private HistoryQueryTask mQueryTask = null;
> +    protected HistoryQueryTask getQueryTask() {

This method is a bit bogus. You don't want to return an existing task in any case, do you? Why do you need it?

@@ +94,5 @@
> +            return "";
> +
> +        @SuppressWarnings("unchecked")
> +        Map<String,Object> historyItem =
> +                (Map<String,Object>) adapter.getChild(groupPosition, childPosition);

This needs to be rebased with latest changes for reading list handling.

::: mobile/android/base/resources/layout/awesomebar_tabs.xml
@@ +22,1 @@
>          </FrameLayout>

Just use /> notation on the opening tag for simplicity?

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 629411 [details] [diff] [review]
Path 5/8 - Some cleanup

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

hmm... ideally, those changes should probably be folded into the previous patches as needed. For example, the patch that removes the last call to updateFavicon should also remove the method from AwesomeBarTabs. And so on. If you really don't want to do this patch surgery, at least set a more useful commit message :-) I'll give r+ to that and let you decide.

Comment 32

[Lucas Rocha (:lucasr)]

Comment on attachment 629413 [details] [diff] [review]
Patch 6/8 - Move actions into tabs

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

I think the common interface should contain common APIs.

::: mobile/android/base/AwesomeBarTabs.java
@@ +56,5 @@
> +        }
> +        return null;
> +    }
> +
> +    public AwesomeBarTab getAwesomeBarTabForView(View view) {

A better approach would probably be to setTag() on the tab view with the tab name (or id as I suggested before) and just do something like this here:

String tag = (String) view.getTag();
for (AwesomeBarTab tab : tabs) {
    if (tag.equals(tab.getTabName())) {
        return tab;
    }
}
return null;

This way you'd avoid to call getView() on each tab here.

@@ +102,5 @@
>          // This should be called before adding any tabs
>          // to the TabHost.
>          setup();
>  
> +        tabs = new AwesomeBarTab[] {

mTabs?

@@ +178,5 @@
>          // Don't let the tab's content steal focus on tab switch
>          setDescendantFocusability(ViewGroup.FOCUS_BLOCK_DESCENDANTS);
>  
>          // Ensure the 'All Pages' tab is selected
> +        setCurrentTabByTag(tabs[0].getTabName());

Code became a bit less obvious but ok...

@@ +188,5 @@
>          int tabsVisibility = (searchTerm.length() == 0 ? View.VISIBLE : View.GONE);
>          getTabWidget().setVisibility(tabsVisibility);
>  
>          // Perform the actual search
> +        getCurrentAwesomeBarTab().filter(searchTerm);

This is wrong. You only want to run search on all pages tab. As I said, filter() should not be part of the common interface.

@@ +195,5 @@
>      public void setSearchEngines(final JSONArray engines) {
>          GeckoAppShell.getMainHandler().post(new Runnable() {
>              public void run() {
> +                for (AwesomeBarTab tab : tabs) {
> +                    tab.setSearchEngines(engines);

Again, you should not add common interface for things that are not share by all tabs. If we know beforehand that setSearchEngines only make sense on AllPagesTab, only have it there and do a specific casting here. Same for filter.

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +57,5 @@
>      private ListView mView = null;
>      public View getView() {
>          if (mView == null) {
>              mView = new ListView(mContext, null, R.style.AwesomeBarList);
> +            ((Activity)mContext).registerForContextMenu(mView);

nit: space between the casting and the variable.

@@ +245,5 @@
> +            Cursor cursor = (Cursor) item;
> +            return cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL));
> +        }
> +
> +        return (String)item;

nit: space between the casting and the variable

Comment 33

[Lucas Rocha (:lucasr)]

Comment on attachment 629415 [details] [diff] [review]
Patch 8/8 - Use a ViewPager

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

I'm a bit concerned about the coupling between AwesomeBar and the tabs. I'd prefer a listener-based approach (like we had before).

::: mobile/android/base/AwesomeBarTabs.java
@@ +43,2 @@
>  
> +    private class AwesomePagerAdapter extends PagerAdapter {

nit: I'd prefer to have inner classes at the end of the class definition.

@@ +48,5 @@
> +
> +        public Object instantiateItem(ViewGroup group, int index) {
> +            if (mSearching)
> +                index = 0;
> +            AwesomeBarTab tab = tabs[index];

I think I mentioned that before but tabs should probably be mTabs for clarity and consistency.

@@ +145,5 @@
> +
> +        mViewPager = (ViewPager) findViewById(R.id.tabviewpager);
> +        mPagerAdapter = new AwesomePagerAdapter();
> +        mViewPager.setAdapter(mPagerAdapter);
> +        mViewPager.setCurrentItem(0);

Do you really have to explicitly set it to 0?

@@ +177,5 @@
> +        TabWidget tabWidget = (TabWidget) findViewById(android.R.id.tabs);
> +        tabWidget.addView(indicatorView);
> + 
> +        // this MUST be done after tw.addView to overwrite the listener added by tabWidget
> +        // which delegates to TabHost (which we don't have)

Isn't AwesomeBarTabs a TabHost itself?

@@ +205,5 @@
>      public void filter(String searchTerm) {
>          // Don't let the tab's content steal focus on tab switch
> +        AwesomeBarTab tab = getCurrentAwesomeBarTab();
> +        ViewGroup group = (ViewGroup)tab.getView();
> +        group.setDescendantFocusability(ViewGroup.FOCUS_BLOCK_DESCENDANTS);

This was setting focus behaviour on AwesomeBarTabs as a whole and now you're making change the focus behaviour just for the list view. Is that intended? Why?

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +62,5 @@
>  
> +            mView.setOnItemClickListener(new AdapterView.OnItemClickListener() {
> +                public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> +                    String url = handleItemClick(position);
> +                    ((AwesomeBar)mContext).handleItemClick(url, false);

bit: space between casting and variable.

::: mobile/android/base/awesomebar/BookmarksTab.java
@@ +56,5 @@
>  
> +            mView.setOnItemClickListener(new AdapterView.OnItemClickListener() {
> +                public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> +                    String url = handleItemClick(parent, view, position, id);
> +                    ((AwesomeBar)mContext).handleItemClick(url, false);

The not-so-explicit coupling between AwesomeBar and the tabs is a bit bad. Why not just keep the callback-based interaction between tabs and awesome bar?

Comment 34

[Lucas Rocha (:lucasr)]

Wes, in terms of landing strategy, I suggest you to submit a build with your patches applied to QA before landing just to make sure we spot any obvious issues/regressions. This is a rather risky set of changes so we better play safe.

Comment 35

[Wesley Johnston (:wesj)]

Attachment: Patch 1/10 - Interface

This is a generic awesomebar tab interface

Comment 36

[Wesley Johnston (:wesj)]

Attachment: Patch 2/10 - Move all pages tab to interface

Move the all pages tab.

Comment 37

[Wesley Johnston (:wesj)]

Attachment: Patch 3/10 - Bookmarks tab

Comment 38

[Wesley Johnston (:wesj)]

Attachment: Patch 1/10 - Interface

Whoops. Wrong patch queue. This should be right.

Comment 39

[Wesley Johnston (:wesj)]

Attachment: Patch 2/10 - Move all pages tab to interface

Comment 40

[Wesley Johnston (:wesj)]

Attachment: Patch 4/10 - History tab

Comment 41

[Wesley Johnston (:wesj)]

Attachment: Patch 5/10 - Move back and context menu coommands into tabs

Comment 42

[Wesley Johnston (:wesj)]

Attachment: Patch 6/10 - Fix context menus in all pages

This fixes bug 763036. I just did it here because its easy to do now.

Comment 43

[Wesley Johnston (:wesj)]

Attachment: Patch 7/10 - Tests for basic functionality

These are tests of the basic functionality I added. Tested to all pass with or without these patches (except for failures from bug 763036).

Comment 44

[Wesley Johnston (:wesj)]

Attachment: Patch 8/10 - Compatibility pieces

Need to update these for nits, but just uploading to keep the set complete.

Comment 45

[Wesley Johnston (:wesj)]

Attachment: Patch 9/10 - Add swipe between tabs

This is the actual swipe patch.

Comment 46

[Wesley Johnston (:wesj)]

Attachment: Patch 10/10 - Tests for swiping

And finally some tests for swiping.

Comment 47

[Lucas Rocha (:lucasr)]

Comment on attachment 631812 [details] [diff] [review]
Patch 2/10 - Move all pages tab to interface

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

Nice and clean.

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +87,5 @@
> +    public String getTag() {
> +        return TAG;
> +    }
> +
> +    public ListView getView() {

Given that you're assuming the view is a ListView, I'd probably rename this to getListView() to make it more explicit.

Comment 48

[Lucas Rocha (:lucasr)]

Comment on attachment 631810 [details] [diff] [review]
Patch 3/10 - Bookmarks tab

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

Looks great. Commit message should be more descriptive: "Move bookmarks tab out of AwesomeBarTabs" or something.

::: mobile/android/base/AwesomeBar.java
@@ +486,5 @@
>      @Override
>      public void onCreateContextMenu(ContextMenu menu, View view, ContextMenuInfo menuInfo) {
>          super.onCreateContextMenu(menu, view, menuInfo);
>          ListView list = (ListView) view;
> +        boolean isBookmarksList = list == mAwesomeTabs.mBookmarksTab.getView();

I'd enclose this in parens for better readability: (list == ...)

Comment 49

[Lucas Rocha (:lucasr)]

Comment on attachment 631810 [details] [diff] [review]
Patch 3/10 - Bookmarks tab

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

Looks great. Commit message should be more descriptive: "Move bookmarks tab out of AwesomeBarTabs" or something.

::: mobile/android/base/AwesomeBar.java
@@ +486,5 @@
>      @Override
>      public void onCreateContextMenu(ContextMenu menu, View view, ContextMenuInfo menuInfo) {
>          super.onCreateContextMenu(menu, view, menuInfo);
>          ListView list = (ListView) view;
> +        boolean isBookmarksList = list == mAwesomeTabs.mBookmarksTab.getView();

Won't this needlessly trigger the creation of the view? You should probably add a peekView() method to just return the current view.

I'd enclose this in parens for better readability: (list == ...)

Comment 50

[Lucas Rocha (:lucasr)]

Comment on attachment 631814 [details] [diff] [review]
Patch 4/10 - History tab

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

Looks good with questions answered and issues fixed.

::: mobile/android/base/AwesomeBar.java
@@ +487,5 @@
>          ListView list = (ListView) view;
>          boolean isBookmarksList = list == mAwesomeTabs.mBookmarksTab.getView();
>          mContextMenuSubject = null;
>  
> +        if (list == mAwesomeTabs.mHistoryTab.getView()) {

Will this getView() call trigger the creation of the list view and respective db query? I guess you want a peekView() that only returns the current view without creating it.

::: mobile/android/base/AwesomeBarTabs.java
@@ +61,3 @@
>      private AllPagesTab mAllPagesTab;
>      public BookmarksTab mBookmarksTab;
> +    public HistoryTab mHistoryTab;

I'd probably add a getter for each tab (getHistoryTab(), getBookmarksTab(), getAllPagesTab()) to avoid exposing those properties directly.

@@ -419,5 @@
> -                } else {
> -                    hideSoftInput = false;
> -                }
> -
> -                // Always dismiss SKB when changing to lazy-loaded tabs

Make sure we're not regressing SKB behavior by removing this code. Do we always dismiss keyboard when switching tabs? Remember that a tab switch will happen if you start typing on text entry while in bookmarks tab. Does the SKB stay visible in that case?

Comment 51

[Lucas Rocha (:lucasr)]

Comment on attachment 631815 [details] [diff] [review]
Patch 5/10 - Move back and context menu coommands into tabs

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

Nice. Just a tip about the patch organization: I would probably do the refactoring of storing tabs into an array in a separate patch then move back and context menu coommands into tabs in another. No need to rearrange the patches, just a tip for future patches.

::: mobile/android/base/AwesomeBarTabs.java
@@ +193,5 @@
> +    public AllPagesTab getAllPagesTab() {
> +        return (AllPagesTab)getAwesomeBarTabForTag("allPages");
> +    }
> +
> +    public AllPagesTab getBookmarksTab() {

I guess you meant BookmarksTab here.

@@ +194,5 @@
> +        return (AllPagesTab)getAwesomeBarTabForTag("allPages");
> +    }
> +
> +    public AllPagesTab getBookmarksTab() {
> +        return (AllPagesTab)getAwesomeBarTabForTag("bookmarks");

Ditto.

@@ +197,5 @@
> +    public AllPagesTab getBookmarksTab() {
> +        return (AllPagesTab)getAwesomeBarTabForTag("bookmarks");
> +    }
> +
> +    public AllPagesTab getHistoryTab() {

I guess you meant HistoryTab here.

@@ +198,5 @@
> +        return (AllPagesTab)getAwesomeBarTabForTag("bookmarks");
> +    }
> +
> +    public AllPagesTab getHistoryTab() {
> +        return (AllPagesTab)getAwesomeBarTabForTag("history");

Ditto.

@@ +257,5 @@
>          });
>      }
>  
>      public boolean isInReadingList() {
> +        BookmarksTab tab = (BookmarksTab)mTabs[1];

Just use getBookmarksTab() here?

Comment 52

[Lucas Rocha (:lucasr)]

Comment on attachment 631816 [details] [diff] [review]
Patch 6/10 - Fix context menus in all pages

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

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +526,1 @@
>              Log.e(LOGTAG, "item at " + info.position + " is not a Cursor");

This log message doesn't make sense as is anymore. Maybe it should be something like: "item at position X is a search item"

Comment 53

[Lucas Rocha (:lucasr)]

Comment on attachment 631817 [details] [diff] [review]
Patch 7/10 - Tests for basic functionality

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

\o/

::: mobile/android/base/tests/testAllPagesTab.java
@@ +6,5 @@
> +/* Tests opening the all pages tab, that items look correct, clicking on an item
> +   and long tapping on an item
> +*/
> +
> +public class testAllPagesTab extends BaseTest {

Strange duplicate file? I guess this shouldn't be here?

::: mobile/android/base/tests/testBookmarksTab.java.in
@@ +21,5 @@
> +import java.util.Arrays;
> +import java.util.ArrayList;
> +import java.io.File;
> +
> +/* Tests opening the all pages tab, that items look correct, clicking on an item

bookmarks tab

Comment 54

[Lucas Rocha (:lucasr)]

Comment on attachment 631818 [details] [diff] [review]
Patch 8/10 - Compatibility pieces

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

I think this patch can be merged with 9/10.

Comment 55

[Lucas Rocha (:lucasr)]

Comment on attachment 631819 [details] [diff] [review]
Patch 9/10 - Add swipe between tabs

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

This patch has several changes that feel a bit unrelated to its core intent. I'm giving r+ for the changes but I think some changes in the patch should be merged into their respective previous patches where applicable.

::: mobile/android/base/AwesomeBarTabs.java
@@ +89,5 @@
> +            return tab;
> +        }
> +
> +        public void destroyItem(ViewGroup group, int index, Object obj) {
> +            AwesomeBarTab tab = (AwesomeBarTab)obj;

style nit: I think we usually have space between the cast and the variable. Same nit applies too all previous patches. Not a big deal though.

@@ +116,5 @@
>      }
>  
>      public AwesomeBarTab getAwesomeBarTabForTag(String tag) {
>          for (AwesomeBarTab tab : mTabs) {
> +            if (tag.equals(tab.getTag())) {

Shouldn't this be fixed in the patch that added this code?

@@ +171,5 @@
> +
> +        mViewPager = (ViewPager) findViewById(R.id.tabviewpager);
> +        mPagerAdapter = new AwesomePagerAdapter();
> +        mViewPager.setAdapter(mPagerAdapter);
> +        mViewPager.setCurrentItem(0);

Let me breath: add an empty line here :-)

@@ +247,5 @@
>          return (AllPagesTab)getAwesomeBarTabForTag("allPages");
>      }
>  
> +    public BookmarksTab getBookmarksTab() {
> +        return (BookmarksTab)getAwesomeBarTabForTag("bookmarks");

This should be fixed in the patch that added this code.

@@ +252,4 @@
>      }
>  
> +    public HistoryTab getHistoryTab() {
> +        return (HistoryTab)getAwesomeBarTabForTag("history");

Same here.

@@ +272,5 @@
>          getTabWidget().setVisibility(tabsVisibility);
>  
>          // Perform the actual search
>          allPages.filter(searchTerm);
> +        mViewPager.setCurrentItem(0);

Won't the setCurrentTabByTag() above ensure that the pager moves to the first page? Why do you need this call here?

@@ +321,5 @@
>      public boolean onInterceptTouchEvent(MotionEvent ev) {
>          // we should only have to hide the soft keyboard once - when the user
>          // initially touches the screen
>          if (ev.getAction() == MotionEvent.ACTION_DOWN)
> +            hideSoftInput(mViewPager);

Why is this necessary?

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +89,5 @@
> +            mView.setOnItemClickListener(new AdapterView.OnItemClickListener() {
> +                public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> +                     handleItemClick(parent, view, position, id);
> +                }
> +            });

Shouldn't this be in the patch that adds AllPagesTab?

@@ +113,5 @@
>              cursor.close();
>      }
>  
>      public void filter(String searchTerm) {
> +        Log.i(LOGTAG, "xxx - filter");

xxx?

@@ +279,5 @@
>          }
>  
>          @Override
>          public View getView(int position, View convertView, ViewGroup parent) {
> +            Log.i(LOGTAG, "Get View at " + position);

Is this debugging log still needed?

::: mobile/android/base/awesomebar/AwesomeBarTab.java
@@ +32,5 @@
>  abstract public class AwesomeBarTab {
>      abstract public String getTag();
>      abstract public int getTitleStringId();
>      abstract public void destroy();
> +    abstract public ListView getView();

This change should be merged with the very first patch, no?

::: mobile/android/base/awesomebar/BookmarksTab.java
@@ +68,5 @@
> +            mView.setOnItemClickListener(new AdapterView.OnItemClickListener() {
> +                public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> +                    handleItemClick(parent, view, position, id);
> +                }
> +            });

Shouldn't this change be done in the patch that adds BookmarksTab?

::: mobile/android/base/awesomebar/HistoryTab.java
@@ +84,5 @@
> +            mView.setOnGroupClickListener(new ExpandableListView.OnGroupClickListener() {
> +                 public boolean onGroupClick(ExpandableListView parent, View v, int groupPosition, long id) {
> +                    return true;
> +                }
> +            });

Same here. Should be done in the patch that adds HistoryTab, no?

Comment 56

[Lucas Rocha (:lucasr)]

Comment on attachment 631820 [details] [diff] [review]
Patch 10/10 - Tests for swiping

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

Nice!

Comment 57

[Wesley Johnston (:wesj)]

Some issues on try getting the compatibility libraries to work, but I pushed the first 7 pieces to avoid bitrot pain:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b179d721bcb7
https://hg.mozilla.org/integration/mozilla-inbound/rev/f910da776f62
https://hg.mozilla.org/integration/mozilla-inbound/rev/504f24dee4c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f96467a448ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a03ae08ea0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ccff407561b
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b071b958f61

Comment 58

[Wesley Johnston (:wesj)]

Please leave this open when it lands on central.

Comment 59

[Wesley Johnston (:wesj)]

Sorry for the delay here. This caused a few test failures, I think because the tests aren't cleaning up correctly. Trying to reproduce locally...

For the compatibility stuff, mfinkle suggested, and I agree that maybe thats taking in more code than we want. I looked into using this on ICS only without the compatibility library, but turns out that's not possible either. So my new plan is to take the source for ViewPager and PagerAdapter (ship with the SDK), remove everything we don't need (which is most of them) and use those classes. At that point, they're actually pretty small manageable classes.

Comment 60

[Wesley Johnston (:wesj)]

These looked fine on try (finally). Leave open for the rest to land once I've had a chance to rewrite them.

https://hg.mozilla.org/integration/mozilla-inbound/rev/956bbba79fd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/36a9ced2cf3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/583d8cbc26bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/6031408381fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/379c9b4964d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb5ea3d1940a
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc563de4ff0e

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/956bbba79fd4
https://hg.mozilla.org/mozilla-central/rev/36a9ced2cf3e
https://hg.mozilla.org/mozilla-central/rev/583d8cbc26bc
https://hg.mozilla.org/mozilla-central/rev/6031408381fa
https://hg.mozilla.org/mozilla-central/rev/379c9b4964d0
https://hg.mozilla.org/mozilla-central/rev/cb5ea3d1940a
https://hg.mozilla.org/mozilla-central/rev/fc563de4ff0e

Comment 62

[Wesley Johnston (:wesj)]

Attachment: Compat patch updated

Comment 63

[Wesley Johnston (:wesj)]

Attachment: Swipe patch updated

I finally got around to looking at the impact of this on our apk. For me on linux builds we go from:

22,399,946 bytes without patches
22,502,807 bytes with patches

i.e. 100k. I'm looking at just pulling out the classes we need and minimizing code in them and seeing what can be done there. Its not hard, but not trivial. Will post again when I know. If this 100k is ok, we can land this for now....

Comment 64

[Sriram Ramasubramanian [:sriram]]

I'm not sure if this is the right approach to do.
1. It creates a ListView every time I swipe
   This also needs to populate the ListView -- with necessary DB queries
2. An addition ViewPager class from Android code.

The other approach would be to add the ListViews to a FrameLayout and have the swipe passed to FrameLayout -- if the ListView doesnt want it. Basically vertical swipes will be used by ListView and the horizontal ones by the FrameLayout.

I've worked on something like this in the past -- and proved to work well.

Comment 65

[Wesley Johnston (:wesj)]

ListViews should be cached somewhat by the ViewPager, so we really just only create them when they're needed. Same as our current model. There's some variables in ViewPager to control how many are cached (right now its one I think). We can try tweaking it.

ViewPager mostly just controls the swipe gesture. Figuring out where you are in the swipe, whether or not to snap to the next screen. etc. Its got a bunch of other code. Accessibility stuff we probably want. Margin drawables stuff we don't as well as fancy blue highlight overscroll stuff I don't care about either. I'm gonna try and trim it down to something a bit more managable, but I think we'd end up writing most of the same code if we wrote our own version of this using a framelayout.

Comment 66

[Wesley Johnston (:wesj)]

Attachment: Patch - Bring in ViewPager src

Finally got back to this. This brings in the ViewPager and PagerAdapter src files for us.

Comment 67

[Wesley Johnston (:wesj)]

Attachment: Patch - Modify the src to build with Fennec

This is mostly removing compatibility pieces we don't need. In a few places I removed API 16 level stuff. In a few other places, I adjusted to keep higher level API's where possible.

There's some wiggle room in those decisions that we can talk about.

Comment 68

[Wesley Johnston (:wesj)]

Attachment: Patch - Updated use the ViewPager

And this uses it all.

Comment 69

[Wesley Johnston (:wesj)]

lucasr ping?

Comment 70

[Lucas Rocha (:lucasr)]

Comment on attachment 652196 [details] [diff] [review]
Patch - Bring in ViewPager src

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

It would be nice if you documented somewhere what you've changed in the original source for future reference. Just an overview of the changes at the head of the file would be enough I think. I'm assuming it's ok to have these files in our repo, license-wise.

::: mobile/android/base/PagerAdapter.java
@@ +13,5 @@
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +package android.support.v4.view;

Shouldn't the java package match the directory structure somehow?

Comment 71

[Lucas Rocha (:lucasr)]

Comment on attachment 652198 [details] [diff] [review]
Patch - Modify the src to build with Fennec

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

You're replacing a few *Compat references with direct APIs. Are you sure the direct APIs you're using are within our minimum Android version (>= 8 I think). Also, it would be good if marcoz and/or eeejay tried a build with this patch to see what a11y issues they find (given that you're disabling a11y-related code here). I'm giving r+ but I'd like really to hear from a11y guys how this change affects our a11y support.

::: mobile/android/base/ViewPager.java
@@ +1106,5 @@
>          if (!checkLayoutParams(params)) {
>              params = generateLayoutParams(params);
>          }
>          final LayoutParams lp = (LayoutParams) params;
> +        lp.isDecor |= false;//child instanceof Decor;

You don't need to keep original code around I think. This patch will serve as a reference of what has been changed in the original source.

@@ +1607,5 @@
>                      break;
>                  }
>  
> +                final int pointerIndex = ev.findPointerIndex(activePointerId);
> +                final float x = ev.getX(pointerIndex);

Is MotionEventCompat really not necessary here? Why?

@@ +2480,5 @@
>      public ViewGroup.LayoutParams generateLayoutParams(AttributeSet attrs) {
>          return new LayoutParams(getContext(), attrs);
>      }
>  
> +    class MyAccessibilityDelegate extends AccessibilityDelegate {

AccessibilityDelegate is only available on Android >= 14, right?

Comment 72

[Lucas Rocha (:lucasr)]

Comment on attachment 652200 [details] [diff] [review]
Patch - Updated use the ViewPager

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

Looks good in general. I'd like to see questions answered and the fix for notifyDataSetChanged() before giving a r+. Giving a r- for now.

::: mobile/android/base/AwesomeBarTabs.java
@@ +83,5 @@
>      }
>  
>      public AwesomeBarTab getAwesomeBarTabForTag(String tag) {
>          for (AwesomeBarTab tab : mTabs) {
> +            if (tag.equals(tab.getTag())) {

Seems like an unrelated fix.

@@ +144,4 @@
>  
> +        mViewPager = (ViewPager) findViewById(R.id.tabviewpager);
> +        mPagerAdapter = new AwesomePagerAdapter();
> +        mViewPager.setAdapter(mPagerAdapter);

nit: add empty line here.

@@ +146,5 @@
> +        mPagerAdapter = new AwesomePagerAdapter();
> +        mViewPager.setAdapter(mPagerAdapter);
> +        mViewPager.setOnPageChangeListener(new ViewPager.OnPageChangeListener() {
> +            public void onPageScrollStateChanged(int state) { }
> +            public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels) { }

nit: add empty line here.

@@ +264,5 @@
>          // Restore normal focus behavior on tab host
>          setDescendantFocusability(ViewGroup.FOCUS_AFTER_DESCENDANTS);
>  
>          // The tabs should only be visible if there's no on-going search
> +        mSearching = searchTerm.length() != 0;

If mSearching affects the number of items in the adapter, you are supposed to call notify notifyDataSetChanged() on the adapter here.

@@ +265,5 @@
>          setDescendantFocusability(ViewGroup.FOCUS_AFTER_DESCENDANTS);
>  
>          // The tabs should only be visible if there's no on-going search
> +        mSearching = searchTerm.length() != 0;
> +        int tabsVisibility = (!mSearching ? View.VISIBLE : View.GONE);

(mSearching ? View.GONE : View.VISIBLE) is simpler to read.

@@ +270,5 @@
>          findViewById(R.id.tab_widget_container).setVisibility(tabsVisibility);
>  
>          // Perform the actual search
>          allPages.filter(searchTerm);
> +        mViewPager.setCurrentItem(0);

Does this cause the first page to be selected with the slide animation? We probably want to avoid the animation in this case. Otherwise there will be too many moving parts in the UI at the same time (i.e. tabs hiding + sliding animation).

Comment 73

[Wesley Johnston (:wesj)]

Dang it you're right. I need to go back and fix a few of these compat things. Stupid jelly bean device making me think everything worked. We target a min of 8, so we're ok with all the motionevent, velociytracker, and ViewConfiguration changes. I thought EdgeEffect was level 11, but its 14. Other notes from digging:


configuration.getScaledPagingTouchSlop(); - level 8
MotionEvent.ACTION_MASK; - api 5
MotionEvent.findPointerIndex(activePointerId); - api 5
MotionEvent.getX/Y(pointerIndex); - api 5
MotionEvent.getPointerId(0); - api 5
MotionEvent.ACTION_POINTER_UP: - api 5
velocityTracker.getXVelocity(mActivePointerId); - api 8

EdgeEffect - level 14
setAccessibilityDelegate - level 14
postInvalidateDelayed(ValueAnimator.getFrameDelay()); - adapted to work since v11, need to do better...
v.canScrollHorizontally(-dx) - api 14
event.hasNoModifiers() - api 11
event.hasModifiers() - api 11
AccessibilityDelegate  - api 14
AccessibilityNodeInfo - api 14

Comment 74

[Aaron Train [:aaronmt]]

This broke removals from the history tab; the view is not updated on individual item removal (i.e, item-context-menu, "Remove").

Comment 76

[Lucas Rocha (:lucasr)]

IMO, we should just ship android's support library instead of patching components from it. My main concern is about maintenance. I'd really prefer to avoid having to maintain patched versions of a code that is officially maintained and supported by the Android team.

The support library is likely to help us write certain features in a backwards compatible way in the long-term. The library is updated after every Android release to cover new platform features. The added 100k to the apk doesn't seem too bad.

Comment 77

[Mark Finkle (:mfinkle) (use needinfo?)]

What other benefits (or potential benefits) does the compatibility library give us? If there was more than "swiping tabs", I'd be more tempted to take it. As it stands, "swiping tabs" feels like a luxury feature, that might not warrant the extra code.

Comment 78

[Wesley Johnston (:wesj)]

I'm just gonna post a dump of what's in there here. Its... a lot, but I don't want to run around in circles here much longer. Personally, I think this is probably worth it for this one feature (and not making us look out of date to early adopters on newer devices):

// Stuff that looks useful to me
NotificationCompat stuff for the new notification changes in Honeycomb and JB (mostly the new JB stuff as far as I can see). For instance you can use NotificationCompat.BigPictureStyle to make one of the new big types.

PagerTabStrip, PagerTitleStrip, PagerAdapter, ViewPager - this is what we're using here (not the PagerTab/TitleStrip stuff)

ViewCompat - There's a bunch of random stuff here.
  overscroll compatibility stuff added in Gingerbread (getOverScrollMode, setOverscrollMode, we don't use these right now). canScrollHorizontally/Vertically,
  setAccessibilityDelegate (plus a bunch of new Accessibility methods that were added in ICS and more that were added in JB by getImportantForAccessibility). I think we've worked around this in our frontend for now by basically rewriting this ourselves?
  There are also compatibility pieces for some animation callbacks that were added in Jellybean. for instance, postOnAnimation lets you designate a runnable to be called on each animation frame. getFrameTime to know what framerate we're running at.

AccessibilityNodeInfoCompat, AccessibilityDelegateCompat, AccessibilityDelegateCompat, AccessibilityEventCompat, AccessibilityManagerCompat, AccessibilityNodeInfoCompat, AccessibilityRecordCompat, AccessibilityServiceInfoCompat - More accessibility stuff

ViewGroupCompat - onRequestSendAccessibilityEvent from ICS. Yay for accessibility again!

ConnectivityManagerCompat - isActiveNetworkMetered might be useful for us

EdgeEffectCompat - Edge effects on old versions. We're likely going to have to do this some day.

VelocityTrackerCompat - We use the velocity tracker now for some of the new animation stuff.

SearchViewCompat - I think this helps sriram with his search integration stuff

// Stuff that's less useful IMO
Fragment support. We could supposedly use these to move some of our separate components (like the tab strip) into fragments of their own. We've worked around that to date though. This is actually in a different library than the one with the ViewPager stuff and wouldn't necessarily be brought in here.

NavUtils and TaskStackBuilder - These are some methods to handle the ordering of the task stack. our stack is so small i don't think these are that useful.

ShareCompat - This isn't as exciting as it sounds. Some stuff for share menuitems which I'm guesing don't help us with our crazy menus. The ability to find out what app shared to you.

ParcelableCompat stuff - We hardly use these, but there's some helper methods here.

Comment 79

[Mark Finkle (:mfinkle) (use needinfo?)]

I assume we would use a ViewPager in the new About:Home designs too?
http://cl.ly/image/1x0A1K0r2g2m

I buy the maintenance argument. I also assume we will try to use more of these features.

I agree to pulling in the Android Compat library.

Comment 80

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #79)
> I assume we would use a ViewPager in the new About:Home designs too?
> http://cl.ly/image/1x0A1K0r2g2m

Surely.

Comment 81

[Wesley Johnston (:wesj)]

Looks like the current build system already has this actually:

https://tbpl.mozilla.org/?tree=Try&rev=9f7873337331&pusher=[e-mail]

I'll upload some unbitrotted patches and push this to try with the robocop tests running.

Comment 82

[Wesley Johnston (:wesj)]

Attachment: Tests for swiping

Tests updated

Comment 83

[Wesley Johnston (:wesj)]

Comment on attachment 674774 [details] [diff] [review]
Tests for swiping

Carrying r+ because there are no changes here.

Comment 84

[Wesley Johnston (:wesj)]

Attachment: Bring in compat library

Bring in compat libraries. Carrying r+.

Comment 85

[Wesley Johnston (:wesj)]

Attachment: Add swipe

Use ViewPager. There have been enough changes in the AllPagesTab since this that I think it should probably be re-reviewed.

Comment 86

[Geoff Brown [:gbrown]]

Comment on attachment 674774 [details] [diff] [review]
Tests for swiping

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

::: mobile/android/base/tests/testAwesomebarSwipes.java.in
@@ +14,5 @@
> +import @ANDROID_PACKAGE_NAME@.*;
> +
> +/* Tests opening the all pages tab, that items look correct, clicking on an item
> +   and long tapping on an item
> +*/

Drive-by nit: identical to testAllPagesTab!

@@ +17,5 @@
> +   and long tapping on an item
> +*/
> +
> +public class testAwesomebarSwipes extends BaseTest {
> +    private static final String ABOUT_HOME_URL = "about:home";

Drive-by nit: ABOUT_HOME_URL is unused

Comment 87

[Lucas Rocha (:lucasr)]

Comment on attachment 674777 [details] [diff] [review]
Add swipe

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

Looks good. Please test the search suggestion animation before landing, just in case.

Comment 88

[Wesley Johnston (:wesj)]

Attachment: Bring in compat library

I had to basically wipe my machine today and when I repulled the sdk with the instructions we have on https://wiki.mozilla.org/Mobile/Fennec/Android this library is in a slightly different place. Oh the try machines its in

extras/android/compatibility/v4/android-support-v4.jar

on a new sdk pull its in

extras/android/support/v4/android-support-v4.jar

to save everyone, I rewrote this to check those two locations at build time.

Comment 89

[Wesley Johnston (:wesj)]

Attachment: Bring in compat library

Wrong patch

Comment 90

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 676392 [details] [diff] [review]
Bring in compat library

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

::: config/android-common.mk
@@ +25,5 @@
> +ifneq ($(wildcard $(ANDROID_SDK)/../../extras/android/compatibility/v4/android-support-v4.jar),)
> +ANDROID_COMPAT_LIB=$(ANDROID_SDK)/../../extras/android/compatibility/v4/android-support-v4.jar
> +else
> +ANDROID_COMPAT_LIB=$(ANDROID_SDK)/../../extras/android/support/v4/android-support-v4.jar
> +endif

move this to configure and do:
 
if test -e "$(ANDROID_SDK)/../../extras/android/compatibility/v4/android-support-v4.jar" ; then 
    ANDROID_COMPAT_LIB=$(ANDROID_SDK)/../../extras/android/compatibility/v4/android-support-v4.jar;
else
    ANDROID_COMPAT_LIB=`$(ANDROID_SDK)/../../extras/android/support/v4/android-support-v4.jar;
fi

Comment 91

[Wesley Johnston (:wesj)]

Attachment: Bring in compat library

Moved this check to the android only section of configure.in.

Comment 92

[Wesley Johnston (:wesj)]

Comment on attachment 676656 [details] [diff] [review]
Bring in compat library

This isn't right

Comment 93

[Wesley Johnston (:wesj)]

Just to update. I fixed the compatibility library, but have been battling some other test failures due to the ViewPager. I don't want to keep cycling through reviews, so I'm waiting to get that all done before I re-upload.

Comment 94

[Wesley Johnston (:wesj)]

Attachment: Compat

Fixed up patch for compatibility library. ANDROID_SDK isn't defined in configure.in, so I put this in android.m4 instead.

Comment 95

[Wesley Johnston (:wesj)]

Attachment: Add swipe

Sorry for the review churn lucas. All green on try.

Comment 96

[Wesley Johnston (:wesj)]

Attachment: Bring in compat library

Grr... missing qref.

Comment 97

[Wesley Johnston (:wesj)]

Attachment: Tests for swiping

Comment 98

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 678383 [details] [diff] [review]
Bring in compat library

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

::: build/autoconf/android.m4
@@ +242,5 @@
>      if test ! -d "$android_platform_tools" ; then
>          android_platform_tools="$android_sdk"/tools # SDK Tools < r8
>      fi
>      ANDROID_SDK="${android_sdk}"
> +    if test -e "$(android_sdk)/../../extras/android/compatibility/v4/android-support-v4.jar" ; then 

fix trailing ws

@@ +246,5 @@
> +    if test -e "$(android_sdk)/../../extras/android/compatibility/v4/android-support-v4.jar" ; then 
> +        ANDROID_COMPAT_LIB="$(android_sdk)/../../extras/android/compatibility/v4/android-support-v4.jar"
> +    else
> +        ANDROID_COMPAT_LIB="$(android_sdk)/../../extras/android/support/v4/android-support-v4.jar";
> +    fi

you should probably fail here if $(ANDROID_COMPAT_LIB) doesn't exist

same comments for js/src version obviously

Comment 99

[Lucas Rocha (:lucasr)]

Comment on attachment 678384 [details] [diff] [review]
Tests for swiping

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

Nice.

Comment 100

[Wesley Johnston (:wesj)]

Compat libraries done:

https://hg.mozilla.org/integration/mozilla-inbound/rev/258b1c1d0d20

I think I may have missed something when I qfolded my fixes into the other patches. Going to ensure they're ok before pushing.

Comment 101

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/258b1c1d0d20

Comment 102

[Wesley Johnston (:wesj)]

Attachment: Patch

Sorry for this last bit. This just moves our tests to use a waitFor test when digging out the listview to look at. I found that was failing on my slower devices. It also fixes some bitrot from the favicons fixes I think.

Try seems good with this!

Comment 103

[Lucas Rocha (:lucasr)]

Comment on attachment 680097 [details] [diff] [review]
Patch

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

Good.

Comment 104

[Wesley Johnston (:wesj)]

folded and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a14e7f19dbf
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8fed46ba90d

Comment 105

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0a14e7f19dbf
https://hg.mozilla.org/mozilla-central/rev/d8fed46ba90d

Comment 107

[Ioana Chiorean]

A TC was added in MozTrap for this on 19 phones, 19 tablets, 20 phones, 20 tablets

https://moztrap.mozilla.org/manage/case/5942/ behaviour added
https://moztrap.mozilla.org/manage/case/5943/ limitations