1315948 - Simplify openLinkIn in utilityOverlay.js

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P3)

Description

[:Gijs (he/him)]

The code here is complex and used by a lot of consumers. It would be great if we could simplify it. In particular, we're dealing with potentially a number of different windows, tabs and browsers that are hard to keep track of, making it bug-prone. In no particular order:

Windows:
1) the current toplevel window
2) its parent, if the toplevel is a popup window
3) the window from which a load was requested. This is almost always identical to (1) but there are edgecases where loads are async and therefore they are not the same (see also bug 1000458 which had this issue but with tabs).

Tabs and browsers (similar to above, but a little more complicated):
1) current selected tab/browser in toplevel window
2) same, but in the parent if we're a popup
3) the tab from which a load was requested (might no longer be selected)
4) new tab in which we're loading content (could be 1 or 2 or 3 or a new tab, which may or may not be selected immediately)


We should see if we can simplify the code so that it's more obvious which tabs/browsers we're manipulating and what role they're playing in the loading process. In particular, maybe we could rewrite it so that we distinguish the following tabs and windows clearly:

a) tab in which we're loading content (and its chrome window)
b) tab where the user / code requested the load (and its chrome window)

which are really the bits we care about. We should handle the popup case separately and then clarify (a) and (b) before doing anything else.

Comment 1

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #0)

> Windows:
> 1) the current toplevel window
> 2) its parent, if the toplevel is a popup window

It's really "the top-most window, unless we want to open a tab in which case we need the top-most window that isn't a popup window." The popup distinction happens early in one place and plays no role in the following control flow. I think your characterization makes things sound more complicated than they are.

> 3) the window from which a load was requested.

Plays no role in openLinkIn as far as I know. (Except implicitly through your currentBrowser parameter, which allows for overriding the window-detecting routine but again hopefully without changing the remaining control flow.)

> In particular, maybe we could rewrite it so that we
> distinguish the following tabs and windows clearly:
> 
> a) tab in which we're loading content (and its chrome window)
> b) tab where the user / code requested the load (and its chrome window)
> 
> which are really the bits we care about. We should handle the popup case
> separately

It's not clear to me why we should do that let alone how exactly.

Comment 2

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #1)
> (In reply to :Gijs Kruitbosch from comment #0)
> 
> > Windows:
> > 1) the current toplevel window
> > 2) its parent, if the toplevel is a popup window
> 
> It's really "the top-most window, unless we want to open a tab in which case
> we need the top-most window that isn't a popup window." The popup
> distinction happens early in one place and plays no role in the following
> control flow. I think your characterization makes things sound more
> complicated than they are.

Meh. It's a 200-line function. It would be easier to follow if we refactored it to use several smaller, more focused, functions / methods. Doesn't seem useful to argue over the specifics here.

> > 3) the window from which a load was requested.
> 
> Plays no role in openLinkIn as far as I know. (Except implicitly through
> your currentBrowser parameter, which allows for overriding the
> window-detecting routine but again hopefully without changing the remaining
> control flow.)

Which is wrong, really. If openLinkIn is called with "tab*" or "current" and the toplevel / most recent window has changed since the chain of events that led to openLinkIn being called had started (like what happened in bug 1000458 with the selected tab changing and "current" leading to the load happening in the wrong tab) then we will use a different window to open the link in.

> > In particular, maybe we could rewrite it so that we
> > distinguish the following tabs and windows clearly:
> > 
> > a) tab in which we're loading content (and its chrome window)
> > b) tab where the user / code requested the load (and its chrome window)
> > 
> > which are really the bits we care about. We should handle the popup case
> > separately
> 
> It's not clear to me why we should do that let alone how exactly.

A method that had a "source" window and "target" window (and corresponding tabs) that would handle just the nitty-gritty of "load link X in either the target tab ("current") or a new one (in the target window)" would be simpler to understand, and we could make openLinkIn call it after it's made the determination of which exact window/tab to use.

Alternatively, we could factor out the current/window/tab cases into mini-functions so there aren't 3 30-odd-line blocks with "prep stuff for this load call with a shedload of parameters" in the main function.

There's options. A lot of them are better than the current state.

Comment 3

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #2)
> > > Windows:
> > > 1) the current toplevel window
> > > 2) its parent, if the toplevel is a popup window
> > 
> > It's really "the top-most window, unless we want to open a tab in which case
> > we need the top-most window that isn't a popup window." The popup
> > distinction happens early in one place and plays no role in the following
> > control flow. I think your characterization makes things sound more
> > complicated than they are.
> 
> Meh. It's a 200-line function. It would be easier to follow if we refactored
> it to use several smaller, more focused, functions / methods. Doesn't seem
> useful to argue over the specifics here.

But the popup thing is like three lines. I'm not sure why you singled that out.

> > > 3) the window from which a load was requested.
> > 
> > Plays no role in openLinkIn as far as I know. (Except implicitly through
> > your currentBrowser parameter, which allows for overriding the
> > window-detecting routine but again hopefully without changing the remaining
> > control flow.)
> 
> Which is wrong, really. If openLinkIn is called with "tab*" or "current" and
> the toplevel / most recent window has changed since the chain of events that
> led to openLinkIn being called had started (like what happened in bug
> 1000458 with the selected tab changing and "current" leading to the load
> happening in the wrong tab) then we will use a different window to open the
> link in.

getTopWin actually just gives you the current window if that's a browser window. When called from a non-browser window, it's not really clear to me that we should use the window that used to be the front-most browser window rather than the current front-most window. I guess you could make an argument either way, but for the sake of not making this code more complicated, I'd suggest we ignore this edge case.

> > > In particular, maybe we could rewrite it so that we
> > > distinguish the following tabs and windows clearly:
> > > 
> > > a) tab in which we're loading content (and its chrome window)
> > > b) tab where the user / code requested the load (and its chrome window)
> > > 
> > > which are really the bits we care about. We should handle the popup case
> > > separately
> > 
> > It's not clear to me why we should do that let alone how exactly.
> 
> A method that had a "source" window and "target" window (and corresponding
> tabs) that would handle just the nitty-gritty of "load link X in either the
> target tab ("current") or a new one (in the target window)" would be simpler
> to understand, and we could make openLinkIn call it after it's made the
> determination of which exact window/tab to use.

Again, the source / target window distinction seems like a complication rather than a simplification.

> Alternatively, we could factor out the current/window/tab cases into
> mini-functions so there aren't 3 30-odd-line blocks with "prep stuff for
> this load call with a shedload of parameters" in the main function.
> 
> There's options. A lot of them are better than the current state.

Whatever we do here, if anything at all, note that utilityOverlay.js is loaded in a bunch of global scopes that we don't want to pollute further, i.e. it should only "export" things that are supposed to be called externally.

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch

Changes:

1. Renamed the currentBrowser parameter to targetBrowser as the former seems ambiguous. currentBrowser could mean "the browser to use when where=='current'" (this wasn't how I originally understood it but I think this was the intent?) or it could refer to gBrowser.mCurrentBrowser. Not a huge difference but slightly confusing, since the use case is passing a browser that might be different from mCurrentBrowser by the time openLinkIn runs.

2. Moved the window-detecting code to after the "save" case because it's not needed for that.

3. Made w be the window we're using also in the case params.targetBrowser is specified.

4. Moved the targetBrowser variable declaration down to where it's needed (after the "window" case). This allows for removing the w null-check.

5. Grouped the targetBrowser, loadInBackground and uriObj declarations more tightly since they're closely related and interdependent.

6. Reused the targetBrowser variable to replace browserUsedForLoad. The distinction didn't seem useful after renaming currentBrowser to targetBrowser.

7. Removed the browserUsedForLoad / targetBrowser null-check at the end because this variable should never be null there.

8. Improved the documentation where that seemed helpful to me.

Comment 5

[Dão Gottwald [:dao]]

Forgot to add: params.targetBrowser is now only honored when where == current. This is the only case where that parameter seems to make sense.

Comment 6

[:Gijs (he/him)]

Is there documentation about what values for "where" openLinkIn accepts? I don't see any next to the function, and AIUI there's no guarantee that it's limited to tab, tabshifted, current, window and save? Am I missing something? This is why I null-checked - if where is something other than those values, targetBrowser will be null.

Comment 7

[Dão Gottwald [:dao]]

The values we listed are the only supported ones. We could throw an exception when it's something else.

Comment 8

[Dão Gottwald [:dao]]

Well, since I removed the null-check it will fail anyway, but we could throw a /more helpful/ exception.

Comment 9

[Dão Gottwald [:dao]]

Err, so I don't depend depend on browserUsedForLoad.getTabBrowser() anymore, so actually right now it wouldn't fail, it would just silently do nothing as with the null-check.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8809774 [details] [diff] [review]
patch

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

Yeah, I think it would be worth doing something like:

if (!["save", "current", "window", "tab", "tabshifted"].includes(where)) {
  throw new Error(`Unrecognized 'where' parameter to openLinkIn: ${where}`);
}

at the beginning (or Cu.reportError and early return, if throwing seems too violent...).

Otherwise, I think this is definitely an improvement - thanks for tackling this.

One thing that I'm wondering if it makes sense to do (in a different bug) is using the |aParams.targetBrowser| also for determining 'relatedBrowser', further down in the tabbrowser code. It seems to me that if I'm using modifier-enter to open things in a new tab and then swap tab before the load runs, the result would be racy. Not sure if that's a problem in practice right now, depends on whether we ever pass aRelatedToCurrent in cases where the actual load is not done synchronously from a user action.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6f3dffe96d
Simplify openLinkIn. r=gijs

Comment 12

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #10)
> One thing that I'm wondering if it makes sense to do (in a different bug) is
> using the |aParams.targetBrowser| also for determining 'relatedBrowser',
> further down in the tabbrowser code. It seems to me that if I'm using
> modifier-enter to open things in a new tab and then swap tab before the load
> runs, the result would be racy. Not sure if that's a problem in practice
> right now, depends on whether we ever pass aRelatedToCurrent in cases where
> the actual load is not done synchronously from a user action.

Unfortunately relatedToCurrent and relatedBrowser are very different things and I'm not sure what you mean. Can you please file the bug?

Comment 13

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #12)
> (In reply to :Gijs Kruitbosch from comment #10)
> > One thing that I'm wondering if it makes sense to do (in a different bug) is
> > using the |aParams.targetBrowser| also for determining 'relatedBrowser',
> > further down in the tabbrowser code. It seems to me that if I'm using
> > modifier-enter to open things in a new tab and then swap tab before the load
> > runs, the result would be racy. Not sure if that's a problem in practice
> > right now, depends on whether we ever pass aRelatedToCurrent in cases where
> > the actual load is not done synchronously from a user action.
> 
> Unfortunately relatedToCurrent and relatedBrowser are very different things
> and I'm not sure what you mean. Can you please file the bug?

Filed bug 1317691.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ad6f3dffe96d