1287642 - Open as dialog for Add-ons and Data Manager should no longer be associated with browser.link.open_external

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #1255354 +++

STR:

 1. Set suite.manager.{addons,dataman}.openAsDialog to "true"
 2. Add-ons and Data Managers open as dialog as expected
 3. Set suite.manager.{addons,dataman}.openAsDialog to "true"
 4. Add-ons and Data Managers open in a browser tab as expected
 5. Set browser.link.open_external to "2"
    that's Edit > Preferences > Browser > Link Behavior
           - Open links passed from other applications in:
             - A new window
 6a. Add-ons and Data Managers open in a separate window = NOT expected
 6b. Add-ons and Data Managers should open in a browser tab as expected

Opening the Add-ons and Data Managers should no longer depend on that setting after bug 1255354.

Comment 1

[rsx11m]

Best guess: Check for "about:data" or "about:addons" URIs here when setting the "where" variable,
https://dxr.mozilla.org/comm-central/source/suite/common/utilityOverlay.js#1680

Comment 2

[rsx11m]

Attachment: Easy fix

I made it an array, thus it's easy to extend if we want to uncouple further special pages from the browser.link.open_external pref later.

Comment 3

[Philip Chee]

Comment on attachment 8772637 [details] [diff] [review]
Easy fix

I think aOpenNew should ignore browser.link.open_external really.

Options:
(1) Completely remove the check for browser.link.open_external and just open tabfocused like Firefox.
(2) "about" pages are all internal (well ours are anyway. Firefox is weird).
> let where = aURI.schemeIs("about") || newWindowPref == kNewTab ? "tabfocused" : "window";

Your choice.

Comment 4

[rsx11m]

Attachment: Easier fix (v2)

Ok, I take it from your comment #3 that you prefer option (1) as the solution.
This obviously makes the code much easier with "tabfocus" being hard-wired.

I wasn't sure if switchToTabHavingURI() is called in a different context,
but all uses from suite/ are for "about:*" pages, thus should be safe.

I can't remove kNewTab as such given that it's needed elsewhere in this file.

We probably also want to have this on the branches, if you want to a+ those.

Comment 5

[Philip Chee]

Comment on attachment 8779535 [details] [diff] [review]
Easier fix (v2)

looks good. a=me CLOSED TREE if necessary

Comment 6

[rsx11m]

Thanks, pushed as http://hg.mozilla.org/comm-central/rev/000c9b1ff4ac

Comment 7

[rsx11m]

Comment on attachment 8779535 [details] [diff] [review]
Easier fix (v2)

Should be easy enough for the branches.

[Approval Request Comment]
Regression caused by (bug #): bug 1255354
User impact if declined: with non-default settings, open as tab may not work
Testing completed (on m-c, etc.): works on c-c
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none

Comment 8

[Ian Neal]

Comment on attachment 8779535 [details] [diff] [review]
Easier fix (v2)

a=me

Comment 9

[rsx11m]

https://hg.mozilla.org/releases/comm-aurora/rev/0451d5cddfa9
https://hg.mozilla.org/releases/comm-beta/rev/8342f65746bc
https://hg.mozilla.org/releases/comm-release/rev/8d830bcece41