Closed Bug 1053994 Opened 10 years ago Closed 10 years ago

Paste & Go attempts a Google search instead of recognizing a bookmark keyword

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox31 wontfix, firefox32 wontfix, firefox33 wontfix, firefox34 verified, firefox35 verified, fennec+)

VERIFIED FIXED
Firefox 35
Tracking Status
firefox31 --- wontfix
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- verified
firefox35 --- verified
fennec + ---

People

(Reporter: RyanVM, Assigned: lviknesh, Mentored)

Details

(Whiteboard: [lang=java])

Attachments

(1 file, 1 obsolete file)

I have a Bugzilla bookmark keyword for bug <whatever> that performs a quick search on whatever term I give it. On desktop, if I Paste & Go "bug XXXXXX" it does the proper action for the keyword.

Using an Aurora build on my phone, if I Paste & Go "bug XXXXXX" it performs a Google search instead. If I paste the same thing into the location bar and then hit Go, it recognizes the keyword.

I would expect it to have behavior that is consistent with desktop and recognize the keyword regardless of how it's being fed into the location bar.
Brian, do you know what's going on here?
Flags: needinfo?(bnicholson)
Paste & Go calls Tabs#loadUrl directly, bypassing the other checks that happen in commitEditingMode() that include keyword lookup. Can reproduce on release, so this isn't a recent regression.
Flags: needinfo?(bnicholson)
tracking-fennec: --- → ?
Mentor: bnicholson
tracking-fennec: ? → +
Whiteboard: [lang=java]
Hai brian , i would like to work on this issue :) could you provide some info on how to start with this . I already have fennec build as i worked with 2 fennec bugs before . Thank you :)
Assignee: nobody → lviknesh
Status: NEW → ASSIGNED
(In reply to vikneshwar from comment #3)
> Hai brian , i would like to work on this issue :) could you provide some
> info on how to start with this . I already have fennec build as i worked
> with 2 fennec bugs before . Thank you :)

Hi vikneshwar, thanks for taking this!

Currently, Paste & Go just calls Tabs.getInstance().loadUrl() directly [1]. The logic that we care about that does keyword lookups is in commitEditingMode at [2].

Since commitEditingMode is coupled with editing mode, we'll want to split this into two methods. commitEditingMode should be stripped down so that it only contains the lines up until [3]. The remaining lines there should be moved to a separate method -- loadUrlOrKeywordSearch -- which is then called by commitEditingMode. That same method can then be called at [1] instead of Tabs#loadUrl.

Let me know if you have any questions!

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=bc145686164e#823
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=bc145686164e#1775
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=bc145686164e#1796
Attached patch bookmark-keyword (obsolete) — Splinter Review
Attachment #8484665 - Flags: feedback?
Sorry , for delaying this , i was busy for past few days . I have made changes as per the comment 4 , please look into it and let me know if further changes are needed :) Thank you :)
Attachment #8484665 - Flags: feedback? → feedback?(bnicholson)
Comment on attachment 8484665 [details] [diff] [review]
bookmark-keyword

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

This is close to what I was expecting, but before we can land, please build/test your changes to make sure they fix the bug! You can test your patch like this:

1) Edit a bookmark and set the keyword to "foo".
2) Copy the text "foo bar" on your Android clipboard.
3) Long press on the URL bar and choose "Paste & Go".

If this patch works correctly, it will not do a Google search for "foo bar". Instead, it will go to the URL of your bookmark.

I'm not sure whether you have any prior experience building Fennec. If not, please take a look at https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec. Please don't hesitate to ask if you need assistance. :)

::: mobile/android/base/BrowserApp.java
@@ +819,5 @@
>          final int itemId = item.getItemId();
>          if (itemId == R.id.pasteandgo) {
>              String text = Clipboard.getText();
>              if (!TextUtils.isEmpty(text)) {
> +                Tabs.getInstance().loadUrlOrKeywordSearch(text);

This will give a compilation error -- loadUrlOrKeywordSearch isn't defined in the Tabs class.
Attachment #8484665 - Flags: feedback?(bnicholson) → feedback+
Hi viknesh,

You said on IRC that you aren't able to download the necessary SDK due to data limitations. If you have an updated version of your patch, you can upload it and I can try building/testing for you. This isn't ideal since you can't actually test your changes, and it will be difficult to work on any other bugs you're interested in. Regardless, thanks for your work so far!
Flags: needinfo?(lviknesh)
Hai Brian , 
    Thanks for the concern , anyway i will try to test the patch tomorrow and ping you :)
Attachment #8484665 - Attachment is obsolete: true
Attachment #8489556 - Flags: feedback?(bnicholson)
Flags: needinfo?(lviknesh)
Comment on attachment 8489556 [details] [diff] [review]
bookmark-keyword-updated

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

This looks good to me. If you're able to test tomorrow and confirm it works, I'll switch this to an r+.

In the meantime, you could push this to tryserver now so that it can land immediately after you test it out.
Attachment #8489556 - Flags: feedback?(bnicholson) → feedback+
Comment on attachment 8489556 [details] [diff] [review]
bookmark-keyword-updated

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

Viknesh confirmed that he tested this, and the patch looks fine to me, so switching to r+.
Attachment #8489556 - Flags: feedback+ → review+
https://hg.mozilla.org/integration/fx-team/rev/d472f51a9ce4
Keywords: checkin-needed
Whiteboard: [lang=java] → [lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d472f51a9ce4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][fixed-in-fx-team] → [lang=java]
Target Milestone: --- → Firefox 35
Vikneshwar, thanks for fixing this! You've definitely made my life better by doing so :)

Don't suppose we can nominate this for Aurora as well since it's a pretty trivial bugfix?
Comment on attachment 8489556 [details] [diff] [review]
bookmark-keyword-updated

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Pasting strings with keywords doesn't do a keyword search.
[Describe test coverage new/current, TBPL]: None
[Risks and why]: Low risk, changes Paste & Go search path to use our existing keyword search functionality.
[String/UUID change made/needed]: None
Attachment #8489556 - Flags: approval-mozilla-aurora?
Comment on attachment 8489556 [details] [diff] [review]
bookmark-keyword-updated

Aurora+
Attachment #8489556 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified on my phone this morning :)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.