<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Dietrich Ayala (:dietrich)

Comment 3

•

16 years ago

(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.

15 years ago

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.

[not reading bugmail]

Comment 15

•

15 years ago

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.

(no longer active)

Comment 16

•

•

15 years ago

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

Comment 23

•

15 years ago

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.

Assignee

•

15 years ago

Attached patch WiP v2 (obsolete) — Details — 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

Comment 28

•

15 years ago

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.

Comment 29

•

15 years ago

Attached image Shows the edge case where the "Switch to tab" label is underlined since it matches the search term. (obsolete) — Details

Assignee

Comment 30

•

15 years ago

Attached patch WiP v3 (obsolete) — Details — 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)

Comment 31

•

15 years ago

Attached image Screenshot of missing tab icon (obsolete) — Details

The latest version doesn't seem to show the tab icon.

Assignee

Comment 32

•

15 years ago

(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.

Assignee

Updated

•

•

15 years ago

Attached patch WiP v4 (obsolete) — Details — 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)

Assignee

Updated

•

15 years ago

Attachment #421223 - Flags: review? → review?(mconnor)

Mike Connor [:mconnor]

Updated

•

15 years ago

Attachment #421223 - Flags: review?(mconnor) → review-

Mike Connor [:mconnor]

Comment 51

•

15 years ago

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.

Comment 52

•

•

•

15 years ago

Attached patch Patch v1 (obsolete) — Details — 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.

Comment 78

•

15 years ago

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 "!".

•

15 years ago

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.

Comment 84

•

15 years ago

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?)

Assignee

Comment 85

•

15 years ago

(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.

Assignee

Comment 86

•

15 years ago

(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.

Reporter

Comment 87

•

15 years ago

(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.

Comment 88

•

15 years ago

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).

Comment 89

•

15 years ago

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.

Reporter

Comment 90

•

15 years ago

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.

Shawn Wilsher :sdwilsh

•

15 years ago

(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?

Comment 97

•

15 years ago

(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.

Comment 98

•

15 years ago

(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)

Assignee

Comment 99

•

15 years ago

Attached patch Patch v2 (obsolete) — Details — Splinter Review

Attachment #422506 - Attachment is obsolete: true

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

Reporter

Comment 100

•

15 years ago

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 101

•

15 years ago

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-

Comment 102

•

15 years ago

(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.

•

15 years ago

Attached patch Patch v3 (obsolete) — Details — Splinter Review

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

•

15 years ago

Attached patch Patch v3.1 (obsolete) — Details — 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)

Assignee

•

15 years ago

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+

Assignee

Comment 113

•

15 years ago

(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.

Assignee

Comment 114

•

15 years ago

Attached patch Patch v4 (obsolete) — Details — Splinter Review

•

15 years ago

Depends on: 551602

Takanori MATSUURA

Updated

•

15 years ago

No longer depends on: 551602

Updated

•

15 years ago

Depends on: 551602

Serge Gautherie (:sgautherie)

Assignee

Comment 121

•

15 years ago

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 → ---

Updated

•

15 years ago

Blocks: FF2SM

Comment 122

•

15 years ago

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

•

15 years ago

Attached patch Patch v4.1 (obsolete) — Details — 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

Comment 126

•

15 years ago

Attached patch patch v5.0 [Checkin: Comment 127] — Details — Splinter Review

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

Comment 127

•

15 years ago

tryserver said green, thus: http://hg.mozilla.org/mozilla-central/rev/dcb347b2d3d4

Status: REOPENED → RESOLVED

Closed: 15 years ago → 15 years ago

Flags: in-litmus? → in-litmus+

Resolution: --- → FIXED

Target Milestone: --- → Firefox 3.7a4

Updated

•

15 years ago

Blocks: 555326

Updated

•

15 years ago

Blocks: 555331

Comment 128

•

•

15 years ago

No longer blocks: 546253, 546254, 546255, 555326, 555331, 555767

Depends on: 555689, 546253, 546254, 546255, 555326, 555331, 555767

Reporter

Updated

•

15 years ago

No longer depends on: 555689

Elmar Ludwig

Updated

•

15 years ago

Depends on: 571672