Closed Bug 480350 (switch-to-tab) Opened 15 years ago Closed 14 years ago

show currently loaded URIs in location bar autocomplete results, allow switching to the tab

Categories

(Firefox :: Address Bar, defect)

Product:
Firefox
Component:
Address Bar
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 3.7a4

People

(Reporter: Gavin, Assigned: Unfocused)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

(Keywords: user-doc-needed, Whiteboard: [icon-3.7] [mozmill-test-needed][switch-to-tab])

Attachments

(1 file, 18 obsolete files)

patch v5.0 [Checkin: Comment 127]
58.39 KB, patch
Details | Diff | Splinter Review
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → DUPLICATE
This isn't really a duplicate of that bug, per 455650 comment 8. Since it has a WIP patch for the other approach, I think it makes sense to keep them separate (and possibly WONTFIX that one in favor of this one).
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Status: REOPENED → NEW
(In reply to comment #2)
> This isn't really a duplicate of that bug, per 455650 comment 8. Since it has a
> WIP patch for the other approach, I think it makes sense to keep them separate
> (and possibly WONTFIX that one in favor of this one).

just FYI, that approach (the annotation one) has already been discarded due to perf.
that other bug is about being able to restrict the search to open tabs as well as visually identify, and select them. i'll morph it to be *only* about the restriction bit.
Attached patch WIP (obsolete) — — Splinter Review 
I couldn't get the extrahidden/extralabel stuff to work for some reason (also needs to be made localizable, obviously).
Attached patch updated WIP (obsolete) — — Splinter Review 
No changes, just merged to tip.
Attachment #365268 - Attachment is obsolete: true
http://limi.net/articles/tab-matching-in-the-location-bar

The proposal here has two selectable items -- one for switching to the tab and another to just open the link.

For another project, I got a custom autocomplete display to work by providing a new binding, so it should work here too. The main thing is that we'll need to create 2 new entries instead of just one and add a class/attribute to detect if it's the "top half" or "bottom half" and use the appropriate binding.

The bindings would have their own "content" block for a 1-line display. I'm not sure exactly how we'll wire up the interaction to have it switch to the tab though.. but maybe if the selected richlistitem can provide a callback from handleCommand, it can override the default open behavior from the binding. ?
(In reply to comment #7)
> I'm not sure exactly how we'll wire up the interaction to have it switch to the > tab though.. 

Might I suggest reviewing Win7 Aeropeek optional tab previews bug 474056 or the ctrl-tabs/all-tabs bug 465076.  Basically you can switch to or preview tabs using both enhancement features.  Maybe this could be similar..
Assignee: nobody → bmcbride
Blocks: 455650
Attachment #369305 - Attachment is obsolete: true
I've put some try server builds here: http://people.mozilla.org/~bmcbride/tabmatches/latest/

Looking for thoughts, feedback, and bugs. WIP patch is pending.
Status: NEW → ASSIGNED
The "two selectable items -- one for switching to the tab and another to just open the link" seems kind of weird to me.  I don't know how to explain it but it just doesn't feel right.

Only real problems I found were:

1) Clicking the drop-down button does not give the option to switch to an opened tab
2) Using the keyboard arrows through the autocompelte list causes "larry" to disapper and "switch to tab:" appears in its place.  It does not disappear when the selection is moved to the second selectable state of an entry and actually never disappears until you click on the favicon to open "larry"
3) This appears in the error console 

Error: name is null
Source File: file:///C:/junk/firefox-tabmatches-win32/firefox/modules/WindowsJumpLists.jsm
Line: 344

I have no clue how to reproduce it but the error showed up quite a few times in the console
4) The "switch to tab:" in the "larry" spot appears a pixel or two too high
Forgot to mention: I'm using $ (dollar sign) as the character to restrict results for tabs-only.


(In reply to comment #10)
> 3) This appears in the error console 
> 
> Error: name is null
> Source File:
> file:///C:/junk/firefox-tabmatches-win32/firefox/modules/WindowsJumpLists.jsm
> Line: 344

That file is unrelated - you can ignore that for this bug (but possibly file another bug for it!).

I'll look into your other points later (thanks!).
I only see that error using your build though, that's why I mentioned it here.  I'll file separate bug if I can figure out what makes the error appear.
Depends on: 530209
Blair, I just tested the 11-16 build.  It seems good to me in general on first use.  I didn't test any previous builds.  

(In reply to comment #10)
> 3) This appears in the error console 
> 
> Error: name is null
> Source File:
> file:///C:/junk/firefox-tabmatches-win32/firefox/modules/WindowsJumpLists.jsm
> Line: 344

(In reply to comment #13)
> I only see that error using your build though, that's why I mentioned it here. 
> I'll file separate bug if I can figure out what makes the error appear.


Kurt, you may want to file that bug anyway, it sounds like an edge case.
I did find that if I have URLs with long titles in my history/bookmarks, Switch to Tab as its the top choice of two, the wording of the titlbar either will show:

Super long URL title Switch to tab
or 
Super long URL title... Switch to tab 

Neither of which is exactly eye pleasing.. I do like the visual appears being on the second option, rather than the first, but I can imagine if it was the second option, users wouldn't switch tabs with it nearly as often with it, not being the default.  So having it be first, seems like the right thing to do, just visually it doesn't mesh the greatest.

I think if possible it to should always show the ellipse first and there should be more white space between ellipse and the 'Switch to tab' text to give it some distinctly visible appearance difference from the title running into it.
Attached image RTL screenshot (obsolete) — 
We need to have some plans to come up with a UI which is both useful in LTR and RTL mode.  To test this interface in RTL mode, you need to install version 2.1 of the Force RTL add-on.

To give you some background, in RTL mode, the only primary browser UI which remains LTR is the location bar, because it's mainly used for displaying URLs, which are inherently LTR.  In addition to this, we don't provide any text in the toolbar organization, so all that we reverse in RTL mode are actually icons on the toolbar.

Adding text to the toolbar makes things tricky for RTL mode.  See the screenshot I'm attaching, which is from the latest tabmatches try server build which I have sneakishly touched with DOM inspector to actually display Persian text inside the location bar.

Now, from the standpoint of RTL users, they expect all text to be mainly laid out from right to left.  RTL users are actually good at parsing bi-directional text, because numbers in RTL texts are written from left to right.  So, for the proposed UI, an RTL user expects to read the contents of the location bar from right to left, which will be (in logical order): [URL, colon, "switch to tab"], which doesn't make any sense, because we intend "Switch to tab:" to be an informative message regarding the URL which is presented on its right.  The correct reading order from right to left should be: ["switch to tab", colon, URL].  The reason that the colon in the screenshot appears at the right side of the text instead of the left side (where it _should_ be) is that the location bar itself is LTR, so the Persian text is being treated as LTR.

With the current UI proposal, we need to move the label to the right side of the location bar in RTL mode, make it inherit the UI direction (which is RTL), so that it appears as:

http://blahblahblah     رفتن به زبانه:‏

This may be easier said than done, because we have all sorts of items at the right side of the location bar (such as the star button, go button, RSS button, not to mention buttons added by extensions to the URL bar).  I am not proposing that the correct solution here is this, I'm just mentioning the obvious solution given the current UI proposal.

This is a UI design problem actually.  I'd be glad to comment on RTL sanity of any future iterations, but it suffices to say for now that the current UI design doesn't support RTL correctly, and it may be a challenging task to make the UI look good in RTL mode.

I hope that this comment raises the correct questions, and can help our UI design folks to try to solve this problem.  Feel free to ask for clarification or anything else RTL related about this!
(In reply to comment #16)
> I hope that this comment raises the correct questions, and can help our UI
> design folks to try to solve this problem.  Feel free to ask for clarification
> or anything else RTL related about this!

Ugh, I'd forgotten the location bar was special-cased - thanks for bringing that up now. I'll make sure Limi sees this.
(In reply to comment #16)
> With the current UI proposal, we need to move the label to the right side of
> the location bar in RTL mode, make it inherit the UI direction (which is RTL),
> so that it appears as:
> 
> http://blahblahblah     رفتن به زبانه:‏

Done. Will throw up another Try build later today with this (and various other fixes) to get some feedback on how it feels.
Attached patch WiP v1 (obsolete) — — Splinter Review 
Work in progress patch.
Just making sure that I've glanced correctly.. The patch has Places keep track of open tabs by exposing a new interface to mark a page as 'open', and this is used by the places autocomplete search to set a result's type as tab for the autocomplete/urlbar bindings to do special handling?
(In reply to comment #20)
> Just making sure that I've glanced correctly.. The patch has Places keep track
> of open tabs by exposing a new interface to mark a page as 'open', and this is
> used by the places autocomplete search to set a result's type as tab for the
> autocomplete/urlbar bindings to do special handling?

Not a new interface (it adds to nsIBrowserHistory), but yes. Storing it in a places table (which is in-memory only) allows for easy frecency ranking of tab matches. And potentially other nice things in the future (such as adding additional weighting to URLs currently open in a tab, faceted search, etc).

UI-wise, I've also tried to take into account bug 524050.
I've put up another Tryserver build (http://people.mozilla.org/~bmcbride/tabmatches/latest/) - this one based on the patch in comment 19.
well, apart the dashed line that hurts my eyes, it's wroking pretty fine.

If i have only one tab match result (and only one result in global) and i key down, down, down i enter a state where the selection is only on the first half of the entry (as if it should switch to tab), but there is no "switch to tab" indication. if at this point i mouseover and mouseout it it will continue switching between "switch to tab" status and normal selection.
(In reply to comment #23)
> If i have only one tab match result (and only one result in global) and i key
> down, down, down i enter a state where the selection is only on the first half
> of the entry (as if it should switch to tab), but there is no "switch to tab"
> indication. if at this point i mouseover and mouseout it it will continue
> switching between "switch to tab" status and normal selection.

Fixed.
(In reply to comment #22)
> I've put up another Tryserver build
> (http://people.mozilla.org/~bmcbride/tabmatches/latest/) - this one based on
> the patch in comment 19.

This is wrong!  You've basically switches the direction of location bar input control to rtl, which is wrong for all URLs!  It should remain LTR.  The only part that should be RTL (and should appear on the right side) is the "Switch to tab:" label.
(In reply to comment #25)
> This is wrong!  You've basically switches the direction of location bar input
> control to rtl, which is wrong for all URLs!  It should remain LTR.  The only
> part that should be RTL (and should appear on the right side) is the "Switch to
> tab:" label.

Oh, duh - I forgot to add something to that patch.

People may have also noticed some weirdness when using a restored session (where tabs wouldn't show in results until after you had switched to them). I'm still trying to fix that.
Attached patch WiP v2 (obsolete) — — Splinter Review 
Still some unresolved issues (and I'm not done with unit tests yet). The build using this patch is up in the usual location: http://people.mozilla.org/~bmcbride/tabmatches/latest/

This has the RTL behavior (comment 16) that I intended the previous build to have. It also fixes various bugs, including session restored tabs not matching until they had been switched to, the issues in comment 23 and comment 11, and a partial fix for the issue in comment 15.
Attachment #414162 - Attachment is obsolete: true
There's a relatively harmless bug where it will highlight the matching text even in the "Switch to tab" text if it exists in what you're typing. Screenshot attached.
Attached patch WiP v3 (obsolete) — — Splinter Review 
This has all the comments here addressed, with additional fixes.

It also has a slightly different UI - "Switch to tab" is now where the URL normally is, to indicate this is an action, rather than a going to a new URL. It also avoid adding additional clutter to the right-hand side with the star, tags, etc.

The builds from this are in the usual location: http://people.mozilla.org/~bmcbride/tabmatches/latest/

I've still got a couple of weird test failures to fix. However, I'd like to get some preliminary reviews, since I'm not expecting any more big changes.
Attachment #414098 - Attachment is obsolete: true
Attachment #415499 - Attachment is obsolete: true
Attachment #417168 - Attachment is obsolete: true
Attachment #417807 - Flags: review?(mak77)
Attached image Screenshot of missing tab icon (obsolete) — 
The latest version doesn't seem to show the tab icon.
(In reply to comment #31)
> Created an attachment (id=417840) [details]
> Screenshot of missing tab icon
> 
> The latest version doesn't seem to show the tab icon.

Whoops - it does, but only on Windows. New build coming up.
Attachment #417807 - Flags: review?(gavin.sharp)
Attachment #417840 - Attachment is obsolete: true
Out of curiosity what icons are we using for this, have we created new artwork yet?
I like the UI change with latest better with the prior design. 

I noticed several bugs with the awesomebar and can file new ones as needed.

If I browse say a news site like www.engadget.com and click on multiple stories so now I have lots of domain history.  I create a tab for engadget and open a second tab for engadget and click on a news story alphabetically lower on the list so the two tabs now have unique URLs, then go to the awesomebar and key in www.engadget.com you will run into the case of the second URL possible being too far down the search results that you don't see a switch to tab option without knowing the URL.  

Is it possible to optimize the awesomebar results so all open tabs show up all the time?  Or is that not the goal to always see your tabs regardless of the awesomebar config setting?  The above scenario, 12 seems to be max limit, so you've have to know how to spell the URL in order to find the second tab.


I don't know if there would be a clean way to display that if there is a lot of open tab matches for the same domain?  Is there a way to increase the max limit of listed items so more tabs would be listed in my results with lots of history?

There is odd awesomebar UI behavior with the drop down, and probably affects the trunk too.  Sometimes I get a listbox, and sometimes I get a combobox.  I also noticed different awesomebar listbox or combobox length occurring depending on how we interact with the awesomebar and which tab is active.  

For example:
A) click down arrow, you get 9 items, no scrollbar
B) type www. you get 12 results no scrollbar for more than 12 matches
C) switch tabs, and type for www. you get a combobox showing 5 results out of 12.
D) type www.engadget  you a listbox but no more history/tabs results and no combo box showing > 12 results.

C & D will happen randomly.  

I should probably file another bug on that too.
(In reply to comment #34)
> I like the UI change with latest better with the prior design. 
> 
Oops, Blair, to clarify, that should say, I like the UI change with the latest (today), better than the prior design.  So that is a welcome UI change, though, would it look better with black text for Switch to Tab so it stands out more?
(In reply to comment #34)
> Is it possible to optimize the awesomebar results so all open tabs show up all
> the time?  

if you type "$", space, the word you're looking for, you're restricting your search to just open tabs.
I agree that we should probably weight tabs a bit heavier, so they tend to show up on top.

So far, I think we prefer to let this be off by default, and be available in the "When using the location bar, suggest:" preference, with "Open Tabs" being one of the available options.
What I meant to say in the previous comment is that being a bit more aggressive on increasing the rank of tabs in results shouldn't be a problem since it's going to be opt-in.
(In reply to comment #33)
> Out of curiosity what icons are we using for this, have we created new artwork
> yet?

As a temporary measure, I photochoped the tab icon from an image in the mac theme. Limi reminded me just the other day that we need a proper icon (even if it looks like the current one).
(In reply to comment #37)
> I agree that we should probably weight tabs a bit heavier, so they tend to show
> up on top.

I'm still worried about starvation of normal results if tab-matches are weighted too heavily, when there are a lot of tabs open (which is where this is primarily going to be useful). In my usage with ~70 open tabs, all the relevant tab matches are already in the results. Can experiment with it though.

(In reply to comment #37)
> So far, I think we prefer to let this be off by default, and be available in
> the "When using the location bar, suggest:" preference, with "Open Tabs" being
> one of the available options.

There's no UI for that yet, so it will have to remain on until bug 530209 is fixed (editing the pref manually isn't an option - its a bitfield). Either way, I'd like to keep this enabled by default on nightlies for awhile to get some more wide-spread testing and feedback from nightly testers (who typically have many tabs open). Previous experiments have shown that having something pref'ed off in nightlies means it doesn't get tested much :)
(In reply to comment #34)
> you will run into the case of the second URL possible being
> too far down the search results that you don't see a switch to tab option
> without knowing the URL.  

Do you mean, the "Switch to tab" item will be the last result, and therefore have no corresponding normal item that shows the URL? Do you need to know the URL? Normal tabs don't show the URL until you switch to them - so we've assumed this carries over. For you, does using the awesomebar to switch tabs make the URL more important than when you switch tabs by clicking on them?

> Or is that not the goal to always see your tabs regardless of the
> awesomebar config setting? 

No, the goal is to only show relevant tabs (relevant to what you typed in). Unless specifically asking for only tabs (using the restrict character), of course.

> The above scenario, 12 seems to be max limit, so
> you've have to know how to spell the URL in order to find the second tab.

It also matches tab titles, too. But yes, see comment 36.

> I don't know if there would be a clean way to display that if there is a lot of
> open tab matches for the same domain?  Is there a way to increase the max limit
> of listed items so more tabs would be listed in my results with lots of
> history?

If you're specifically looking for a tab, using the restrict character (comment 36) is the way to go. Of course, that not an obvious behavior.

However, this is specifically a case that I'm worried about if tab matches are weighted heavier. If there are a lot of tabs matching the search string (eg, a lot of tabs on the same domain, and the domain name is typed in), then its going to be difficult to get any result that isn't an open tab.

> There is odd awesomebar UI behavior with the drop down, and probably affects
> the trunk too.  Sometimes I get a listbox, and sometimes I get a combobox.  I
> also noticed different awesomebar listbox or combobox length occurring
> depending on how we interact with the awesomebar and which tab is active.

Er, no idea if that's related or not. Could you attach a screenshot?
Almost forgot (and sorry for the bugspam) - latest build has the icon fixed:
http://people.mozilla.org/~bmcbride/tabmatches/latest/
Attached file first pass review wip v3 (obsolete) — 
first pass, lots to cleanup. Btw globally it looks interesting, i just don't like much hooking into browserHistory (toolkit) for tabs data (that is really frontend specific), and the fact we need to create the full thing at startup.
Attachment #417807 - Flags: review?(mak77)
Attached image RTL screenshot (obsolete) — 
The recent build still doesn't look good on RTL.  See the screenshot.

The text box inside the urlbar is set to RTL, but it should be LTR.  The Switch to tab text in the autocomplete list moves to the left, whereas it should stick to the right (and be RTL, by the way.)
>As a temporary measure, I photochoped the tab icon from an image in the mac
>theme. Limi reminded me just the other day that we need a proper icon (even if
>it looks like the current one).

Adding whiteboard term [icon-3.7] to track, this is actually the first request for that version
Whiteboard: [icon-3.7]
I'll try to answer the best I can, I'll let you decide if you think we should open bugs for anything of interest.

(In reply to comment #41)
> (In reply to comment #34)
> > you will run into the case of the second URL possible being
> > too far down the search results that you don't see a switch to tab option
> > without knowing the URL.  
> 
> Do you mean, the "Switch to tab" item will be the last result, and therefore
> have no corresponding normal item that shows the URL? Do you need to know the
> URL? Normal tabs don't show the URL until you switch to them - so we've assumed
> this carries over. For you, does using the awesomebar to switch tabs make the
> URL more important than when you switch tabs by clicking on them?
>

The short answer here would probably be yes.  The long answer in its shortest form is because by default the URL pull path is always visible and I use it often to navigate or create load new tabs or load history, I rarely browse history to go back to something, unless its navigation back to a pervious URL.  I do use it more like a search bar to find stuff I've been to before then going into bookmarks to load something now.

(FWIW, I come from the era of DOS, and I use Windows Explorer the same way, I both type the path and i love to use tree view, but Breadcrumbs are still a bit hard to use for me.  I have yet to use Windows Search like I use the Awesomebar to search though.)  So its a habit I think.
 
> > Or is that not the goal to always see your tabs regardless of the
> > awesomebar config setting? 
> 
> No, the goal is to only show relevant tabs (relevant to what you typed in).
> Unless specifically asking for only tabs (using the restrict character), of
> course.
> 

ok, good to know.


> > The above scenario, 12 seems to be max limit, so
> > you've have to know how to spell the URL in order to find the second tab.
> 
> It also matches tab titles, too. But yes, see comment 36.
> 

Sometimes this is an annoying part of awesomebar matches if tab titles have W and I start typing www. :)

> > I don't know if there would be a clean way to display that if there is a lot of
> > open tab matches for the same domain?  Is there a way to increase the max limit
> > of listed items so more tabs would be listed in my results with lots of
> > history?
> 
> If you're specifically looking for a tab, using the restrict character (comment
> 36) is the way to go. Of course, that not an obvious behavior.
> 

Sounds good, though here is an idea.  Would it be easier to remember and fun to type 'open tab' first? (which would be like using TaskFox), but would get in the way of Title matches.  I'm just thinking I might remember something like that much more than a special character (much like voice dictation) and maybe good for Accessibility.

> However, this is specifically a case that I'm worried about if tab matches are
> weighted heavier. If there are a lot of tabs matching the search string (eg, a
> lot of tabs on the same domain, and the domain name is typed in), then its
> going to be difficult to get any result that isn't an open tab.
> 
> > There is odd awesomebar UI behavior with the drop down, and probably affects
> > the trunk too.  Sometimes I get a listbox, and sometimes I get a combobox.  I
> > also noticed different awesomebar listbox or combobox length occurring
> > depending on how we interact with the awesomebar and which tab is active.
> 
> Er, no idea if that's related or not. Could you attach a screenshot?

I've taken a few screenshots, but before I start attaching a bunch or upload a zip to share, the listbox and combobox I cannot tell based on results at what point do we change the view from 12 items combo to 12 items listbox or < 12 items combobox. This feels like a UX bug more than anything.  I believe this is on the trunk also.

I see the list size changes as we filter matches, ie list gets smaller than 12.  That's ok I think, I had to think about that for awhile till it made sense.

The issue with switching between tabs and getting a different awesomebar list sometimes happens, not sure if I can find STR before opening another bug for that.  Basically I don't know if its related to total query result mix.

The only other awesomebar issue that I might be seeing relates to just clicking the drop down arrow varies in total list length, again, I suppose its related somehow to saved query mix results.  This is trunk think it looks like.
Attachment #417807 - Flags: review?(gavin.sharp) → review?(mconnor)
Blair, I'm going to wait on doing any real review of this until Marco's comments are addressed, or there's going to be a lot of rehashing.  I'd really like to get this in on trunk for feedback ASAP, so I'm going to be responsive once you get it updated! :)
Depends on: 538899
Some (rather random) comments:

(In reply to comment #43)
> +  let windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"]
> +                         .getService(Ci.nsIWindowMediator);
> 
> Can we make this a lazy getter? i don't see the need to get a new wm for each
> tab switch.

Sadly, nsIWindowMediator is used a lot like this in browser.js - I'd rather not change all those cases in this bug. Filed bug 538899.


> +                if (this.mTabBrowser.mCurrentTab == this.mTab) {
> +                  for (let i = 0; i < this.mTabBrowser.mProgressListeners.length; i++) {
> +                    let p = this.mTabBrowser.mProgressListeners[i];
> +                    if (p)
> +                      try {
> +                        p.onLocationChange(aWebProgress, aRequest, aLocation);
> +                      } catch (e) {
> +                        // don't inhibit other listeners
> +                        Components.utils.reportError(e);
> +                      }
> +                  }
> +                }
> +
> +                for (let i = 0; i < this.mTabBrowser.mTabsProgressListeners.length; i++) {
> +                  let p = this.mTabBrowser.mTabsProgressListeners[i];
>                    if (p)
>                      try {
> -                      p.onLocationChange(aWebProgress, aRequest, aLocation);
> +                      p.onLocationChange(this.mBrowser, aWebProgress, aRequest, aLocation);
>                      } catch (e) {
>                        // don't inhibit other listeners
>                        Components.utils.reportError(e);
>                      }
>                  }
>                }
> 
> hm, these 2 loops are looping against same dataset, i guess the first if could be handled inside the second loop
> so that we don't have a worst case where we loop the same dataset twice.

Not the same dataset - one is mProgressListeners, the other is mTabsProgressListeners.


> +  for (let url in tabs) {
> +    if (!is_expected_in_db(url)) // ignore URLs that should never be in the places db
> 
> are we adding them in this test?
> How do you handle such urls with switch to tab? is this path tested?

about:blank is tested, I think (need to confirm that). It's not possible to switch to such URLs, as they're never in the places DB and therefore never in the autocomplete results. To change that, I think I'd need to either duplicate some of the schema of moz_places (and make a lot of queries very ugly and complex), or add them to moz_places with a flag to ignore it for most cases.


> +let kURIs = [
> +  "http://abc.com/",
> +  "moz-action:switchtab,http://abc.com/",
> +  "http://xyz.net/",
> +  "moz-action:switchtab,http://xyz.net/"
> +];
> 
> const URIS
> 
> +let kTitles = [
> +  "ABC rocks",
> +  "xyz.net - we're better than ABC"
> +];
> 
> const TITLES

The autocomplete test framework (in head_autocomplete.js) requires using kURIs and kTitles.
(In reply to comment #48)
> > hm, these 2 loops are looping against same dataset, i guess the first if could be handled inside the second loop
> > so that we don't have a worst case where we loop the same dataset twice.
> 
> Not the same dataset - one is mProgressListeners, the other is
> mTabsProgressListeners.

Oh, none of that was any actual code change - just an indent change.
Attached patch WiP v4 (obsolete) — — Splinter Review 
This should fix the various review comments and RTL issues. Except:

* I haven't moved anything out of nsIBrowserHistory, as I can't find a better place for it (not convinced mozIPlacesAutoComplete is any better)
* No private browsing mode test yet. I've verified it works as intended, and pretty sure it can't work contrary to that, due to the way toggling private browsing is designed. Still, a test would be nice.
* I'd like a test for opening/closing a window

Ehsan: Would you mind scrutinizing RTL again?
Attachment #417807 - Attachment is obsolete: true
Attachment #418249 - Attachment is obsolete: true
Attachment #418254 - Attachment is obsolete: true
Attachment #421223 - Flags: review?
Attachment #417807 - Flags: review?(mconnor)
Attachment #421223 - Flags: review? → review?(mconnor)
Attachment #421223 - Flags: review?(mconnor) → review-
Comment on attachment 421223 [details] [diff] [review]
WiP v4

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js

>+pref("browser.urlbar.restrict.tab", "$");

I'm mildly concerned that this won't be especially friendly for non-US keyboard layouts.  Maybe it's common, I honestly don't know.  What about ! instead?

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

> 
>+#urlbar .urlbar-display {
>+  display: none;
>+  -moz-appearance: none;
>+}
>+
>+#urlbar[actiontype] .urlbar-display {
>+  display: -moz-box;
>+}
>+
>+#urlbar[actiontype="switchtab"] .urlbar-display {
>+  -moz-binding: url("chrome://browser/content/urlbarBindings.xml#urlbar-display-switchtab");
>+}

hmm, does this attach/detach the binding each time we set that attribute?  what's the overhead like on that?

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+function switchToTabHavingURL(aUrl) {

You want this to take an nsIURI...

>+  function switchIfURLInWindow(aWindow) {
>+    if (!("gBrowser" in aWindow))
>+      return false;
>+    for (let bIndex = 0; bIndex < aWindow.gBrowser.browsers.length; bIndex++) {
>+      let browser = aWindow.gBrowser.browsers[bIndex];
>+      if (browser.currentURI.spec == aUrl) {

because uri == uri is about 30x faster than uri.spec == uri.spec

(not to mention it's probably just good sanity checking to not even call this unless it's actually a valid URI, never underestimate the ability of add-ons to muck with your actions)

>+  // Prioritise this window.
>+  if (switchIfURLInWindow(window))
>+    return true;

>+  let windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"]
>+                         .getService(Ci.nsIWindowMediator);
>+  let winEnum = windowMediator.getEnumerator("navigator:browser");

it's a shame getZOrder is broken on Mac/Linux, because we wouldn't need to special-case :(

>+  while (winEnum.hasMoreElements()) {
>+    let browserWin = winEnum.getNext();
>+    if (browserWin.closed) // Skip closed (but not yet destroyed) windows.
>+      continue;
>+    if (browserWin == window) // Don't check this window again.
>+      continue;

just combine these two into one if statement

>+    if (switchIfURLInWindow(browserWin))
>+      return true;
>+  }
>+  // No openned tab has that url.

opened

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>@@ -525,42 +529,49 @@
> 
>               if (aWebProgress.DOMWindow == this.mBrowser.contentWindow &&
>                   aWebProgress.isLoadingDocument)
>                 this.mTabBrowser.getBrowserForTab(this.mTab).mIconURL = null;
> 
>               // changing location, clear out the missing plugins list
>               this.mBrowser.missingPlugins = null;
> 
>-              if (this.mBlank)
>-                return;
>-
>-              if (this.mTabBrowser.mCurrentTab == this.mTab) {
>-                for (let i = 0; i < this.mTabBrowser.mProgressListeners.length; i++) {
>-                  let p = this.mTabBrowser.mProgressListeners[i];
>+              var browserHistory = this.mTabBrowser.mBrowserHistory;
>+              if ("lastURI" in this.mBrowser && this.mBrowser.lastURI)
>+                browserHistory.unregisterOpenTab(this.mBrowser.lastURI);
>+              browserHistory.registerOpenTab(aLocation);
>+
>+              if (!this.mBlank) {
>+                if (this.mTabBrowser.mCurrentTab == this.mTab) {
>+                  for (let i = 0; i < this.mTabBrowser.mProgressListeners.length; i++) {
>+                    let p = this.mTabBrowser.mProgressListeners[i];
>+                    if (p)
>+                      try {
>+                        p.onLocationChange(aWebProgress, aRequest, aLocation);
>+                      } catch (e) {
>+                        // don't inhibit other listeners
>+                        Components.utils.reportError(e);
>+                      }
>+                  }
>+                }
>+               
>+                for (let i = 0; i < this.mTabBrowser.mTabsProgressListeners.length; i++) {
>+                  let p = this.mTabBrowser.mTabsProgressListeners[i];
>                   if (p)
>                     try {
>-                      p.onLocationChange(aWebProgress, aRequest, aLocation);
>+                      p.onLocationChange(this.mBrowser, aWebProgress, aRequest, aLocation);
>                     } catch (e) {
>                       // don't inhibit other listeners
>                       Components.utils.reportError(e);
>                     }
>                 }
>               }
>-
>-              for (let i = 0; i < this.mTabBrowser.mTabsProgressListeners.length; i++) {
>-                let p = this.mTabBrowser.mTabsProgressListeners[i];
>-                if (p)
>-                  try {
>-                    p.onLocationChange(this.mBrowser, aWebProgress, aRequest, aLocation);
>-                  } catch (e) {
>-                    // don't inhibit other listeners
>-                    Components.utils.reportError(e);
>-                  }
>-              }
>+              
>+              this.mBrowser.lastURI = aLocation.clone();
>+              

I'm not sure why you changed the flow here.  It seems like you could just move this last bit to before the loops with the added block, and leave the existing code blocks including the early return in place after the added code.  Am I missing something?

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml
>--- a/browser/base/content/urlbarBindings.xml
>+++ b/browser/base/content/urlbarBindings.xml
>@@ -33,19 +33,48 @@
> # use your version of this file under the terms of the MPL, indicate your
> # decision by deleting the provisions above and replace them with the notice
> # and other provisions required by the GPL or the LGPL. If you do not delete
> # the provisions above, a recipient may use your version of this file under
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK *****
> 
>-<bindings id="urlbarBindings" xmlns="http://www.mozilla.org/xbl">
>+<bindings id="urlbarBindings"
>+          xmlns="http://www.mozilla.org/xbl"
>+          xmlns:html="http://www.w3.org/1999/xhtml"
>+          xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+          xmlns:xbl="http://www.mozilla.org/xbl">
> 
>   <binding id="urlbar" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete">
>+    <content sizetopopup="pref">
>+      <xul:hbox class="autocomplete-textbox-container" flex="1" xbl:inherits="focused">
>+        <children includes="image|deck|stack|box">
>+          <xul:image class="autocomplete-icon" allowevents="true"/>
>+        </children>
>+
>+        <xul:hbox anonid="textbox-input-box" class="textbox-input-box" flex="1" xbl:inherits="tooltiptext=inputtooltiptext">
>+          <children/>
>+          <xul:hbox class="urlbar-display-container" flex="1">
>+            <xul:label class="urlbar-display"/>
>+            <html:input anonid="input" class="autocomplete-textbox textbox-input"
>+                        flex="1" allowevents="true"
>+                        xbl:inherits="tooltiptext=inputtooltiptext,onfocus,onblur,value,type,maxlength,disabled,size,readonly,tabindex,accesskey"/>
>+          </xul:hbox>
>+        </xul:hbox>
>+        <children includes="hbox"/>
>+      </xul:hbox>
>+
>+      <xul:dropmarker anonid="historydropmarker" class="autocomplete-history-dropmarker"
>+                      allowevents="true"
>+                      xbl:inherits="open,enablehistory,parentfocused=focused"/>
>+
>+      <xul:popupset anonid="popupset" class="autocomplete-result-popupset"/>
>+    </content>

It seems that we should simply add the xul:label to the core binding.  This feels like unnecessary forking.


>@@ -65,16 +94,54 @@
>       <destructor><![CDATA[
>         this._prefs.removeObserver("", this);
>         this._prefs = null;
>         this.inputField.controllers.removeController(this._copyCutController);
>         this.inputField.removeEventListener("mousedown", this, false);
>         this.inputField.removeEventListener("mousemove", this, false);
>         this.inputField.removeEventListener("mouseout", this, false);
>       ]]></destructor>
>+      
>+      <field name="_value"></field>
>+      <property name="value">
>+        <getter>
>+          if (this.hasAttribute("isempty"))
>+            return "";
>+          if (this.hasAttribute("actiontype"))
>+            return this._value;
>+          return this.inputField.value;
>+        </getter>
>+        <setter>
>+          this._value = val;
>+          var displayValue = val;
>+          if (/^moz-action:/.test(val)) {
>+            let [,action, param] = val.match(/^moz-action:([^,]+),(.*)$/);
>+            displayValue = param;
>+            this.setAttribute("actiontype", action);
>+          } else {
>+            this.removeAttribute("actiontype");
>+          }
>+
>+          this.mIgnoreInput = true;
>+          if (displayValue) {
>+            // clear the emptyText _before_ setting a new non-empty value
>+            this._clearEmptyText();
>+            this.inputField.value = displayValue;
>+          } else {
>+            // display the emptyText _after_ setting a value that's an empty string
>+            this.inputField.value = displayValue;
>+            this._updateVisibleText();
>+          }
>+          this.mIgnoreInput = false;
>+          var event = document.createEvent('Events');
>+          event.initEvent('ValueChange', true, true);
>+          this.inputField.dispatchEvent(event);
>+          return val;
>+        </setter>
>+      </property>

So if this is where the moz-action setting gets handled, why is other stuff in the core binding?  We're sort of half and half here, and reasoning would be good.

>+          if (/^moz-action:/.test(url)) {
>+            // url is in the format moz-action:ACTION,PARAM
>+            let [,action, param] = url.match(/^moz-action:([^,]+),(.*)$/);
>+
>+            if (action == "switchtab") {
>+              switchToTabHavingURL(param);
>+            } else {
>+              // XXXunf do something better here
>+              Components.utils.reportError("Unknown action: " + action);
>+            }

what would be better than this? Seems like it's fine to do this.


>           // Check for middle-click or modified clicks on the URL bar
>           if (gURLBar && this.mInput == gURLBar) {
>             var url = controller.getValueAt(this.selectedIndex);
> 
>             // close the autocomplete popup and revert the entered address
>             this.closePopup();
>             controller.handleEscape();
> 
>+            // Check if this is meant to be an action
>+            if (/^moz-action:/.test(url)) {
>+            let [,action, param] = url.match(/^moz-action:([^,]+),(.*)$/);

this pattern is repeated four times... probably should be a method somewhere, maybe on the main binding if we're exposing actions there?


>+  <binding id="urlbar-display-switchtab">
>+    <content>
>+      <xul:stringbundle anonid="bundle" src="chrome://global/locale/actions.properties"/>
>+      <xul:label anonid="label" class="action-label"/>
>+    </content>
>+    <implementation>
>+      <constructor>
>+        this._label.value = this._bundle.getString("switchtab.urlbar.label");
>+      </constructor>
>+      <field name="_bundle">
>+        document.getAnonymousElementByAttribute(this, "anonid", "bundle");
>+      </field>
>+      <field name="_label">
>+        document.getAnonymousElementByAttribute(this, "anonid", "label");
>+      </field>

should these be readonly?  seems like yes.

>diff --git a/browser/themes/gnomestripe/browser/browser.css b/browser/themes/gnomestripe/browser/browser.css
>diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css
>diff --git a/browser/themes/winstripe/browser/browser.css b/browser/themes/winstripe/browser/browser.css

>+richlistitem[type="action"][actiontype="switchtab"] .ac-action-icon {
>+  list-style-image: url("data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABcAAAAQCAYAAAD9L+QYAAAABGdBTUEAALGPC/xhBQAAAAlwSFlzAAAOwgAADsIBFShKgAAAABl0RVh0U29mdHdhcmUAUGFpbnQuTkVUIHYzLjUuMU7nOPkAAACiSURBVDhP7ZFLCoMwFEXdQubOsyiX4cydWPpRgrS0nXQopYOuLN4DCg5MNULBgQ8OAe/H8JIk+2xhA19d4iPeC8FLZtG0cr3EUzzEPQAaHrxk5sd7n4OcjajFJQBaM/LPl48clJ9EGQANz6q5KXX+UY6GJ36MMVelKnGY+AHfqt4TX+6cK6y1x8DeazQ88c1K6KFSkQ0PNnGipavK99DfNtABA3pmvysbBWEAAAAASUVORK5CYII=");

Why is this image inline?

> .autocomplete-treebody::-moz-tree-cell-text(treecolAutoCompleteComment) {
>   color: GrayText;
> }
> 
> .ac-comment[selected="true"], .ac-url-text[selected="true"] {
>   color: inherit !important;
> }

>diff --git a/nsprpub/configure b/nsprpub/configure

>-DEFS=`sed -f conftest.defs confdefs.h | tr '\012' ' ' | tr '\015' ' '` # Manually modified for MKS support.
>+DEFS=`sed -f conftest.defs confdefs.h | tr '\012' ' '`

what's up here?  bad merge, maybe?

>diff --git a/toolkit/components/places/public/nsIBrowserHistory.idl b/toolkit/components/places/public/nsIBrowserHistory.idl
>--- a/toolkit/components/places/public/nsIBrowserHistory.idl
>+++ b/toolkit/components/places/public/nsIBrowserHistory.idl
>@@ -143,9 +143,24 @@ interface nsIBrowserHistory : nsIGlobalH
> 
>     /**
>      * markPageAsTyped
>      *
>      * Designate the url as having been explicitly typed in by
>      * the user, so it's okay to be an autocomplete result.
>      */
>     void markPageAsTyped(in nsIURI aURI);
>+    
>+    /**
>+     * registerOpenTab
>+     *
>+     * Mark a page as being currently open in a tab.
>+     */
>+    void registerOpenTab(in nsIURI aURI);
>+    
>+    /**
>+     * unregisterOpenTab
>+     *
>+     * Mark a page as no longer being open in a tab (either by closing or
>+     * by navigating away from that page).
>+     */
>+    void unregisterOpenTab(in nsIURI aURI);
> };

So, these are only implemented if MOZ_XUL is defined, not sure that's right, really.  At the least, should be documented in the IDL that it's not implemented for non-MOZ_XUL embeddors

Given the API changes, I think next round should be r?gavin, sr?mconnor, but I think we're on the right path.
>-DEFS=`sed -f conftest.defs confdefs.h | tr '\012' ' ' | tr '\015' ' '` # Manually modified for MKS support.
>+DEFS=`sed -f conftest.defs confdefs.h | tr '\012' ' '`

FYI you should not anymore run configure in nsprpub folder if you pymake. doing so will cause this spurious change. see bug 538555.
(In reply to comment #52)
> FYI you should not anymore run configure in nsprpub folder if you pymake. doing
> so will cause this spurious change. see bug 538555.


Excellent - thanks!



(In reply to comment #51)
> >+pref("browser.urlbar.restrict.tab", "$");
> 
> I'm mildly concerned that this won't be especially friendly for non-US keyboard
> layouts.  Maybe it's common, I honestly don't know.  What about ! instead?


Will get some l10n input, but I think its pretty common.


> >+#urlbar[actiontype="switchtab"] .urlbar-display {
> >+  -moz-binding: url("chrome://browser/content/urlbarBindings.xml#urlbar-display-switchtab");
> >+}
> 
> hmm, does this attach/detach the binding each time we set that attribute? 
> what's the overhead like on that? 


Yes. AFAIK, there isn't a high overhead, and the binding should be cached (this technique is used in various other places too, without noticeable overhead). It's mostly there for future-proofing, for richer displays (such as bug 524050).


> >+              this.mBrowser.lastURI = aLocation.clone();
> >+              
> 
> I'm not sure why you changed the flow here.  It seems like you could just move
> this last bit to before the loops with the added block, and leave the existing
> code blocks including the early return in place after the added code.  Am I
> missing something?


lastURI was originally set by one of the listeners, which still uses that property. Setting it before the listeners get called would mean it'd be useless to them (as it would equal the current URI).


> >diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml
> >--- a/browser/base/content/urlbarBindings.xml
> >+++ b/browser/base/content/urlbarBindings.xml
> It seems that we should simply add the xul:label to the core binding.  This
> feels like unnecessary forking.


It felt wrong to (further) pollute whats meant to be a generic widget. I've changed it so its added to the base binding now (although bug 539357 may end up changing it back).


> So if this is where the moz-action setting gets handled, why is other stuff in
> the core binding?  We're sort of half and half here, and reasoning would be
> good.


Essentially, because its already a mess. After discussions on IRC, I'll clean all this up in bug 539357.


> >+              // XXXunf do something better here
> >+              Components.utils.reportError("Unknown action: " + action);
> >+            }
> 
> what would be better than this? Seems like it's fine to do this.


Ok.


> 
> >           // Check for middle-click or modified clicks on the URL bar
> >           if (gURLBar && this.mInput == gURLBar) {
> >             var url = controller.getValueAt(this.selectedIndex);
> > 
> >             // close the autocomplete popup and revert the entered address
> >             this.closePopup();
> >             controller.handleEscape();
> > 
> >+            // Check if this is meant to be an action
> >+            if (/^moz-action:/.test(url)) {
> >+            let [,action, param] = url.match(/^moz-action:([^,]+),(.*)$/);
> 
> this pattern is repeated four times... probably should be a method somewhere,
> maybe on the main binding if we're exposing actions there?


Done, but in urlbarBindings.xml - since it would just be moved there anyway in bug 539357.


> >-DEFS=`sed -f conftest.defs confdefs.h | tr '\012' ' ' | tr '\015' ' '` # Manually modified for MKS support.
> >+DEFS=`sed -f conftest.defs confdefs.h | tr '\012' ' '`
> 
> what's up here?  bad merge, maybe?


Ugh, see comment 52.


> >diff --git a/toolkit/components/places/public/nsIBrowserHistory.idl b/toolkit/components/places/public/nsIBrowserHistory.idl
> >--- a/toolkit/components/places/public/nsIBrowserHistory.idl
> >+++ b/toolkit/components/places/public/nsIBrowserHistory.idl
> 
> So, these are only implemented if MOZ_XUL is defined, not sure that's right,
> really.  At the least, should be documented in the IDL that it's not
> implemented for non-MOZ_XUL embeddors


This is as per review comments in comment 43. I saw the discussion on IRC about this - the code doesn't depend on XUL in any way, and the behavior doesn't change by removing that code (aside from when explicitly calling those functions). So I'll remove the check for MOZ_XUL.
(In reply to comment #53)
> (In reply to comment #51)
> > >+pref("browser.urlbar.restrict.tab", "$");
> > 
> > I'm mildly concerned that this won't be especially friendly for non-US keyboard
> > layouts.  Maybe it's common, I honestly don't know.  What about ! instead?

I can confirm that several of the European keyboards have hard-to-reach $ character. I agree that ! would be better.
(In reply to comment #54)
> I can confirm that several of the European keyboards have hard-to-reach $
> character. I agree that ! would be better.

I see $ in shift+4 for most european keyboards, which ones don't have that?
On french keyboards, $ is near the return key, no need for shift or AltGr.
(In reply to comment #55)
> (In reply to comment #54)
> > I can confirm that several of the European keyboards have hard-to-reach $
> > character. I agree that ! would be better.
> 
> I see $ in shift+4 for most European keyboards, which ones don't have that?

I'm neither for nor against the idea of using '$', but in case it helps, this Wikipedia page has a fairly good summary of various keyboard layouts:
http://en.wikipedia.org/wiki/Keyboard_layout#QWERTY

FWIW, keyboard layouts without $ at Shift-4 include:
* Belgian   (AltGr + [key near Enter])
* Faroese   (AltGr + 4)
* Finnish   (AltGr + 4)
* French    (AltGr + [key near Enter])
* Hungarian (AltGr + [key near Enter])
* Norwegian (AltGr + 4)
* Romanian  (AltGr + [key near Enter])
* Swedish   (AltGr + 4)
* Swiss     (I was unable to find the '$' key -- but maybe I missed it? http://en.wikipedia.org/wiki/File:KB_Swiss.svg )
* Russian   (I was unable to find the '$' key -- but maybe I missed it? http://en.wikipedia.org/wiki/File:KB_Russian.svg )
(In reply to comment #57)
> * Swiss     (I was unable to find the '$' key -- but maybe I missed it?
> http://en.wikipedia.org/wiki/File:KB_Swiss.svg )

On second look, I do see the '$' on the Swiss layout now; it's (AltGr + [key near Enter]).
French and Belgian keyboards do not need AltGr (as I said before).
If you type AltGr + $ you get ¤
Swiss layout doesn't need AltGr too (for both Swiss French and Swiss German).
even if the wikipedia russian layout does not have the dollar sign, i can find various russian layouts on the Net that have it as shift+4, especially photos of real keyboards.
The "second part" of a good experience with this has now been filed as bug 539598.
Let's try that again, and see if it links:

The "second part" of a good experience with this has now been filed as bug
#539598.
See Also: → 539598
Attached patch WiP v5 (obsolete) — — Splinter Review 
I have a failing test that I can't reproduce locally - I'm still working on that.
Attachment #421223 - Attachment is obsolete: true
Attachment #421726 - Flags: review?(gavin.sharp)
some drive-by comments.

maybe you could use ContentAreaUtils.ioService to call newURI in browser.js, should be available i think. there are various bugs about consolidating services in browser though.

you still have b/nsprpub/configure change in the patch, you should get rid of it.

+  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
+      "INSERT OR REPLACE INTO moz_opentabs_temp "
+      "(place_id, open_count) "
+      "VALUES (?1, "
+      "  IFNULL("
+      "    (SELECT open_count + 1 FROM moz_opentabs_temp WHERE place_id = ?1), "
+      "    1"
+      "  )"
+      ")"),
move apices with indent, per Places code style

+  PRBool canAdd = PR_FALSE;
+  nsresult rv = CanAddURI(aURI, &canAdd);
please add a comment above that canAddURI will also check privateBrowsing.

 #ifdef MOZ_XUL
     mDBFeedbackIncrease,
+    mDBRegisterOpenTab,
+    mDBUnregisterOpenTab,
 #endif

this is wrong, you removed MOZ_XUL from apis using these statements, so they should be out of the ifdef

+#ifdef MOZ_XUL
+  nsCOMPtr<mozIStorageStatement> mDBRegisterOpenTab; // used by RegisterOpenTab
+  nsCOMPtr<mozIStorageStatement> mDBUnregisterOpenTab; // used by UnregisterOpenTab
+#endif
+

ditto
makeURI in contentAreaUtils would work, yeah, and it's in scope for browser.js (just look for makeURI calls)
(In reply to comment #66)
> maybe you could use ContentAreaUtils.ioService to call newURI in browser.js,
> should be available i think. there are various bugs about consolidating
> services in browser though.
!!!

NetUtil.newURI! http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#179
(In reply to comment #68)
> NetUtil.newURI!
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#179

makeURI() is already in scope - NetUtil.jsm isn't imported there yet.
Attached patch Patch v1 (obsolete) — — Splinter Review 
This has Marco's comments addressed (comment 66), the additional tests I wanted, and the failing test fixed. 

Note that there's a bit of a hack in the browser chrome test using setTimout. I loathe using that, but it that was the only reliable way I could get it working.

I haven't changed the restrict character - waiting on some i18n input.
Attachment #421726 - Attachment is obsolete: true
Attachment #422506 - Flags: review?(gavin.sharp)
Attachment #421726 - Flags: review?(gavin.sharp)
Do we know that the restricting characters are widely-enough used that it's worth extending this tool? Seems like a strange and obscure UI to me.
(In reply to comment #71)
> Do we know that the restricting characters are widely-enough used that it's
> worth extending this tool? Seems like a strange and obscure UI to me.

it is obscure indeed and for sure we need a more discoverable way, but since it's in place from a long time would be crazy to not update it.
I disagree that not adding another magic character would be crazy...
I don't see why adding one more hurts.  Unless you think lots of searches will begin with "$ "?  If we have the feature, it makes little sense to add another result type without the corresponding filter option.  If you really want to debate the future of the feature, file a separate bug, but killing it incrementally by limiting it to the original set of result types is not the way to go about it.
Right, I'd rather kill it altogether. It's showing its weakness and occupying development time right now, as we're running out of suitable characters (which is not to say that the current characters are great).
I think the ability for a feature to "scale" in terms of filters isn't a sign of weakness, per se.  It's a feature we've advertised to users, and we have _no_ data at all to show cost vs. benefit, but please file a new bug if you're so inclined (and cc me please!)
I don't actually care myself. It was meant as a suggestion to those actively working on this. If people think this is useful (or used, even) and if the same people are willing to spend time on it, I'm not going to work against this.

The advertisement I know of was a single blog post by edlee, but it's entirely possible that I missed something else.
FWIW, I'd rather swap the characters for words (tab, bookmark, etc) - can do this right now by just changing the relevant prefs. It'd also benefit from a better UI for showing that's what is being used, and something to expose it better.

Anyway, I got some i18n input from smontagu - he suggests that "%" is a bit more common than either "$" or "!".
(In reply to comment #78)
> FWIW, I'd rather swap the characters for words (tab, bookmark, etc) - can do
> this right now by just changing the relevant prefs.

It should be in a separate bug, since it's not about this bug (and we need a bug to think about better UI after all). But if we do that, what happens if i want to search a page containing the word "bookmark"? At that point would be better to have a single special char followed by the word, supposing to use "?" it would end up like "?bookmark mysearch", "?tab mysearch".
ps: but notice that it will end up being a lot of typing if we don't autocomplete the thing, to search bookmarks i should type the full "b o o k m a r k" word... it's a bit crazy. maybe just "?b", "?t" "?h".
Comment on attachment 422506 [details] [diff] [review]
Patch v1

This review was done against the previous version of patch, but I don't think any of the changes in the newer version affect these comments.

Not ready to r+ this yet, but figured I'd post these comments just to keep things rolling.

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

>+#urlbar .urlbar-display {
>+  display: none;
>+  -moz-appearance: none;
>+}

-moz-appearance none seems redundant?

>+#urlbar[actiontype] .urlbar-display {
>+  display: -moz-box;
>+}

Don't both of these rules belongs in the toolkit, to avoid changing the behavior for non-Firefox consumers of that binding? The "display: none" should probably even instead be replaced by a "hidden" attribute on urlbar-display.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+function switchToTabHavingURI(aURI) {

>+  function switchIfURIInWindow(aWindow) {

>+    for (let bIndex = 0; bIndex < aWindow.gBrowser.browsers.length; bIndex++) {

Use a temp variable for aWindow.gBrowser.browsers ? Also would rather "i" instead of "bIndex".

>+        aWindow.gBrowser.tabContainer.selectedIndex = bIndex;

aWindow.gBrowser.selectTabAtIndex(bIndex);

>+        aWindow.gBrowser.selectedBrowser.focus();

Is this necessary?

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>         <textbox id="urlbar" flex="1"

>-                 oninput="gBrowser.userTypedValue = this.value;"
>+                 oninput="gBrowser.userTypedValue = this.value; this.resetActionType();"

Why is this needed here?

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>+      <field name="mBrowserHistory" readonly="true">

"readonly" doesn't do anything on fields, so remove this (and other instances in this patch). I guess the existing fields have this problem too, wouldn't hurt to clean them up.

The tabbrowser being responsible of tracking "loaded page" makes me nervous, for some reason. My first reaction was that this should be handled at a lower level, but I suppose that could also run into issues (e.g. things being loaded in a non-user visible way being marked as loaded). I suppose this is probably the best way forward, but I'll have to give this a bit more thought.

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>-<bindings id="urlbarBindings" xmlns="http://www.mozilla.org/xbl">
>+<bindings id="urlbarBindings"
>+          xmlns="http://www.mozilla.org/xbl"
>+          xmlns:html="http://www.w3.org/1999/xhtml"
>+          xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+          xmlns:xbl="http://www.mozilla.org/xbl">

These changes no longer appear to be needed.

>+      <field name="_value"></field>
>+      <property name="value">

Overriding this setter/getter worries me a bit, but I'll need to think through the implications a bit more.

>+  <binding id="urlbar-rich-result-box" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistbox">

Why is this binding needed? Don't see it doing anything but setting the "beforecurrent" attribute, which doesn't appear to be used...

>+  <binding id="urlbar-display-switchtab">

This binding doesn't seem very useful. Seems like it could be replaced by having "urlbar-display" inheriting an attribute from its parent (i.e. xbl:inherits="label=displaystring" or somesuch). Though come to think of it, I think I disagree with mconnor about putting this in the core binding, especially given bug 539357. Why can't you just add it to as a child of the <textbox> in browser.xul, as we do with the identity handler? urlbar-display-container seems to be used for RTL styling, but I'm not sure whether it's actually needed.

>diff --git a/browser/themes/gnomestripe/browser/browser.css b/browser/themes/gnomestripe/browser/browser.css

>+#urlbar:-moz-locale-dir(rtl) .urlbar-display-container {
>+  direction: rtl;
>+}
>+
>+#urlbar html|*.autocomplete-textbox {
>+  direction: ltr;
>+}

>+richlistitem[type="action"]:-moz-locale-dir(rtl) .ac-url-box {
>+  direction: rtl;
>+}

If these are needed, shouldn't they live in content/ rather than the theme?

>diff --git a/toolkit/components/places/public/mozIPlacesAutoComplete.idl b/toolkit/components/places/public/mozIPlacesAutoComplete.idl

>-[scriptable, uuid(a5ae8332-333c-412a-bb02-a35df8247714)]
>+[scriptable, uuid(cb269885-85ef-4f85-9f2c-84d678bb5e82)]

>+  const long BEHAVIOR_TAB = 1 << 7;

No need to rev the IID just for adding a constant.

>diff --git a/toolkit/components/places/public/nsIBrowserHistory.idl b/toolkit/components/places/public/nsIBrowserHistory.idl

>+    /**
>+     * registerOpenTab
>+     *
>+     * Mark a page as being currently open in a tab.
>+     */
>+    void registerOpenTab(in nsIURI aURI);
>+    
>+    /**
>+     * unregisterOpenTab
>+     *
>+     * Mark a page as no longer being open in a tab (either by closing or
>+     * by navigating away from that page).
>+     */
>+    void unregisterOpenTab(in nsIURI aURI);

Don't think these need to mention "tabs" at all, right? Seems like "currently loaded in a user-visible way" is all we really care about, and we can leave specifics to the app rather than over-specifying.

>diff --git a/toolkit/components/places/src/nsPlacesAutoComplete.js b/toolkit/components/places/src/nsPlacesAutoComplete.js

>-const kQueryIndexTyped = 7;
>-const kQueryIndexPlaceId = 8;
>-const kQueryIndexQueryType = 9;
>+const kQueryIndexTabOpenCount = 7;
>+const kQueryIndexTyped = 8;
>+const kQueryIndexPlaceId = 9;
>+const kQueryIndexQueryType = 10;

Why change these, OOC?
Attachment #422506 - Flags: review?(gavin.sharp)
(In reply to comment #81)
> >diff --git a/toolkit/components/places/public/nsIBrowserHistory.idl b/toolkit/components/places/public/nsIBrowserHistory.idl
> >
> >+    void registerOpenTab(in nsIURI aURI);
>
> Don't think these need to mention "tabs" at all, right? Seems like "currently
> loaded in a user-visible way" is all we really care about, and we can leave
> specifics to the app rather than over-specifying.

this suggestion is awesome, something a-la registerVisiblePage, even if this name sucks a bit, registerPageInVisibleContent?. uh, naming is hard.
I've just realized an unfortunate side-effect of how normalized the places DB is: When a tab's URL is sanitized (completely removed from the DB), it will no longer be considered a tab match (until the page is visited again and re-added to the DB). 

I don't see any simple fix for this. I might be able to work around it by duplicating data from moz_places in moz_opentabs_temp, and doing an additional subquery (note: that may mean the tab match and normal result would not be grouped together anymore, depending on how its done). I don't think that'd be a huge hit on performance of the queries, but it would affect it somehow.
Just to reiterate our conversation on IRC:

I don't think this is a showstopper, but it would of course be great if we can work around it somehow. The users that clear their history all the time probably aren't in the same group of people that rely on their history to do everything, including switching tabs. :)

That being said, if you can find a way around this without affecting performance, go for it. :)

(Could we not clear the entries in history for the tabs that are currently open, or something?)
(In reply to comment #79)
> (In reply to comment #78)
> > FWIW, I'd rather swap the characters for words (tab, bookmark, etc) - can do
> > this right now by just changing the relevant prefs.
> 
> It should be in a separate bug, since it's not about this bug

Yea, sorry - was just thinking aloud :) Filed bug 541891.
(In reply to comment #84)
> (Could we not clear the entries in history for the tabs that are currently
> open, or something?)

I don't think that's a viable option - when someone wants to clear their history, or remove all traces of a site (Forget this site), there shouldn't be exceptions. Adding exceptions would just neuter that functionality.
(In reply to comment #86)
> I don't think that's a viable option - when someone wants to clear their
> history, or remove all traces of a site (Forget this site), there shouldn't be
> exceptions.

Isn't this information only kept in memory? I thought that's what the _temp implied - not super familiar with all of the Places stuff. If this never hits the disk, we don't have to worry about its implications on "forget this site" functionality.
PS: if you're taking Gavin's suggestion to rename RegisterOpenTab to something more generic, please also rename the temp table to not include "tab".

About this last issue, are you complaining about the fact you could have open tabs that would not appear in the autocomplete? Since if they appear in autocomplete then they have for sure an entry in the Places table. If instead you want urls open in tabs regardless the fact they are in history or bookmarks or whatever, then what you want is a completely different query, and sure will have to dupe dummy informations (url and title) using them only if the official entry is gone. At that point, you could also add urls that are usually not addable to history (like about: urls open in tabs).
and your query would end up being a select from your temp table, with a left join with places tables, and a bunch of COALESCE to select url and title from your temp table as last option. i'm just guessing what frecency (weight) value will you give to entries that are open in tabs but have no history nor bookmarks.
Thinking about this some more, I wonder if it wouldn't be be simpler to avoid involving the places database at all for this, and just keep the list of loaded URLs in memory in some place that's easily accessible to the places autocomplete code, and have it just do filtering after the DB query as needed? Maybe that's crazy, but I thought I'd throw it out there.
(In reply to comment #90)
> Thinking about this some more, I wonder if it wouldn't be be simpler to avoid
> involving the places database at all for this, and just keep the list of loaded
> URLs in memory in some place that's easily accessible to the places
> autocomplete code, and have it just do filtering after the DB query as needed?
> Maybe that's crazy, but I thought I'd throw it out there.
That sounds far more ideal to me, but then you'd have to re-implement the matching logic, right?  So, maybe not.
> keep the list of loaded URLs in memory in some place

that is after all what a temp table does, and what he is doing :)
(In reply to comment #87)
> Isn't this information only kept in memory? I thought that's what the _temp
> implied

Yes, but it depends on having a corresponding row in main places tables (moz_places, moz_places_temp). The only data stored in moz_opentabs_temp is the number of open tabs - everything else (url, title, etc etc) is still in the other tables. So if the row in moz_places is deleted (via clearing history), then moz_opentabs_temp points to a row that doesn't exist (thus, I'm deleting the row from moz_opentabs_temp whenever its corresponding row is deleted from moz_places).

(In reply to comment #88)
> About this last issue, are you complaining about the fact you could have open
> tabs that would not appear in the autocomplete? Since if they appear in
> autocomplete then they have for sure an entry in the Places table. If instead
> you want urls open in tabs regardless the fact they are in history or bookmarks
> or whatever, then what you want is a completely different query, and sure will
> have to dupe dummy informations (url and title) using them only if the official
> entry is gone. At that point, you could also add urls that are usually not
> addable to history (like about: urls open in tabs).

(In reply to comment #89)
> and your query would end up being a select from your temp table, with a left
> join with places tables, and a bunch of COALESCE to select url and title from
> your temp table as last option. i'm just guessing what frecency (weight) value
> will you give to entries that are open in tabs but have no history nor
> bookmarks.

Yes, exactly. Fixing it means messy (messier?) queries and probably a performance hit. Another way would be to move any deleted rows we want to keep into moz_places_temp, and have an additional column to mark that these rows have been "deleted" and should never be moved to moz_places (with columns like visit_count reset). That's still pretty messy though :\

Anyway, since the current behavior is acceptable (albeit not not perfect), I'll investigate deeper in a followup bug.
(In reply to comment #91)
> That sounds far more ideal to me, but then you'd have to re-implement the
> matching logic, right?  So, maybe not.

Having it managed outside of places was the first strategy I looked at, and dismissed. It means (as Shawn pointed out) that a lot of things have to be reimplemented that places gives you for free. And it wouldn't be possible to change the ranking weight of tab matches (Limi wants me to look at doing that in a followup bug). FWIW, I do think that this belongs together with the rest of the places data, regardless of whether it works best implementation-wise.
(In reply to comment #81)
> >+        aWindow.gBrowser.selectedBrowser.focus();
> 
> Is this necessary?

Yes, I specifically want the content focused (not the urlbar), since that's what the user asked for.

> 
> >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> 
> >         <textbox id="urlbar" flex="1"
> 
> >-                 oninput="gBrowser.userTypedValue = this.value;"
> >+                 oninput="gBrowser.userTypedValue = this.value; this.resetActionType();"
> 
> Why is this needed here?

Possibly not the best location, but when the value changes it likely no longer a valid tab to switch to. And if the user is changing the value, they're probably not wanting to switch to that tab anyway.


> >+      <field name="mBrowserHistory" readonly="true">
> 
> "readonly" doesn't do anything on fields, so remove this (and other instances
> in this patch). I guess the existing fields have this problem too, wouldn't
> hurt to clean them up.

Ah, indeed. Filed bug 542406 to remove the rest (although, see bug 542406  comment 1).


> The tabbrowser being responsible of tracking "loaded page" makes me nervous,
> for some reason. My first reaction was that this should be handled at a lower
> level, but I suppose that could also run into issues (e.g. things being loaded
> in a non-user visible way being marked as loaded). I suppose this is probably
> the best way forward, but I'll have to give this a bit more thought.

Yea, I'd like it lower level, but then you get cases of browsers that aren't visible, aren't designed to be the main content, etc etc. I guess one way would be to add an additional load flag, to make it opt-in.


> >+  <binding id="urlbar-rich-result-box" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistbox">
> 
> Why is this binding needed? Don't see it doing anything but setting the
> "beforecurrent" attribute, which doesn't appear to be used...

Ah, good catch - no longer needed now that the tab match result and normal result are displayed as separate results (previously, they were displayed as one combination result, but that changed).


> >diff --git a/browser/themes/gnomestripe/browser/browser.css b/browser/themes/gnomestripe/browser/browser.css
> 
> >+#urlbar:-moz-locale-dir(rtl) .urlbar-display-container {
> >+  direction: rtl;
> >+}
> >+
> >+#urlbar html|*.autocomplete-textbox {
> >+  direction: ltr;
> >+}
> 
> >+richlistitem[type="action"]:-moz-locale-dir(rtl) .ac-url-box {
> >+  direction: rtl;
> >+}
> 
> If these are needed, shouldn't they live in content/ rather than the theme?

They're alongside the existing RTL overrides. For whatever reason, they're all in the themes, not in content/.


> >diff --git a/toolkit/components/places/src/nsPlacesAutoComplete.js b/toolkit/components/places/src/nsPlacesAutoComplete.js
> 
> >-const kQueryIndexTyped = 7;
> >-const kQueryIndexPlaceId = 8;
> >-const kQueryIndexQueryType = 9;
> >+const kQueryIndexTabOpenCount = 7;
> >+const kQueryIndexTyped = 8;
> >+const kQueryIndexPlaceId = 9;
> >+const kQueryIndexQueryType = 10;
> 
> Why change these, OOC?

I guess you could call it that. I added the additional column next to similar columns for it to appear less messy (and so the indexes needed updated accordingly). But there's no technical reason why the columns needs to be in any specific order, as long as the indexes match up.
(In reply to comment #82)
> this suggestion is awesome, something a-la registerVisiblePage, even if this
> name sucks a bit, registerPageInVisibleContent?. uh, naming is hard.

Hmm, yes. Although "visible" implies you can see them all at once. registerOpenPage?
(In reply to comment #95)
> (In reply to comment #81)
> > >+        aWindow.gBrowser.selectedBrowser.focus();
> > 
> > Is this necessary?
> 
> Yes, I specifically want the content focused (not the urlbar), since that's
> what the user asked for.

The line before this (aWindow.gBrowser.tabContainer.selectedIndex = bIndex) implies this already, I think, by calling updateCurrentBrowser in tabbrowser.xml.

> > >+#urlbar:-moz-locale-dir(rtl) .urlbar-display-container {

Any reason why this isn't just .urlbar-display-container:-moz-locale-dir(rtl)?

> > >+  direction: rtl;
> > >+}
> > >+
> > >+#urlbar html|*.autocomplete-textbox {
> > >+  direction: ltr;
> > >+}
> > 
> > >+richlistitem[type="action"]:-moz-locale-dir(rtl) .ac-url-box {

Can you use a more efficient selector here (and elsewhere), e.g. by using the child selector or by letting ac-url-box inherit the action attribute?

> > >+  direction: rtl;
> > >+}
> > 
> > If these are needed, shouldn't they live in content/ rather than the theme?
> 
> They're alongside the existing RTL overrides. For whatever reason, they're all
> in the themes, not in content/.

The ones you added belong in content/ indeed, as there's no reason why a theme should decide to do this differently, as far as I can see. It's possible that some of the existing stuff belongs in content/, too.
(In reply to comment #96)
> (In reply to comment #82)
> > name sucks a bit, registerPageInVisibleContent?. uh, naming is hard.
> 
> Hmm, yes. Although "visible" implies you can see them all at once.
> registerOpenPage?

non visible pages are still open, registerOpenInContentPage?

I have some concern about data integrity if you flag rows in moz_places_temp (just some fear that we could end up with bookmarks orphan of their page if we don't sync down correctly, or queries finding deleted pages).

Btw this is how it could work:
- the INSTEAD OF DELETE trigger on moz_places_view should not delete temp pages if they have a corresponding open page in your temp table. it should instead flag them as zombies. zombie pages are pages no more existent in history or bookmarks, but open.
- nsPlacesDBFLush should skip zombie pages when syncing.
- the INSTEAD OF INSERT trigger on moz_places_view should remove a zombie entry before trying to insert a new entry.
- your delete trigger should remove zombie entries when open count goes to 0.

This was the easy part! Now we have these problems:
- we need to fix all queries that delete from moz_places_temp directly to skip zombie entries. You're lucky, mxr tells me we don't have any at first glance, and we have already fixed the view trigger.
- we need to fix all queries that select from moz_places_temp directly to ignore those entries. This is a lot of work, mxr tells me that's about 70 queries. It's also adding a condition that in most cases won't be useful. that's a perf hit (we could survive it though).
- we would like one day to kill temp table, then this would need to be reimplemented.

So far, the only possible way looks like duplicating url and title in your temp table, and adding a new query using coalesce. should not be a big perf hit, i'm only concerned by the frecency value you'll give to those entries that are not in history nor bookmarks (even if those cases are edge cases, happening when you use the locationbar just after a clear history, so should not be a big concern)
Attached patch Patch v2 (obsolete) — — Splinter Review 

        Attachment #422506 -
        Attachment is obsolete: true

        Attachment #424533 -
        Flags: review?(gavin.sharp)

    
    

    
  
Comment on attachment 424533 [details] [diff] [review]
Patch v2

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

>+#urlbar:-moz-locale-dir(rtl) > .autocomplete-textbox-container > .textbox-input-box {
>+  direction: rtl;
>+}

>+#urlbar html|*.autocomplete-textbox {

Can you explain what these selectors are for (maybe add a comment)?

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+function switchToTabHavingURI(aURI) {
>+  function switchIfURIInWindow(aWindow) {

>+        aWindow.gBrowser.selectedBrowser.focus();

Is part 1 of comment 97 wrong (i.e. does removing this line actually cause problems)?

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>+          <vbox id="urlbar-display"/>

Why not just make this a label with the value hardcoded, and hide/show it using CSS, and skip all the XBL (i.e. get rid of urlbar-display-switchtab)?

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>+              
>+              this.mBrowser.lastURI = aLocation.clone();
>+

Seems like we should be clearing this in the <browser> destructor (or in its "destroy" method, though in that case it probably makes sense to give <browser>s an explicit field...)

Why did you add the clone()? Adding overhead to onLocationChange doesn't seem ideal, and things seemed to be working fine before when we kept a reference to the actual URI, so it seems like we should just leave that as-is?

(nit: remove unnecessary trailing whitespace on these lines)

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>+      <field name="_value"></field>
>+      <property name="value">

I think we should probably add an extension mechanism to the base binding to avoid the code duplication here. Similar to _openAutocompletePopup, but less hacky - something like:

if (typeof this.onBeforeValueSet == "function")
  val = this.onBeforeValueSet(val);

in the base setter?

>        <method name="handleCommand">

>+          if (action) {
>+            if (action.type == "switchtab")
>+              switchToTabHavingURI(action.param);
>+            else
>+              Components.utils.reportError("Unknown action: " + action.type);

Is there a good reason to warn here? Seems like we should just ignore unknown actions (same comment applies to onPopupClick).

>+      <method name="resetActionType">

Shouldn't this be handled by the base binding, given that that's where actiontype is set?

>diff --git a/browser/themes/gnomestripe/browser/browser.css b/browser/themes/gnomestripe/browser/browser.css

>+richlistitem[type="action"]:-moz-locale-dir(rtl) > .ac-url-box {
>+  direction: rtl;
>+}

I think this belongs in the content/ browser.css (applies to the three theme files)?

>diff --git a/toolkit/components/places/public/nsIBrowserHistory.idl b/toolkit/components/places/public/nsIBrowserHistory.idl

>+    /**
>+     * registerOpenPage
>+     *
>+     * Mark a page as being currently open.
>+     */
>+    void registerOpenPage(in nsIURI aURI);
>+    
>+    /**
>+     * unregisterOpenPage
>+     *
>+     * Mark a page as no longer being open (either by closing or
>+     * by navigating away from that page).
>+     */
>+    void unregisterOpenPage(in nsIURI aURI);

Need an IID rev for this.

>diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml

>   <binding id="autocomplete-richlistitem" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">

>-      <xul:hbox align="center">
>+      <xul:hbox align="center" class="ac-title-box">

Unused, looks like? Guess it doesn't really hurt to add.

>       <method name="_adjustAcItem">

>+            let desc = this.parentNode
>+                           .mActionsStringBundle
>+                           .GetStringFromName(action + ".result.label");

Seems like a DTD-loaded string would be better here too, to avoid the need for the stringbundle.

I'll r+ a patch that addresses these comments. I didn't review the places parts closely, but Marco's got that covered, per IRC convo (you should flag him specifically on the next rev). You need mconnor to sr the interface changes, I guess, but they look fine to me.

        Attachment #424533 -
        Flags: review?(gavin.sharp) → review-

    
    

    
  
Comment on attachment 424533 [details] [diff] [review]
Patch v2

diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

+function switchToTabHavingURI(aURI) {

+  // This can be passed either nsIURI or a string.
+  if (!(aURI instanceof Ci.nsIURI))
+    aURI = makeURI(aURI);

If we allow to pass in something that is not an nsIURI, we lose the is-a-valid-uri effect in the method call.
I think you should throw if somebody passes in a non nsIURI, OR try/catch here, in case the passed in value is a bad url (or not an url at all)

+  let windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"]
+                         .getService(Ci.nsIWindowMediator);

nit: i think Gavin's  patch for common services getters is about to land (bug 512784).
could make sense add it to your queue and use Services.wm since i think this patch will land later than that?

diff --git a/browser/base/content/test/browser_tabMatchesInAwesomebar.js b/browser/base/content/test/browser_tabMatchesInAwesomebar.js

+  function() {
+    gPrivateBrowsing.privateBrowsingEnabled = true;
+
+    executeSoon(function() {
+      ensure_opentabs_match_db();
+      nextStep();
+    });
+  },
+  function() {
+    gPrivateBrowsing.privateBrowsingEnabled = false;
+    setTimeout(function() {
+      ensure_opentabs_match_db();
+      nextStep();
+    }, 100);
+  }

why executeSoon in enabling and timeout in disabling? the timeout looks fragile

+  var db = gNavHistoryService.QueryInterface(Ci.nsPIPlacesDatabase)
+                             .DBConnection;

nit: just fyi, since this is a browser chrome test, you could use PlacesUtils.history for history svc.

if you're adding uris to history could make sense to call a removeAllPages() before finishing. browser-chrome tests don't create a new profile for each test.

diff --git a/browser/themes/winstripe/browser/actionicon-tab.png b/browser/themes/winstripe/browser/actionicon-tab.png

will we add an actionicon-tab-aero.png in future?

diff --git a/toolkit/components/places/public/mozIPlacesAutoComplete.idl b/toolkit/components/places/public/mozIPlacesAutoComplete.idl
+
+  /**
+   * Search for open pages.
+   */
+  const long BEHAVIOR_OPENPAGE = 1 << 7;

please specify what "open" means and how to register an open page (and add a @see nsIBrowserHistory.idl)

diff --git a/toolkit/components/places/public/nsIBrowserHistory.idl b/toolkit/components/places/public/nsIBrowserHistory.idl

As Gavin said, uuid rev. here

+    /**
+     * registerOpenPage
+     *
+     * Mark a page as being currently open.
+     */
+    void registerOpenPage(in nsIURI aURI);
+
+    /**
+     * unregisterOpenPage
+     *
+     * Mark a page as no longer being open (either by closing or
+     * by navigating away from that page).
+     */
+    void unregisterOpenPage(in nsIURI aURI);

i'd drop the repeated names of the method in the javadoc (yes, even if it's done above in this file). that does not bring anything useful.

diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

+NS_IMETHODIMP
+nsNavHistory::RegisterOpenPage(nsIURI* aURI)
+{
+  NS_ASSERTION(NS_IsMainThread(), "This can only be called on the main thread");
+  NS_ENSURE_ARG(aURI);
+
+  // CanAddURI handles Private Browsing mode for us
+  PRBool canAdd = PR_FALSE;
+  nsresult rv = CanAddURI(aURI, &canAdd);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  PRInt64 placeId;
+  rv = GetUrlIdFor(aURI, &placeId, canAdd);

add a comment above this about the fact that if the uri has never been added to history but is addable, due to LAZY_ADD this call
will add an orphan page to the database till the visit is added.

Hm, we should make this thing async somehow, either here or in a follow-up, since it happens on any toplevel visit.
we have a sync query here to convert an URI to a place id (going against our reduce main-thread I/O goal)
The write is not a problem since it happens on a temp table.
On the other side making this async will convert your test into an hell.
I don't see a clean solution with async.
So, since in future you could want to replicate uri and title here, what about making uri your primary key,
so here you just insert (uri, count) regardless canAdd and history and join moz_places on uri.

i don't have perf data on the differences between joining on an id or on a string.
memory should not be an issue, i doubt a user will have thousands of open tabs.

clearly this means opening a tab will add your element here but won't add a place entry (Will be added eventually later on lazy addVisit).
This means the open tab won't appear immediately in the locationbar if the page was unknown to history till the visit is added (3s later) till we implement the full plan
(replicating data and allowing to register any page). This won't be a big deal for your test, just call addVisit at the start for your urls!

All of this "could" be done in that followup if we plan to do that _immediately_ once this is done.
Otherwise if that's "future", i think we should at least try the uri join here, and let the rest for the followup.

+nsNavHistory::UnregisterOpenPage(nsIURI* aURI)
+{
+  NS_ASSERTION(NS_IsMainThread(), "This can only be called on the main thread");
+  NS_ENSURE_ARG(aURI);
+
+  // If we're in Private Browsing mode, there shouldn't be anything in the
+  // moz_openpages_temp table - so we can stop now without doing any
+  // unnecessary work.
+  if (InPrivateBrowsingMode())
+    return NS_OK;
+
+  PRInt64 placeId;
+  nsresult rv = GetUrlIdFor(aURI, &placeId, PR_FALSE);

and this is even worst, since when i visit a new page, we first query the db for the old uri->place_id and immediately later we query for newuri->place_id
So we have 2 queries for each toplevel visit :(

I can survive this for Minefield, but i would not want it in a release.

diff --git a/toolkit/components/places/src/nsPlacesAutoComplete.js b/toolkit/components/places/src/nsPlacesAutoComplete.js

-const kQueryIndexTyped = 7;
-const kQueryIndexPlaceId = 8;
-const kQueryIndexQueryType = 9;
+const kQueryIndexOpenPageCount = 7;
+const kQueryIndexTyped = 8;
+const kQueryIndexPlaceId = 9;
+const kQueryIndexQueryType = 10;

why changing all indices instead of just enqueuing yours with value 10?
looks like unnecessary blame change

   function sql_base_fragment(aTableName) {
     return "SELECT h.url, h.title, f.url, " + kBookTagSQLFragment + ", " +
-                  "h.visit_count, h.typed, h.id, :query_type, h.frecency " +
+                  "h.visit_count, t.open_count, h.typed, h.id, :query_type, " +
+                  "h.frecency " +

ditto, you changed 2 lines when you could have just added one :)

            "FROM " + aTableName + " h " +
            "LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id " +
+           "LEFT OUTER JOIN moz_openpages_temp t ON t.place_id = h.id " +

if you implement uri joining won't be hard to change this.

won't repeat comments for the next queries, since depending on these changes.

@@ -381,16 +397,19 @@ nsPlacesAutoComplete.prototype = {
     //       consistent.  See bug 412730 for more details.

     // We want to store the original string with no leading or trailing
     // whitespace for case sensitive searches.
     this._originalSearchString = aSearchString.trim();

     this._currentSearchString =
       this._fixupSearchText(this._originalSearchString.toLowerCase());
+
+    var searchParamParts = aSearchParam.split(" ");
+    this._enableActions = searchParamParts.indexOf("enable-actions") != -1;

is this faster then searching "enable-actions" in the string? do we know where "enable-actions" is (start? end?)? a substr could be faster in such a case

@@ -921,18 +950,31 @@ nsPlacesAutoComplete.prototype = {
 
+    // And finally add this to our results,
+    // but only if not restricting to only open-page matches.
+    if (!this._onlyHasBehavior("openpage"))
+      this._addToResults(entryId, escapedEntryURL, title, entryFavicon, style);

in comments, reasoning is better then anticipating code with words!

@@ -981,19 +1023,35 @@ nsPlacesAutoComplete.prototype = {
    * Determines if the specified AutoComplete behavior is set.
    *
    * @param aType
    *        The behavior type to test for.
    * @return true if the behavior is set, false otherwise.
    */
   _hasBehavior: function PAC_hasBehavior(aType)
   {
+    if (aType == "everything")
+      return this._behavior == 0;
     return (this._behavior &
             Ci.mozIPlacesAutoComplete["BEHAVIOR_" + aType.toUpperCase()]);
   },

Where is this "everything" set up? i see only comparisons on it but i don't see its definition.

diff --git a/toolkit/components/places/tests/autocomplete/test_tabmatches.js b/toolkit/components/places/tests/autocomplete/test_tabmatches.js
+function addOpenPages(aUri, aCount) {
+  let num = aCount || 1;
+  for (let i = 0; i < num; i++)
+    bhist.registerOpenPage(iosvc.newURI(kURIs[aUri], null, null));
+}
+
+function removeOpenPages(aUri, aCount) {
+  let num = aCount || 1;
+  for (let i = 0; i < num; i++)
+    bhist.unregisterOpenPage(iosvc.newURI(kURIs[aUri], null, null));

you should be able to use uri(kURIs[aUri]) instead of iosvc.newURI, our tests have that helper in head (usually).

        Attachment #424533 -
        Flags: review-

    
    

    
  
(In reply to comment #101)
> +  if (!(aURI instanceof Ci.nsIURI))
> +    aURI = makeURI(aURI);
> I think you should throw if somebody passes in a non nsIURI, OR try/catch here,
> in case the passed in value is a bad url (or not an url at all)

er... this throws already so the question is more about if we want just to throw or catch silently and return.

    
    

    
  
No, this should throw. The caller should catch it in case it's expected that it would pass in invalid URIs.

    
    

    
  
please, also file the follow-up bug about the "not showing items that are not in history" problem.

    
    

    
  
(In reply to comment #100)
> Is part 1 of comment 97 wrong (i.e. does removing this line actually cause
> problems)?

Oops, I missed comment 97 - it is indeed correct. Adjusted accordingly.


> Why not just make this a label with the value hardcoded, and hide/show it using
> CSS, and skip all the XBL (i.e. get rid of urlbar-display-switchtab)?

Discussed this further on IRC. Done.

 
> Seems like we should be clearing this in the <browser> destructor (or in its
> "destroy" method, though in that case it probably makes sense to give
> <browser>s an explicit field...)

Done.


> Why did you add the clone()? Adding overhead to onLocationChange doesn't seem
> ideal, and things seemed to be working fine before when we kept a reference to
> the actual URI, so it seems like we should just leave that as-is?

I can't remember the reasoning, so I assume it was bad reasoning :) Fixed.


> I think we should probably add an extension mechanism to the base binding to
> avoid the code duplication here. Similar to _openAutocompletePopup, but less
> hacky - something like:

Done - works quite well. The function names are onBeforeValueSet and onBeforeValueGet.


> Is there a good reason to warn here? Seems like we should just ignore unknown
> actions (same comment applies to onPopupClick).

I guess not. Removed.


> >+      <xul:hbox align="center" class="ac-title-box">
> 
> Unused, looks like? Guess it doesn't really hurt to add.

Ah, yes - leftover from when results were mashed together. Good to have anyway, I think.


> >       <method name="_adjustAcItem">
> 
> >+            let desc = this.parentNode
> >+                           .mActionsStringBundle
> >+                           .GetStringFromName(action + ".result.label");
> 
> Seems like a DTD-loaded string would be better here too, to avoid the need for
> the stringbundle.

As much as I'd like to avoid a stringbundle, there's no non-crazy way to use a DTD string here.

    
    

    
  
(In reply to comment #101)
> +function switchToTabHavingURI(aURI) {
>
> +  // This can be passed either nsIURI or a string.
> +  if (!(aURI instanceof Ci.nsIURI))
> +    aURI = makeURI(aURI);
> 
> If we allow to pass in something that is not an nsIURI, we lose the
> is-a-valid-uri effect in the method call.
> I think you should throw if somebody passes in a non nsIURI, OR try/catch here,
> in case the passed in value is a bad url (or not an url at all)

As per comment 102 and comment 103, makeURI will throw, and the caller of switchToTabHavingURI() should be the one to catch it.


> +  let windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"]
> +                         .getService(Ci.nsIWindowMediator);
> 
> nit: i think Gavin's  patch for common services getters is about to land (bug
> 512784).
> could make sense add it to your queue and use Services.wm since i think this
> patch will land later than that?

I'd rather not add an unnecessary dependancy - will fix it up once bug 512784 lands.

> +  function() {
> +    gPrivateBrowsing.privateBrowsingEnabled = false;
> +    setTimeout(function() {
> +      ensure_opentabs_match_db();
> +      nextStep();
> +    }, 100);
> +  }
> 
> why executeSoon in enabling and timeout in disabling? the timeout looks fragile

See comment 70. Alternative methods welcome!

 
> Hm, we should make this thing async somehow, either here or in a follow-up,
> since it happens on any toplevel visit.

After discussions on IRC, I'll do this in a followup. And sdwilsh will break my thumbs if I don't.


> +    var searchParamParts = aSearchParam.split(" ");
> +    this._enableActions = searchParamParts.indexOf("enable-actions") != -1;
> 
> is this faster then searching "enable-actions" in the string? do we know where
> "enable-actions" is (start? end?)? a substr could be faster in such a case

I'm using this as a space-seperated list of flags (so we don't get subtle breakage later on when it's used for something else), so it could be anywhere.


>    _hasBehavior: function PAC_hasBehavior(aType)
>    {
> +    if (aType == "everything")
> +      return this._behavior == 0;
>      return (this._behavior &
>              Ci.mozIPlacesAutoComplete["BEHAVIOR_" + aType.toUpperCase()]);
>    },
> 
> Where is this "everything" set up? i see only comparisons on it but i don't see
> its definition.

It's an implicit definition - 0 already acts as a catch-all (ie, it's not restricted to any specific behavior), and "everything" is just testing for that. Added a comment to clarify that.

> you should be able to use uri(kURIs[aUri]) instead of iosvc.newURI, our tests
> have that helper in head (usually).

Not for the autocomplete tests, but there is toURI() instead.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v3 (obsolete)
        — 
          — Splinter Review
      

    


  
Fixes above comments. Also corrects mixed line-endings in urlbarBindings.xml (inadvertantly added in bug 480443).

        Attachment #424533 -
        Attachment is obsolete: true

        Attachment #426950 -
        Flags: superreview?(mconnor)

        Attachment #426950 -
        Flags: review?(gavin.sharp)

        Attachment #426950 -
        Flags: review?(mak77)
Blocks: 546253
Blocks: 546254
Blocks: 546255

    
    

    
  
(In reply to comment #105)
> As much as I'd like to avoid a stringbundle, there's no non-crazy way to use a
> DTD string here.

You can put the entity directly in the XBL XML (either in a field or just in-line as in http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings.xml#266 ), which doesn't seem so crazy to me :)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v3.1 (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to comment #108)
> You can put the entity directly in the XBL XML (either in a field or just
> in-line as in
> http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings.xml#266 ),
> which doesn't seem so crazy to me :)

Oh! Hadn't concidered that - thanks!

        Attachment #426950 -
        Attachment is obsolete: true

        Attachment #427025 -
        Flags: superreview?(mconnor)

        Attachment #427025 -
        Flags: review?(gavin.sharp)

        Attachment #426950 -
        Flags: superreview?(mconnor)

        Attachment #426950 -
        Flags: review?(mak77)

        Attachment #426950 -
        Flags: review?(gavin.sharp)

        Attachment #427025 -
        Flags: review?(mak77)

    
    

    
  
Comment on attachment 427025 [details] [diff] [review]
Patch v3.1

>diff --git a/toolkit/components/places/public/mozIPlacesAutoComplete.idl b/toolkit/components/places/public/mozIPlacesAutoComplete.idl
>+  
>+  /**

trailing spaces in the first line

>+   * Search for pages that have been marked as being opened, such as a tab
>+   * in a tabbrowser, via:
>+   *  nsIBrowserHistory.registerOpenPage(url)

nit: fancy indenting, either inline on the previous line or indent by 2 spaces

>diff --git a/toolkit/components/places/public/nsIBrowserHistory.idl b/toolkit/components/places/public/nsIBrowserHistory.idl

>+    
>+    /**

trailing spaces on first line

>+     * Mark a page as being currently open.
>+     */
>+    void registerOpenPage(in nsIURI aURI);
>+    
>+    /**

ditto

>+     * Mark a page as no longer being open (either by closing or

nit: "by closing" what?

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

>+NS_IMETHODIMP
>+nsNavHistory::RegisterOpenPage(nsIURI* aURI)
>+{
>+  NS_ASSERTION(NS_IsMainThread(), "This can only be called on the main thread");
>+  NS_ENSURE_ARG(aURI);
>+  
>+  // CanAddURI handles Private Browsing mode for us

nit: end comments with period

>+}
>+

2 newlines between methods please

>+NS_IMETHODIMP
>+nsNavHistory::UnregisterOpenPage(nsIURI* aURI)
>+{
>+  NS_ASSERTION(NS_IsMainThread(), "This can only be called on the main thread");
>+  NS_ENSURE_ARG(aURI);
>+
>+  // If we're in Private Browsing mode, there shouldn't be anything in the
>+  // moz_openpages_temp table - so we can stop now without doing any
>+  // unnecessary work.
>+  if (InPrivateBrowsingMode())
>+    return NS_OK;

Where are you removing pages that were added BEFORE pb mode was enabled?
I think this is probably wrong, you still want to unregister pages, since they could have been registered before entering PB.

>diff --git a/toolkit/components/places/src/nsPlacesAutoComplete.js b/toolkit/components/places/src/nsPlacesAutoComplete.js

>+    
>+    var searchParamParts = aSearchParam.split(" ");
>+    this._enableActions = searchParamParts.indexOf("enable-actions") != -1;

trailing spaces on the empty line. please check the full patch since looks like there are some.

>+    // Add a special entry for an open-page match.
>+    if ((this._hasBehavior("openpage") || this._hasBehavior("everything")) &&
>+        openPageCount > 0)
>+      this._addToResults(entryId, "moz-action:switchtab," + escapedEntryURL, title, entryFavicon, "action");
>+
>+    // If restricting to only open-page matches, there should only be the
>+    // switch-to-tab results.
>+    if (!this._onlyHasBehavior("openpage"))
>+      this._addToResults(entryId, escapedEntryURL, title, entryFavicon, style);
>     return true;

nit: newline before the return

r=me with the PB thing fixed, and followup bug 546255 being your nightmare till you fix it :)

        Attachment #427025 -
        Flags: review?(mak77) → review+

    
    

    
  
(In reply to comment #110)
> >+  // If we're in Private Browsing mode, there shouldn't be anything in the
> >+  // moz_openpages_temp table - so we can stop now without doing any
> >+  // unnecessary work.
> >+  if (InPrivateBrowsingMode())
> >+    return NS_OK;
> 
> Where are you removing pages that were added BEFORE pb mode was enabled?
> I think this is probably wrong, you still want to unregister pages, since they
> could have been registered before entering PB.

er, rethinking to this, if the pb notification here is fired after all tabs are closed by entering pb mode, it is correct.
Btw, the comment needs to be better, it should say something like "Entering PB mode will unregister all open pages, so that now there is nothing in the table."
Btw, this is relying on implementer to know that he should unregister all pages when entering pb mode (that is true for Firefox, but what for other implementers?). If we are willing to rely on that assumption maybe a comment in the idl could be worth it. otherwise we could remove all remaining table contents when we get the pb notification.

    
    

    
  
Comment on attachment 427025 [details] [diff] [review]
Patch v3.1

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

>+richlistitem[type="action"]:-moz-locale-dir(rtl) > .ac-url-box {
>+  direction: rtl;
>+}

This is new in this patch - why is it needed? Does it belong above with the other RTL-related rules?

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>         <textbox id="urlbar" flex="1"

>-                 oninput="gBrowser.userTypedValue = this.value;"
>+                 oninput="gBrowser.userTypedValue = this.value; this.resetActionType();"

To expand further on my previous comment, I think this too belongs in the base binding's input handler.

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>+<bindings id="urlbarBindings"
>+          xmlns="http://www.mozilla.org/xbl"
>+          xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

"xul" no longer appears to be used, so no need to add it

>+      <method name="onBeforeValueGet">

>+      <method name="onBeforeValueSet">

Add a comment before these two, mentioning that they're called by the base binding?

>diff --git a/browser/themes/winstripe/browser/actionicon-tab-aero.png b/browser/themes/winstripe/browser/actionicon-tab-aero.png

>diff --git a/browser/themes/winstripe/browser/actionicon-tab.png b/browser/themes/winstripe/browser/actionicon-tab.png

No need to add these both if they're identical, unless you expect to update the -aero in short order - just add it once and refer to the same images from both locations in the manifest.

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

>+nsNavHistory::UnregisterOpenPage(nsIURI* aURI)

>+  // If we're in Private Browsing mode, there shouldn't be anything in the
>+  // moz_openpages_temp table - so we can stop now without doing any
>+  // unnecessary work.

This doesn't seem to be true - if there's an entry in history, registerOpenPage will still add entries to moz_openpages_temp. Which means that already-visited pages viewed in PB mode will never be removed from the table. That indeed appears to be the case in testing - I ended up with "switch to tab" entries for closed tabs in PB mode. I think you just need to remove this early return to fix it? Should also adjust the test to catch this bug too.

>diff --git a/toolkit/components/places/src/nsPlacesAutoComplete.js b/toolkit/components/places/src/nsPlacesAutoComplete.js

>+  XPCOMUtils.defineLazyServiceGetter(this, "_windowMediator",
>+                                     "@mozilla.org/appshell/window-mediator;1",
>+                                     "nsIWindowMediator")

Looks to be unused... not sure why the autocomplete code would need a window mediator anyways :)

>diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml

>+      <property name="value">

>         <setter><![CDATA[
>           this.mIgnoreInput = true;
>+          if (typeof this.onBeforeValueSet == "function")
>+            val = this.onBeforeValueSet(val);

nit: add newlines around this?

r=me with those fixed.

        Attachment #427025 -
        Flags: review?(gavin.sharp) → review+

    
    

    
  
(In reply to comment #111)
> > Where are you removing pages that were added BEFORE pb mode was enabled?
> > I think this is probably wrong, you still want to unregister pages, since they
> > could have been registered before entering PB.
> 
> er, rethinking to this, if the pb notification here is fired after all tabs are
> closed by entering pb mode, it is correct.

Yes, correct.


> Btw, the comment needs to be better

Agreed - fixed comments here and in the .idl.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v4 (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to comment #112)
> >+richlistitem[type="action"]:-moz-locale-dir(rtl) > .ac-url-box {
> >+  direction: rtl;
> >+}
> 
> This is new in this patch - why is it needed? Does it belong above with the
> other RTL-related rules?

New in that file, not new in the patches (it was originally in theme-specific files). RTL locales need it so the "Switch to tab" label in autocomplete results is to the right (this label is in the same element as URLs normally are, which are always LTR). I've moved it to be with the other RTL rules, and added a comment to explain this.


> >+  // If we're in Private Browsing mode, there shouldn't be anything in the
> >+  // moz_openpages_temp table - so we can stop now without doing any
> >+  // unnecessary work.
> 
> This doesn't seem to be true - if there's an entry in history, registerOpenPage
> will still add entries to moz_openpages_temp. Which means that already-visited
> pages viewed in PB mode will never be removed from the table. That indeed
> appears to be the case in testing - I ended up with "switch to tab" entries for
> closed tabs in PB mode. I think you just need to remove this early return to
> fix it? Should also adjust the test to catch this bug too.

I'd forgotten that this was the reason why I had been explicitly checking Private Browsing mode in RegisterOpenPage(), rather than relying on CanAddURI(). Before I removed that explicit check, pages were never added to moz_openpages_temp when in Private Browsing - both to avoid leaking information about other windows (that might otherwise stay hidden and private), and to avoid the case where only previously visited pages showed up as tab-matches (this case would be fixed in bug 546253). Re-added this check.

        Attachment #427025 -
        Attachment is obsolete: true

        Attachment #428133 -
        Flags: superreview?(mconnor)

        Attachment #427025 -
        Flags: superreview?(mconnor)

    
    

    
  
(In reply to comment #114)
> Created an attachment (id=428133) [details]
> Patch v4
> 
> (In reply to comment #112)
> > >+richlistitem[type="action"]:-moz-locale-dir(rtl) > .ac-url-box {
> > >+  direction: rtl;
> > >+}
> > 
> > This is new in this patch - why is it needed? Does it belong above with the
> > other RTL-related rules?
> 
> New in that file, not new in the patches (it was originally in theme-specific
> files). RTL locales need it so the "Switch to tab" label in autocomplete
> results is to the right (this label is in the same element as URLs normally
> are, which are always LTR). I've moved it to be with the other RTL rules, and
> added a comment to explain this.

Oh, I also didn't get this change, but now that you explained this, it makes a lot of sense, since we don't want themes to inadvertently change the directionality of that box by dropping this rule from their themes by mistake.

    
    

    
  
Comment on attachment 428133 [details] [diff] [review]
Patch v4

sr=me!

        Attachment #428133 -
        Flags: superreview?(mconnor) → superreview+

    
    

    
  
can we land this?
better getting early feedback and followup on issues found, rather than having the bug sleeping.

    
    

    
  
(In reply to comment #117)
> can we land this?
> better getting early feedback and followup on issues found, rather than having
> the bug sleeping.

This is ready to land, Blair was just waiting for a calmer tree, since it was lit up like a christmas tree for much of yesterday. I sort of suspect that he wouldn't mind if you decided to land it for him, in the interim, otherwise I expect he'll get to it today.

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/c7562242f88e
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 14 years ago
Flags: in-testsuite+
Flags: in-litmus?
Resolution: --- → FIXED

    
  
Depends on: 551602

    
  
No longer depends on: 551602

    
  
Depends on: 551602

    
    

    
  
Had to back this out, due to additional bustage - which I assume is merge-related, but don't know yet.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268269120.1268270790.27445.gz&fulltext=1

http://hg.mozilla.org/mozilla-central/rev/673aeae9f68a
http://hg.mozilla.org/mozilla-central/rev/ff00eb889811
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
hm, was this on all tinderboxes? it's a strange error for your changes, doesn't look related at all :\

    
    

    
  
(In reply to comment #106)
> > +  let windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"]
> > +                         .getService(Ci.nsIWindowMediator);
> > 
> > nit: i think Gavin's  patch for common services getters is about to land (bug
> > 512784).
> > could make sense add it to your queue and use Services.wm since i think this
> > patch will land later than that?
> 
> I'd rather not add an unnecessary dependancy - will fix it up once bug 512784
> lands.

It has landed.

    
    

    
  
Is this is minefield nightly builds?
I'm keen to test this feature out.
Blocks: 554234

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v4.1 (obsolete)
        — 
          — Splinter Review
      

    


  
Updated with the Services.jsm change, test fix, and un-bittrotted.

There still may be a test issue - I haven't had time to look into it. As such, Marco has offered to look into that and finally land this thing.

        Attachment #428133 -
        Attachment is obsolete: true

    
    

    
  


  
this one should be good to land.
Fixed a leftover in a handler, some minor indentation, some crlf (that were already there, not blair's fault), and some part of the b-c test.

        Attachment #434971 -
        Attachment is obsolete: true

    
    

    
  
tryserver said green, thus:
http://hg.mozilla.org/mozilla-central/rev/dcb347b2d3d4
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Flags: in-litmus? → in-litmus+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a4

    
  
Blocks: 555326

    
  
Blocks: 555331

    
    

    
  
bah i set the wrong test flag, sorry, restoring in-litmus?
Flags: in-litmus+ → in-litmus?

        Attachment #435279 -
        Attachment description: patch v5.0 → patch v5.0
[Checkin: Comment 127]
No longer blocks: FF2SM
Depends on: 555558

    
    

    
  
Is it intentional that URLs which are in history (or bookmarks) and also loaded in a tab show up twice in the autocomplete results? This often causes some duplicated entries in the dropdown (one with the URL, one with "switch to tab") and makes the URL bar harder to use for me. It even suggests to switch to the current tab, which is kind of pointless, I think.

    
    

    
  
(In reply to comment #129)
> Is it intentional that URLs which are in history (or bookmarks) and also loaded
> in a tab show up twice in the autocomplete results? This often causes some
> duplicated entries in the dropdown (one with the URL, one with "switch to tab")
> and makes the URL bar harder to use for me. It even suggests to switch to the
> current tab, which is kind of pointless, I think.

please file separate enh request bugs, with dependency on this one, then we can evaluate them.

Btw, about the former, at first glance i think makes sense to have 2 entries, because i could want to switch to the tab or open a new tab to reload the same page. But UX could have ideas about that.

The latter is probably a polish thing, but i doubt we can easily know if the tab is currently selected in toolkit part, maybe the browser part could skip the result. Worth filing an enh bug, then we'll evaluate.

    
  
Depends on: 555689

    
  
Depends on: 555694

    
  
Keywords: user-doc-needed

    
  
Blocks: 557650
No longer blocks: 557650
Depends on: 557650

    
  
Depends on: 558354
Depends on: 558626
Depends on: 559878

    
  
Depends on: 564395

    
  
Depends on: 564573

    
  
Depends on: 564676
No longer depends on: 555689
No longer depends on: 555689

    
  
Depends on: 571672

    
  
Depends on: 570412

    
    

    
  
QA will need to automate the testing of this feature as much as possible in Mozmill.  Adding keyword to reflect that.
Whiteboard: [icon-3.7] → [icon-3.7] [mozmill-test-needed]

    
    

    
  
Not a programmer, but it seems to me that it would be best to bump open tabs to the top of the list. I.E. when I type "Gmail" I get:
http://mail.google.com/mail/
http://gmail.com/
https://www.google.com/accounts/ServiceLogin?service=VeryLongString
https://mail.google.com/mail/?shva=1#inbox
5 More or so, and my tab Titled: "Gmail: Email from Google" doesn't even make the list.

    
    

    
  
(In reply to comment #132)
> Not a programmer, but it seems to me that it would be best to bump open tabs to
> the top of the list.

See bug 546254.

    
  
Depends on: 575609

    
  
Depends on: 584658

    
  
Depends on: 586054

    
  
Depends on: 598403

    
  
Depends on: 598619

    
  
Depends on: 604457
Depends on: 596485
Depends on: 610720
Depends on: 587611
Alias: (switch-to-tab)
Alias: (switch-to-tab) → switch-to-tab
Depends on: 595046
Depends on: 612639
Depends on: 589324
Depends on: 605710

    
  
Depends on: 620290

    
  
Whiteboard: [icon-3.7] [mozmill-test-needed] → [icon-3.7] [mozmill-test-needed][switch-to-tab]

    
  
Blocks: 654303

    
  
Depends on: 782863

    
  
Depends on: 793723

    
  
Depends on: 743323

    
  
Flags: in-litmus?

    
  
Depends on: 1223712

    
  
Depends on: 1231025

    
  
Depends on: 636992

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: