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)
Tracking
(firefox31 wontfix, firefox32 wontfix, firefox33 wontfix, firefox34 verified, firefox35 verified, fennec+)
VERIFIED
FIXED
Firefox 35
People
(Reporter: RyanVM, Assigned: lviknesh, Mentored)
Details
(Whiteboard: [lang=java])
Attachments
(1 file, 1 obsolete file)
2.12 KB,
patch
|
bnicholson
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Comment 2•10 years ago
|
||
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.
status-firefox31:
--- → affected
status-firefox32:
--- → affected
status-firefox33:
--- → affected
status-firefox34:
--- → affected
Flags: needinfo?(bnicholson)
Reporter | ||
Updated•10 years ago
|
tracking-fennec: --- → ?
Updated•10 years ago
|
Mentor: bnicholson
tracking-fennec: ? → +
Whiteboard: [lang=java]
Assignee | ||
Comment 3•10 years ago
|
||
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 :)
Updated•10 years ago
|
Assignee: nobody → lviknesh
Status: NEW → ASSIGNED
Comment 4•10 years ago
|
||
(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
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8484665 -
Flags: feedback?
Assignee | ||
Comment 6•10 years ago
|
||
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 :)
Updated•10 years ago
|
Attachment #8484665 -
Flags: feedback? → feedback?(bnicholson)
Comment 7•10 years ago
|
||
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+
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
Try push https://tbpl.mozilla.org/?tree=Try&rev=d69f4216ba7d
Comment 12•10 years ago
|
||
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+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 13•10 years ago
|
||
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
Reporter | ||
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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 17•10 years ago
|
||
Comment on attachment 8489556 [details] [diff] [review] bookmark-keyword-updated Aurora+
Attachment #8489556 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Reporter | ||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/dec5cef6d119
Reporter | ||
Comment 19•10 years ago
|
||
Verified on my phone this morning :)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•