1475606 - Extend addTab to allow selecting a tab and remove the loadOneTab() API

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P3)

Description

[:Gijs (he/him)]

There are only a handful of loadOneTab() consumers, and most of the code in loadOneTab() is about passing along all the arguments to addTab(). Otherwise the two public APIs don't offer substantially different features to their consumers, so we should go from having 2 very similar APIs to having just 1.

Given addTab() has more consumers, we can extend it with the extra functionality offered by loadOneTab() and convert the consumers to call addTab() directly.

Comment 1

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1475606 - Removal of loadOneTab in favour of consolidating logic into addTab().

Comment 2

[Jonathan Kingston [:jkt] he/him]

Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=800637c8beca33c75de32f9c144c76cee39cbbb2

Comment 3

[Dão Gottwald [:dao]]

(In reply to Jonathan Kingston [:jkt] (Away until 20th Aug) from comment #1)
> Created attachment 8999953 [details]
> Bug 1475606 - Removal of loadOneTab in favour of consolidating logic into
> addTab().

This looks like it changes the behavior for current addTab calls...

Comment 4

[Jonathan Kingston [:jkt] he/him]

I think it will only impact existing loads if Services.prefs.getBoolPref("browser.tabs.loadInBackground") has been changed from it's default which is true. Maybe this is still too much change though, I will test but I should have highlighted certainly.
Thanks.

Comment 5

[Jonathan Kingston [:jkt] he/him]

Perhaps it makes more sense to have another boolean property 'selectTab' which controls if the tab should be selected.

Something like:

if (selectTab) {
  const bgLoad = (inBackground != null) ? inBackground :
    Services.prefs.getBoolPref("browser.tabs.loadInBackground");
  if (!bgLoad && ownerTab == null) {
    ownerTab = this.selectedTab;
  }
}
...
if (selectTab && !bgLoad) {
  this.selectedTab = t;
}

Comment 6

[Dão Gottwald [:dao]]

(In reply to Jonathan Kingston [:jkt] (Away until 20th Aug) from comment #5)
> Perhaps it makes more sense to have another boolean property 'selectTab'
> which controls if the tab should be selected.

I think that was the intent of this bug, yes.

> Something like:
> 
> if (selectTab) {
>   const bgLoad = (inBackground != null) ? inBackground :
>     Services.prefs.getBoolPref("browser.tabs.loadInBackground");
>   if (!bgLoad && ownerTab == null) {
>     ownerTab = this.selectedTab;
>   }
> }
> ...
> if (selectTab && !bgLoad) {
>   this.selectedTab = t;
> }

{ selectTab: true, inBackground: true } or { selectTab: false, inBackground: false } would be confusing. I think we should replace inBackground with select, and also provide a way to identify content links, because that's when the pref is supposed to be used ("When you open a link in a new tab, switch to it immediately").

Comment 7

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #6)
> I think we should replace inBackground with
> select, and also provide a way to identify content links, because that's
> when the pref is supposed to be used ("When you open a link in a new tab,
> switch to it immediately").

Or maybe we could have only "select" and read the pref on the call site that's responsible for content links.

Comment 8

[Jonathan Kingston [:jkt] he/him]

All the current callsites minus tests pass in an explicit loadInBackground property meaning this code is redundant anyway and likely unexpected. If we needed to refactor to make it work again I sugest this is done after this work as that is a functionality change.

I don't this was the case when I first looked at this code, eitherway we should take the chance to simplify for now.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:dao, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 10

[:Gijs (he/him)]

Attachment: Bug 1475606 - remove separate logic for tabbrowser's loadOneTab(), r?dao

This used to do three things:

1. determine whether we're background loading
2. determine a default owner tab
3. force allowInheritPrincipal to a bool value

For the background loading, I removed the default pref check and moved the
selection action into addTab. I defaulted it to true, so that existing addTab
callers continue to open background tabs. I removed the code checking the pref:
all existing loadOneTab callsites pass an explicit value and where appropriate
check the pref themselves (sometimes doing the opposite in response to shift
keypresses etc.).
The one exception is browser_bug597218.js, but there the default of true gets
used right now, which is still the same post-patch.

For the owner tab, I moved setting the default into addTab. This shouldn't
affect existing addTab callers as they don't pass inBackground anyway, so we
default inBackground to true, in which case it just assigns null, only when
ownerTab wasn't provided in the first place.

For allowInheritPrincipal, the only reads of this in addTab use boolean negation
anyway so forcing its type to bool first made no difference.

Comment 11

[:Gijs (he/him)]

Attachment: Bug 1475606 - remove loadOneTab and switch its callers over, r?dao

Depends on D165774

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2efc919bab1
remove separate logic for tabbrowser's loadOneTab(), r=dao
https://hg.mozilla.org/integration/autoland/rev/6ba37ceb1599
remove loadOneTab and switch its callers over, r=dao,perftest-reviewers,sparky

Comment 13

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/b2efc919bab1
https://hg.mozilla.org/mozilla-central/rev/6ba37ceb1599