1132591 - Shortcut pinned tabs never become unpinned when navigating to a very different page

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Bug 937253 added support for pinning a tab to a specific origin URL, used to switch back to the tab if the homescreen shortcut is used again. This keeps Firefox from opening new tabs from homescreen shortcut launches.

If a tab becomes pinned like this, we should probably unpin it if the user navigates to a very different page, or maybe types a new URL to any page.

Thoughts on the trigger?
1. Any onLocationChange where the URL domain is not the same as the origin URL?
2. Any manually typed onLocationChange?

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Margaret has done similar work on Desktop with Pinned tabs.

Comment 2

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> Margaret has done similar work on Desktop with Pinned tabs.

The thing I did for desktop is just for link clicks, so it doesn't take into account changing the location from the location bar. We added logic in docshell for this because we wanted to catch this link click so that we could open a new tab before the location change happened (see bug 575561).

However, on Fennec, if we're cool with the location changing in the same tab, I would say we should just add something in our onLocationChange handler to check if the host changed, and if it did, unpin the tab.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: unpin WIP

This patch will "unpin" the Tab if the new domain is not the same as the pinned domain. Needs some cleanup and more testing, maybe some optimization too.

Comment 4

[:Margaret Leibovic]

We should fix this. I recently noticed that my homescreen bookmark will appear to stop working, because it will open a tab that I have navigated elsewhere.

Without an understanding of how this is implemented, that just makes Fennec seem busted.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: unpin v0.1

This patch adds the required cleanup to SessionStore to support removing a TabValue. It also resets the TabValue for the "appOrigin" if we navigate to a new location without the same domain as the "app" ,effectively unpinning the Tab. As noted in the comments, using the BACK button will not make the Tab pinned again.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8588152 [details] [diff] [review]
unpin v0.1

Review of attachment 8588152 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +4512,5 @@
> +      let originDomain = this._getDomainFromURL(originURL);
> +      if (originDomain != domain) {
> +        // Note: going 'back' will not make this tab pinned again
> +        ss.deleteTabValue(this, "appOrigin");
> +      }

I know it's an edge case, but what if a user navigates to a page that will fail to return a domain, such as an about: page. In that case, we wouldn't hit this logic. You might as well move this out of the if statement, since originalDomain != domain if the domain is null.

@@ +4559,5 @@
>  
> +  // For a given URL: http://www.example.com
> +  // return the domain: "www.example.com" or null if we can't get the domain
> +  _getDomainFromURL: function(url) {
> +    let parts = url.match(/^((?:[a-z]+:\/\/)?(?:[^\/]+@)?)(.+?)(?::\d+)?(?:\/|$)/);

Any reason not to create a new nsIURI and get the domain from that instead? It looks like this regex logic was just added for domain highlighting [1], and I suspect that's why we needed something more specific than just looking for the host. Although maybe we don't even need it for that?

I see that we do the same thing in aboutPasswords.js... if we change our mind here, we should change that as well.

[1] http://hg.mozilla.org/mozilla-central/rev/fa9887910dba

Comment 7

[:Margaret Leibovic]

Comment on attachment 8588152 [details] [diff] [review]
unpin v0.1

Review of attachment 8588152 [details] [diff] [review]:
-----------------------------------------------------------------

f+ for now until we discuss this regex issue.

Comment 8

[:Margaret Leibovic]

Maybe mcomella would be interested in pushing this across the finish line...

Comment 9

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #6)

> @@ +4559,5 @@
> >  
> > +  // For a given URL: http://www.example.com
> > +  // return the domain: "www.example.com" or null if we can't get the domain
> > +  _getDomainFromURL: function(url) {
> > +    let parts = url.match(/^((?:[a-z]+:\/\/)?(?:[^\/]+@)?)(.+?)(?::\d+)?(?:\/|$)/);
> 
> Any reason not to create a new nsIURI and get the domain from that instead?
> It looks like this regex logic was just added for domain highlighting [1],
> and I suspect that's why we needed something more specific than just looking
> for the host. Although maybe we don't even need it for that?
> 
> I see that we do the same thing in aboutPasswords.js... if we change our
> mind here, we should change that as well.
> 
> [1] http://hg.mozilla.org/mozilla-central/rev/fa9887910dba

Looking into this a bit more, I don't think we should use this regex for this. It's really designed for highlighting, not for functional host comparisons. For example, "http://bar.com" will return the same host as "file://bar.com", when the latter doesn't actually have a host. This is a silly example, but I think it would be more robust to just use nsIURI.host.

Comment 10

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1132591 - Unpin shortcut tabs when navigating to a new host. r=mcomella

Bug 1132591 - Unpin shortcut tabs when navigating to a new host. r=mcomella

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to :Margaret Leibovic from comment #9)
> For example, "http://bar.com" will return the same host as
> "file://bar.com", when the latter doesn't actually have a host.

To my understanding, file URIs skip over the host field - i.e. in your example, it should be `file:///bar.com`, where the third / skips the host and starts the path: `<scheme>://<host>/<path>`

But I agree - using URI parsing APIs sounds more robust.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8634835 [details]
MozReview Request: Bug 1132591 - Unpin shortcut tabs when navigating to a new host. r=mcomella

https://reviewboard.mozilla.org/r/13453/#review12335

Looks good though it'd be good to address the sub-domain issue.

::: mobile/android/components/SessionStore.js:1211
(Diff revision 1)
> -      throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG);
> +    }

Why do we no longer need to return this error code?

::: mobile/android/components/SessionStore.js:1210
(Diff revision 1)
> -    else
> +      this.saveStateDelayed();

This makes sense in my head: we change session state so we need to save the new session state. Is there anything more to it?

::: mobile/android/chrome/content/browser.js:4655
(Diff revision 1)
> +      if (originHost != aLocationURI.host) {

This will be true for sub-domains, clearing the pinned state. Is that intended?

I would prefer if this did not affect changed subdomains, just the main domain.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Michael Comella (:mcomella) from comment #12)
> Comment on attachment 8634835 [details]
> MozReview Request: Bug 1132591 - Unpin shortcut tabs when navigating to a
> new host. r=mcomella

I added the session store changes, so:

> ::: mobile/android/components/SessionStore.js:1211
> (Diff revision 1)
> > -      throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG);
> > +    }
> 
> Why do we no longer need to return this error code?

We don't want to always check for existence before calling this code to avoid an exception, and desktop doesn't throw either.

> ::: mobile/android/components/SessionStore.js:1210
> (Diff revision 1)
> > -    else
> > +      this.saveStateDelayed();
> 
> This makes sense in my head: we change session state so we need to save the
> new session state. Is there anything more to it?

Nope. That's it. We never called this method before, so it was buggy.

> 
> ::: mobile/android/chrome/content/browser.js:4655
> (Diff revision 1)
> > +      if (originHost != aLocationURI.host) {
> 
> This will be true for sub-domains, clearing the pinned state. Is that
> intended?
> 
> I would prefer if this did not affect changed subdomains, just the main

Keep in mind that m.twitter.com != twitter.com

I'd rather "unpin" if anything about the host is different.
> domain.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Mark Finkle (:mfinkle) from comment #13)
> > I would prefer if this did not affect changed subdomains, just the main
> 
> Keep in mind that m.twitter.com != twitter.com

But calendar.google.com ~= google.com/calendar

But to be fair, this automatically redirects so I'm not sure how one might have created a calendar.google.com homescreen shortcut. I'm pretty sure I've seen pages where each link opens a new subdomain with related content though I can't find any at the moment.

I'll defer to your judgment.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #14)
> I'm pretty sure I've seen pages where each link opens a new subdomain with
> related content though I can't find any at the moment.

Oh - Bugzilla!

reviewboard.mozilla.org from bugzilla.mozilla.org. Similarly, viewing attachments opens them in a new sub-domain. But then again, navigating to mozilla.org would be a completely different experience.

The majority case probably doesn't use subdomains for related content so perhaps you are right.

Comment 16

[:Margaret Leibovic]

url:        https://hg.mozilla.org/integration/fx-team/rev/09d779eb4be2c328c5d2b9c42e4b25a0d8f954ad
changeset:  09d779eb4be2c328c5d2b9c42e4b25a0d8f954ad
user:       Margaret Leibovic <[e-mail]>
date:       Thu Jul 16 11:01:03 2015 -0700
description:
Bug 1132591 - Unpin shortcut tabs when navigating to a new host. r=mcomella

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09d779eb4be2