Closed Bug 1210616 Opened 9 years ago Closed 9 years ago

Implement Synced Tabs in awesome bar

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 45
Iteration:
44.3 - Nov 2
Tracking Status
firefox44 --- wontfix
firefox45 --- verified
relnote-firefox --- 45+

People

(Reporter: edwong, Assigned: markh)

References

Details

User Story

So that I can more easily find pages I have open on other devices, as a user syncing multi-devices, I want to see synced tabs highlighted in the awesome bar just like history and bookmarks.
Acceptance criteria: When syncing multiple devices, when the user types characters into the awesome bar that return results from other devices, the sync tab icon (a variation of switch to tab icon) should appear in the results with the name of the device.

Attachments

(2 files, 6 obsolete files)

No description provided.
Summary: Implement Synced Tabs in awesomebar → Implement Synced Tabs in awesome bar
need draft of mocks to implement.
Assignee: nobody → rfeeley
Iteration: --- → 44.2 - Oct 19
Flags: firefox-backlog+
I added above a link to a mock from Ryan Feeley.
Awesomebar behavior simply copies "Switch to tab" design but instead shows tab icon + device name. Detailed here: https://mozilla.invisionapp.com/share/NA47717JG#/screens/105579242
Assignee: rfeeley → markh
This patch implements a new "Autocomplete provider" for "tabs from other devices" as known by Sync. I'll attach a screenshot, but it displays the "device name" and an image of either a desktop computer or a phone, depending on what type of device the tab is open from. It makes no attempt to prevent being included if the site already exists from other providers - eg, using twitter as an example, it will be possible to see "Visit twitter.com", "Switch to tab" if it is open on this device plus this new entry naming the remote device. The CSS and images need love, but things look OK on Windows - I'll get new assets and tweak the CSS if this approach looks OK. I've started working on tests, but I decided to get feedback before completing them.
Attachment #8671188 - Flags: feedback?(mak77)
Attached image autocomplete-remote-tabs.png (deleted) —
This is a screenshot of the previously attached patch. In this example, a desktop device named "skip's Nightly on Eden, sync profile" has one tab open to twitter.com.
With tests - they weren't as horrible as I expected :)
Attachment #8671188 - Attachment is obsolete: true
Attachment #8671188 - Flags: feedback?(mak77)
Attachment #8671305 - Flags: feedback?(mak77)
Priority: -- → P1
Comment on attachment 8671305 [details] [diff] [review] 0002-Bug-1210616-have-remote-sync-tabs-appear-in-awesomeb.patch Review of attachment 8671305 [details] [diff] [review]: ----------------------------------------------------------------- The approach looks good to me... were you looking for an actual review, or still playing around with the code? I'm happy it's much easier to add stuff to UnifiedComplete. ::: toolkit/components/places/UnifiedComplete.js @@ +867,5 @@ > this._addingHeuristicFirstMatch = false; > > + yield this._matchRemoteTabs(); > + if (!this.pending) > + return; Is there a reason to run this before the delay? The only thing here should be autofill/firstrow (probably adding a comment explaining that would help)
Attachment #8671305 - Flags: feedback?(mak77) → feedback+
Bryan, would it be possible for you to supply me with the assets? In attachment 8671190 [details], I've simply used https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/sync-desktopIcon.png and https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/sync-mobileIcon.png, which seem fine to me - if you agree, I think I only need a hi-dpi 32x32 version of each icon. If you'd prefer a different image, please provide both 16x16 and 32x32 versions.
Flags: needinfo?(bbell)
(In reply to Marco Bonardo [::mak] from comment #8) > The approach looks good to me... were you looking for an actual review, or > still playing around with the code? Thanks Mak - at this stage I've only done the CSS changes for Windows, so it's not quite ready yet - but that is the only thing I'm aware of that needs to be changed - so if you spot anything else, it would make final review easier ;) > I'm happy it's much easier to add stuff to UnifiedComplete. Me too :) > Is there a reason to run this before the delay? The only thing here should > be autofill/firstrow (probably adding a comment explaining that would help) Nope - I've made that change here and it will be in the next (hopefully final) version. The comment I added reads: + // We sleep a little between adding the heuristicFirstMatch and matching + // any other searches. but it did strike me that I'm not sure *why* we do it...
Blocks: 996638
User Story: (updated)
(In reply to Mark Hammond [:markh] from comment #9) > Bryan, would it be possible for you to supply me with the assets? In > attachment 8671190 [details], I've simply used > https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/sync- > desktopIcon.png and > https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/sync- > mobileIcon.png, which seem fine to me - if you agree, I think I only need a > hi-dpi 32x32 version of each icon. If you'd prefer a different image, please > provide both 16x16 and 32x32 versions. I discussed this with Horlander, and we both now feel confident that the right thing to show is the standard tab icon, just as it's shown for open tabs already on the device you're on. The primary message the icon needs to convey is "this thing is already open". Currently we do that with the tab icon. We don't see how adding 2 new icons (desktop, and mobile) would reinforce that message. Where a thing is open is secondary information and we cover that with the labeling. So, can we give it a try with just the standard tab icon plus device name label? example https://invis.io/UX4KY24JK
Flags: needinfo?(bbell)
from IRC - rfeeley also agrees with `standard tab icon plus device name label` - we're clear to land that.
Ryan Feeley and I talked and were agreed on the tab icon, but that it needs to look different (even slightly) than a Switch to Tab, to signify this is a remote tab. Maybe change the shading or the color slightly. I think this is really important, and we can always tweak it later, but for now the user is trained to see that icon as a tab in their browser, not on another device. But after talking to Ryan I agree the icons ruin the awesomebar processing model, which is rows of text.
This patch reuses the same "tab" icon as desired by UX. Bryan Bell asked me to do it this way and we can reevaluate tweaks once it has landed, which is fine with me. This version looks the same as the previous attachment except the standard "tab" icon used for "switch to tab" is used instead of the device icon in that attachment.
Attachment #8671305 - Attachment is obsolete: true
Attachment #8676590 - Flags: review?(mak77)
Iteration: 44.2 - Oct 19 → 44.3 - Nov 2
(In reply to Mark Hammond [:markh] from comment #10) > but it did strike me that I'm not sure *why* we do it... Searching is expensive, we don't want to launch 5 or 6 I/O tasks every time the user types a char. Usually autocomplete solves this by adding a timeout before any search is started, but then autofill would have lag, and it would be noticeable and annoying. So we remove the timeout from the autocomplete widget and implement it in the search component, after autofill but before any other kind of search.
Comment on attachment 8676590 [details] [diff] [review] 0001-Bug-1210616-have-remote-sync-tabs-appear-in-awesomeb.patch Review of attachment 8676590 [details] [diff] [review]: ----------------------------------------------------------------- A small downside of this change as it is, is that remote tabs will get priority over local results, so for example if you have the same url open locally and remotely, you'd be suggested the remote entry instead of a switch to tab entry. The only solution to that (so far) would be to move matching just before the MATCH_BOUNDARY_ANYWHERE catch-all query. But then this would be pushed out by search suggestions :( I think we need at least a follow-up to figure out a solution... Off-hand we may do this: where we do duplicate detection in UnifiedComplete, we could first check if an incoming local match has the same url as an existing remote match, if so update the remote match with the local data. ::: toolkit/components/places/PlacesRemoteTabsAutocompleteProvider.jsm @@ +21,5 @@ > + return string.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); > +} > + > +// Build the in-memory structure we use. > +function buildItems() { How fast/slow is this? if it's cached data fine, if it's data that may take a while to fetch, it will slow down the first AC query and may be worth doing something like we do for search suggestions (start fetching and handle asynchronously). On the other side an asynchronous behavior may complicate merging matches a little bit. ::: toolkit/components/places/UnifiedComplete.js @@ +866,5 @@ > yield this._matchFirstHeuristicResult(conn); > this._addingHeuristicFirstMatch = false; > > + // We sleep a little between adding the heuristicFirstMatch and matching > + // any other searches. you can probably improve this comment with what I explained before @@ +872,5 @@ > if (!this.pending) > return; > > + yield this._matchRemoteTabs(); > + if (!this.pending) you should check this._enableActions and I think it would be wise to make these depend on Open Tabs option in preferences, that means checking this.hasBehavior("openpage"). In the end these are Open Pages, just on another device. @@ +873,5 @@ > return; > > + yield this._matchRemoteTabs(); > + if (!this.pending) > + return; as I said, for now it may be worth moving this down after the queries, then we can look at how prioritize it in a follow-up. @@ +1216,5 @@ > + value: makeActionURL("remotetab", { url, deviceName }), > + comment: title || url, > + style: "action", > + frecency: FRECENCY_DEFAULT, > + icon, We should either mark this match as remote (by adding remote: true). The difference is that local results are added only if they have an high frecency (> FRECENCY_DEFAULT) and are very often what the user is looking for, we try to put them high. We also don't want to break muscle memory, so we always try to provide a minimum number of local results (MINIMUM_LOCAL_MATCHES). Then we have "remote" matches, they are always appended as they come, but they may be pushed down by local matches. Thus they could also be pushed out if there are enough high frecency (> FRECENCY_DEFAULT) local results, but this happens also for local matches with a low frecency. To sum up, if you set this as a local match with FRECENCY_DEFAULT, as you did, it's likely it will be pushed out both by remote and higher frecency results. If you set it as a remote match, it will be appended after local results, it may still be pushed out but only if there are many high frecency results, other remote results will be appended after this. ::: toolkit/components/places/tests/unifiedcomplete/test_remotetabmatches.js @@ +62,5 @@ > + }], > + } > + }); > + > + // no match of ours. nit: this comment could be improved ::: toolkit/content/widgets/autocomplete.xml @@ +1707,5 @@ > displayUrl = action.params.url; > let desc = this._stringBundle.GetStringFromName("switchToTab"); > this._setUpDescription(this._action, desc, true); > + } else if (action.type == "remotetab") { > + this.classList.add("overridable-action"); I don't think this action is override-able. Overriding switch-to-tab means "tropen the url in this tab, do not switch", but I can't find a valid option to override a remote tab.
Attachment #8676590 - Flags: review?(mak77)
nevermind this: > as I said, for now it may be worth moving this down after the queries, then we can look at how > prioritize it in a follow-up. We can't do that as I explained at the beginning cause we'd be pushed out by search suggestions... I forgot to remove this comment. We need the deduping thing.
(In reply to Marco Bonardo [::mak] from comment #17) > ::: toolkit/components/places/PlacesRemoteTabsAutocompleteProvider.jsm > @@ +21,5 @@ > > + return string.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); > > +} > > + > > +// Build the in-memory structure we use. > > +function buildItems() { > > How fast/slow is this? if it's cached data fine, It is all locally cached - all I'm doing there is transforming it into a structure that's faster to access for the awesomebar requirements. > To sum up, if you set this as a local match with FRECENCY_DEFAULT, as you > did, it's likely it will be pushed out both by remote and higher frecency > results. If you set it as a remote match, it will be appended after local > results, it may still be pushed out but only if there are many high frecency > results, other remote results will be appended after this. Hmm - I thought I tried that and it was pushed lower than searches - I'll dig a little deeper, but if you can offer a clue as to why I might have seen that I'd appreciate it :) > where we do duplicate detection in UnifiedComplete, we could first check if an incoming local > match has the same url as an existing remote match, if so update the remote match with the > local data. Can you please explain what you mean by "update the remote match with the local data."? And specifically, if you look at the attached screenshot you will see "visit twitter.com/", the new remote-tab entry, then a bookmarked "https://twitter.com" - I assume that's the kind of duplicate you are talking about? And I assume the duplicate in this case is the "bookmarked" entry and not the "visit" entry? If so, how exactly should that screenshot look with what you are suggesting?
Flags: needinfo?(mak77)
(In reply to Mark Hammond [:markh] from comment #19) > Hmm - I thought I tried that and it was pushed lower than searches - I'll > dig a little deeper, but if you can offer a clue as to why I might have seen > that I'd appreciate it :) Well, if addMatch for these is invoked before the addMatch for searches, it should come first. Don't be confused by the action row that may indicate search, this is just about search suggestions. At the point where you put it, if it's marked as remote it should appear after local matches but before search suggestions. We insert at index = this._remoteMatchesStartIndex + this._remoteMatchesCount; If that doesn't happen, sounds like a bug. > > where we do duplicate detection in UnifiedComplete, we could first check if an incoming local > > match has the same url as an existing remote match, if so update the remote match with the > > local data. > > Can you please explain what you mean by "update the remote match with the > local data."? As I said, suppose you have the same identical url open on a mobile device and locally on desktop. You start typing something that matches, as it is now you'd get a Remote Tab match. But since the tab is already open locally, you should instead get a Switch to tab match. In this specific case, we may have to identify the remote match in the result and replace it with the local match (a switch to tab match) > And specifically, if you look at the attached screenshot you > will see "visit twitter.com/", the new remote-tab entry, then a bookmarked > "https://twitter.com" - I assume that's the kind of duplicate you are > talking about? Well, it depends if the remote tab entry points exactly to https://twitter.com rather than http://twitter.com or https://www.twitter.com. We are not that good at duplicates detection yet. If they are exactly the same url, then addMatch will skip any entries after the first one. > And I assume the duplicate in this case is the "bookmarked" > entry and not the "visit" entry? If so, how exactly should that screenshot > look with what you are suggesting? I don't know if we think Remote Tabs should have priority over local visits/bookmarks, this is something we must decide here. But for sure Switch to tab should have priority over a Remote Tab.
Flags: needinfo?(mak77)
After much confusion, I discovered that most of the time, the "switch to tab" result comes from |this._searchQuery| - so if I make sure |_matchRemoteTabs()| is called after the |queries| array is processed, make the frecency equal to |FRECENCY_DEFAULT + 1| and ensure the result has a |placesId| we seem to get the desired behaviour - a matching "switch to tab" entry will prevent the new remote tab entry from appearing, and it remains at the bottom of the local matches and isn't pushed out by remote matches. I believe I addressed all other comments, and added a test that a matching "switch to tab" prevents the new entry from appearing.
Attachment #8676590 - Attachment is obsolete: true
Attachment #8679244 - Flags: review?(mak77)
(In reply to Mark Hammond [:markh] from comment #21) > Created attachment 8679244 [details] [diff] [review] > 0001-Bug-1210616-have-remote-sync-tabs-appear-in-awesomeb.patch > > After much confusion, I discovered that most of the time, the "switch to > tab" result comes from |this._searchQuery| - so if I make sure > |_matchRemoteTabs()| is called after the |queries| array is processed, make > the frecency equal to |FRECENCY_DEFAULT + 1| Yes, I didn't suggest that cause I was worried local results would have pushed away the remote tab entry too easily, cause there may be many results with frecency > frecency_default + 1 and we will only show MINIMUM_LOCAL_MATCHES of them. If you think that for the first incarnation that's acceptable (and then we can eventually improve over it), it's fine for me.
(In reply to Marco Bonardo [::mak] from comment #22) > Yes, I didn't suggest that cause I was worried local results would have > pushed away the remote tab entry too easily, cause there may be many results > with frecency > frecency_default + 1 and we will only show > MINIMUM_LOCAL_MATCHES of them. Hm, but actually the minimum is only active when local matches have a frecency lower than frecency_default so.... it might just work. I will review this asap.
Comment on attachment 8679244 [details] [diff] [review] 0001-Bug-1210616-have-remote-sync-tabs-appear-in-awesomeb.patch Review of attachment 8679244 [details] [diff] [review]: ----------------------------------------------------------------- So, it is mostly good, but I have some problems understanding how the cached data is updated. Let's assume a week long session (many users don't close firefox for a long time), how are remote tabs updated? we seem to cache clients and tabs once, and I can't see where we update them... I don't think we should requery them at every AC search, but probably we should invalidate the cache after a certain amount of time, or if Sync receives new tab data? (not sure if there's a notification for that) What we do for search engines, is listening to search service notifications and keeping the cache updated, for example. In this case I'd be fine with a timed invalidation plus some specific events (login/logout) I think. ::: toolkit/components/places/PlacesRemoteTabsAutocompleteProvider.jsm @@ +27,5 @@ > + let tabs = new Map(); // keyed by string URL, value is {clientId, tab} > + > + let engine = Weave.Service.engineManager.get("tabs"); > + > + for (let [guid, client] in Iterator(engine.getAllClients())) { I assume getting an engine and querying it for data works (and returns nothing) even if the user is not logged in to Sync? @@ +47,5 @@ > + if (!_items) { > + _items = buildItems(); > + } > + return _items; > +} nit: you could replace this with a defineLazyGetter @@ +52,5 @@ > + > +// An observer to invalidate _items. > +function observe(subject, topic, data) { > + switch (topic) { > + case "weave:engine:sync:finish": so far this is only observing one topic, so I'd just inline the function in AddObserver and don't make any kind of check, we know we can only get weave:engine:sync:finish. We can expand it in future if and when needed. @@ +54,5 @@ > +function observe(subject, topic, data) { > + switch (topic) { > + case "weave:engine:sync:finish": > + if (data == "tabs") { > + _items = null; removeObserver? Does this happen only at shutdown, or also if a user logs out of Sync in the middle of a session? I assume if the user logs out from Sync and then logs in again, we want to throw the cache away, he may be logging 2 different users. @@ +74,5 @@ > + return Promise.resolve([]); > + } > + > + let re = new RegExp(escapeRegExp(searchString), "i"); > + let result = []; nit: results or matches ::: toolkit/components/places/UnifiedComplete.js @@ +1219,5 @@ > + let placeId; > + try { > + let placeInfo = yield PlacesUtils.promisePlaceInfo(NetUtil.newURI(url)); > + placeId = placeInfo.placeId; > + } catch (ex) { /* no local place exists for this remote tab */ } this should not be needed: 1311 if ((match.placeId && this._usedPlaceIds.has(match.placeId)) || 1312 this._usedURLs.has(urlMapKey)) { the second ORed check is what we are aiming at here, we don't want a local url identical to this remote url. pleace id (so far) is only needed for keywords cause a keyword url contains %s but we replace it with the string (likely there are smart ways to also avoid that, now that keywords are actions). If that doesn't work please let me know. I think you may have been confused by the if parenthesis, we could probably assign the first part of the condition to a local dupedPlaceId var to improve readability.
Attachment #8679244 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #24) > Let's assume a week long session (many users don't close firefox for a long > time), how are remote tabs updated? we seem to cache clients and tabs once, > and I can't see where we update them... Every time we get new tabs from Sync, the "weave:engine:sync:finish" observer fires. This deletes _items, so the next time the AC data is queried we get the latest available. So for a user with a week-long session, we can expect this observer to fire multiple times - once per sync. > I don't think we should requery them at every AC search, but probably we > should invalidate the cache after a certain amount of time, or if Sync > receives new tab data? (not sure if there's a notification for that) That's what the observer is for. Note that we don't really "requery" - after the sync completes, the list of tabs is already in memory but in a format that isn't optimal for fast AC queries. I could have written this without the local cache but with less efficient looping on each AC query (ie, a loop similar to the one used to build the cache), but it seems better to perform that inefficient looping exactly once when we have new data rather than on each query. > What we do for search engines, is listening to search service notifications > and keeping the cache updated, for example. In this case I'd be fine with a > timed invalidation plus some specific events (login/logout) I think. I hope the above makes sense - the login/logout events aren't really relevant here - that existing observer tells us precisely when the list of tabs has been re-fetched from the server (although it is possible the actual list hasn't changed, which we don't take into account because there's no obvious way to do that) > I assume getting an engine and querying it for data works (and returns > nothing) even if the user is not logged in to Sync? That is a good point - it works, but it may initialize more of Sync than is ideal - I'll upload a new version that short-circuits in that case. > @@ +47,5 @@ > > + if (!_items) { > > + _items = buildItems(); > > + } > > + return _items; > > +} > > nit: you could replace this with a defineLazyGetter I don't think I can, because as mentioned above, _items goes null each time the observer fires. > so far this is only observing one topic, so I'd just inline the function in > AddObserver and don't make any kind of check, we know we can only get > weave:engine:sync:finish. We can expand it in future if and when needed. See below - the new version will handle a second notification, so I don't think I can inline it. > @@ +54,5 @@ > > +function observe(subject, topic, data) { > > + switch (topic) { > > + case "weave:engine:sync:finish": > > + if (data == "tabs") { > > + _items = null; > > removeObserver? This isn't a one-off notification, it fires every time a new list is fetched from the server by Sync. > Does this happen only at shutdown, or also if a user logs out of Sync in the > middle of a session? > I assume if the user logs out from Sync and then logs in again, we want to > throw the cache away, he may be logging 2 different users. Ah, yeah, that's a good point - I'll update it to listen for the notification Sync sends when it resets. > ::: toolkit/components/places/UnifiedComplete.js > @@ +1219,5 @@ > > + let placeId; > > + try { > > + let placeInfo = yield PlacesUtils.promisePlaceInfo(NetUtil.newURI(url)); > > + placeId = placeInfo.placeId; > > + } catch (ex) { /* no local place exists for this remote tab */ } > > this should not be needed: > > 1311 if ((match.placeId && this._usedPlaceIds.has(match.placeId)) || > 1312 this._usedURLs.has(urlMapKey)) { > > the second ORed check is what we are aiming at here, we don't want a local > url identical to this remote url. pleace id (so far) is only needed for > keywords cause a keyword url contains %s but we replace it with the string > (likely there are smart ways to also avoid that, now that keywords are > actions). > If that doesn't work please let me know. It doesn't - the urlMapKey for the "switch to tab" action is |moz-action:switchtab,{"url":"http://foo.com/"}| while ours is |moz-action:remotetab,{"url":"http://foo.com/","deviceName":"My%20Phone"}| so they don't match. With the patch as it stands, they do both get the same placeId, so it is recognised as a dupe. So I'll leave this in the next patch I upload and you can tell me if it still needs changing in some way. Let me know if any of the above doesn't make sense.
As discussed.
Attachment #8679244 - Attachment is obsolete: true
Attachment #8681042 - Flags: review?(mak77)
(In reply to Mark Hammond [:markh] from comment #25) > Every time we get new tabs from Sync, the "weave:engine:sync:finish" > observer fires. This deletes _items, so the next time the AC data is queried > we get the latest available. So for a user with a week-long session, we can > expect this observer to fire multiple times - once per sync. Sorry, I completely misunderstood what weave:engine:sync:finish was, I thought it was a shutdown notification. Now it makes much more sense! > It doesn't - the urlMapKey for the "switch to tab" action is > |moz-action:switchtab,{"url":"http://foo.com/"}| while ours is > |moz-action:remotetab,{"url":"http://foo.com/","deviceName":"My%20Phone"}| > so they don't match. With the patch as it stands, they do both get the same > placeId, so it is recognised as a dupe. > > So I'll leave this in the next patch I upload and you can tell me if it > still needs changing in some way. Ah good point... For most actions we should extract the final url from the action url and that is what we should add/compare in the duplicates detection. Unfortunately we don't have a module that unifies all actions support in one place, so this may duplicate a bunch of code. For now let's just handle it properly for switch-to-tab and remote-tabs. Basically let urlMapKey = stripHttpAndTrim(match.value); should be something like let urlMapKey = stripHttpAndTrim(urlFromAction(match.value)); urlFromAction for now may be incomplete and just return the action url for other cases. We can fix them in a follow-up.
(In reply to Marco Bonardo [::mak] from comment #27) > Unfortunately we don't have a module that unifies all actions > support in one place FYI: that is bug 1054816.
Comment on attachment 8681042 [details] [diff] [review] 0001-Bug-1210616-have-remote-sync-tabs-appear-in-awesomeb.patch Review of attachment 8681042 [details] [diff] [review]: ----------------------------------------------------------------- looks good, will r+ once the place id thing is solved. Sorry for the back and forth.
Attachment #8681042 - Flags: review?(mak77) → feedback+
(In reply to Marco Bonardo [::mak] from comment #29) > looks good, will r+ once the place id thing is solved. Sorry for the back > and forth. For my information, why don't we want the placeId to be specified?
(In reply to Marco Bonardo [::mak] from comment #27) > Ah good point... For most actions we should extract the final url from the > action url and that is what we should add/compare in the duplicates > detection. Unfortunately we don't have a module that unifies all actions > support in one place, so this may duplicate a bunch of code. For now let's > just handle it properly for switch-to-tab and remote-tabs. > Basically > let urlMapKey = stripHttpAndTrim(match.value); > should be something like > let urlMapKey = stripHttpAndTrim(urlFromAction(match.value)); > urlFromAction for now may be incomplete and just return the action url for > other cases. We can fix them in a follow-up. I don't understand what is being asked here - I should write urlFromAction, that only handles the switchtab and remotetab actions, and otherwise returns the passed-in URL? I also don't understand why the use of the placeId isn't correct - switchtab already uses it, and if remotetab uses it everything works perfectly. Why are switchtab and remotetab supposed to do different things here? Why does a placeid make sense for one but not the other, especially when it makes everything work without this additional effort?
Flags: needinfo?(mak77)
(In reply to Mark Hammond [:markh] from comment #31) > I don't understand what is being asked here - I should write urlFromAction, > that only handles the switchtab and remotetab actions, and otherwise returns > the passed-in URL? yes. > I also don't understand why the use of the placeId isn't correct - switchtab > already uses it, and if remotetab uses it everything works perfectly. Why > are switchtab and remotetab supposed to do different things here? Why does a > placeid make sense for one but not the other, especially when it makes > everything work without this additional effort? there's no technical reason, but we're trying to limit use of internal database ids everywhere we can avoid it, cause it's generally wrong, it's also one more data we need to query that we could avoid 99% of the cases and in some cases that may simplify queries. The place id support there exists only for legacy reasons (keyword urls, but I expect we can also avoid that there in future) and the fact it works for switch to tab is just due to a bug that I'm asking you to address, the fact we are comparing action urls with plain urls.
Flags: needinfo?(mak77)
Attachment #8681042 - Attachment is obsolete: true
Attachment #8682800 - Flags: review?(mak77)
Comment on attachment 8682800 [details] [diff] [review] 0001-Bug-1210616-have-remote-sync-tabs-appear-in-awesomeb.patch Review of attachment 8682800 [details] [diff] [review]: ----------------------------------------------------------------- thank you, it looks good.
Attachment #8682800 - Flags: review?(mak77) → review+
This patch causes browser_tabMatchesInAwesomebar_perwindowpb.js to fail. The problem is that the URLFromAction change is causing the expected switchtotab action to be discard as it is considered a dupe. The first match added is: > {"value":"http://example.org/browser/browser/base/content/test/general/dummy_page.html","comment":"example.org/browser/browser/base/content/test/general/dummy_page.html","finalCompleteValue":"http://example.org/browser/browser/base/content/test/general/dummy_page.html","icon":null,"frecency":2200,"style":"autofill"} which is added. The next match is: > {"placeId":9,"style":"action switchtab","value":"moz-action:switchtab,{\"url\":\"http://example.org/browser/browser/base/content/test/general/dummy_page.html\"}","comment":"Dummy test page","frecency":2200} which is rejected as it is now considered a dupe, and the test times out due to it expecting the switchtab action to also appear. I think the test is correct - ie, that "switch to tab" should *not* be removed here. The test passes if I change URLFromAction to ignore switchtab actions (thus leaving only remotetab) but that makes the entire method seem pointless and incorrectly named. :mak, how should this be resolved?
Flags: needinfo?(mak77)
(In reply to Mark Hammond [:markh] from comment #35) > > {"value":"http://example.org/browser/browser/base/content/test/general/dummy_page.html","comment":"example.org/browser/browser/base/content/test/general/dummy_page.html","finalCompleteValue":"http://example.org/browser/browser/base/content/test/general/dummy_page.html","icon":null,"frecency":2200,"style":"autofill"} > > which is added. The next match is: > > > {"placeId":9,"style":"action switchtab","value":"moz-action:switchtab,{\"url\":\"http://example.org/browser/browser/base/content/test/general/dummy_page.html\"}","comment":"Dummy test page","frecency":2200} > > which is rejected as it is now considered a dupe awesome (not), the first one is the autofill entry, it's the action/heuristic row. It's an edge case. Basically the test is typing the whole url, we suggest it as an autoFill entry. Unfortunately it's already open in another tab and we don't support a switch-to-tab entry in the action row, but likely we should. So basically the placeId de-duping was hiding a bug :( Example: suppose a user types "moz", we autocomplete to "mozilla.com", but the user already has a "mozilla.com" tab open. as it is now, he'll get both a mozilla.com entry and a switch to tab entry. He should only get a switch to tab entry. For now I think we can remove switchtotab from URLFromAction, but please file a follow-up bug where we should support switch to tab in autofill code and fix de-duping of all the action rows in URLFromAction. Above URLFromAction add a "// TODO (bug XYZ): description" comment so we know it's an incomplete method.
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] from comment #36) > Example: suppose a user types "moz", we autocomplete to "mozilla.com", but > the user already has a "mozilla.com" tab open. as it is now, he'll get both > a mozilla.com entry and a switch to tab entry. He should only get a switch > to tab entry. I don't agree - he may know he has that tab and wants a new one. Once we support session state restore for remote tabs, I think we will want remote tabs too - that entry then becomes materially different than either "visit" or "switchtab" as the user may know there is form state in the remote tab. > "// TODO (bug XYZ): description" comment so we know it's > an incomplete method. OK - but could you please open that bug and I'll add the reference - I don't think I could summarize it accurately.
(In reply to Mark Hammond [:markh] from comment #37) > (In reply to Marco Bonardo [::mak] from comment #36) > > Example: suppose a user types "moz", we autocomplete to "mozilla.com", but > > the user already has a "mozilla.com" tab open. as it is now, he'll get both > > a mozilla.com entry and a switch to tab entry. He should only get a switch > > to tab entry. > > I don't agree - he may know he has that tab and wants a new one. that's why we have actions override, by holding shift. > OK - but could you please open that bug and I'll add the reference - I don't > think I could summarize it accurately. ok, I will list those here.
(In reply to Mark Hammond [:markh] from comment #37) > Once we support session state restore for remote tabs, I think we will want > remote tabs too - that entry then becomes materially different than either > "visit" or "switchtab" as the user may know there is form state in the > remote tab. we should just pick what we think is the best option, and allow override. The space in the popup is too small to also allow dupes.
(In reply to Marco Bonardo [::mak] from comment #38) > that's why we have actions override, by holding shift. I have no idea what that does :( Now I know I can obviously experiment, but I doubt many users are aware of this - do we have telemetry?
no, I don't think we have telemetry and I agree it's not discoverable. But switch-to-tab always worked like that, I don't think it's wise to make a special case for an edge case. Rather would be better to figure a way to make action override more discoverable. Btw, when you are on a switch-to-tab entry you can press shift, the switch-to-tab disappears and the page is opened again in the current tab rather than switching. I think time ago zpao also proposed a blacklist of pages that should never switch and implemented an add-on to support that. but I think I'm going off-topic.
(In reply to Marco Bonardo [::mak] from comment #36) > For now I think we can remove switchtotab from URLFromAction, *sob* - this doesn't work. By removing switchtab from URLFromAction, the new remotetab action is not seen as a duplicate of switchtab. IOW: * URLFromAction needs *both* |remotetab| and |switchtab| for it to correctly identify duplicates within those 2 actions. * But URLFromAction having |switchtab| means we incorrectly treat a URL in a |switchtab| action as a duplicate of an autofill entry. I don't see a way around this, other than to reinstate the "placeid" code and have the TODO comment there. Mak, thoughts?
Flags: needinfo?(mak77)
sigh, the problem is that promisePlaceInfo is not free at all, it's a disk access just to fetch an id we ideally don't need, and that happens everytime we have a remote match to add... If we can avoid IO roundtrips, we should. What about we do a very dumb hack, convert both switchtab and remotetab urls to a generic moz-action:tab:url in urlFromAction. it should solve this problem and won't clash with existing urls, for now.
Flags: needinfo?(mak77)
Yeah, that sounds like a good idea! I renamed the function to URLKeyFromAction() and had it call stripHttpAndTrim() instead of the caller calling that function on the result - look ok? https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73f3803fc4a
Attachment #8682800 - Attachment is obsolete: true
Attachment #8685668 - Flags: review?(mak77)
Comment on attachment 8685668 [details] [diff] [review] 0001-Bug-1210616-have-remote-sync-tabs-appear-in-awesomeb.patch Review of attachment 8685668 [details] [diff] [review]: ----------------------------------------------------------------- Yeah that change makes sense. As a last minute thought, I'd probably rename urlKeyFromAction to getKeyForUrl or buildKeyforUrl or something like that, in the end it's processing any url, not just actions. I'll leave you the burden to find a smart-and-nice-proper-english name :)
Attachment #8685668 - Flags: review?(mak77) → review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Depends on: 1224689
Depends on: 1225194
The bug's fix is now verified on Latest Aurora 45.0a2! Build ID: 20151230004007 User Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Firefox/45.0
QA Whiteboard: [bugday-20151230]
Added to the release notes with "Synced Tabs in awesome bar" as wording.
Status: RESOLVED → VERIFIED
Sounds like it might be neat to mention in blog post. We'll plan on calling out for 45 Release.
Fabio, as Ryan and Edwin pointed out in the cross functional meeting today, there are lots of moving parts, and we're not going to wait on all of them to land incremental changes in release channel. However, it might be worth talking about when's the right time to promote them in a more deliberate way.
(In reply to Chris Karlof [:ckarlof] from comment #52) > Fabio, as Ryan and Edwin pointed out in the cross functional meeting today, > there are lots of moving parts, and we're not going to wait on all of them > to land incremental changes in release channel. However, it might be worth > talking about when's the right time to promote them in a more deliberate way. Agree. We won't proactively communicate until future releases. TBD. For now, just a release note as they come. Thanks.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: