Open Bug 678380 Opened 13 years ago Updated 2 years ago

Redirect loadURI calls to a new tab if the current tab is pinned

Categories

(Firefox :: Tabbed Browser, defect)

defect

Tracking

()

People

(Reporter: Margaret, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
Looking into the dependency tree for bug 579874, it seems like the only time we actually want to overwrite an app tab with a new URI is when the user navigates within the website with links clicks (except for external links, as fixed by bug 575561).

That being said, it seems like we want to avoid overwriting the app tab when the browser is the one loading a new URI. I made a patch that adds a pinned tab check to tabbrowser's loadURI and loadURIWithFlags method and loads a new tab if the current one is pinned, and this fixes bug 579873 and bug 598587.

I haven't audited all the places we call these methods, but like I said above, I can't really think of cases where we call these methods and actually want to load the new URI in a currently pinned tab.
Comment on attachment 552524 [details] [diff] [review]
patch

Does this seem like a good idea, or are there places that this would break things? Also, what kinds of tests would be needed here?
Attachment #552524 - Flags: feedback?(gavin.sharp)
Comment on attachment 552524 [details] [diff] [review]
patch

>       <method name="loadURI">
>         <parameter name="aURI"/>
>         <parameter name="aReferrerURI"/>
>         <parameter name="aCharset"/>
>         <body>
>           <![CDATA[
>+            // Avoid overwriting pinned tabs by loading the new URI in a new tab
>+            if (this.mCurrentTab.pinned)
>+              return this.loadOneTab(aURI, { referrerURI: aReferrerURI,
>+                                             charset: aCharset,
>+                                             inBackground: false });
>+
>             return this.mCurrentBrowser.loadURI(aURI, aReferrerURI, aCharset);
>           ]]>
>         </body>
>       </method>

loadURI isn't supposed to return a tab. (And the existing 'return' is bogus; it always returns undefined.)

Generally, I think this is going too far, unless you check the host before redirecting the load.
Attachment #552524 - Flags: feedback-
Comment on attachment 552524 [details] [diff] [review]
patch

Thinking about this more, this may not be as magical a fix as I had hoped. For example, if you load a URI from the location bar, this patch does not handle reverting the urlbar value in the app tab, so something would probably still need to be done to fix bug 598587. If that's the case, we might be better off doing spot fixes where these problems are cropping up.
Attachment #552524 - Flags: feedback?(gavin.sharp)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: