489154 - Allow openTab for contentTab tab panels to switch to, rather than open, existing tabs

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Mark Banner (:standard8)]

Attachment: The fix

We now have a "contentTab" that we can use to display various content, e.g. about:rights, What's New etc. At the moment, if I want to open a panel, without opening a duplicate I have to:

- Search through existing panels to find the existing one.
- Call selectTabByIndex or openTab depending on the results.

It would be far easier if I could just call tabmail.openTab and let it deal with it for me (and give tab authors the decision on what to do).

The attached patch makes this easier by letting tabMode have a "shouldSwitchTo" function - I've then implemented this in the contentTab. This can be tested using the extension on bug 480533.

Comment 1

[Mark Banner (:standard8)]

Comment on attachment 373641 [details] [diff] [review]
The fix

Thinking about it, Phil's deep in autoconfig review, so let's leave him there a bit longer and share the load.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 373641 [details] [diff] [review]
The fix

>+      var selectedIndex;
>+      for (selectedIndex = 0; selectedIndex < tabInfo.length; ++selectedIndex) {

Move |var selectedIndex| (or let) into the for statement.

>+        // XXX this still needs to deal with the case where we have urls like:
>+        // about:rights and about:rights#webServices

Hmm, this got me thinking. Is it really worth having the suggested method? From a quick look it looks fairly complex to fix (even) such a small thing like the above xxx comment...

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> (From update of attachment 373641 [details] [diff] [review])
> >+      var selectedIndex;
> >+      for (selectedIndex = 0; selectedIndex < tabInfo.length; ++selectedIndex) {
> 
> Move |var selectedIndex| (or let) into the for statement.

Opps, I was going to use selectedIndex after the for statement, but ended up not doing that.

> >+        // XXX this still needs to deal with the case where we have urls like:
> >+        // about:rights and about:rights#webServices
> 
> Hmm, this got me thinking. Is it really worth having the suggested method? From
> a quick look it looks fairly complex to fix (even) such a small thing like the
> above xxx comment...

My thought was IOService.newURI() for each case, followed by probably a combination of nsIURL::param = ""... and then nsIURI::equals.

I think we do need something to check for opening duplicates, because once we hit the tab limit, we may need to increase the size of the tab limit, but we could still hit it.

We're not a browser, so I don't think it is sensible to re-use tabs for different pages.

At the same time, I also think we shouldn't be opening lots of tabs for the same thing, unless the user really wants it - so maybe we should be adding another parameter for the content tab to turn off the check for existing, so that extensions can play around more.

I think I want to avoid the check being on the caller side of openTab - that seems like we would have big potential for duplicates.

Comment 4

[Magnus Melin [:mkmelin]]

(In reply to comment #3)
> My thought was IOService.newURI() for each case, followed by probably a
> combination of nsIURL::param = ""... and then nsIURI::equals.

Sure, but then e.g. for the named anchor case, when you found out the urls are the same page - then that they are same but different would still be lost. So the page wouldn't focus on the named anchor anyways, we'd just select the tab it's in. Therefore I think this logic needs to be elsewhere/different.

Comment 5

[Mark Banner (:standard8)]

Attachment: The fix v2

This should be a better fix - it takes care of anchors (which I think are really the main thing we want to be careful of here) and ensures the page is loaded to the correct location.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 374448 [details] [diff] [review]
The fix v2

Looks better thx, r=mkmelin

Comment 7

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/eaae6740e85e