First pass - needs a few touch-ups such as checking existing bookmark keywords and better UI for editing aliases.

(In reply to comment #1) > This patch adds the ability to assign keywords to search plugins, essentially mimicking the current bookmark keyword functionality. It essentially turns search plugins into pseudo-bookmarks allowing them to be addressed in the address bar by keyword alone (in which case the plugin's SearchForm property or URL minus query params is loaded) or paired with terms to search for. In the engine manager, the current tree has a keyword column and edit button added for changing keywords (and possibly names later down the road). The manager also disallows the use of keywords already in use by other plugins or bookmarks, but the same is not done for bookmarks themselves (which currently can use keywords already in use by other bookmarks anyway). If a bookmark is assigned a keyword that's already assigned to a search plugin, the plugin takes precedence and will be used instead. https://addons.mozilla.org/en-US/firefox/addon/4328 (minus the duplicate prevention) is almost identical to what's been implemented in this patch.

Comment on attachment 262733 [details] [diff] [review] Patch Yup, this is a great first step to enable the functionality. Some quick thoughts (which we should expand on in dev.apps.firefox) on where we go from here in addition to prettying up the UI a titch are: - ensuring that users understand how the keywords are used - figuring out how this matches up against existing bookmark keywords - figuring out the interaction model for when the user invokes a keyword

Comment on attachment 262733 [details] [diff] [review] Patch >Index: locales/en-US/chrome/browser/engineManager.properties >+editTitle=Edit Keyword >+editMsg=Enter a new keyword for %S: >+duplicateTitle=Duplicate Keyword >+duplicateMsg=You have chosen a keyword that is currently in use by %S. Please select another. >+duplicateBookmark=a bookmark Add a localization note that explains that the last string is substituted into the second last? >Index: locales/en-US/chrome/browser/engineManager.dtd >+<!ENTITY edit.label "Edit..."> Shouldn't this be "Edit Keyword...", or something similar? "Edit" alone seems a bit unclear, it gives me the impression that it should open a general "edit engine" dialog or something. >Index: components/search/content/engineManager.js >+ // Check for duplicates in changes we haven't commited yet nit: "committed" >+ if (alias.value != '' && eduplicate || bduplicate) { nit: parenthesize to make this clearer (could use a comment, even) >+ title = strings.getString("duplicateTitle"); >+ bookmark = strings.getString("duplicateBookmark"); >+ msg = strings.getFormattedString("duplicateMsg", >+ [(bduplicate) ? bookmark : engine.name]); Perhaps we should add quotes around the engine name, when it's used. I'm not sure if that introduces any l10n issues (choices of quotes or anything like that), but we do it in other places so hopefully not. >+ prompt.alert(window, title, msg); >+ this.edit(); It might be a bit clearer (and more efficient) to implement this using a loop instead of recursion. >Index: components/search/content/engineManager.xul >- <command id="cmd_editalias" >- oncommand="gEngineManagerDialog.editAlias();" >+ <command id="cmd_edit" >+ oncommand="gEngineManagerDialog.edit();" I think this should be "editKeyword". >+ <button id="edit" >+ label="&edit.label;" >+ accesskey="&edit.accesskey;" >+ command="cmd_edit"/> Similarly, edit.label be something like "Edit Keyword", instead of just "Edit"? Just "Edit" is kind of ambiguous. >Index: base/content/browser.js >+function getShortcutOrURI(aURL, aPostDataRef) { >+ var engine = searchService.getEngineByAlias(cmd); > try { >- var shortcutURL = null; >+ if (engine) >+ shortcutURI = engine.getSubmission(text, "text/html").uri; > #ifdef MOZ_PLACES_BOOKMARKS >+ else >+ shortcutURI = PlacesUtils.bookmarks.getURIForKeyword(cmd); >+#endif >+ > if (shortcutURI) > shortcutURL = shortcutURI.spec; >+#ifndef MOZ_PLACES_BOOKMARKS >+ else >+ shortcutURL = BMSVC.resolveKeyword(cmd, aPostDataRef); > #endif I'd write this as: if (engine) shortcutURL = engine.getSubmission(text, null).uri.spec; else #ifdef MOZ_PLACES_BOOKMARKS shortcutURL = PlacesUtils.bookmarks.getURIForKeyword(cmd).spec; #else shortcutURL = BMSVC.resolveKeyword(cmd, aPostDataRef); #endif and get rid of shortcutURI (it's in a try/catch so you don't need to worry about getSubmission or getURIForKeyword returning null).

Another thing I think I forgot to mention: we should remove aliases when "removing" a default engine, because otherwise the names could still conflict even though the engine isn't visible to the user (since we actually keep the engine around in that case). If you don't want to do that in this patch, it should be done in a bug that blocks this one, at least.

Comment on attachment 262733 [details] [diff] [review] Patch Waiting for an update here.

- Renames "Edit..." to "Edit Keyword..." - Renames edit to editKeyword - Fixes bitrot from bug 329842 and incorporates the getShortcutOrURI bits from bug 317472 in browser.js - Removes aliases when hiding default engines - Adds quotes around engine names when referred to in either dialog - Uses a loop in gEngineManagerDialog::editKeyword() rather than recursion

Comment on attachment 265908 [details] [diff] [review] Patch v2 >Index: base/content/browser.js >+function getShortcutOrURI(aURL, aPostDataRef) { You didn't address the last part of comment 4, any reason why? My example code just needs a "aPostDataRef.value = PlacesUtils.getPostDataForURI(shortcutURI);" and some brackets added, I think. >+ var shortcutURL = null; >+ var cmd = null; >+ var text = null; nit: use "" for text. "keyword" and "param" might be better variable names for "cmd" and "text"? >+ var offset = aURL.indexOf(" "); >+ var searchService = Cc["@mozilla.org/browser/search-service;1"]. >+ getService(Ci.nsIBrowserSearchService); >+ >+ if (offset > 0) { nit: declare offset after searchService, next to where it's first used. >+ cmd = aURL.substr(0, offset); >+ text = aURL.substr(offset + 1); >+ } >+ else >+ cmd = aURL; Initialize cmd to aURL at declaration, remove this else? >+ if (engine) >+ shortcutURL = engine.getSubmission(text, null).uri.spec; > #ifdef MOZ_PLACES_BOOKMARKS >+ else { >+ var shortcutURI = PlacesUtils.bookmarks.getURIForKeyword(cmd); >+ if (shortcutURI) { >+ shortcutURL = shortcutURI.spec; >+ aPostDataRef.value = PlacesUtils.getPostDataForURI(shortcutURI); >+ } > } > #else >+ else >+ shortcutURL = BMSVC.resolveKeyword(cmd, aPostDataRef); > #endif You can return early here if !shortcutURL, I believe, and remove the two null checks. > #ifndef MOZ_PLACES_BOOKMARKS >- // FIXME: Bug #317472, we don't have last charset in places yet. >- else if (/%s/.test(shortcutURL) || >- (aPostDataRef && /%s/.test(aPostDataRef.value))) { >- try { >- charset = BMSVC.getLastCharset(shortcutURL); >- } catch (ex) { >- } >- } >+ charset = BMSVC.getLastCharset(shortcutURL); >+#else >+ charset = PlacesUtils.getLastCharSetForURI(PlacesUtils._uri(shortcutURL)); > #endif The FIXME comment you're removing refers to bug 317472, which doesn't seem to be fixed yet. Is getLastCharSetForURI really what we want? Should that bug be marked fixed? If it is, flip those two lines around and change the #ifndef to an #ifdef, that "n" can be easy to miss and complicates thing unnecessarily. >+ } catch (ex) {} I don't really like silently discarding exceptions here... add a Components.utils.reportError, at least? And maybe a comment that says what you exception this is supposed to catch? >+ if (charset) >+ encodedText = escape(convertFromUnicode(charset, text)); >+ else // default case: charset=UTF-8 >+ encodedText = encodeURIComponent(text); >+ >+ if (aPostDataRef && aPostDataRef.value) { >+ // XXXben - currently we only support "application/x-www-form-urlencoded" >+ // enctypes. >+ aPostDataRef.value = unescape(aPostDataRef.value); >+ if (aPostDataRef.value.match(/%[sS]/)) { Not your code, but this should use /%s/i.test() (same with the /%[sS]/ below). >+ if (shortcutURL) >+ aURL = shortcutURL; This could be refactored to remove the assignment to aURL and just return shortcutURL directly from the two earlier branches (POST data and not - these could use comments, too!) >Index: locales/en-US/chrome/browser/engineManager.properties >+# Localization Note: duplicateBookmark is substituted into duplicateMsg. >+duplicateBookmark=a bookmark Should have noted this earlier, but should mention that the engine name could also be substituted in. I kind of fear that this could cause problems for locales that need to treat an engine name and "a bookmark" differently, but I'm not sure what better way there is to do this. >Index: components/search/content/engineManager.js >+ editKeyword: function engineManager_editKeyword() { >+ if (!engine) { Flip this around to have the "if (engine)" first, and you can avoid all of these checks if alias == "". >+ try { >+#ifdef MOZ_PLACES_BOOKMARKS >+ var bmserv = Cc["@mozilla.org/browser/nav-bookmarks-service;1"]. >+ getService(Ci.nsINavBookmarksService); >+ if (bmserv.getURIForKeyword(alias.value)) >+ bduplicate = true; >+#else >+ var bmserv = Cc["@mozilla.org/browser/bookmarks-service;1"]. >+ getService(Ci.nsIBookmarksService); >+ if (bmserv.resolveKeyword(alias.value, {})) >+ bduplicate = true; >+#endif >+ } catch(ex) {} Why is the try/catch needed here? >+ // Check for duplicates in changes we haven't committed yet >+ var engines = gEngineView._engineStore.engines; >+ for (var i = 0; i < engines.length; i++) { Could use a |for each| loop here. >+ // Notify the user if they have chosen an existing engine/bookmark keyword >+ if (alias.value != '' && (eduplicate || bduplicate)) { No need for the alias.value check, since it should always be true if (eduplicate || bduplicate), right? >Index: components/search/nsSearchService.js >+ if (engineToRemove._isDefault) >+ engineToRemove.alias = null; Add a comment here to describe why we're removing the alias? I think this should be in the if (engineToRemove._readOnly) block just below this, since that's what we use to determine whether we hide or delete, and you want to remove it in all cases where we hide.

(In reply to comment #8) > (From update of attachment 265908 [details] [diff] [review]) > >Index: base/content/browser.js > > >+function getShortcutOrURI(aURL, aPostDataRef) { > > You didn't address the last part of comment 4, any reason why? My example code > just needs a "aPostDataRef.value = PlacesUtils.getPostDataForURI(shortcutURI);" > and some brackets added, I think. > done. > >+ var shortcutURL = null; > >+ var cmd = null; > >+ var text = null; > > nit: use "" for text. "keyword" and "param" might be better variable names for > "cmd" and "text"? > done. > >+ var offset = aURL.indexOf(" "); > >+ var searchService = Cc["@mozilla.org/browser/search-service;1"]. > >+ getService(Ci.nsIBrowserSearchService); > >+ > >+ if (offset > 0) { > > nit: declare offset after searchService, next to where it's first used. done. > >+ cmd = aURL.substr(0, offset); > >+ text = aURL.substr(offset + 1); > >+ } > >+ else > >+ cmd = aURL; > > Initialize cmd to aURL at declaration, remove this else? done. > >+ if (engine) > >+ shortcutURL = engine.getSubmission(text, null).uri.spec; > > #ifdef MOZ_PLACES_BOOKMARKS > >+ else { > >+ var shortcutURI = PlacesUtils.bookmarks.getURIForKeyword(cmd); > >+ if (shortcutURI) { > >+ shortcutURL = shortcutURI.spec; > >+ aPostDataRef.value = PlacesUtils.getPostDataForURI(shortcutURI); > >+ } > > } > > #else > >+ else > >+ shortcutURL = BMSVC.resolveKeyword(cmd, aPostDataRef); > > #endif > > You can return early here if !shortcutURL, I believe, and remove the two null > checks. done. > > #ifndef MOZ_PLACES_BOOKMARKS > >- // FIXME: Bug #317472, we don't have last charset in places yet. > >- else if (/%s/.test(shortcutURL) || > >- (aPostDataRef && /%s/.test(aPostDataRef.value))) { > >- try { > >- charset = BMSVC.getLastCharset(shortcutURL); > >- } catch (ex) { > >- } > >- } > >+ charset = BMSVC.getLastCharset(shortcutURL); > >+#else > >+ charset = PlacesUtils.getLastCharSetForURI(PlacesUtils._uri(shortcutURL)); > > #endif > > The FIXME comment you're removing refers to bug 317472, which doesn't seem to > be fixed yet. Is getLastCharSetForURI really what we want? Should that bug be > marked fixed? If it is, flip those two lines around and change the #ifndef to > an #ifdef, that "n" can be easy to miss and complicates thing unnecessarily. > I forgot I had applied the patch in 317472 and thought getLastCharSetForURI was already in places. I've left it as #ifndef for now and re-added the note about bug 317472. > >+ } catch (ex) {} > > I don't really like silently discarding exceptions here... add a > Components.utils.reportError, at least? And maybe a comment that says what you > exception this is supposed to catch? done. > >+ if (charset) > >+ encodedText = escape(convertFromUnicode(charset, text)); > >+ else // default case: charset=UTF-8 > >+ encodedText = encodeURIComponent(text); > >+ > >+ if (aPostDataRef && aPostDataRef.value) { > >+ // XXXben - currently we only support "application/x-www-form-urlencoded" > >+ // enctypes. > >+ aPostDataRef.value = unescape(aPostDataRef.value); > >+ if (aPostDataRef.value.match(/%[sS]/)) { > > Not your code, but this should use /%s/i.test() (same with the /%[sS]/ below). done. > >+ if (shortcutURL) > >+ aURL = shortcutURL; > > This could be refactored to remove the assignment to aURL and just return > shortcutURL directly from the two earlier branches (POST data and not - these > could use comments, too!) done. > >Index: locales/en-US/chrome/browser/engineManager.properties > > >+# Localization Note: duplicateBookmark is substituted into duplicateMsg. > >+duplicateBookmark=a bookmark > > Should have noted this earlier, but should mention that the engine name could > also be substituted in. I kind of fear that this could cause problems for > locales that need to treat an engine name and "a bookmark" differently, but I'm > not sure what better way there is to do this. Done, including a note that if an engine name is displayed it'll be in quotes. > >Index: components/search/content/engineManager.js > > >+ editKeyword: function engineManager_editKeyword() { > > >+ if (!engine) { > > Flip this around to have the "if (engine)" first, and you can avoid all of > these checks if alias == "". done. > >+ try { > >+#ifdef MOZ_PLACES_BOOKMARKS > >+ var bmserv = Cc["@mozilla.org/browser/nav-bookmarks-service;1"]. > >+ getService(Ci.nsINavBookmarksService); > >+ if (bmserv.getURIForKeyword(alias.value)) > >+ bduplicate = true; > >+#else > >+ var bmserv = Cc["@mozilla.org/browser/bookmarks-service;1"]. > >+ getService(Ci.nsIBookmarksService); > >+ if (bmserv.resolveKeyword(alias.value, {})) > >+ bduplicate = true; > >+#endif > >+ } catch(ex) {} > > Why is the try/catch needed here? I believe resolveKeyword will throw if there's a problem with the datasource, but I'm not certain and just followed getShortcutOrURI's lead ;) > >+ // Check for duplicates in changes we haven't committed yet > >+ var engines = gEngineView._engineStore.engines; > >+ for (var i = 0; i < engines.length; i++) { > > Could use a |for each| loop here. done. > >+ // Notify the user if they have chosen an existing engine/bookmark keyword > >+ if (alias.value != '' && (eduplicate || bduplicate)) { > > No need for the alias.value check, since it should always be true if > (eduplicate || bduplicate), right? Right, fixed. > >Index: components/search/nsSearchService.js > > >+ if (engineToRemove._isDefault) > >+ engineToRemove.alias = null; > > Add a comment here to describe why we're removing the alias? I think this > should be in the if (engineToRemove._readOnly) block just below this, since > that's what we use to determine whether we hide or delete, and you want to > remove it in all cases where we hide. done. I also did a little clean up on getShortcutOrURI()'s helper functions removing a 1.8 branch ifdef, swapping to Cc/Ci and even though it's currently useless, changed the loop on normalizePostData().

Comment on attachment 267269 [details] [diff] [review] Patch v3 >Index: base/content/browser.js >+function getShortcutOrURI(aURL, aPostDataRef) { >+ var engine = searchService.getEngineByAlias(keyword); > try { >- var shortcutURL = null; >+ if (engine) >+ aURL = engine.getSubmission(param, null).uri.spec; Avoid assigning to aURL, just return from here. Which means that you can get rid of the else and no longer need: >+ if (!shortcutURL) >+ return aURL; >+ var encodedText = ""; encodedText is confusing, use "encodedParam"? >+ if (charset) >+ encodedText = escape(converFromUnicode(charset, param)); >+ else // Default charset is UTF-8 >+ encodedText = encodeURIComponent(param); >+ >+ // Format this request for POSTing, otherwise substitute param into the query string. >+ if (postData.match(/%s/i)) { /%s/i.test(postData) >+ //XXX Currently we only support "application/x-www-form-urlencoded enctypes. >+ aPostDataRef.value = getPostDataStream(postData, param, encodedText, >+ "application/x-www-form-urlencoded"); >+ return shortcutURL; > } >+ else >+ aPostDataRef.value = null; Else after return. Just change the "else" to a comment saying "not a POST keyword", or something like that (and add a newline). >+ if (/%s/i.test(shortcutURL)) >+ aURL = shortcutURL.replace(/%s/g, encodedText).replace(/%S/g, param); > return aURL; I think this code would be better factored if you first did the /%s/i.test(postData) and (/%s/i.test(shortcutURL)), returning aURL early if both of those are false, and then doing the charset stuff afterwards. The overall flow for this function, taking into account these comments, would be something like: get keyword and param if (has engine for keyword) getsubmission return submission.uri.spec get bookmark for keyword if (no bookmark for keyword) return aURL if (is GET BM keyword (/%s/i.test(shortcutURL)) or is POST BM keyword (/%s/i.test(postData)) get charset replace %s and %S in shortcutURL/postData accordingly return shortcutURL >Index: locales/en-US/chrome/browser/engineManager.properties >+# Localization Note: The substitution in duplicateMsg will either be a search >+# plugin name (in quotes) or the duplicateBookmark string below. Give an example of each case, to make this easy to understand? >Index: components/search/content/engineManager.js >+ editKeyword: function engineManager_editKeyword() { >+ var dtitle = strings.getString("duplicateTitle"); >+ var bookmark = strings.getString("duplicateBookmark"); >+ var dmsg = strings.getFormattedString("duplicateMsg", >+ [(bduplicate) ? bookmark : '"' + >+ engine.name + '"']); We might end up having to change this to a fully localized string, if the hard coded quotes aren't suitable here for some locales, but we can cross that bridge when we come to it, I think. > onSelect: function engineManager_onSelect() { >+ var engineSelected = (gEngineView.selectedIndex == -1); ITYM "noEngineSelected" :) It doesn't look like we do anything to prevent users from later adding a bookmark keyword with the same value as an existing search engine keyword. Perhaps we should? Can be a followup. Sorry if I didn't suggest some of these things in the previous review cycles, we probably should have nailed down the desired control flow before having you iterate this much. It should be a quick review once these get addressed.

Comment on attachment 270010 [details] [diff] [review] Patch v4 r=me with the return getSubmission() removed from the try/catch, as discussed on IRC, and a followup filed for the problem when removing a default engine with an alias and then restoring it without committing the change (alias is still displayed even though it doesn't exist).

mozilla/browser/base/content/browser.js 1.806 mozilla/browser/locales/en-US/chrome/browser/engineManager.dtd 1.5 mozilla/browser/locales/en-US/chrome/browser/engineManager.properties 1.1 mozilla/browser/components/search/nsSearchService.js 1.96 mozilla/browser/components/search/content/engineManager.js 1.15 mozilla/browser/components/search/content/engineManager.xul 1.8

mozilla/browser/locales/jar.mn 1.67

(In reply to comment #10) > >Index: components/search/content/engineManager.js > > >+ editKeyword: function engineManager_editKeyword() { > > >+ var dtitle = strings.getString("duplicateTitle"); > >+ var bookmark = strings.getString("duplicateBookmark"); > >+ var dmsg = strings.getFormattedString("duplicateMsg", > >+ [(bduplicate) ? bookmark : '"' + > >+ engine.name + '"']); > > We might end up having to change this to a fully localized string, if the hard > coded quotes aren't suitable here for some locales, but we can cross that > bridge when we come to it, I think. Quotes are only part of the problem, not the most imporant one, though. I really can't localize a sentence like this one into Polish. The word "bookmark" is declensed differently than the word for "search engine" - which I need to use as a prefix, so that I don't need to declense the (unknown!) name of the search engine. I mean, the Polish translation would be: "Wybrałeś słowo kluczowe używane przez wyszukiwarkę %S", where "wyszukiwarkę" is the declensed word "wyszukiwarka" = search engine. This translated back into English would be: "You have chosen a keyword that is currently in use by the %S search engine." so we CAN'T really have %S evaluate to "a bookmark". This should be split into two strings, like this: duplicateMsgEngine=You have chosen a keyword that is currently in use by %S. Please select another. duplicateMsgBookmark=You have chosen a keyword that is currently in use by a bookmark. Please select another. reopening, ccing Pike.

You should generally only reopen bugs if a) the patch has been backed out, b) the patch doesn't fix the problem or c) you think the patch should be backed out. I don't think any of those apply here - please file a new bug about the localization issue, CC me and Ryan, and set priority/blocking flags as you see fit.

(That being said, I should have requested that that issue be fixed instead of just mentioning it - my bad. We'll fix it quickly in the new bug.)

I assume that it's not going to be long until we back out patches for intl issues. I'll raise the issue on the next granparadiso meeting.

I don't think it makes sense to back out patches for any issue that can be quickly fixed, l10n or not.

Really neat feature, but it would be even greater if it worked with unicode characters. So far i got no luck with letters like ąčęėįšųūž (Lithuanian language). Just a note, I am almost sure you are aware of this problem.

(In reply to comment #20) > Really neat feature, but it would be even greater if it worked with unicode > characters. So far i got no luck with letters like ąčęėįšųūž > (Lithuanian language). > > Just a note, I am almost sure you are aware of this problem. > That's likely bug 387723.

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102504 Minefield/3.0a9pre.

https://litmus.mozilla.org/show_test.cgi?id=4686 has been added to cover the Litmus test case request.