Closed Bug 927692 Opened 11 years ago Closed 8 years ago

Preload search engines

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

Product:
Firefox for Android Graveyard
Component:
Awesomescreen
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: rnewman, Assigned: aakash.muttineni, Mentored)

Details

(Keywords: perf, Whiteboard: [lang=java][good first bug])

Attachments

(1 file, 3 obsolete files)

Bug927692
8.31 KB, patch
mcomella
: feedback+
Details | Diff | Splinter Review 
We do this lazily, which means we're slow to show the search suggestion UI.

We should trigger a background task to do this after a few milliseconds idle on about:home.
Keywords: perf
Whiteboard: [mentor=rnewman][lang=java][good first bug]
Hi,
I would like to work on this.
I'm new to this and would like to solve this as my first bug.
Can someone help me on this
Sure!

First you need to get set up for development: Android developer tools, the Mozilla codebase, etc.

Start by reading these:

https://wiki.mozilla.org/Mobile/Get_Involved
https://developer.mozilla.org/docs/Introduction

Those should be enough to get you a checkout and get a working build of Firefox for Android that you can run on your own Android device.

Come find us in IRC if you get stuck: #mobile at irc://irc.mozilla.org/mobile .

Let me know when you're up and running!
Thanks,
I have setup a working Fennec build.
I'll acclimatize with the issue and contact you through irc for doubts.
I'd like to work on it if it is still not assigned.
Assignment need not matter. We'll review the first patch that comes in attached to this bug. You're more than welcome to work on this despite earlier interest from vivek.
Ad(In reply to AdityaSingh [:MacroMayhem] from comment #4)
> I'd like to work on it if it is still not assigned.

Hi, Aditya - Are you still interested in working on this?
(In reply to Mike Hoye [:mhoye] from comment #6)
> Ad(In reply to AdityaSingh [:MacroMayhem] from comment #4)
> > I'd like to work on it if it is still not assigned.
> 
> Hi, Aditya - Are you still interested in working on this?

Yes, can you please guide me through?
Aditya: first, are you set up to do Fennec development? I've seen you contribute patches for other parts of Firefox, but not on Android, which requires some additional tooling. If not, please see Comment 2.

The issue here should be relatively straightforward, but it's a real chunk of work, involving figuring some stuff out, measuring, then trying to improve those measurements.

As far as I can tell, BrowserSearch.onViewCreated is the source of sending a SearchEngines:GetVisible message to Gecko. browser.js handles that around line 6840. This is the core of populating the search engine list when you tap the URL bar.

We'd like to begin the process of fetching these search engines sooner, if it helps responsiveness.

To do that involves:

* Measuring the delays between the user trying to start a search and the list being ready
* Gecko being ready and the list being ready

then sending that message sooner, handling it sooner, and potentially caching the results to avoid the need to re-fetch them, OR sending a different message to warm things up without having to return all of the values.

That improvement needs to not regress page load, too.

Make sense?
Mentor: rnewman
Whiteboard: [mentor=rnewman][lang=java][good first bug] → [lang=java][good first bug]
Can I grab this one?
(In reply to Epransky from comment #9)
> Can I grab this one?

Sure; get started!
Cool, I'll get on it by this weekend probably, if not before.  Thanks!
Assignee: nobody → Epransky
(In reply to Richard Newman [:rnewman] from comment #8)

> As far as I can tell, BrowserSearch.onViewCreated is the source of sending a
> SearchEngines:GetVisible message to Gecko. browser.js handles that around
> line 6840. This is the core of populating the search engine list when you
> tap the URL bar.

Hi Richard,

I started taking a look at this and I just wanted clarify something. From what I can see, BrowserSearch.onViewCreated registers a listener for SearchEngines:Data [1], which browser.js defines in the _handleSearchEnginesGetVisible(rv, all) function [2]. I only see SearchEngines:GetVisible being called in BrowserSearch.onResume [3]. I guess my question is what is the process for all of this, because it really seems like the populating of the list happens here [4] which I'm not sure how its being called at all. Can I get a little more context for this?

[1] https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#340
[2] https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6953
[3] https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#241
[4] https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#373
Flags: needinfo?(rnewman)
The list is populated some time after BrowserSearch inits itself. It does so by registering an observer and then requesting that Gecko sends it data:

https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#239

That message goes to Gecko, and results in a SearchEngines:Data response being sent, triggering the registered observer.

The goal of this bug is to get that list of search engines in BrowserSearch initialized *before* we create and attach the view.

That will involve shifting responsibility for the list (and the observers) from this fragment and out to a separate small search engine manager, whose lifecycle can be longer than BrowserSearch's.

We'd initialize that search engine manager shortly after Gecko reports that it has started up; by the time the user taps in the search bar, we'd already know the list of engines.
Flags: needinfo?(rnewman)
(In reply to Richard Newman [:rnewman] from comment #13)

Ok, I think I understand what the goal is here. We need to create a new activity which starts running outside of BrowserSearch, when the app is initially loaded?

> That will involve shifting responsibility for the list (and the observers)
> from this fragment and out to a separate small search engine manager, whose
> lifecycle can be longer than BrowserSearch's.

So basically, that involves moving:

340         EventDispatcher.getInstance().registerGeckoThreadListener(this,
341             "SearchEngines:Data");

and

239         // Fetch engines if we need to.
240         if (mSearchEngines.isEmpty() || !Locale.getDefault().equals(mLastLocale)) {
241             GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("SearchEngines:GetVisible", null));
242         }

to this new activity and creating a getter method to be used in the setSearchEngines method?

> 
> We'd initialize that search engine manager shortly after Gecko reports that
> it has started up; by the time the user taps in the search bar, we'd already
> know the list of engines.

Where does this occur? I'm still not so familiar with the whole Gecko lifecycle, so it'd help if I knew when/where Gecko reports it has started up! 

Thanks
Flags: needinfo?(rnewman)
> Ok, I think I understand what the goal is here. We need to create a new
> activity which starts running outside of BrowserSearch, when the app is
> initially loaded?

Not an activity, and not immediately, but otherwise along those lines.


> to this new activity and creating a getter method to be used in the
> setSearchEngines method?

Pretty much, yes. Step one is moving the logic; step two is doing the fetch sooner, triggered by something earlier than BrowserSearch.


> Where does this occur? I'm still not so familiar with the whole Gecko
> lifecycle, so it'd help if I knew when/where Gecko reports it has started
> up! 

We'd probably trigger the fetch in a Gecko:DelayedStartup handler; see how it's done in BrowserApp.
Flags: needinfo?(rnewman)
Unassigning due to inactivity. Epransky, let us know if you're still working on this!
Assignee: Epransky → nobody
Hey folks. I see that this bug has been unassigned and I would like to pick it up as my first bug. I already have fennec built and installed on an android device, the mozilla source code, a github account, and am working on a Linux Mint 17.0 OS, so I have ready access to java. Is there anything else I should have prepared in order to persue this? 

Also, let me see if I'm understanding the bug issue: upon opening a new instance of a FirefoxMobile browser, being directed to the about:home page, and tapping the search bar, the search engine API (ie; the list containing Google, Bing, Yahoo, etc. search engines to tap and use to preform the search) doesn't show up until AFTER you start typing, but we want it to show up upon only a few milliseconds of beginning a search -- preferably before the user even has a chance to start typing. Is that correct?
Yup, that's it. Comment 8 and onwards have some discussion about how you might implement this.

There are some complications here (e.g., during first run we don't want to accidentally skip search engines that are defined in a distribution) but I think most of those are already handled by the code you'll be touching.

It sounds like you're good to get started, so let me know if you have any more questions, or want feedback on a patch!
Hey folks. Still tinkering about with the code base -- which is massive -- in whatever time I can rummage up. Haven't been able to nail anything down yet, just want to let you folks know I'm still about and still working on it.

I did do some thinking though -- we want the search engine list to populate and display in the event that the user doesn't begin to type something into the search bar, but in tinkering with FireFox-Android I discovered a bit of a conflict. As it stands, FireFox-Android has a search bar that starts empty until something has been typed in. I imagine that this 'empty space' is where we want to the search engine UI to pop up. But if it pops up at this point, what happens to the suggested searches UI that pops up when a user begins to type? Current, this suggested searches UI remains present until the user picks a word or exhausts the search's algorithm and it gives up. In either case, FireFox-Android then pulls up the search engine UI to allow the user to feed their search into a chosen search engine.

To which my question is: what becomes of this suggested search UI? I may be overthinking this, but if the search engine UI kicks in immediately, but the user doesn't select one of the search engines from the list and begins to type a word, wouldn't the engine UI and this suggested UI run in conflict of one another, given that they populated the same view space?
Flags: needinfo?(rnewman)
(In reply to Noe J Dotson from comment #19)

> I did do some thinking though -- we want the search engine list to populate
> and display in the event that the user doesn't begin to type something into
> the search bar…

I think you might be either over-thinking this, or conflating the search engine *data* with the search engine *list*.

See Comment 13.

In short, rather than this:

  Browser starts
  Gecko launches
  User taps in bar
  User starts typing
  Search UI is built
  Search UI sends message to Gecko
  Search UI receives array of search engines -- names, URLs, suggestion templates, etc.
  Search UI processes user's text entry
  Search UI shows list of engines, search terms, and suggestions

I'd like to do this:

  Browser starts
  Gecko launches
  Search manager sends message to Gecko
  Search manager receives array of search engines
  User taps in bar
  User starts typing
  Search UI is built
  Search UI fetches pre-munged list of search engines from Java-side search manager
  Search UI processes user's text entry
  Search UI shows list of engines, search terms, and suggestions

Note that nothing changes about how the UI is created or handled. The second half of this flow is essentially what happens right now the *second* time you do a search -- we only fetch the engines from Gecko once, so the second time is super fast.

Let's make the first time like the second time. Make sense?

((Sidenote: Margaret has already done some related work to do search engine lookup in Java, which might eventually remove the need to talk to Gecko at all, but breaking out engine management out of BrowserSearch and decoupling the fetching of engines from view initialization is still a huge improvement.))
Flags: needinfo?(rnewman)
It does make sense. I apologise for my lack of activity as of late -- I've been sick.

I also apologise for not being terribly good at this. I'm going to hop into the #mobile irc at some point in the next week to try and divulge where all the threads I have uncovered run to. The most obvious lead I see is putting
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#251
just after #219, where the mSearchEngine variable is populated. This seems like far too easy a fix, so I highly doubt it will work. As far as I can tell, it's the only place BrowserSearch.Java asks if mSearchEngine is empty, though. 

Another less obvious lead is the setSearchEngine method
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#538
which the comment says is called via a runnable withing the Gecko thread, which is a fairly complicated method. I need to go looking for the Gecko and see where this method is getting called out. It might be the source of populating the engine list ahead of time. To be honest, I just haven't had -- or haven't made -- enough time to look into what I need to look into, but I am still trying.
Attached patch Bug927692.patch (obsolete) — — Splinter Review 
Hi, I would like to take this up as my first bug! According to the comments above I have made a fix which successfully preloads the search engines. As of now I integrated the event dispatch into BrowserApp which will initialize the search engines in BrowserSearch. I have built the code and it runs properly on my device. What are the test cases which should be considered?
Flags: needinfo?(rnewman)
Comment on attachment 8666146 [details] [diff] [review]
Bug927692.patch

I'm traveling on business this week, but I'll try to get to this before next week. Thanks for the patch, Aakash.
Flags: needinfo?(rnewman)
Attachment #8666146 - Flags: feedback?(rnewman)
Assignee: nobody → aakash.muttineni
Status: NEW → ASSIGNED
Comment on attachment 8666146 [details] [diff] [review]
Bug927692.patch

Michael, do you have time to roll this in with the search stuff that you're mentoring right now?
Attachment #8666146 - Flags: feedback?(rnewman) → feedback?(michael.l.comella)
Mentor: rnewman
Comment on attachment 8666146 [details] [diff] [review]
Bug927692.patch

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

This is looking like a step in the right direction – thanks Aakash!

I apologize for the delay – it won't be that long in the future.

::: mobile/android/base/BrowserApp.java
@@ +1879,5 @@
>          return (TextUtils.equals(packageName, getPackageName()));
>      }
>  
>      @Override
> +    public void handleMessage(String event,final JSONObject message) {

nit: space after comma

@@ +1904,5 @@
>                          // Force tabs panel inflation once the initial
>                          // pageload is finished.
>                          ensureTabsPanelExists();
> +                        //Preloading search engines for BrowserSearch
> +                        //BrowserSearch instance needs a context to create SearchEngine Objects

nit: Space after `//`

@@ +1907,5 @@
> +                        //Preloading search engines for BrowserSearch
> +                        //BrowserSearch instance needs a context to create SearchEngine Objects
> +                        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("SearchEngines:GetVisible", null));
> +                        mBrowserSearch.setActivityContext(getContext());
> +                        EventDispatcher.getInstance().registerGeckoThreadListener(mBrowserSearch, "SearchEngines:Data");

I'd prefer if this code was encapsulated in BrowserSearch – can you move this code into a method of BrowserSearch and call it from DelayedStartup? You can re-use the SearchEngines:Data listener already in BrowserSearch.

::: mobile/android/base/home/BrowserSearch.java
@@ +131,5 @@
> +    //preloading is done before fragment transaction occurs, getActivity() will return null at that time
> +    private Context activityContext;
> +
> +    //Boolean to keep track of whether the gecko thread is registered
> +    private boolean geckoThreadRegistered;

nit: -> geckoThreadListenerRegistered

@@ +565,5 @@
>              final JSONArray engines = data.getJSONArray("searchEngines");
>  
>              mSuggestionsEnabled = suggest.getBoolean("enabled");
>  
> +            searchEngines = new ArrayList<SearchEngine>();

If this is the first time this is called, why not store the JSONObject and call setSearchEngines in onResume instead? That way we don't need to do this activityContext hacking.
Attachment #8666146 - Flags: feedback?(michael.l.comella) → feedback+
Attached patch Bug927692-second.patch (obsolete) — — Splinter Review 
Hi, just implemented the suggested changes. How does it look?
There is an odd situation that occurs when the orientation is changed before the search engines are loaded. In the onDestroyView() of BrowserSearch Line 287, the GeckoThreadListener is being unregistered, if this occurs before the search engines are loaded then they will not be loaded afterwards. 
The approach that came to my mind involve 2 cases: 
1. The orientation is changed: in this case the GeckoThreadListener need not be unregistered
2. The browserSearch is closed: in this case it should be unregistered if the data is loaded. But it should be unregistered after the data is saved in BrowserSearch

Is this a proper way to approach this situation?
Comment on attachment 8677449 [details] [diff] [review]
Bug927692-second.patch

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

Hey Aakash. When you upload a patch, be sure to tag a reviewer so they'll get a persistent notification and don't lose the request in their email. I've already tagged a reviewer (me!) for this patch.

I'll get back to you shortly.
Attachment #8677449 - Flags: feedback?(michael.l.comella)
Sorry, Aakash – I'll get to this tomorrow for sure.
Comment on attachment 8677449 [details] [diff] [review]
Bug927692-second.patch

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

Sorry for not getting to this when I said I would – I got ill. I'll be much quicker on the turn around from now on, I promise.

Can you merge this in with your previous patch? Generally, if you make changes in response to a review, you should keep them in the same patch – it's easier to review them that way.

I don't think we re-register the gecko listener after browser search is destroyed, right? We should do that.

::: mobile/android/base/home/BrowserSearch.java
@@ +126,5 @@
>  
>      // The bar on the bottom of the screen displaying search engine options.
>      private SearchEngineBar mSearchEngineBar;
>  
> +    // Store serach engine data, setSearchEngines is called with this object in onResume()

nit: -> search

@@ +242,5 @@
>      public void onResume() {
>          super.onResume();
>  
> +        if(searchEngineData != null) {
> +            setSearchEngines(searchEngineData);

It seems inefficient to do this every time onResume is called if we only need to do it the first time – do you have any solutions/thoughts here?

To be fair, I believe we short-circuit the method (or will) so it's not super expensive.
Attachment #8677449 - Flags: feedback?(michael.l.comella) → feedback+
Still working on this Aakash?
Flags: needinfo?(aakash.muttineni)
Sorry for the inactivity, I've been busy with my sem finals. I will get back to this bug by the weekend.
Flags: needinfo?(aakash.muttineni)
Attached patch Bug927692.patch (obsolete) — — Splinter Review 
The patch incorporates the things you have mentioned. I also moved the checking of the searchEnginesData from onResume to onCreate, in that way it will only be checked the first time. Is that what you meant by short-circuiting or did you have anything else in mind?
Attachment #8666146 - Attachment is obsolete: true
Attachment #8677449 - Attachment is obsolete: true
Attachment #8696330 - Flags: review?(michael.l.comella)
Hi, can I be assigned this bug please?
(In reply to Pas from comment #33)
> Hi, can I be assigned this bug please?

It looks like there is already work on-going on this bug, however there is no shortage of other bugs that might peak your interest! http://www.joshmatthews.net/bugsahoy/?mobileandroid=1
Hey Aakash – we've been at a conference this week and things have been pretty busy. I'll get to this on Monday. Sorry for the delay!
Comment on attachment 8696330 [details] [diff] [review]
Bug927692.patch

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

The code is confusing me, but maybe it will make more sense when you have a chance to respond to my comments.

::: mobile/android/base/home/BrowserSearch.java
@@ +128,5 @@
>      private SearchEngineBar mSearchEngineBar;
>  
> +    // Store search engine data, setSearchEngines is called with this object in onResume()
> +    // setSearchEngines should be called after fragment transaction occurs, so we store data until the transaction occurs
> +    private JSONObject searchEngineData;

nit: member variables in this file should be pre-fixed with `m`.

I think this name could be more descriptive to indicate its purpose. Maybe something like, "restorableSearchEngineData"

Also, the comment is confusing – I think we want to say is what this variable does (as opposed to where the code that uses it is called).

@@ +133,5 @@
> +
> +    //Boolean to keep track of whether the gecko thread is registered
> +    private boolean geckoThreadListenerRegistered;
> +
> +    private ArrayList<SearchEngine> searchEngines;

nit: This name is confusing – how is it different from mSearchEngines?

@@ +229,5 @@
>          super.onCreate(savedInstanceState);
>  
>          mSearchEngines = new ArrayList<SearchEngine>();
> +
> +        if(searchEngineData != null) {

Isn't onCreate only called when the Fragment is created (meaning searchEngineData will always be null)? I might be misunderstanding the Fragment lifecycle, however.

@@ +381,5 @@
>          mCursorLoaderCallbacks = new CursorLoaderCallbacks();
>          loadIfVisible();
>      }
>  
> +    //The message is now being received in BrowserApp which will call setSearchEngines,

This is not true anymore, right?

@@ +390,5 @@
> +            searchEngineData = message;
> +
> +            // If fragment transaction has already occurred then we should set the search engines
> +
> +            if(getActivity() != null) {

`getActivity() != null` seems to be ambiguous – why does that indicate whether the fragment transaction has already occurred? Would `Fragment.isAdded` be more appropriate here? If not, can you elaborate on the comment?

Also, why shouldn't we call `setSearchEngines` if the fragment transaction hasn't occurred yet? Add to the comment.

@@ +403,5 @@
>          }
>      }
>  
> +    // Called from BrowserApp to preload the Search Engines
> +    public void preloadSearchEngines()

This method is not called – I think you're missing your BrowserApp changes in here.

@@ +407,5 @@
> +    public void preloadSearchEngines()
> +    {
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("SearchEngines:GetVisible", null));
> +        EventDispatcher.getInstance().registerGeckoThreadListener(this, "SearchEngines:Data");
> +        geckoThreadListenerRegistered = true;

I'm not a huge fan of this boolean as it adds complexity and I'd like to get rid of it if possible – is there a way to ensure preloadSearchEngines is called after some lifecycle method that is guaranteed to be called? e.g. onCreate, onResume?

@@ +612,5 @@
>              mLastLocale = Locale.getDefault();
>  
> +            //Search engines are stored and if Browser search is hidden, updating the
> +            //SearchEngineBar will take place when the Browser search is made visible
> +            if (mView == null) {

Should this be `Fragment.isVisible`?
Attachment #8696330 - Flags: review?(michael.l.comella) → feedback+
Attached patch Bug927692 — — Splinter Review 
I've responded to your previous review below:
@@ +133,5 @@
It isn't needed, removed.

@@ +229,5 @@
	The loading of the search engines is started by a call in the BrowserApp (call of mBrowserSearch.preloadSearchEngines()), and when the data is loaded it is stored in mBrowserSearch (so it is stored in the fragment). onCreate() of the fragment is called after the fragment is attached. So at the time onCreate() is called, it is possible to have searchEngineData 

@@ +381,5 @@
Removed.

@@ +390,5 @@ 
Yeah Fragment.isAdded would be more appropriate. (I wasn't aware of the Fragment.isAdded earlier so I used the sloppy way of getActivity()).

Comment Added.


@@ +403,5 @@ 
Added the missing line in BrowserApp


@@ +612,5 @@
Changed mView == null to Fragment.isVisible


I am not sure how to approach the removal of the boolean(for checking whether the GeckoThreadListener is registered or not). The application became really prone to crashes when orientation changes occur (Due to repeated unregistering of the listener), therefore I used the boolean. The preloading is usually done before the BrowserSearch fragment is even attached, so I'm not quite sure how to ensure that preloadSearchEngines is called after a lifecycle method of BrowserSearch Fragment. Any suggestions?
Attachment #8696330 - Attachment is obsolete: true
Attachment #8701074 - Flags: review?(michael.l.comella)
Comment on attachment 8701074 [details] [diff] [review]
Bug927692

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

This patch doesn't apply – can you make sure to pull the latest revision and rebase? There was a patch that moved all of the file paths. You can use sed on the patch file to correct the paths:
 sed -i".bak" "s.mobile/android/base/.mobile/android/base/java/org/mozilla/gecko/.g" <input-file>

This should work directly in linux or with the `gnu-sed` package (so `gsed` binary) from homebrew on OS X. Then there are a few more merge conflicts.
---

I'm concerned that in the case that the user opens BrowserSearch and Gecko hasn't sent out "DelayedStartup" yet (probably possible on slower devices), this patch would be a regression in the time to load the search engines. Can we make `preloadSearchEngines` an idempotent operation and perhaps duplicate the call in a BrowserSearch lifecycle method (onViewCreated?)? (Then it should probably be `loadSearchEngines` or similar)

However, it may not be worth the added complexity – let me know what you think! If you can test on older devices (or want me to), we could look for issues that way.
---

(In reply to Aakash Muttineni from comment #37)
> I am not sure how to approach the removal of the boolean(for checking
> whether the GeckoThreadListener is registered or not). The application
> became really prone to crashes when orientation changes occur (Due to
> repeated unregistering of the listener), therefore I used the boolean. The
> preloading is usually done before the BrowserSearch fragment is even
> attached, so I'm not quite sure how to ensure that preloadSearchEngines is
> called after a lifecycle method of BrowserSearch Fragment. Any suggestions?

Yeah, I can see that. The only way I can think of is to match register/unregister to opposing lifecycle methods that are guaranteed to be called, but it doesn't look like this matches our lifecycle. Boolean wfm.
---

I'm in the middle of reviewing the actual code – I'll finish that up today or, at latest, tomorrow.

::: mobile/android/base/home/BrowserSearch.java
@@ -339,5 @@
>              }
>          });
>  
>          registerForContextMenu(mList);
> -        EventDispatcher.getInstance().registerGeckoThreadListener(this,

If `onDestroyView` is ever called during the lifecycle but the fragment is not destroyed, it is my understanding that the listener won't be attached.

I think you should leave this method in, but guard it with `if (!geckoThreadListenerRegistered)`
Comment on attachment 8701074 [details] [diff] [review]
Bug927692

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

We're going in the right direction but there are still a few unanswered questions.

::: mobile/android/base/home/BrowserSearch.java
@@ -234,5 @@
>          super.onResume();
>  
> -        // Fetch engines if we need to.
> -        if (mSearchEngines.isEmpty() || !Locale.getDefault().equals(mLastLocale)) {
> -            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("SearchEngines:GetVisible", null));

If we remove this broadcast, afaik, we'll never update the search engines data again (e.g. if the prefs change). (in fact, it seems wrong that we only do this if mSearchEngines is empty – perhaps we have a preference change listener?).

I think this needs some clarification.

@@ +380,5 @@
>          if (event.equals("SearchEngines:Data")) {
> +            mRestorableSearchEngineData = message;
> +            // If fragment transaction has already occurred then we should set the search engines
> +            //otherwise, setSearchEngines will be called in onActivityCreated() just after fragment transaction occurs
> +            if(this.isAdded()) {

I feel like this condition should be the negation of the condition we use to set the search engines in `onActivityCreated` so we only do this once – do you agree? I'm not sure `onActivityCreated` being called necessitates `isAdded()` from being true.

@@ +602,5 @@
>              mLastLocale = Locale.getDefault();
>  
> +            //Search engines are stored and if Browser search is hidden, updating the
> +            //SearchEngineBar will take place when the Browser search is made visible
> +            if (!this.isVisible()) {

Why don't we want to update the search engine bar if it's not visible again? Is this an optimization or does it cause issues?
Attachment #8701074 - Flags: review?(michael.l.comella)
Comment on attachment 8701074 [details] [diff] [review]
Bug927692

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

We're going in the right direction but there are still a few unanswered questions.

::: mobile/android/base/home/BrowserSearch.java
@@ -234,5 @@
>          super.onResume();
>  
> -        // Fetch engines if we need to.
> -        if (mSearchEngines.isEmpty() || !Locale.getDefault().equals(mLastLocale)) {
> -            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("SearchEngines:GetVisible", null));

If we remove this broadcast, afaik, we'll never update the search engines data again (e.g. if the prefs change). (in fact, it seems wrong that we only do this if mSearchEngines is empty – perhaps we have a preference change listener?).

I think this needs some clarification.

@@ +380,5 @@
>          if (event.equals("SearchEngines:Data")) {
> +            mRestorableSearchEngineData = message;
> +            // If fragment transaction has already occurred then we should set the search engines
> +            //otherwise, setSearchEngines will be called in onActivityCreated() just after fragment transaction occurs
> +            if(this.isAdded()) {

I feel like this condition should be the negation of the condition we use to set the search engines in `onActivityCreated` so we only do this once – do you agree? I'm not sure `onActivityCreated` being called necessitates `isAdded()` from being true.

@@ +602,5 @@
>              mLastLocale = Locale.getDefault();
>  
> +            //Search engines are stored and if Browser search is hidden, updating the
> +            //SearchEngineBar will take place when the Browser search is made visible
> +            if (!this.isVisible()) {

Why don't we want to update the search engine bar if it's not visible again? Is this an optimization or does it cause issues?
Attachment #8701074 - Flags: feedback+
This is a cool optimization but given that most users will probably 1) start Fennec hot, 2) open fennec from an external site and never see BrowserSearch, or 3) have phones fast enough to not even notice, I'm going to WONTFIX to keep things simple.

If we were to simplify the existing code to make the lazy load simple as well, it could be worth it but it'd be a lot of rejiggering and we can better spend our time elsewhere. Let's re-open this if it becomes an issue again.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
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: