1025195 - switchToTabHavingURI should have an option to ignore URL hashes when looking for already opened tabs

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

(Marco, can you please add this bug to the current iteration)

Comment 1

[Marco Mucci [:MarcoM]]

Added to Iteration 33.1

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8440181 [details] [diff] [review]
Patch

>+      if ((aIgnoreRef && browser.currentURI.equalsExceptRef(aURI)) ||
>+          browser.currentURI.equals(aURI)) {
>         // Focus the matching window & tab
>         aWindow.focus();
>         aWindow.gBrowser.tabContainer.selectedIndex = i;
>+        if (aIgnoreRef) {
>+          openUILinkIn(aURI.spec, "current", aOpenParams);
>+        }

openUILinkIn won't necessarily load the URI in aWindow here. Seems like you should call browser.loadURI.

It would be nice if you didn't add another boolean parameter to switchToTabHavingURI. It may make sense to overload aOpenParams here, e.g. rename it to aParams, support aParams.ignoreHash but delete that before passing aParams to openUILinkIn.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Addressed feedback.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8440854 [details] [diff] [review]
Patch v1.1

>+  let ignoreRef = aOpenParams.ignoreRef;

I wouldn't know what "ref" means here without doing some research, so I'd like to avoid that term even if this means being inconsistent with nsIURI. If Wikipedia and whatwg.org are any indication, then this portion of the URI is usually called fragment.

>+      if ((ignoreRef && browser.currentURI.equalsExceptRef(aURI)) ||
>+          browser.currentURI.equals(aURI)) {

if (ignoreRef ? browser.currentURI.equalsExceptRef(aURI)
              : browser.currentURI.equals(aURI)) {

>+        if (ignoreRef) {
>+          browser.loadURI(aURI.spec, aOpenParams.referrerURI, aOpenParams.charset);
>+        }

What if the parameter was used without the URI containing a fragment id? Seems like this would reload the page, which would be wrong.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [:dao] from comment #5)
> >+        if (ignoreRef) {
> >+          browser.loadURI(aURI.spec, aOpenParams.referrerURI, aOpenParams.charset);
> >+        }
> 
> What if the parameter was used without the URI containing a fragment id?
> Seems like this would reload the page, which would be wrong.

I don't see a good way to clear the ref/fragment portion of the URI. Do you have any tips as to how to do this for this case? Or should I just add a "#" to aURI.spec if aURI.ref is ""?

Comment 7

[Dão Gottwald [:dao]]

You could just leave the present fragment intact when switchToTabHavingURI is called with ignoreFragment without a fragment being specified in the URI...

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

That would be fine if we don't support about:preferences being the representative URI for the general pane. If the preferences are open and showing the privacy tab (about:preferences#privacy), and the 

That would be fine if we are OK with the following behavior:
1) On about:home, click on the Sync button
2) See that about:preferences#sync is opened
3) Go back to about:home and click on the Options button
4) See that the about:preferences#sync tab is switched to, showing the Sync preferences

Of course, this is implying that we use about:preferences as the canonical name for the General tab.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

That would be fine if we are OK with the following behavior:
1) On about:home, click on the Sync button
2) See that about:preferences#sync is opened
3) Go back to about:home and click on the Options button
4) See that the about:preferences#sync tab is switched to, showing the Sync preferences

Of course, this is implying that we use about:preferences as the canonical name for the General tab.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.2

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8440951 [details] [diff] [review]
Patch v1.2

this appears to be the same as the previous attachment

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.2 (qref'd)

Sorry about that, it appears that the qref I did before failed and corrupted my repo :(

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8441375 [details] [diff] [review]
Patch v1.2 (qref'd)

>+ *        passed via this object. This object also allows the 'ignoreFragment'
>+ *        property to be set to true to exlcude reference-portion matching when
>+ *        comparing URIs.

s/reference/fragment/

>+  let ignoreFragment = aOpenParams && aOpenParams.ignoreFragment;
>+  // This property is only used by switchToTabHavingURI and should
>+  // not be used as a parameter for the new load.
>+  if (aOpenParams)
>+    delete aOpenParams.ignoreFragment;

How about letting aOpenParams default to {} and getting rid of the null checks?

>+        let spec = aURI.spec;
>+        if (ignoreFragment && !aURI.ref)
>+          spec += "#";
>+        if (ignoreFragment) {
>+          browser.loadURI(spec, aOpenParams.referrerURI, aOpenParams.charset);
>+        }

The first three lines should be inside the if-block.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.3

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8441446 [details] [diff] [review]
Patch v1.3

>+        if (ignoreFragment) {
>+          let spec = aURI.spec;
>+          if (!aURI.ref)
>+            spec += "#";
>+          browser.loadURI(spec, aOpenParams.referrerURI, aOpenParams.charset);

last nit for the code changes: aOpenParams.referrerURI and aOpenParams.charset aren't useful here, since we don't expect anything to be loaded. So this should just be browser.loadURI(spec).

I hadn't looked at the test before. Turns out there are some issues too.

>+add_task(function() {
>+  let {tab: tabRefAboutHome} = yield promiseLoadTab("about:home#1");
>+  let {tab: tabRefAboutMozilla} = yield promiseLoadTab("about:mozilla");
>+  registerCleanupFunction(function() {
>+    while (gBrowser.tabs.length > 1)
>+      gBrowser.removeTab(gBrowser.tabs[gBrowser.tabs.length - 1]);
>+  });
>+
>+  gBrowser.selectedTab = tabRefAboutMozilla;
>+  let numTabsAtStart = gBrowser.tabs.length;
>+
>+  yield switchTab("about:home#1", false, true);
>+  yield switchTab("about:mozilla", false, true);
>+  yield switchTab("about:home#2", true, true);
>+  is(tabRefAboutHome, gBrowser.selectedTab, "The same about:home tab should be switched to");
>+  is(gBrowser.selectedBrowser.currentURI.ref, "2", "The ref should be updated to the new ref");
>+  yield switchTab("about:mozilla", false, true);
>+  yield switchTab("about:home#1", false, false);
>+  isnot(tabRefAboutHome, gBrowser.selectedTab, "Selected tab should not be initial about:blank tab");
>+  is(gBrowser.tabs.length, numTabsAtStart + 1, "Should have one new tab opened");
>+});
>+
>+function* switchTab(aURI, aIgnoreRef, aShouldFindExistingTab) {
>+  let tabSelectPromise = promiseTabSelect();
>+  let tabSwitched = window.switchToTabHavingURI(aURI, true, {ignoreFragment: aShouldFindExistingTab});
>+  is(tabSwitched, aShouldFindExistingTab, "Should switch to existing " + aURI + " tab if one existed");
>+  yield tabSelectPromise;
>+}

aIgnoreRef is completely unused. Did you intend to use that for ignoreFragment?

tabSwitched is a misnomer, since switchToTabHavingURI won't switch tabs if the found tab is already selected.

What's the point of tabSelectPromise? Can you reduce switchTab to it's second and third line, thereby getting rid of the yields all over the place?

>+function promiseLoadTab(url) {
>+  let deferred = Promise.defer();
>+
>+  let tab = gBrowser.selectedTab = gBrowser.addTab(url);
>+  let browser = tab.linkedBrowser;
>+
>+  browser.addEventListener("load", function onLoad() {
>+    browser.removeEventListener("load", onLoad, true);
>+    deferred.resolve({tab: tab, browser: browser});
>+  }, true);
>+
>+  return deferred.promise;
>+}

There's already promiseTabLoaded. This adds little value on top of that and the name is confusingly similar.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.4

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8441644 [details] [diff] [review]
Patch v1.4

>+  registerCleanupFunction(function() {
>+    while (gBrowser.tabs.length > 1)
>+      gBrowser.removeTab(gBrowser.tabs[gBrowser.tabs.length - 1]);

you could also just call gBrowser.removeCurrentTab in the loop

>+  is(gBrowser.selectedBrowser.currentURI.ref, "2", "The ref should be updated to the new ref");

gBrowser.currentURI is the same as gBrowser.selectedBrowser.currentURI

>+  let tabFound = window.switchToTabHavingURI(aURI, true, {ignoreFragment: aIgnoreFragment});

"window." is redundant

>+     (aIgnoreFragment ? "ignoring" : "including") + " reference portion");

s/reference/fragment/

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

https://tbpl.mozilla.org/?tree=Try&rev=165d806c7737

https://hg.mozilla.org/integration/fx-team/rev/748d9c5a8516

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/748d9c5a8516