1049650 - Searching for something in search activity it will open a new about:blank page in stead of search results

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[Flaviu Cos]

Environment: 
Device: Asus Transformer Tab (Android 4.2.1);
Build: Nightly 34.0a1 (2014-08-06);

Steps to reproduce:
1. Launch Nightly Search;
2. Search for something;

Expected result:
A new tab is opened in Firefox containing the search results.

Actual result:
A new about:blank page is opened in Firefox.

Comment 1

[:Margaret Leibovic]

This sounds like it was caused by bug 1043522.

I assume our "is this still a search result page?" check is saying "nope" and launching an intent.

Comment 2

[Eric Edens [:eedens]]

I was able to repro this on an HTC Glacier 2.3.7; patch coming shortly. :)

Comment 3

[Eric Edens [:eedens]]

Attachment: bug-1049650-fix.patch

Fix confirmed on HTC Glacier 2.3.7

PR: https://github.com/ericedens/FirefoxSearch/pull/50

Comment 4

[:Margaret Leibovic]

Comment on attachment 8468562 [details] [diff] [review]
bug-1049650-fix.patch

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

This is a good approach, but I want us to change things around a bit to make this logic more intuitive.

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +70,5 @@
>       * @param url to test.
>       * @return true if <code>url</code> is a page of search results.
>       */
>      protected boolean isSearchResultsPage(String url) {
> +        return TextUtils.equals(ABOUT_BLANK, url) || url.contains(Constants.YAHOO_WEB_SEARCH_RESULTS_FILTER);

I think we should flip around the logic here to make this method return true if we want to launch an intent with this URL, since adding this about:blank check doesn't align with the semantics of this method name.

So we could rename it to something like `isExternalURL` or `shouldOpenInBrowser`, and then change around the logic where it's used.

We can also make this method private.

Comment 5

[Eric Edens [:eedens]]

Attachment: bug-1049650-fix.v2.patch

> This is a good approach, but I want us to change things around a bit to make
> this logic more intuitive.

Good call :)

Tested again with two gingerbread phones and a 4.4 phone.

Comment 6

[Eric Edens [:eedens]]

Also, removed the noop call to
  
  super.onPageStarted(view, url, favicon);

Comment 7

[:Margaret Leibovic]

Comment on attachment 8468592 [details] [diff] [review]
bug-1049650-fix.v2.patch

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

Nice. I'll land this for you on fx-team.

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +96,5 @@
>  
>          @Override
>          public void onPageStarted(WebView view, String url, Bitmap favicon) {
> +            if (shouldSendToBrowser(url)) {
> +                view.stopLoading();

Good call to stop the WebView. Does this mean that without this patch we're still loading the page in the WebView while we load it in Fennec?

Comment 8

[Eric Edens [:eedens]]

> Good call to stop the WebView. Does this mean that without this patch we're
> still loading the page in the WebView while we load it in Fennec?

It was just a rearrangement -- `view.stopLoading()` used to be below the call to telemetry; I just moved it above so that we'd stop the loading as quickly as possible.

Comment 9

[:Margaret Leibovic]

(In reply to Eric Edens [:eedens] from comment #8)
> > Good call to stop the WebView. Does this mean that without this patch we're
> > still loading the page in the WebView while we load it in Fennec?
> 
> It was just a rearrangement -- `view.stopLoading()` used to be below the
> call to telemetry; I just moved it above so that we'd stop the loading as
> quickly as possible.

Oh, sorry, didn't notice that :)

https://hg.mozilla.org/integration/fx-team/rev/1082f601f769

Comment 10

[Eric Edens [:eedens]]

https://github.com/ericedens/FirefoxSearch/commit/d5a3c985300ee33214973f8e75301239cad5be84

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1082f601f769

Comment 12

[Flaviu Cos]

Verified as fixed in build 34.0a1 (2014-08-07);
Device: Samsung Galaxy Nexus (Android 4.2.1).