658242 - Search in bookmarks doesn't find javascript: bookmarks

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: 2011-05-19 nightly

STEPS TO REPRODUCE:
1)  Start with a new profile.
2)  Bookmarks > Show All Bookmarks
3)  Select "Unsorted Bookmarks"
4)  Context-click the right-hand pane, select "New Bookmark..."
5)  Type "Pushlog range" for the name.
6)  Type "javascript:foopy" for the location.
7)  Click in the search field at top right
8)  Type "range"
9)  Select the "Unsorted Bookmarks" button for "Search In"

ACTUAL RESULTS: Bookmark created in steps 4-6 not found.

EXPECTED RESULTS: Bookmark found

ADDITIONAL NOTES: I tried some other schemes: "http", "data", "javascrip" (missing 't').  All of those work.

Comment 1

[Marco Bonardo [:mak]]

Interesting, if I search "javascript:" it appears.
I think this breaks a common use of bookmarklets and search, so giving it some priority.

Comment 2

[Alice0775 White]

Regression window(m-c hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/2e5e4197b9db
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110418 Firefox/6.0a1 ID:20110418013520
Fails:
http://hg.mozilla.org/mozilla-central/rev/202537779786
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110418 Firefox/6.0a1 ID:20110418054324
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2e5e4197b9db&tochange=202537779786

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hrm.  The frecency changes somehow?

Comment 4

[Marco Bonardo [:mak]]

yes, that bug touched searches code. Thanks.

Comment 5

[Shawn Wilsher :sdwilsh]

I thought we did this deliberately...

Comment 6

[Marco Bonardo [:mak]]

no, what we did is not storing javascript: in history, not bookmarks.

Comment 7

[Marco Bonardo [:mak]]

well, while cooking I rethought to what you said about we did this deliberately, actually you were half right.
Among other changes, I made the searches reuse a part of the locationbar search code, and in the locationbar we don't match on javascript: uris, unless you type "javascript:". This is exactly what happens!
I guess I should just add a param to AUTOCOMPLETE_MATCH to tell if it should skip javascript: uris or not.

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1.0

I'm not 100% sure about naming of the argument but I don't have better ideas (actually I'd love if the behavior would be better configurable and I'd not need to add an argument :\).

Comment 9

[Marco Bonardo [:mak]]

bah I didn't add the test to the diff

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 11

[Shawn Wilsher :sdwilsh]

I'm a bit concerned about this because I know there are add-ons out there that use this SQL method now.  Can we fix this without changing that?

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 533939 [details] [diff] [review]
patch v1.1

Yeah, I suppose it would be better to evaluate a non-breaking approach, but I didn't have time to think about it yet. Clearing review request since I share your concern.

For now a simple workaround is to search for "javascript:", this should return all existing bookmarklets.

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v2.0

This is an alternative approach that doesn't touch the interface, apart adding a constant, that is allowed without uuid changes.
It introduces a MATCH_ANYWHERE_UNMODIFIED that acts on pure underlying data skipping any transformation of it.
Thoughts?

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 542785 [details] [diff] [review]
patch v2.0

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

looks a-ok, r=me.

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/bb92a1746f59