411226 - Migrate SeaMonkey's smart browsing preferences to new pref window

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Stefan [:stefanh]]

Just filing this since I'm working on this...

Comment 1

[Robert Kaiser]

It might be a good idea to merge keywords and domain guessing into a "Location Bar Actions" groupbox or something like that, and merging the advanced autocomplete prefs into the autocomplete groupbox on that pane, improving the wording of those items a bit and killing the preview instead.
I doubt anyone who really needs that preview actually does find his way clicking through all the needed items to even see it, esp. as that is labeled as a dialog for advanced stuff.

Comment 2

[Stefan [:stefanh]]

I'd rather see that go in a separate bug.

Comment 3

[Stefan [:stefanh]]

Attachment: Migrate smart browsing prefs

Comment 4

[Stefan [:stefanh]]

Attachment: Migrate smart browsing prefs (-w version)

Comment 5

[Stefan [:stefanh]]

Neil, Ian - see comment #1. If there's a general agreement upon this, I guess I can do it here. There's no point adding a file that we know will be removed. That said, for 3,5 years I've been waiting for the right occasion to remove the "More information button" and put the link in Help instead (bug 156792). Perhaps this is the right occasion? ;-)

Comment 6

[Robert Kaiser]

Actually, yes, I think it's the right occasion to fix bug 156792 - and I'm with you that it's probably better to not port a file that will be removed. So, if the reviewers and you agree, it might be best to do this all at once.

Comment 7

[Robert Kaiser]

Here's a proposal from me in ASCII art:

+-------------------------------------------------------------------------+
| Smart Browsing                                                          |
+-------------------------------------------------------------------------+
+-- Handling Unknown Locations -------------------------------------------+
| [x] Perform a web search if entered text can not be recognized as a     |
|     valid web location                                                  |
| [ ] Automatically add "www." and ".com" to the entered location if      |
|     no web page can be found for it                                     |
+-------------------------------------------------------------------------+
+-- Location Bar Autocomplete --------------------------------------------+
| [x] Automatically complete text entered into the Location Bar           |
|    [ ] Autocomplete best match as you type                              |
|    [x] Show dropdown list of matching results                           |
|    [x] Show internet search engine in dropdown                          |
|    [ ] Match only websites you've manually typed previuosly             |
+-------------------------------------------------------------------------+

Note that this fixes bug 156792 along the way, help should point out tat the default search engine is used, that this feature is sometimes called "Internet Keywords" and link to the external URL for more info.
Also, help should point out the name "Domain Guessing" often used for the second feature, and that autocomplete uses browsing history as base for the suggestions.
I'm almost tempted to even suggest renaming the pref pane to "Location Bar", as I don't think normal users know that "Smart Browsing" means tweaks about the location bar.
My proposal also indicates a change in defaults for "Handling Unknown Locations", which I think would be a good idea, but can happen in a different bug.

Comment 8

[Stefan [:stefanh]]

Hmm, we might want to have the "Handling Unknown Locations" stuff in a radiogroup. Looking at the more info page (http://www.mozilla.org/docs/end-user/internet-keywords.html) it seems the two conflicts with each other.

Comment 9

[Karsten Düsterloh]

Reading <http://mxr.mozilla.org/seamonkey/source/docshell/base/nsDefaultURIFixup.cpp#149>, if both keyword.enabled and browser.fixup.alternate.enabled are true, the course of action on "enter stuff into urlbar and hit enter" is:
- if it's a known keyword (including the special keyword ?), execute that 
  special bookmark
- else if keyword.URL (= default search engine) is valid, go there
- else do fixup with browser.fixup.alternate.prefix/.suffix, eg add www. and .com

IMO, the current text(s) for keywords are too centered on the default search, folks won't recognize that bookmark keywords are enabled/disabled here.

Comment 10

[Stefan [:stefanh]]

Also "Handling Unknown Locations" is a bit confusing, because there's nothing that tells the user that we're talking about the Location Bar. Renaming the "Smart Browsing" entry to "Location Bar" is not recommended either because we have "Location Bar History" in the History prefs.

Comment 11

[Robert Kaiser]

Karsten:
Are you sure that bookmark keywords depend on that pref? IIRC, I have used them with the default setting, which is "Internet Keywords" being off, and they worked fine...

Comment 12

[Karsten Düsterloh]

(In reply to comment #11)
> Are you sure that bookmark keywords depend on that pref? IIRC, I have used them
> with the default setting, which is "Internet Keywords" being off, and they
> worked fine...

You are right, they are independent of each other. 
Sorry for that, dunno what I broke in my build. :(

Comment 13

[Robert Kaiser]

(In reply to comment #12)
> You are right, they are independent of each other. 

That's actually one of the main reasons why I proposed rephrasing this in a way that doesn't mention "keywords", as that easily gets confused with bookmark keywords ;-)

Comment 14

[Stefan [:stefanh]]

Current look:

[ ] browser.urlbar.autocomplete.enabled
-------- Advanced dialog ------------
    [x] browser.urlbar.autoFill
    [x] browser.urlbar.showPopup
    [x] browser.urlbar.showSearch
    [x] browser.urlbar.matchOnlyTyped
-------------------------------------

On a mac trunk build, all "Advanced" prefs are set to true and only browser.urlbar.showSearch works. So, the "enabled" pref controls all the others except one in a way that they don't work. I assume the same behaviour on Win. On Linux, it's a different story (I hope I got it right); the last pref doesn't work, but all the others do in a sense that local paths are autofilled and displayed in the drop-down.

So, as we've discussed on irc today, the prefs needs to be reorganized a bit. On Linux, having the "browser.urlbar.autocomplete.enabled" checkbox as a "main" pref, would probably not work. The "main" pref will also be problematic when you've unchecked all the sub-prefs since (non-Linux) nothing would work for the user.

Comment 15

[Robert Kaiser]

From what I know now, I'd propose to do the following:

+-- Location Bar Autocomplete --------------------------------------------+
| [x] Use matches from browsing history for autocomplete                  |
|     (browser.urlbar.autocomplete.enabled)                               |
|    [ ] Match only websites you've typed previuosly                      |
|        (browser.urlbar.matchOnlyTyped)                                  |
| [ ] Autocomplete best match as you type                                 |
|     (browser.urlbar.autoFill)                                           |
| [x] Show list of matching results                                       |
|     (browser.urlbar.showPopup)                                          |
|    [x] Show internet search engine                                      |
|        (browser.urlbar.showSearch)                                      |
+-------------------------------------------------------------------------+

Comments:
1) The pref names in brackets are only there to show what pref this sets, they should not be in UI.
2) I'm not sure about indenting the two prefs where I did it here, but those should only be activated when the one above them is checked.

Comment 16

[Stefan [:stefanh]]

As I said on irc, I think KaiRo's proposal is good, but the main problem is that if you uncheck the first checkbox none of the other prefs would have any effect except for the last (if both "Show list" and "Show search" are checked). 

So, after another round of discussion on irc another proposal came up: Remove "browser.urlbar.autocomplete.enabled" from the ui (set to "true"), and have the checkboxes layed out like this (last checkbox could also go between 1 and 2):

-- Location Bar Autocomplete -------------------
[x] Autocomplete best match as you type
[x] Show list of matching results
   [x] Show internet search engine
[x] Match only websites you've typed previuosly

The argument for this proposal would be that we better match the prefs with what actually happens for the user.

Comment 17

[Stefan [:stefanh]]

> -- Location Bar Autocomplete -------------------
> [x] Autocomplete best match as you type
> [x] Show list of matching results
>    [x] Show internet search engine
> [x] Match only websites you've typed previuosly

I didn't really think of the state of the checkboxes here. So, I'm not really proposing that every checkbox should be checked.

Comment 18

[Vlado Valastiak [:wladow]]

I think this is a good occasion to remove DONT_TRANSLATE note for smartBrowse.label in preftree.dtd

Comment 19

[Stefan [:stefanh]]

I hope to get to this in a week or 2.

Comment 20

[Stefan [:stefanh]]

(Note to self)

Remember this: "suite/locales/en-US/chrome/common/help/suite-toc.rdf#779"

Comment 21

[Stefan [:stefanh]]

(Note to self)

See https://bugzilla.mozilla.org/show_bug.cgi?id=424724#c2

Comment 22

[Stefan [:stefanh]]

Attachment: Migrate prefs, take 2

Some details:

1) This implements the new panel in line with comment #7 (unknown locations part) and comment #16. Slightly different wording in some places, though. I also put the Autocomplete stuff first. Also tweaked the existing wording a bit. This will also fix bug 156792(yay!).

2) There where no accesskeys in the advanced dialog, so I added a bunch in the new panel. I don't really know anything about accesskeys since I'm on mac, but I did my best to follow the recommendations in http://developer.mozilla.org/en/docs/XUL_Accesskey_FAQ_and_Policies

3) Help update: I don't think there's any reason to point out that the unknown locations features are called "Domain Guessing" and "Internet Keywords" - but users will get a hint about this because the links are named "Domain Guessing" and "Internet Keywords". I removed the "Internet Keywords" stuff in Help, because it just describes something that we don't have anymore (we just send the keywords to Google's search engine). Note btw, that the "Internet Keywords" doc on m.o might need an update if this patch gets in (just change some seamonkey-specific text). 

I didn't touch the "The default search engine" stuff in nav_help.xhtml, so I suspect that there where a few non-unix linebreaks in there... I also fixed a few missing ' (because I forgot using ' myself and then did a search and replace).

Comment 23

[Stefan [:stefanh]]

Attachment: Migrate prefs, take 2 (-w version)

-w version of attachment #311439 [details] [diff] [review].

Comment 24

[[e-mail]]

(In reply to comment #22)
> 2) There were no accesskeys in the advanced dialog, so I added a bunch
You're not supposed to use lowercase vowels, so a possible suggestion is to use "w" for the domain guessing and then move "A" to Autocomplete and use "h" for Show Internet search engine (iirc Internet still has a captial I, right?)

Comment 25

[[e-mail]]

Comment on attachment 311440 [details] [diff] [review]
Migrate prefs, take 2 (-w version)

>+function Startup()
> {
>+  var isAutoCompletePref = document.getElementById("browser.urlbar.autocomplete.enabled");
>+  document.getElementById("matchOnlyTyped").disabled = !isAutoCompletePref.value;
How does this relate? (And it doesn't check the locked state either.)

>+  var isShowPopupPref = document.getElementById("browser.urlbar.showPopup");
>+  document.getElementById("showSearch").disabled = !isShowPopupPref.value || isShowPopupPref.locked;
You're checking the wrong locked preference. Apart from that, updateShowPopup can use this code, i.e. make one call the other.

>+        <checkbox id="showPopup"
>+                  label="&autoCompleteShowPopup.label;"
>+                  accesskey="&autoCompleteShowPopup.accesskey;"
>+                  oncommand="updateShowPopup();"
>+                  preference="browser.urlbar.showPopup"/>
updateShowPopup needs to be in a change event on the <preference> instead.

Comment 26

[Stefan [:stefanh]]

(In reply to comment #25)
> (From update of attachment 311440 [details] [diff] [review])
> >+function Startup()
> > {
> >+  var isAutoCompletePref = document.getElementById("browser.urlbar.autocomplete.enabled");
> >+  document.getElementById("matchOnlyTyped").disabled = !isAutoCompletePref.value;
> How does this relate? (And it doesn't check the locked state either.)

If you set "browser.urlbar.autocomplete.enabled" to false in about:config I was under the impression that "browser.urlbar.matchOnlyTyped" doesn't makes sense, so I figured it should be disabled.

Comment 27

[Robert Kaiser]

(In reply to comment #24)
> (In reply to comment #22)
> > 2) There were no accesskeys in the advanced dialog, so I added a bunch
> You're not supposed to use lowercase vowels

Erm, why that? I never heard of that rule. Bad accesskeys are l and i for being so tight and having a very short underline, but I never heard anything about casing (other than the accesskey having the same casing as the character it should underline).

Comment 28

[Stefan [:stefanh]]

+      <li><strong>Add <qt>www.</qt>

Oops...

Comment 29

[[e-mail]]

(In reply to comment #26)
> If you set "browser.urlbar.autocomplete.enabled" to false in about:config I was
> under the impression that "browser.urlbar.matchOnlyTyped" doesn't makes sense,
> so I figured it should be disabled.

So there's no indication of how to enable it? IMHO that doesn't look good.

Also, you're not checking whether it's locked.

Comment 30

[Stefan [:stefanh]]

(In reply to comment #24)
> (In reply to comment #22)
> > 2) There were no accesskeys in the advanced dialog, so I added a bunch
> You're not supposed to use lowercase vowels, so a possible suggestion is to use
> "w" for the domain guessing and then move "A" to Autocomplete and use "h" for
> Show Internet search engine (iirc Internet still has a captial I, right?)

Is "d" better for the domain guessing? 

Comment 31

[Stefan [:stefanh]]

(In reply to comment #29)
> (In reply to comment #26)
> > If you set "browser.urlbar.autocomplete.enabled" to false in about:config I was
> > under the impression that "browser.urlbar.matchOnlyTyped" doesn't makes sense,
> > so I figured it should be disabled.
> 
> So there's no indication of how to enable it? IMHO that doesn't look good.
> 

Hmm, right. So, should I go with my proposal but without disabling the matchOnlyTyped checkbox when autocomplete is turned off:

- Autocomplete -------------------
[ ] Autocomplete best match as you type
[x] Show list of matching results
   [x] Show Internet search engine
[ ] Match only websites you've typed previously

or do you want to do KaiRo's proposal in comment #15? I think both have their pros and cons. Mine should work fine as long as autocomplete is not turned off. KaiRo's would make the matchOnlyTyped checkbox's dependency of autocomplete clear, but non-linux users might wonder why the "best match" doesn't work when autocomplete is turned off (and it'd be incorrect to make it depend on autocomplete - if I understood everything correct).

Comment 32

[[e-mail]]

(In reply to comment #27)
> (In reply to comment #24)
> > (In reply to comment #22)
> > > 2) There were no accesskeys in the advanced dialog, so I added a bunch
> > You're not supposed to use lowercase vowels
> I never heard of that rule.
Strange, I was sure I'd read it somewhere.

(In reply to comment #30)
> Is "d" better for the domain guessing? 
Sounds fine to me.

(In reply to comment #31)
> or do you want to do KaiRo's proposal in comment #15?
Yes, I think I like that better, although if you are really worried you could always test for the file view component e.g.
if (!("@mozilla.org/autocomplete/search;1?name=file" in Components.classes))
  document.getElementById("matchOnlyTyped").className = "indent";

Finally, a couple of suggestions for alternative strings:

> [x] Use matches from browsing history for autocomplete
Autocomplete from your browsing history as you type

> [ ] Autocomplete best match as you type
Automatically prefill the best match

Comment 33

[Ian Neal]

With line 1127 of autocomplete.xml changed to:
if (!this.mMenuOpen && (this.showPopup || (this.minResultsForPopup == 0)) && this.focused &&

then you can have the following pref UI:
+-- Location Bar Autocomplete --------------------------------------------+
| [x] Use matches from browsing history for autocomplete                  |
|     (browser.urlbar.autocomplete.enabled)                               |
|    [ ] Autocomplete best match as you type                              |
|        (browser.urlbar.autoFill)                                        |
|    [x] Show list of matching results                                    |
|        (browser.urlbar.showPopup)                                       |
|    [ ] Match only websites you've typed previously                      |
|        (browser.urlbar.matchOnlyTyped)                                  |
| [x] Show internet search engine                                         |
|     (browser.urlbar.showSearch)                                         |
+-------------------------------------------------------------------------+
Note that setting browser.urlbar.matchOnlyTyped to true only makes sense if at least one of browser.urlbar.autoFill or browser.urlbar.showPopup is also set to true. Whether you put some script behind that to enforce it is another matter though.
Additionally with the above change browser.urlbar.showSearch should probably be taken out of the autocomplete groupbox.

Comment 34

[Robert Kaiser]

Doesn't .showSearch only make sense (or even work) when .showPopup is set?

Comment 35

[Stefan [:stefanh]]

(In reply to comment #34)
> Doesn't .showSearch only make sense (or even work) when .showPopup is set?

I think it makes sense to make it possible to show the search engine regardless of whether you have checked the "Show list of matching results" or not. That is, you'll need to change autocomplete.xml a bit since atm the showSearch depends on showPopup.

Comment 36

[Robert Kaiser]

You imply that either .showPopup or "Show list of matching results" is a misnomer as you imply that "Show list of matching results" does mean something else than "Show Popup" which actually is what the current pref does.
I'm not sure if a pref would make sense that does what you imply that the text in the UI says, and actually I'm not even sure if the current pref makes sense.
I get the impression that some features, like this here, are over-preffed (esp. when thinking of functionality it still lacks compared to what FF3 is shipping - even if their UI is debatable).

Comment 37

[Stefan [:stefanh]]

(In reply to comment #36)
> You imply that either .showPopup or "Show list of matching results" is a
> misnomer as you imply that "Show list of matching results" does mean something
> else than "Show Popup" which actually is what the current pref does.

Yes I do. "Show popup with list of matching results" and "Show Internet search engine at the bottom of the list of results" is what it does atm.
 
> I'm not sure if a pref would make sense that does what you imply that the text
> in the UI says,

I think it makes more sense than the current setup and the previous proposals.

>and actually I'm not even sure if the current pref makes sense.

Can you give an example of how you'd like it to look?

Comment 38

[Robert Kaiser]

What I mean is I wonder if we need a pref for this at all. I believe we could just remove the .showPopup pref (probably from UI _and_ the backend) and practically nobody would notice.

Comment 39

[Stefan [:stefanh]]

Attachment: screenshot of layout

fwiw, this is how the proposal in comment #38 would look on mac (should start each word in the search engine box with a capital). Linux won't have the last 2 checkboxes in the Autocomplete groupbox indented.

Comment 40

[Stefan [:stefanh]]

(In reply to comment #39)
> Created an attachment (id=312739) [details]
> screenshot of layout
> 
> fwiw, this is how the proposal in comment #38 would look on mac

comment #33, of course

Comment 41

[Stefan [:stefanh]]

Attachment: Migrate prefs, take 3

Take 3:

For mac/win layout as in attachment #312739 [details], for Linux autoFill and ShowPopup is not indented and does not depend on "Automcomplete from your browsning history as you type".

I couldn't figure out how to differentiate the layout in Help, so I just added a comment for Linux users.

Comment 42

[Stefan [:stefanh]]

Attachment: Migrate prefs, take 3 (-w version)

Comment 43

[Stefan [:stefanh]]

As IanN pointed out, I forgot the autocomplete.xml diff. Will have a new patch up in a few days, that includes autocomplete.xml

Comment 44

[Stefan [:stefanh]]

Attachment: Migrate prefs, take 4

Basically the same as "take 3", except:
- included diff of autocomplete.xml
- some polish of pref-smart_browsing.js (changed a couple of variables, added comments and now using hasAttribute in updateDependent)

Comment 45

[Stefan [:stefanh]]

Attachment: Migrate prefs, take 4 (-w)

Comment 46

[Ian Neal]

Wasn't there talk of changing filenames too?

Comment 47

[Stefan [:stefanh]]

(In reply to comment #46)
> Wasn't there talk of changing filenames too?
> 

Sure. How do we preceed? Do I first change the filenames, commit the "new" files (provide a patch) and then attach a new patch that changes the "new" files?

Comment 48

[Stefan [:stefanh]]

Attachment: Re-named files (checked in)

OK, so here are the 3 "new" files. If this is ok, I'll check each one of those in with a comment saying something like "File rename from xx to yy, see  mozilla/suite/common/pref/xx for the old history".

Comment 49

[Ian Neal]

(In reply to comment #47)
> (In reply to comment #46)
> > Wasn't there talk of changing filenames too?
> > 
> 
> Sure. How do we preceed? Do I first change the filenames, commit the "new"
> files (provide a patch) and then attach a new patch that changes the "new"
> files?
> 
Correct, see pref-tags.js/xul/dtd checkins for how to comment about history.

Comment 50

[Stefan [:stefanh]]

> Correct, see pref-tags.js/xul/dtd checkins for how to comment about history.

See attachment #316695 [details] [diff] [review] ;-)

Comment 51

[Ian Neal]

Comment on attachment 315768 [details] [diff] [review]
Migrate prefs, take 4

>Index: suite/common/pref/pref-smart_browsing.js
>===================================================================
>+  // On systems that has the file view component, autoFill and showPopup will
>+  // return results from local browsing "history", even if autocomplete.enabled
>+  // is turned off, so we'll need to remove the dependent look in the iu.
Nit: ui instead of iu perhaps?

>+function toggleCheckbox(aCheckbox, aPrefValue)
> {
>+  if (!document.getElementById("browser.urlbar." + aCheckbox).locked)
>+  {
>+    document.getElementById(aCheckbox).disabled = !aPrefValue;
>+  }
Nit: Don't really need to have the braces here.
> }
> 
>+function updateMatchOnlyTyped()
> {
>+  var matchOnlyTypedPref = document.getElementById("browser.urlbar.matchOnlyTyped");
Nit: any reason why you don't use the above directly below instead of assigning it to a variable?
>+
>+  if (!matchOnlyTypedPref.locked)

>Index: suite/common/pref/pref-smart_browsing.xul
>===================================================================
>+<!DOCTYPE overlay [
> <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
> %brandDTD;
I don't think you need the above entity anymore.

> <!ENTITY % prefSmartBrowsingDTD SYSTEM "chrome://communicator/locale/pref/pref-smart_browsing.dtd" >
> %prefSmartBrowsingDTD;
> ]>
Which means this one can be rolled into the !DOCTYPE line.

Other than that r=me with filename changes (commenting on the change in the checkin of the renamed original files as per pref-tags)

Comment 52

[Ian Neal]

Comment on attachment 315768 [details] [diff] [review]
Migrate prefs, take 4

>Index: suite/locales/en-US/chrome/common/pref/pref-smart_browsing.dtd
>===================================================================
>+<!ENTITY autoComplete.label                    "Autocomplete">
>+<!ENTITY enableAutoComplete.label              "Autocomplete from your browsing history as you type">
>+<!ENTITY enableAutoComplete.accesskey          "A">
>+<!ENTITY autoCompleteAutoFill.label            "Automatically prefill the best match">
>+<!ENTITY autoCompleteAutoFill.accesskey        "c">
Any reason why you cannot use "u"?

>+<!ENTITY showSearchEngine.label                "Show default search engine">
>+<!ENTITY showSearchEngine.accesskey            "h">
Isn't "h" used for help?

Comment 53

[Stefan [:stefanh]]

(In reply to comment #52)
> (From update of attachment 315768 [details] [diff] [review])
> >Index: suite/locales/en-US/chrome/common/pref/pref-smart_browsing.dtd
> >===================================================================
> >+<!ENTITY autoComplete.label                    "Autocomplete">
> >+<!ENTITY enableAutoComplete.label              "Autocomplete from your browsing history as you type">
> >+<!ENTITY enableAutoComplete.accesskey          "A">
> >+<!ENTITY autoCompleteAutoFill.label            "Automatically prefill the best match">
> >+<!ENTITY autoCompleteAutoFill.accesskey        "c">
> Any reason why you cannot use "u"?

Yeah, comment #24
> 
> >+<!ENTITY showSearchEngine.label                "Show default search engine">
> >+<!ENTITY showSearchEngine.accesskey            "h">
> Isn't "h" used for help?
> 

Hmm, yes. How about "w"?

Comment 54

[Ian Neal]

(In reply to comment #53)
> (In reply to comment #52)
> > (From update of attachment 315768 [details] [diff] [review] [details])
> > >Index: suite/locales/en-US/chrome/common/pref/pref-smart_browsing.dtd
> > >===================================================================
> > >+<!ENTITY autoComplete.label                    "Autocomplete">
> > >+<!ENTITY enableAutoComplete.label              "Autocomplete from your browsing history as you type">
> > >+<!ENTITY enableAutoComplete.accesskey          "A">
> > >+<!ENTITY autoCompleteAutoFill.label            "Automatically prefill the best match">
> > >+<!ENTITY autoCompleteAutoFill.accesskey        "c">
> > Any reason why you cannot use "u"?
> 
> Yeah, comment #24

See comment #27 :)
> > 
> > >+<!ENTITY showSearchEngine.label                "Show default search engine">
> > >+<!ENTITY showSearchEngine.accesskey            "h">
> > Isn't "h" used for help?
> > 
> 
> Hmm, yes. How about "w"?
> 
Maybe "e" from "default".

Comment 55

[Stefan [:stefanh]]

> > > >+<!ENTITY autoCompleteAutoFill.accesskey        "c">
> > > Any reason why you cannot use "u"?
> > 
> > Yeah, comment #24
> 
> See comment #27 :)
> > > 
> > > >+<!ENTITY showSearchEngine.label                "Show default search engine">
> > > >+<!ENTITY showSearchEngine.accesskey            "h">
> > > Isn't "h" used for help?
> > > 
> > 
> > Hmm, yes. How about "w"?
> > 
> Maybe "e" from "default".

OK, sure. I don't see them on mac, so this is really not my area ;-)

Comment 56

[[e-mail]]

Comment on attachment 315768 [details] [diff] [review]
Migrate prefs, take 4

>+  // is turned off, so we'll need to remove the dependent look in the iu.
s/iu/ui/

>+    //matchOnlyTyped doesn't makes sense if both autoFill and showPopup prefs
>+    //are false or if autocomplete is turned off.
Space after //

>-          if (!this.mMenuOpen && this.showPopup && this.focused &&
>-               (this.getResultCount("") >= this.minResultsForPopup
>-                || this.mFailureItems)) {
>+          if (!this.mMenuOpen && (this.showPopup || (this.minResultsForPopup == 0)) &&
>+              this.focused && (this.getResultCount("") >= this.minResultsForPopup ||
>+              this.mFailureItems)) {
It seems to me as if you can simply remove the original test for this.showPopup, as if it's false then you'll never add results and therefore your result count will always be zero and therefore your min results need to be zero anyway.
Put another way, if (this.showPopup || (this.minResultsForPopup == 0)) is false then so is this.getResultCount("") >= this.minResultsForPopup

>           if (this.focused) {
>-            this.view.addResults(aResults);
>+            if (this.showPopup)
>+              this.view.addResults(aResults);
>             this.resultsPopup.adjustHeight();
>           }
Is there any point adjusting the height unless we actually add the results? if (this.focused && this.showPopup) should suffice.

Comment 57

[[e-mail]]

Comment on attachment 316695 [details] [diff] [review]
Re-named files (checked in)

Please mention their previous xpfe location too.

Comment 58

[Stefan [:stefanh]]

(I'll land this tomorrow)

Comment 59

[Stefan [:stefanh]]

Comment on attachment 316695 [details] [diff] [review]
Re-named files (checked in)

Checked in this.

Comment 60

[Stefan [:stefanh]]

Attachment: Final patch (checked in)

This is what I landed: comment #51-57 addressed (and with the new pref-locationbar paths). Also removed the old files etc.

Comment 61

[Stefan [:stefanh]]

(In reply to comment #60)
>comment #51-57 addressed

51-56, since 57 was already addressed... Resolving this as everything has landed. JFTR, I also removed the "<!-- extracted from content/pref-smart_browsing.xul -->" line (and the empty line under it) from pref-locationbar.dtd

Comment 62

[Vlado Valastiak [:wladow]]

Comment on attachment 319598 [details] [diff] [review]
Final patch (checked in)

>-<p>If you don&apos;t know a company&apos;s stock ticker symbol, type the
>-  company&apos;s name. The quote page will list all companies with similar
>-  names, and you can choose the one for which you want a quote.</p>
>-
>-<p>[<a href="#searching_the_web">Return to beginning of section</a>]</p>
>-
>-<h2 id="searching_within_a_page">Searching Within a Page</h2>
>-
> <p>To find text within the page you are currently viewing in the browser:</p>
> 

Last two lines are removed by mistake I guess...

Comment 63

[Stefan [:stefanh]]

(In reply to comment #62)
> (From update of attachment 319598 [details] [diff] [review])
> >-<p>If you don&apos;t know a company&apos;s stock ticker symbol, type the
> >-  company&apos;s name. The quote page will list all companies with similar
> >-  names, and you can choose the one for which you want a quote.</p>
> >-
> >-<p>[<a href="#searching_the_web">Return to beginning of section</a>]</p>
> >-
> >-<h2 id="searching_within_a_page">Searching Within a Page</h2>
> >-
> > <p>To find text within the page you are currently viewing in the browser:</p>
> > 
> 
> Last two lines are removed by mistake I guess...
> 

Hmm, looks like this was in the original patch as well. Yeah, sorry - that was a mistake. Can you file a bug on that (Mozilla Application Suite: Help) and assign it to me, please? 

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/971e6fa6771f
Migrate SeaMonkey's smart browsing preferences to new pref window (also rename panel to 'Location Bar'). r=IanN, sr=Neil.