Closed Bug 759041 Opened 8 years ago Closed 7 years ago

Add swipe between screens in AwesomeScreen

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 19
Tracking Status
firefox19 --- verified

People

(Reporter: wesj, Assigned: wesj)

References

Details

(Whiteboard: ui-responsiveness)

Attachments

(11 files, 36 obsolete files)

40.22 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
2.20 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
49.92 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
38.33 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
30.43 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
5.34 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
26.60 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
38.24 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
4.08 KB, patch
blassey
: review+
Details | Diff | Splinter Review
3.25 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
13.49 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
Swiping between "tabs" is an ICS design pattern. We should support it!
Attached patch WIP Patch (obsolete) — Splinter Review
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....
Attachment #627624 - Flags: feedback?(lucasr.at.mozilla)
(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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 715447
Attached patch WIP 1/5 - Interface (obsolete) — Splinter Review
I want to refactor this stuff because I hate it. So here goes. This creates an AwesomeBarTab interface that each tab can implement/extend.
This moves the all pages tab over
Attached patch WIP 3/5 - Move bookmarks tab (obsolete) — Splinter Review
Attached patch WIP 4/5 - Move history tab (obsolete) — Splinter Review
Attached patch WIP 5/5 - Cleanup AwesomebarTabs (obsolete) — Splinter Review
This is just removing a bunch of stuff that isn't needed anymore at this point.
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.
Attachment #629003 - Flags: feedback?(lucasr.at.mozilla)
Attached patch WIP 1/5 - Interface (obsolete) — Splinter Review
So halfway through uploading, I realized I was looking at the wrong patch set... Reuploading the first four guys here. Sorry for the bugspam....
Attachment #628998 - Attachment is obsolete: true
Attachment #628999 - Attachment is obsolete: true
Attached patch WIP 3/5 - Move bookmarks tab (obsolete) — Splinter Review
Attachment #629000 - Attachment is obsolete: true
Attached patch WIP 4/5 - Move history tab (obsolete) — Splinter Review
Attachment #629001 - Attachment is obsolete: true
This bug has the patches. Duping bug 715447 to this one.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Duplicate of this bug: 715447
Blocks: 750167
(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.
Status: REOPENED → NEW
Attached patch Patch 1/8 (obsolete) — Splinter Review
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.
Assignee: nobody → wjohnston
Attachment #629005 - Attachment is obsolete: true
Attachment #629407 - Flags: review?(lucasr.at.mozilla)
Attached patch Path 2/8 - Move All tabs page (obsolete) — Splinter Review
And now we move each tab to use this... one by one.
Attachment #629408 - Flags: review?(lucasr.at.mozilla)
Attachment #629408 - Attachment is patch: true
Attached patch Path 3/8 - Move bookmarks tab (obsolete) — Splinter Review
Attachment #629007 - Attachment is obsolete: true
Attachment #629409 - Flags: review?(lucasr.at.mozilla)
Attached patch Patch 4/8 - Move history tab (obsolete) — Splinter Review
Attachment #629008 - Attachment is obsolete: true
Attachment #629410 - Flags: review?(lucasr.at.mozilla)
Attached patch Path 5/8 - Some cleanup (obsolete) — Splinter Review
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.
Attachment #629002 - Attachment is obsolete: true
Attachment #629411 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #629413 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #629414 - Flags: review?(blassey.bugs)
Attached patch Patch 8/8 - Use a ViewPager (obsolete) — Splinter Review
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.
Attachment #627624 - Attachment is obsolete: true
Attachment #629003 - Attachment is obsolete: true
Attachment #629006 - Attachment is obsolete: true
Attachment #627624 - Flags: feedback?(lucasr.at.mozilla)
Attachment #629003 - Flags: feedback?(lucasr.at.mozilla)
Attachment #629415 - Flags: review?(lucasr.at.mozilla)
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 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
Attachment #629414 - Flags: review?(blassey.bugs) → review+
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.
Attachment #629407 - Flags: review?(lucasr.at.mozilla) → review-
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.
Attachment #629408 - Flags: review?(lucasr.at.mozilla) → review-
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.
Attachment #629409 - Flags: review?(lucasr.at.mozilla) → review-
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?
Attachment #629410 - Flags: review?(lucasr.at.mozilla) → review-
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.
Attachment #629411 - Flags: review?(lucasr.at.mozilla) → review+
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
Attachment #629413 - Flags: review?(lucasr.at.mozilla) → review-
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?
Attachment #629415 - Flags: review?(lucasr.at.mozilla) → review-
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.
Attached patch Patch 1/10 - Interface (obsolete) — Splinter Review
This is a generic awesomebar tab interface
Attachment #629407 - Attachment is obsolete: true
Attachment #631808 - Flags: review?(lucasr.at.mozilla)
Move the all pages tab.
Attachment #629408 - Attachment is obsolete: true
Attachment #631809 - Flags: review?(lucasr.at.mozilla)
Attachment #629409 - Attachment is obsolete: true
Attachment #631810 - Flags: review?(lucasr.at.mozilla)
Whoops. Wrong patch queue. This should be right.
Attachment #631808 - Attachment is obsolete: true
Attachment #631808 - Flags: review?(lucasr.at.mozilla)
Attachment #631811 - Flags: review?(lucasr.at.mozilla)
Attachment #631809 - Attachment is obsolete: true
Attachment #631809 - Flags: review?(lucasr.at.mozilla)
Attachment #631812 - Flags: review?(lucasr.at.mozilla)
Attachment #629410 - Attachment is obsolete: true
Attachment #631814 - Flags: review?(lucasr.at.mozilla)
Attachment #629411 - Attachment is obsolete: true
Attachment #629413 - Attachment is obsolete: true
Attachment #631815 - Flags: review?(lucasr.at.mozilla)
This fixes bug 763036. I just did it here because its easy to do now.
Attachment #631816 - Flags: review?(lucasr.at.mozilla)
These are tests of the basic functionality I added. Tested to all pass with or without these patches (except for failures from bug 763036).
Attachment #631817 - Flags: review?(lucasr.at.mozilla)
Need to update these for nits, but just uploading to keep the set complete.
Attachment #631818 - Flags: review+
This is the actual swipe patch.
Attachment #631819 - Flags: review?(lucasr.at.mozilla)
Attached patch Patch 10/10 - Tests for swiping (obsolete) — Splinter Review
And finally some tests for swiping.
Attachment #631820 - Flags: review?(lucasr.at.mozilla)
Attachment #629414 - Attachment is obsolete: true
Attachment #629415 - Attachment is obsolete: true
Attachment #631811 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #631812 - Flags: review?(lucasr.at.mozilla) → review+
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 == ...)
Attachment #631810 - Flags: review?(lucasr.at.mozilla) → review+
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 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?
Attachment #631814 - Flags: review?(lucasr.at.mozilla) → review+
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?
Attachment #631815 - Flags: review?(lucasr.at.mozilla) → review+
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"
Attachment #631816 - Flags: review?(lucasr.at.mozilla) → review+
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
Attachment #631817 - Flags: review?(lucasr.at.mozilla) → review+
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 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?
Attachment #631819 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 631820 [details] [diff] [review]
Patch 10/10 - Tests for swiping

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

Nice!
Attachment #631820 - Flags: review?(lucasr.at.mozilla) → review+
Please leave this open when it lands on central.
Whiteboard: [leave-open]
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.
Whiteboard: [leave-open] → [leave open]
Attached patch Compat patch updated (obsolete) — Splinter Review
Attachment #631818 - Attachment is obsolete: true
Attachment #644987 - Flags: review+
Attached patch Swipe patch updated (obsolete) — Splinter Review
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....
Attachment #631819 - Attachment is obsolete: true
Attachment #644991 - Flags: review+
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.
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.
Attached patch Patch - Bring in ViewPager src (obsolete) — Splinter Review
Finally got back to this. This brings in the ViewPager and PagerAdapter src files for us.
Attachment #652196 - Flags: review?(lucasr.at.mozilla)
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.
And this uses it all.
Attachment #644991 - Attachment is obsolete: true
Attachment #652198 - Flags: review?(lucasr.at.mozilla)
Attachment #652200 - Attachment is patch: true
Attachment #652200 - Flags: review?(lucasr.at.mozilla)
lucasr ping?
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?
Attachment #652196 - Flags: review?(lucasr.at.mozilla) → review+
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?
Attachment #652198 - Flags: review?(lucasr.at.mozilla) → review+
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).
Attachment #652200 - Flags: review?(lucasr.at.mozilla) → review-
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
Depends on: 787877
This broke removals from the history tab; the view is not updated on individual item removal (i.e, item-context-menu, "Remove").
Whiteboard: [leave open] → [leave open] ui-responsiveness
Duplicate of this bug: 701366
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.
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.
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.
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.
(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.
Looks like the current build system already has this actually:

https://tbpl.mozilla.org/?tree=Try&rev=9f7873337331&pusher=wjohnston@mozilla.com

I'll upload some unbitrotted patches and push this to try with the robocop tests running.
Attached patch Tests for swiping (obsolete) — Splinter Review
Tests updated
Attachment #631820 - Attachment is obsolete: true
Comment on attachment 674774 [details] [diff] [review]
Tests for swiping

Carrying r+ because there are no changes here.
Attachment #674774 - Flags: review+
Attached patch Bring in compat library (obsolete) — Splinter Review
Bring in compat libraries. Carrying r+.
Attachment #644987 - Attachment is obsolete: true
Attachment #652196 - Attachment is obsolete: true
Attachment #652198 - Attachment is obsolete: true
Attachment #652200 - Attachment is obsolete: true
Attachment #674775 - Flags: review+
Attached patch Add swipe (obsolete) — Splinter Review
Use ViewPager. There have been enough changes in the AllPagesTab since this that I think it should probably be re-reviewed.
Attachment #674777 - Flags: review?(lucasr.at.mozilla)
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 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.
Attachment #674777 - Flags: review?(lucasr.at.mozilla) → review+
Attached patch Bring in compat library (obsolete) — Splinter Review
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.
Attachment #674775 - Attachment is obsolete: true
Attachment #676370 - Flags: review?(blassey.bugs)
Attached patch Bring in compat library (obsolete) — Splinter Review
Wrong patch
Attachment #676370 - Attachment is obsolete: true
Attachment #676370 - Flags: review?(blassey.bugs)
Attachment #676392 - Flags: review?(blassey.bugs)
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
Attachment #676392 - Flags: review?(blassey.bugs) → review-
Attached patch Bring in compat library (obsolete) — Splinter Review
Moved this check to the android only section of configure.in.
Attachment #676392 - Attachment is obsolete: true
Attachment #676656 - Flags: review?(blassey.bugs)
Comment on attachment 676656 [details] [diff] [review]
Bring in compat library

This isn't right
Attachment #676656 - Flags: review?(blassey.bugs)
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.
Attached patch Compat (obsolete) — Splinter Review
Fixed up patch for compatibility library. ANDROID_SDK isn't defined in configure.in, so I put this in android.m4 instead.
Attachment #676656 - Attachment is obsolete: true
Attachment #678378 - Flags: review?(blassey.bugs)
Attached patch Add swipeSplinter Review
Sorry for the review churn lucas. All green on try.
Attachment #678382 - Flags: review?(lucasr.at.mozilla)
Grr... missing qref.
Attachment #678378 - Attachment is obsolete: true
Attachment #678378 - Flags: review?(blassey.bugs)
Attachment #678383 - Flags: review?(blassey.bugs)
Attachment #674774 - Attachment is obsolete: true
Attachment #674777 - Attachment is obsolete: true
Attachment #678384 - Flags: review?(lucasr.at.mozilla)
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
Attachment #678383 - Flags: review?(blassey.bugs) → review+
Attachment #678382 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 678384 [details] [diff] [review]
Tests for swiping

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

Nice.
Attachment #678384 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attached patch PatchSplinter Review
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!
Attachment #680097 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 680097 [details] [diff] [review]
Patch

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

Good.
Attachment #680097 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/0a14e7f19dbf
https://hg.mozilla.org/mozilla-central/rev/d8fed46ba90d
Status: NEW → RESOLVED
Closed: 8 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Status: RESOLVED → VERIFIED
Duplicate of this bug: 653282
Depends on: 817586
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
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.