Closed Bug 1184211 Opened 9 years ago Closed 9 years ago

Replace TwoWayView in SearchBar with RecyclerView

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox42 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: jonalmeida, Assigned: jonalmeida)

References

Details

Attachments

(2 files, 3 obsolete files)

MozReview Request: Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian
40 bytes, text/x-review-board-request
sebastian
: review+
Details
no_get_adapter.log
1.91 KB, text/x-log
Details 
Falls under bug 1158303.
Assignee: nobody → jalmeida
Blocks: 1167942
Part of search list bug is particular (See: bug 1167942)
Blocks: 1158303
No longer blocks: 1167942
Attached patch Recycler Search Bar patch (obsolete) — — Splinter Review 
This is my patch for replacing the TwoWayView with a RecyclerView. The problems that I'm running into are with Robocop not able to find the symbols for RecyclerView.

Sebastian, would you know anything about it since you worked on bug 1171288?

This the error output I see (Please format this correctly Bugzilla, please):

```
➜  mozilla-central  ./mach build build/mobile/robocop
/usr/bin/make -C /Users/jalmeida/hg/mozilla-central/objdir-droid -j8 -s backend.RecursiveMakeBackend
/usr/bin/make -C build/mobile/robocop -j8 -s
/Users/jalmeida/hg/mozilla-central/mobile/android/tests/browser/robocop/testAddSearchEngine.java:154: error: cannot access RecyclerView
                SearchEngineBar searchEngineBar = (SearchEngineBar) mSolo.getView(R.id.search_engine_bar);
                                                  ^
  class file for android.support.v7.widget.RecyclerView not found
/Users/jalmeida/hg/mozilla-central/mobile/android/tests/browser/robocop/testAddSearchEngine.java:155: error: cannot find symbol
                if (searchEngineBar == null || searchEngineBar.getAdapter() == null) {
                                                              ^
  symbol:   method getAdapter()
  location: variable searchEngineBar of type SearchEngineBar
/Users/jalmeida/hg/mozilla-central/mobile/android/tests/browser/robocop/testAddSearchEngine.java:160: error: cannot find symbol
                        + searchEngineBar.getAdapter().getCount()
                                         ^
  symbol:   method getAdapter()
  location: variable searchEngineBar of type SearchEngineBar
/Users/jalmeida/hg/mozilla-central/mobile/android/tests/browser/robocop/testSearchSuggestions.java:104: error: cannot access OnItemTouchListener
        BrowserSearch.sSuggestClientFactory = new BrowserSearch.SuggestClientFactory() {
                                                                                       ^
  class file for android.support.v7.widget.RecyclerView$OnItemTouchListener not found
/Users/jalmeida/hg/mozilla-central/mobile/android/tests/browser/robocop/testSearchSuggestions.java:105: error: method does not override or implement a method from a supertype
            @Override
            ^
/Users/jalmeida/hg/mozilla-central/mobile/android/tests/browser/robocop/testSearchSuggestions.java:104: error: cannot find symbol
        BrowserSearch.sSuggestClientFactory = new BrowserSearch.SuggestClientFactory() {
                                              ^
  symbol: constructor ()
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: /Users/jalmeida/hg/mozilla-central/mobile/android/tests/browser/robocop/SessionTest.java uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
6 errors
make[1]: *** [classes.dex] Error 1
make: *** [default] Error 2
```
Flags: needinfo?(s.kaspari)
So from the try push above there is no issue, which may mean there's something wrong with my local setup. Pushed for review.
Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian
Attachment #8637049 - Flags: review?(s.kaspari)
(In reply to Jonathan Almeida [:jonalmeida] from comment #4)
> So from the try push above there is no issue, which may mean there's
> something wrong with my local setup. Pushed for review.

I'm not sure if your try push ran all the tests. Even though you specified "-u all" it only ran rc1 to rc4 while on TryChooser[1] you can select rc1 - rc10 nowadays.

I usually use: "try: -b do -p android-api-9,android-api-11,android-x86 -u robocop-1,robocop-2,robocop-3,robocop-4,robocop-5,robocop-6,robocop-7,robocop-8,robocop-9,robocop-10 -t none"

[1] http://trychooser.pub.build.mozilla.org/
Flags: needinfo?(s.kaspari)
Comment on attachment 8637049 [details]
MozReview Request: Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

https://reviewboard.mozilla.org/r/13783/#review12399

::: mobile/android/base/home/SearchEngineAdapter.java:27
(Diff revision 1)
> +    private List<SearchEngine> mSearchEngines = new ArrayList<>();

Super NIT: You can use Collections.emptyList() if you want to avoid allocating an empty list. But the returned list will be immutable. See comment below.

::: mobile/android/base/home/SearchEngineAdapter.java:30
(Diff revision 1)
> +        mSearchEngines.addAll(searchEngines);

This is not really what the method signature promisses: Setting instead of adding. We probably always want to set, right?

::: mobile/android/base/home/SearchEngineAdapter.java:76
(Diff revision 1)
> +                return new SearchEngineViewHolder(createSearchEngineView(parent));

The only point of returning a different view type in getItemViewType() is to return different ViewHolder objects here (because different views will require different view holders).

::: mobile/android/base/home/SearchEngineAdapter.java:85
(Diff revision 1)
> +            holder.bindItem(null);

Calling this for position 0 seems to be unnecessary.

::: mobile/android/base/home/SearchEngineAdapter.java:93
(Diff revision 1)
> +        return mSearchEngines.size();

I think you want to add +1 here for the label or we'll lose the last search engine.

::: mobile/android/base/home/SearchEngineAdapter.java:105
(Diff revision 1)
> +        iconView.setScaleType(ImageView.ScaleType.FIT_XY);

NIT: We can move this to the XML file (I probably left this here..)

::: mobile/android/base/home/SearchEngineBar.java:127
(Diff revision 1)
> -            view.setLayoutParams(params);
> +        // do nothing

I wrote RecyclerViewItemClickListener for a specific use case where I needed to listen for normal clicks and long clicks. I thought about later uses cases where you just want to click handler and don't care about long clicks but I didn't want to write code that isn't used.

Maybe it makes sense to refactor RecyclerViewItemClickListener to handle these use cases without implementing empty methods. But that's something we can look at in a follow-up bug.

::: mobile/android/base/home/SearchEngineBar.java:84
(Diff revision 1)
> -        final int searchEngineCount = adapter.getCount() - 1;
> +        final int searchEngineCount = mAdapter.getItemCount();

Why did you remove the "-1"? There's still the label we need to take care of?

::: mobile/android/base/home/SearchEngineBar.java:98
(Diff revision 1)
>              }

The latest changes for "cutting off" the last visible icon didn't make it into this patch I think.

::: mobile/android/base/home/SearchEngineBar.java:15
(Diff revision 1)
> +import android.util.Log;

NIT: This import is not used (anymore)

::: mobile/android/base/home/SearchEngineAdapter.java:39
(Diff revision 1)
> +        private View mView;

This shouldn't be needed as you are able to access itemView of the super class.

However the point of the ViewHolder is that you do all your findViewById() calls in the constructor and assign the results to members of your ViewHolder class - so that later in bindItem() (which will be called very often when scrolling!) does not need to search views but can just access the members.

::: mobile/android/base/home/SearchEngineAdapter.java:119
(Diff revision 1)
> +        faviconView.setImageResource(R.drawable.search_icon_inactive);

This shouldn't be needed. This view shouldn't be visible before bind() is called. Did you need this to work around some issue? Otherwise let's avoid loading an image we do not display.
Attachment #8637049 - Flags: review?(s.kaspari)
(In reply to Sebastian Kaspari (:sebastian) from comment #6)
> I'm not sure if your try push ran all the tests. Even though you specified
> "-u all" it only ran rc1 to rc4 while on TryChooser[1] you can select rc1 -
> rc10 nowadays.
> 
> I usually use: "try: -b do -p android-api-9,android-api-11,android-x86 -u
> robocop-1,robocop-2,robocop-3,robocop-4,robocop-5,robocop-6,robocop-7,
> robocop-8,robocop-9,robocop-10 -t none"
> 
> [1] http://trychooser.pub.build.mozilla.org/

I think I'm going to use that from now on. Looks like that build issue isn't just local: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5b8731bd40d

Yesterday, I was trying to figure out how to add the support library to `build/mobile/robocop/Makefile.in` but I had troubles at finding a way to reference the local android sdk path in there.

Any idea at what I could look at?

Fixing your comments in the review for now. Thanks!
Flags: needinfo?(s.kaspari)
(In reply to Jonathan Almeida [:jonalmeida] from comment #8)
> Any idea at what I could look at?

I don't - yet. I filed bug 1186532 blocking this one. Let's see if we can find a solution.
Flags: needinfo?(s.kaspari)
https://reviewboard.mozilla.org/r/13783/#review12399

> I think you want to add +1 here for the label or we'll lose the last search engine.

I removed the +1 because I get IndexOutOfBoundsException when I scroll to the end of the list.

> Calling this for position 0 seems to be unnecessary.

Good point, I'm not sure why I did that, ha!

> The only point of returning a different view type in getItemViewType() is to return different ViewHolder objects here (because different views will require different view holders).

I use it to determine if it's a label or search engine because I found it easier to construct each one with the parent ViewGroup. I'm not sure what would be the best way to change this.

> I wrote RecyclerViewItemClickListener for a specific use case where I needed to listen for normal clicks and long clicks. I thought about later uses cases where you just want to click handler and don't care about long clicks but I didn't want to write code that isn't used.
> 
> Maybe it makes sense to refactor RecyclerViewItemClickListener to handle these use cases without implementing empty methods. But that's something we can look at in a follow-up bug.

Created bug 1186529 for this.

> Super NIT: You can use Collections.emptyList() if you want to avoid allocating an empty list. But the returned list will be immutable. See comment below.

Thanks, TIL!

> The latest changes for "cutting off" the last visible icon didn't make it into this patch I think.

Oops! Patch incoming.
Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

Fixes from review comments (Part 1.. of 1?)
Attachment #8637660 - Flags: review?(s.kaspari)
Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

These are actually changes from bug 1158291 applied on top of the re-written view.
Attachment #8637661 - Flags: review?(s.kaspari)
https://reviewboard.mozilla.org/r/13783/#review12399

> I removed the +1 because I get IndexOutOfBoundsException when I scroll to the end of the list.

I assume this happened because you need to substract one again when accessing the search engine list because the 'label' is not in there. Summarizing: The adapter takes care of label + search engines (because it needs to create views for them), but the internal list only contains the search engines.

> Created bug 1186529 for this.

Thanks! :)

> I use it to determine if it's a label or search engine because I found it easier to construct each one with the parent ViewGroup. I'm not sure what would be the best way to change this.

You base some of your decisions on the 'position' of an item and some others on the ViewHolder. The ViewHolder should take care of most decisions. Their purpose is to avoid the performance overhead of searching views in a tree, see: http://developer.android.com/training/improving-layouts/smooth-scrolling.html#ViewHolder

You probably want to create two different ViewHolders when return two different view types. Both view holders can implement a common interface to avoid the need to use instanceof / if statements all over the place.
Whenever updating a patch feel free to update the existing patch with 'hg commit --amend' and push to reviewboard. Reviewboard is able to handle the different versions over time and I can look at the final result as well as intermediate steps without keeping track of all the different patches (Reviewboard has some nice sliders for that). In this case it would be easier to just review the final result. :)
https://reviewboard.mozilla.org/r/13781/#review12511

::: mobile/android/base/home/SearchEngineAdapter.java:87
(Diff revision 2)
> +        return mSearchEngines.size();

Your current implementation has still an off-by-one error. One search engine is missing. This here is at least one part of the problem. Your adapter size should be mSearchEngines.size() + 1.

::: mobile/android/base/home/SearchEngineBar.java:127
(Diff revision 2)
> -        private View getSearchEngineView(final int position, View view, final ViewGroup parent) {
> +        final SearchEngine searchEngine = mAdapter.getItem(position + 1);

Are you sure you want to add one here? getItem() substracts one to access mSearchEngines. Everything should be fine without adding one?
Attachment #8637660 - Flags: review?(s.kaspari) → feedback+
Attachment #8637661 - Flags: review?(s.kaspari) → feedback+
This is definitely going into the right direction. My only concerns with the current version:

* There's an off-by-one error. Review all your +1 / -1 and compare with the list of a Nightly build.

* Try to follow the view holder pattern, avoiding findViewById() in bind()
https://reviewboard.mozilla.org/r/13783/#review12399

> You base some of your decisions on the 'position' of an item and some others on the ViewHolder. The ViewHolder should take care of most decisions. Their purpose is to avoid the performance overhead of searching views in a tree, see: http://developer.android.com/training/improving-layouts/smooth-scrolling.html#ViewHolder
> 
> You probably want to create two different ViewHolders when return two different view types. Both view holders can implement a common interface to avoid the need to use instanceof / if statements all over the place.

I think I got it now. I've moved the findViewById for the ImageView to the constructor of the ViewHolder. On bind, I just set the resource there.
Comment on attachment 8637661 [details]
MozReview Request: Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

 - These are actually changes from bug 1158291 applied on top of the re-written view.
 - Also includes one off errors, and corrected placement of findViewById in ViewHolder.
Attachment #8637661 - Flags: feedback+ → review?(s.kaspari)
(In reply to Sebastian Kaspari (:sebastian) from comment #14)
> Whenever updating a patch feel free to update the existing patch with 'hg
> commit --amend' and push to reviewboard. Reviewboard is able to handle the
> different versions over time and I can look at the final result as well as
> intermediate steps without keeping track of all the different patches
> (Reviewboard has some nice sliders for that). In this case it would be
> easier to just review the final result. :)

I was trying to separate the original patch from amends, as well as the patch for bug 1158291. I put my last changes to the end of the last patch though (couldn't amend the previous one).

> * There's an off-by-one error. Review all your +1 / -1 and compare with the list of a Nightly build.
> * Try to follow the view holder pattern, avoiding findViewById() in bind()

Fixed these changes! Although note that I've tested everything locally only since I can't test on try until bug 1186532 lands there.
(In reply to Sebastian Kaspari (:sebastian) from comment #6)
> I usually use: "try: -b do -p android-api-9,android-api-11,android-x86 -u
> robocop-1,robocop-2,robocop-3,robocop-4,robocop-5,robocop-6,robocop-7,
> robocop-8,robocop-9,robocop-10 -t none"

The number of robocop suites can vary depending on the number of tests so this may miss a suite when tests get added. You can specify 'robocop' to run them all:
  "try: -b do -p android-api-9,android-api-11,android-x86 -u robocop -t none"
(In reply to Jonathan Almeida [:jonalmeida] from comment #2)
> This the error output I see (Please format this correctly Bugzilla, please):

For better formatting, you could post this as an attachment (which I usually do, with the important excerpts poorly formatted in my comment), or link to an external source that can do better than plain text attachments (e.g. pastebin, gist), but this is generally less preferable because those sources aren't guaranteed to exist in the future.
Comment on attachment 8637661 [details]
MozReview Request: Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

https://reviewboard.mozilla.org/r/13955/#review12645

Awesome! That's basically a r+. There are two things, see below. Oh, and I like how SearchEngineBar now seems to be much simpler, just by moving out the adapter. :)

::: mobile/android/base/home/SearchEngineAdapter.java:83
(Diff revision 2)
> -            holder.bindItem(mSearchEngines.get(position));
> +            holder.bindItem(mSearchEngines.get(position - 1));

You are duplicating the functionality of getItem() here. Just call getItem(position) :)

NIT: Maybe add a comment why we are omitting position 0. (I think the original implementation had something like that.)

::: mobile/android/base/home/SearchEngineAdapter.java:40
(Diff revision 2)
> +        private ImageView faviconView;

This should be final.
Attachment #8637661 - Flags: review?(s.kaspari)
Before landing we should probably "--collapse" these changes into a single commit.
> Awesome! That's basically a r+. There are two things, see below. Oh, and I like how SearchEngineBar now seems to be much simpler, just by moving out the adapter. :)

I like it too! :)

I've fixed the two changes you mentioned and folded all the commits into one. By EOD I'll push to try which is plenty of time for your robocop/recyclerview patch to have landed there.
Comment on attachment 8637049 [details]
MozReview Request: Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

 - Also includes changes from bug 1158291 applied on top of the re-written view.
Attachment #8637049 - Flags: review?(s.kaspari)
Attachment #8637660 - Attachment is obsolete: true
Attachment #8637661 - Attachment is obsolete: true
bug 1170724 recently landed and will likely bitrot this.
Depends on: 1170724
Blocks: 1186529
Attachment #8636935 - Attachment is obsolete: true
(In reply to Michael Comella (:mcomella) from comment #20)
> The number of robocop suites can vary depending on the number of tests so
> this may miss a suite when tests get added. You can specify 'robocop' to run
> them all:
>   "try: -b do -p android-api-9,android-api-11,android-x86 -u robocop -t none"

(In reply to Jonathan Almeida [:jonalmeida] from comment #26)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5be2db52163f

This try run with "-u robocop" just ran rc1 - rc4 again. Is this intentional? Locally testAddSearchEngine fails for me. Is this happening for you too?
Flags: needinfo?(jalmeida)
Comment on attachment 8637049 [details]
MozReview Request: Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

https://reviewboard.mozilla.org/r/13783/#review12865

::: mobile/android/base/home/SearchEngineAdapter.java:35
(Diff revision 2)
> +    public void setIconContainerWidth(int iconContainerWidth) {

NIT: Maybe we should add a comment here explaining what the container size is - and remove the 'public' keyword to make the method access 'package private'.
Attachment #8637049 - Flags: review?(s.kaspari)
The code itself is r+ but testAddSearchEngine is failing for me locally.
(In reply to Sebastian Kaspari (:sebastian) from comment #28)
> This try run with "-u robocop" just ran rc1 - rc4 again. Is this
> intentional?

I noticed an fx-team landing [1] also only ran rc1 - rc4 – I wonder if they've been regrouped, or if `-u robocop`, which is probably used for those pushes, is broken.

[1]: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=d98c9b5082f1
(In reply to Michael Comella (:mcomella) from comment #31)
> I noticed an fx-team landing [1] also only ran rc1 - rc4

via irc:

<gbrown> mcomella: yes, I think so. are you thinking of rc5 ... on Android 4.0? We don't run those, except on try and beta now (and those for a limited time)
09:42 <gbrown> (you get all the same tests on rc1 .. rc4 on 2.3/4.3 as rc1 .. rc10 on 4.0 -- they are just split up differently)
> Locally testAddSearchEngine fails for me. Is this happening for you too?

Just tried it out, and I'm not seeing any failures.

> NIT: Maybe we should add a comment here explaining what the container size is - and remove the 'public' keyword to make the method access 'package private'.

Fixed!

I've pushed to try again with the comments and access changes with my old try syntax:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5a62c817467
Flags: needinfo?(jalmeida)
Try looks like it's passing with my last push. Sending for review.
Comment on attachment 8637049 [details]
MozReview Request: Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

 - Also includes changes from bug 1158291 applied on top of the re-written view.
Attachment #8637049 - Flags: review?(s.kaspari)
Comment on attachment 8637049 [details]
MozReview Request: Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

https://reviewboard.mozilla.org/r/13783/#review12947

Ship It!
Attachment #8637049 - Flags: review?(s.kaspari) → review+
Rebased and pushed to try one more time to make sure there aren't any issues hitting bug 1170724.
I'm seeing those testAddSearchEngine failures on try now, but I'm still not seeing it locally. Looking into this today..
Attached file no_get_adapter.log — 
Sebastian, I'm seeing an error I'm seeing is that there is no method for getAdapter in RecyclerView. Do you know if this is related to bug 1186532?

java.lang.NoSuchMethodError: No virtual method getAdapter()Landroid/support/v7/widget/RecyclerView$Adapter; in class Lorg/mozilla/geck    o/home/SearchEngineBar; or its super classes (declaration of 'org.mozilla.gecko.home.SearchEngineBar' appears in /data/app/org.mozilla.fennec_jalmeida-1/base.apk)
Flags: needinfo?(s.kaspari)
(In reply to Jonathan Almeida (:jonalmeida) from comment #42)
> Created attachment 8641203 [details]
> no_get_adapter.log
> 
> Sebastian, I'm seeing an error I'm seeing is that there is no method for
> getAdapter in RecyclerView. Do you know if this is related to bug 1186532?

Proguard seems to be very aggressive and strips a bunch of things. I got it working by overriding getAdapter() and adding the RobocopTarget[1] annotation. I can't come up with anything that's avoiding additional code. Unfortunately we can't annotate RecyclerView's method.

> +    @RobocopTarget
> +    @Override
> +    public SearchEngineAdapter getAdapter() {
> +        return mAdapter;
> +    }

[1]: https://wiki.mozilla.org/Mobile/Fennec/Android/UITest
Flags: needinfo?(s.kaspari)
See last try build (comment 44) which includes a test fix.
Keywords: checkin-needed
Doesn't apply. Did you push the fixed-up patch to mozreview?
Keywords: checkin-needed
Comment on attachment 8637049 [details]
MozReview Request: Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

 - Also includes changes from bug 1158291 applied on top of the re-written view.
Attachment #8637049 - Flags: review+ → review?(s.kaspari)
Comment on attachment 8637049 [details]
MozReview Request: Bug 1184211 - Replace TwoWayView in SearchBar with RecyclerView. r?sebastian

https://reviewboard.mozilla.org/r/13783/#review13313

Still r+. The last change added some more code that is not really part of this bug or needed?

With this being the last week of the 42 cycle I wonder whether we should land this in 43 next week to give it some more time in Nightly to test. What do you think? But feel free to land this now if you feel positive. :)

::: mobile/android/tests/browser/robocop/AboutHomeTest.java:183
(Diff revision 4)
> +    protected final RecyclerView findRecyclerViewWithTag(String tag) {

This method is not used? I assume this refactoring of findListViewWithTag() is not really part of this bug?

::: mobile/android/tests/browser/robocop/AboutHomeTest.java:245
(Diff revision 4)
> -     * @param AboutHomeTabs enum item
> +     * @param tab

You fixed the param name but removed the comment :)
Attachment #8637049 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #48)
> ::: mobile/android/tests/browser/robocop/AboutHomeTest.java:245
> (Diff revision 4)
> > -     * @param AboutHomeTabs enum item
> > +     * @param tab
> 
> You fixed the param name but removed the comment :)

Reading this line again I think we can remove the whole line. The comment isn't adding any value. ;)
https://reviewboard.mozilla.org/r/13783/#review13321

::: mobile/android/base/home/SearchEngineBar.java:139
(Diff revision 4)
> -        public int getViewTypeCount() {
> +    public SearchEngineAdapter getAdapter() {

Drive-by: add a comment explaining why this getAdapter override/@RobocopTarget is necessary.

Also, weird - can you file, CC me, ckitching, rnewman, and NI nalexander?
See comment 50 for try build with r+ (comment 52 includes only comments mentioned in comment 51).
Keywords: checkin-needed
https://reviewboard.mozilla.org/r/13783/#review13313

> This method is not used? I assume this refactoring of findListViewWithTag() is not really part of this bug?

I added this initially for trying to fix the build errors with getAdapter(), but I let it in since it might have been useful later. Removed it for now.

> You fixed the param name but removed the comment :)

Removed entirely!
https://reviewboard.mozilla.org/r/13783/#review13321

> Drive-by: add a comment explaining why this getAdapter override/@RobocopTarget is necessary.
> 
> Also, weird - can you file, CC me, ckitching, rnewman, and NI nalexander?

Created bug 1190567 for this.
this caused conflicts when importing to fx-team

patching file mobile/android/base/home/SearchEngineBar.java
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/home/SearchEngineBar.java.rej

could you take a look, thanks!
Flags: needinfo?(jalmeida)
Keywords: checkin-needed
I've rebased and fixed the conflict, as well as kicked off another try build to verify. If all goes well, I'll mark it for checkin.
Flags: needinfo?(jalmeida)
Rebase looks good, see comment 57.
Keywords: checkin-needed
MozReview rev 08c9f8f4c0798e6f04cf791fbe13f6287126f070 is still not applying to fx-team tip.
Keywords: checkin-needed
FWIW, the parent rev on that is 2ee9895e032c, which is from 28-July.
https://hg.mozilla.org/mozilla-central/rev/f52bfe71d212
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: