1045648 - Sometimes header is not removed from results page

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

I think we discussed this before, but I couldn't find a bug.

I believe this happens if you submit the same query twice in a row, so the WebView refreshes itself in a way that doesn't trigger our terror script.

Comment 1

[Eric Edens [:eedens]]

We inject the script using WebChromeClient's onReceivedTitle hook, which is invoked only when the title *changes*. When the same query is submitted for a second time, the title of the page probably is not going to change, and hence we aren't going to inject the script.

As a first fix, I'll write a patch that ensures the same URL doesn't get loaded into a webview that already has that URL.

Comment 2

[Eric Edens [:eedens]]

Attachment: bug-1045648-fix.patch

This makes sure that we don't re-load a URL if the webview is already loaded to that page.

Comment 3

[:Margaret Leibovic]

Comment on attachment 8464086 [details] [diff] [review]
bug-1045648-fix.patch

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

Sounds like a reasonable approach, since it also prevents us from doing unnecessary work in general. f+ for now, since you'll need to upload another patch for me to land anyway :)

::: mobile/android/search/java/org/mozilla/search/MainActivity.java
@@ +60,5 @@
>          startPostsearch();
>          storeQuery(query);
>  
>          ((PostSearchFragment) getSupportFragmentManager().findFragmentById(R.id.postsearch))
> +                .startSearch(query);

Looks like some extraneous changes ended up in here. That reminds me - you should finish up your patch for bug 1042943!

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +75,5 @@
>      public void setUrl(String url) {
> +        // Load the url if:
> +        //   a. The webview hasn't loaded a URL yet, or
> +        //   b. The webview is loaded to a *different* URL.
> +        if (webview.getUrl() == null || !webview.getUrl().equals(url)) {

You could replace this line with:

if (!TextUtils.equals(webview.getUrl(), url)) {

TextUtils will take care of the null checking for you.

Comment 4

[Eric Edens [:eedens]]

(In reply to :Margaret Leibovic from comment #3)
> Comment on attachment 8464086 [details] [diff] [review]
> bug-1045648-fix.patch
> 
> Review of attachment 8464086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sounds like a reasonable approach, since it also prevents us from doing
> unnecessary work in general. f+ for now, since you'll need to upload another
> patch for me to land anyway :)
> 
> ::: mobile/android/search/java/org/mozilla/search/MainActivity.java
> @@ +60,5 @@
> >          startPostsearch();
> >          storeQuery(query);
> >  
> >          ((PostSearchFragment) getSupportFragmentManager().findFragmentById(R.id.postsearch))
> > +                .startSearch(query);
> 
> Looks like some extraneous changes ended up in here. That reminds me - you
> should finish up your patch for bug 1042943!

Actually this was an intentional change -- it was something that I've wanted to do while I was in the area :)

RE bug 1042943: I put that aside for the moment in favor of the P1 nightly-blocking bugs.

 
> ::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
> @@ +75,5 @@
> >      public void setUrl(String url) {
> > +        // Load the url if:
> > +        //   a. The webview hasn't loaded a URL yet, or
> > +        //   b. The webview is loaded to a *different* URL.
> > +        if (webview.getUrl() == null || !webview.getUrl().equals(url)) {
> 
> You could replace this line with:
> 
> if (!TextUtils.equals(webview.getUrl(), url)) {
> 
> TextUtils will take care of the null checking for you.

Sounds good!

Comment 5

[Eric Edens [:eedens]]

Attachment: bug-1045648-fix.v2.patch

Switched to use TextUtils.equals

Comment 6

[:Margaret Leibovic]

(In reply to Eric Edens [:eedens] from comment #4)
> (In reply to :Margaret Leibovic from comment #3)
> > Comment on attachment 8464086 [details] [diff] [review]
> > bug-1045648-fix.patch
> > 
> > Review of attachment 8464086 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Sounds like a reasonable approach, since it also prevents us from doing
> > unnecessary work in general. f+ for now, since you'll need to upload another
> > patch for me to land anyway :)
> > 
> > ::: mobile/android/search/java/org/mozilla/search/MainActivity.java
> > @@ +60,5 @@
> > >          startPostsearch();
> > >          storeQuery(query);
> > >  
> > >          ((PostSearchFragment) getSupportFragmentManager().findFragmentById(R.id.postsearch))
> > > +                .startSearch(query);
> > 
> > Looks like some extraneous changes ended up in here. That reminds me - you
> > should finish up your patch for bug 1042943!
> 
> Actually this was an intentional change -- it was something that I've wanted
> to do while I was in the area :)

Oh, I didn't realize that the startSearch method was already implemented. Yeah, we should definitely do that!

Comment 7

[:Margaret Leibovic]

Comment on attachment 8464217 [details] [diff] [review]
bug-1045648-fix.v2.patch

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

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +75,5 @@
>  
>      public void setUrl(String url) {
> +        // Only load URLs if they're different than what's already
> +        // loaded in the webview.
> +        if (TextUtils.equals(webview.getUrl(), url)) {

This is the opposite of what you want :) Can you upload a new patch with a !

Comment 8

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #6)
> (In reply to Eric Edens [:eedens] from comment #4)
> > (In reply to :Margaret Leibovic from comment #3)
> > > Comment on attachment 8464086 [details] [diff] [review]
> > > bug-1045648-fix.patch
> > > 
> > > Review of attachment 8464086 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Sounds like a reasonable approach, since it also prevents us from doing
> > > unnecessary work in general. f+ for now, since you'll need to upload another
> > > patch for me to land anyway :)
> > > 
> > > ::: mobile/android/search/java/org/mozilla/search/MainActivity.java
> > > @@ +60,5 @@
> > > >          startPostsearch();
> > > >          storeQuery(query);
> > > >  
> > > >          ((PostSearchFragment) getSupportFragmentManager().findFragmentById(R.id.postsearch))
> > > > +                .startSearch(query);
> > > 
> > > Looks like some extraneous changes ended up in here. That reminds me - you
> > > should finish up your patch for bug 1042943!
> > 
> > Actually this was an intentional change -- it was something that I've wanted
> > to do while I was in the area :)
> 
> Oh, I didn't realize that the startSearch method was already implemented.
> Yeah, we should definitely do that!

Actually, while you're updating the patch, could you also get rid of the setUrl method? You could also make it private, but startSearch could also just call webview.loadUrl directly.

Comment 9

[Eric Edens [:eedens]]

(In reply to :Margaret Leibovic from comment #7)
> Comment on attachment 8464217 [details] [diff] [review]
> bug-1045648-fix.v2.patch
> 
> Review of attachment 8464217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
> @@ +75,5 @@
> >  
> >      public void setUrl(String url) {
> > +        // Only load URLs if they're different than what's already
> > +        // loaded in the webview.
> > +        if (TextUtils.equals(webview.getUrl(), url)) {
> 
> This is the opposite of what you want :) Can you upload a new patch with a !

Holy batman, good catch!

Comment 10

[Eric Edens [:eedens]]

Attachment: bug-1045648-fix.v3.patch

- Fixed the logic error around checking URL equality
- Pulled back the visibility of setUrl to private.

Comment 11

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/837781ec135a

Comment 12

[Eric Edens [:eedens]]

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/837781ec135a