262575 - "Visit Homepage" and "Get More Extensions/Themes" in Extension and Theme manager should respect tabbed browsing preferences

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Ger Teunis]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041001 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041001 Firefox/0.10

Theme manager and Extension manager open a new window for "Visit Homepage" even
when "Force links that open new windows" is selected and set to "a new tab" in
Tools -> Options -> Advanced Options -> Tabbed Browsing.

Reproducible: Always
Steps to Reproduce:
1. Go to Tools -> Options -> Advanced Options -> Tabbed Browsing
2. Select "Force links that open new windows" and set to "a new tab"
3. Go to Extensions manager, click an extension and click Visit Homepage.

Actual Results:  
A new window with the extension's homepage is loaded

Expected Results:  
A new tab in current browser with the Extensions homepage.

Comment 1

[Ger Teunis]

Forgot to mention:
"Get more themes" and "Get more extensions" links in the theme/extension
managers also don't use the Tabbed Browsing preferences.

Comment 2

[Anbo Motohiko]

Attachment: patch

This patch looks pref "browser.link.open_newwindow.ui", not
"browser.link.open_newwindow". Firefox does not have UI for this pref.

Comment 3

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #2)
> This patch looks pref "browser.link.open_newwindow.ui", not
> "browser.link.open_newwindow". Firefox does not have UI for this pref.

browser.link.open_newwindow is the correct pref; the other is a companion pref
used to work around quirks in the way the UI configuration code was written.

(At least, that's how I understood the attachments in the bug)

Comment 4

[José Jeria]

Will this patch also fix bug 262685? That might even be a dupe?

Comment 5

[José Jeria]

*** Bug 262685 has been marked as a duplicate of this bug. ***

Comment 6

[Francois Ingelrest]

I reported some time ago this one :
https://bugzilla.mozilla.org/show_bug.cgi?id=251181 which max marked as WONTFIX...

Comment 7

[Hemiola SUN]

(In reply to comment #6)
> I reported some time ago this one :
> https://bugzilla.mozilla.org/show_bug.cgi?id=251181 which max marked as WONTFIX...

it is different. for bug#251181, we didn't have the tab browsing option so "get
more extensions/themes" should open in a new window by default.

but now we have already the tab browsing, and i think it is necessary to respect
the preferences.

Comment 8

[Ger Teunis]

I also think the patch has to use the browser.link.open_newwindow
Is more consistent and expected behavior to the Tabbed Browsing settings.

Comment 9

[Anbo Motohiko]

This patch does not care about 'link modifier' (ctrl+click to open in tab,
shift+click to open in new window, etc), is it right?

Comment 10

[Ryan Polk (Quark)]

Some general comments about the patch:
has the if be on a single line, ie. "if(window.opener &&
window.opener.gBrowser)".  Boolean short-circuit should take care of you.

Follow the file syntax, i.e.
if() {
  blah {
    code
  }
}

>  case 3: // open in new tab
>    var tab = window.opener.gBrowser.addTab(aURL);
>    if(!pref.getBoolPref(PREF_BROWSER_LOADINBACKGROUND))
>      window.opener.gBrowser.selectedTab = tab;
>    return;
Shouldn't that be 'break;'?

I've seen preferences being grabbed within a try{} catch(), but I'm not sure if
that's needed or not.

Comment 11

[Anbo Motohiko]

(In reply to comment #10)
> Some general comments about the patch:
> has the if be on a single line, ie. "if(window.opener &&
> window.opener.gBrowser)".  Boolean short-circuit should take care of you.
Thank you. I'm confused about expression evaluating order.


> >  case 3: // open in new tab
> >    var tab = window.opener.gBrowser.addTab(aURL);
> >    if(!pref.getBoolPref(PREF_BROWSER_LOADINBACKGROUND))
> >      window.opener.gBrowser.selectedTab = tab;
> >    return;
> Shouldn't that be 'break;'?
You say "'break;' after 'return;'"?
... oh, sorry. The previous 'case' is incorrect thing. I'll update it.


> I've seen preferences being grabbed within a try{} catch(), but I'm not sure if
> that's needed or not.
In this case, try~catch clauses are not needed. An error will be thrown if there
is no pref value, but default values are defined in all.js.

Comment 12

[Anbo Motohiko]

Attachment: updated patch

This patch still ignores link modifiers. Without it, this patch works well for
me.

Comment 13

[Ger Teunis]

My opinion is that the 'link modifiers' can be ignored on UI links.

Comment 14

[Anbo Motohiko]

Attachment: ugly hack

This patch uses whereToOpenLink() and openUILinkIn() function in
utilityOverlay.js.

When clicking mouse left button without modifiers, the link will open like
target="_blank" (pref 'browser.link.open_newwindow').
Pressing middle button opens in new tab, and link modifiers work (e.g.
shift+click to new window).

Comment 15

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #14)
> Created an attachment (id=161465)
> ugly hack
> 
> This patch uses whereToOpenLink() and openUILinkIn() function in
> utilityOverlay.js.

Ugly hack indeed! I'm not sure if modifying gDoCommand() for the sake of a
single command is necessarily a Good Thing.

IMO openURL() would be better if it looked similar to this (taken from TBP):

function TBP_openURL(aURL)

{

   try {

       var targetPref = gPref.getIntPref("browser.link.open_newwindow");

   }

   catch (e) {}

   if (aURL == null) aURL = "about:blank";



   // check for an existing window and focus it; it's not application modal

   var anyWindow = WindowManager.getMostRecentWindow("navigator:browser");


   if (anyWindow) {

       // get the charset of anyWindow, if it exists
       // XXX: is this even necessary?

       var charset;

       if (anyWindow.getBrowser() && anyWindow.getBrowser().characterSet) {

           charset = anyWindow.getBrowser().characterSet;

       }


       switch (targetPref) {

       	   case 2 : {

               anyWindow.openNewWindowWith(aURL, null, true, null);

               break;

       	   }

       	   case 3 : {

               var newTab = anyWindow.getBrowser().addTab(aURL, null, charset);

               anyWindow.getBrowser().selectedTab = newTab;

               break;

       	   }

       	   case 1 : {

               anyWindow.loadURI(aURL, null, originCharset);

               break;

       	   }

       }

   }

   else {

       // open browser.xul anyway
       anyWindow.openNewWindowWith(aURL, null, true, null);

   }

   return;

}

This way the user can use the existing tabbed UI prefs to choose how they want
links from the EM and TM to be opened, without modifying any other part of
extensions.xul or extensions.js.

Comment 16

[Anbo Motohiko]

(In reply to comment #15)
> IMO openURL() would be better if it looked similar to this (taken from TBP):
> 
(snip)
> 
> This way the user can use the existing tabbed UI prefs to choose how they want
> links from the EM and TM to be opened, without modifying any other part of
> extensions.xul or extensions.js.
Yes, the code you quoted is similar to attatchment 161384.

By this code, you cannot use any link modifiers and/or middle mouse button. Are
you sure?

Comment 17

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Attachment: teach openURL() about browser.link.open_newwindow

This patch is based upon the function I just posted and teaches openURL() to
honour the new pref, browser.link.open_newwindow. I have used similar code in
TBP for some time.

I also taught openURL() about browser.tabs.loadInBackground; however, ben has
given review+ and approval-aviary+ to bug 262537 attachment 161279 [details] [diff] [review], which means
that PREF_BROWSER_BACKGRND_OPEN may need to be changed to use
browser.tabs.loadDivertedInBackground once the former patch is checked in, and
if this patch is selected.

Comment 18

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #16)
> Yes, the code you quoted is similar to attatchment 161384.
> 
> By this code, you cannot use any link modifiers and/or middle mouse button. Are
> you sure?

I wish I had seen this before posting my patch...

Anyway, to answer your question, do you think anyone is likely to mod+click a
link in the Extensions or Theme Managers? Middle-click is a possibility, but
mod+click sounds very unlikely to me - I have personally never done it, and no
one has asked me to add it to TBP in the past.

Comment 19

[Anbo Motohiko]

Comment on attachment 161465 [details] [diff] [review]
ugly hack

(In reply to comment #18)
Sorry, I misunderstood comment #13.

Comment 20

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Comment on attachment 161467 [details] [diff] [review]
teach openURL() about browser.link.open_newwindow

Ben has checked in bug 262537 attachment 161279 [details] [diff] [review], so this patch is obsolete now.
New patch coming up...

Comment 21

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Attachment: teach openURL() about browser.link.open_newwindow (alternate)

Same as before, only openURL() uses browser.tabs.loadDivertedInBackground for
tab selection.

Comment 22

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Please note that attachment 161476 [details] [diff] [review] is just for proof-of-concept; it was made
against a compiled build and not against a source tree.

Comment 23

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Comment on attachment 161476 [details] [diff] [review]
teach openURL() about browser.link.open_newwindow  (alternate)

Obsoleted; real patch against latest Aviary snapshot coming up.

Comment 24

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Attachment: teach openURL() about browser.link.open_newwindow (final)

This is the final version of my idea, diff'ed with -urNp against the 0.10.1
extensions.js. The Mozilla download page makes references to daily snapshots
but I was unable to find them.

Comment 25

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Comment on attachment 162067 [details] [diff] [review]
teach openURL() about browser.link.open_newwindow (final)

Tentatively requesting review? from Ben.

http://www.mozilla.org/owners.html is not communicative on the precise
ownership of the EM/TM.

Comment 26

[Asaf Romano (gone)]

Comment on attachment 162067 [details] [diff] [review]
teach openURL() about browser.link.open_newwindow (final)


>+  var gPref = Components.classes[kPrefServiceID]
>+                        .getService(nsIPrefBranch);
>+  var gMediator	= Components.classes[kAppShellMediatorID]

Never prefix locals with 'g'.

Comment 27

[Steffen Wilberg]

> This is the final version of my idea, diff'ed with -urNp against the 0.10.1
> extensions.js. The Mozilla download page makes references to daily snapshots
> but I was unable to find them.
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-0.9/

Even better: Get the source code and build yourself:
http://www.mozilla.org/cvs.html
http://www.mozilla.org/projects/firefox/build.html

Comment 28

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Comment on attachment 162067 [details] [diff] [review]
teach openURL() about browser.link.open_newwindow (final)

Canceling review and obsoleting. New patch coming up.

Comment 29

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Attachment: fixed patch

Fixed variable naming nits, as well as a silly bug.

Steffen: I don't have CVS access. My reference to snapshots was to _source_
snapshots.

Comment 30

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Comment on attachment 162072 [details] [diff] [review]
fixed patch

Re-requesting review? from Ben.

Comment 31

[Anbo Motohiko]

> +    // 1: open in currently selected tab, 2: open in new window, 3: open in
new tab
> +    switch (targetPref) {
> +      case 1 : {
> +        anyWindow.loadURI(aURL, null, originCharset);
> +        break;
> +      }
> +      case 2 : {
> +        anyWindow.openDialog("chrome://browser/content/browser.xul",
"_blank", "chrome,all,dialog=no", aURL, null, null);
> +        break;
> +      }
> +      case 3 : {
> +        var newTab = anyWindow.getBrowser().addTab(aURL, null, charset);
> +        if (!backgroundPref) anyWindow.getBrowser().selectedTab = newTab;
> +        break;
> +      }
> +    }

I think 'default' is needed in this switch~case clause.

See also:
http://lxr.mozilla.org/aviarybranch/source/browser/base/content/browser.js#3671

Comment 32

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #31)
> I think 'default' is needed in this switch~case clause.
> 
> See also:
> http://lxr.mozilla.org/aviarybranch/source/browser/base/content/browser.js#3671

Hmmmm. Are you saying that the case 2 block should also be able to be executed
by default? Or just not bother to check for targetPref = 2?

Comment 33

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Attachment: patch incorporating amotohiko's suggestion

amotohiko, is this what you had in mind?

Comment 34

[Anbo Motohiko]

(In reply to comment #33)
> Created an attachment (id=162187)
> patch incorporating amotohiko's suggestion
> 
> amotohiko, is this what you had in mind?

Sure. My opinion is the former one. The pref takes an integer value (because
Mozilla does not support 3-state pref), so we need to think about fool proof...

Comment 35

[Nickolay_Ponomarev]

*** Bug 266305 has been marked as a duplicate of this bug. ***

Comment 36

[José Jeria]

*** Bug 266467 has been marked as a duplicate of this bug. ***

Comment 37

[Phil Ringnalda (:philor)]

*** Bug 266671 has been marked as a duplicate of this bug. ***

Comment 38

[Phil Ringnalda (:philor)]

*** Bug 267380 has been marked as a duplicate of this bug. ***

Comment 39

[Phil Ringnalda (:philor)]

*** Bug 268737 has been marked as a duplicate of this bug. ***

Comment 40

[Mike Connor [:mconnor]]

Comment on attachment 162072 [details] [diff] [review]
fixed patch

adding more app-specific stuff to toolkit isn't okay.  We need a better
solution for openURL than we have currently, hopefully that will feed into
something we can control from /browser.

Comment 41

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #40)
> (From update of attachment 162072 [details] [diff] [review] [edit])
> adding more app-specific stuff to toolkit isn't okay.  We need a better
> solution for openURL than we have currently, hopefully that will feed into
> something we can control from /browser.

What code should I look at to implement such a solution?

Comment 42

[José Jeria]

*** Bug 283835 has been marked as a duplicate of this bug. ***

Comment 43

[Anbo Motohiko]

Attachment: patch based on attachment 162072

This patch uses openURI method of browser window.
This behaves as URI from 'external app', not 'internal link'.

How about this one?

Comment 44

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #43)
> Created an attachment (id=175711) [edit]
> patch based on attachment 162072 [details] [diff] [review] [edit]
> 
> This patch uses openURI method of browser window.
> This behaves as URI from 'external app', not 'internal link'.
> 
> How about this one?

FWIW this looks much better, albeit slightly more complex.

Comment 45

[Anbo Motohiko]

Attachment: revised patch

Comment 46

[Asaf Romano (gone)]

Comment on attachment 177004 [details] [diff] [review]
revised patch

don't expect this to be reviewed without asking a particular browser/toolkit
peer.

Comment 47

[Chris Dolan]

(In reply to comment #45)
> Created an attachment (id=177004) [edit]
> revised patch
> 

I imagine there should be some error checking in the "if (anyWindow){...}"
clause.   Otherwise, there could be a silent failure to open the url (just an
error message in the JS console).  It should probably fall back to the old
behavior in case of failures.  However, I don't really know much about the code
involved.

Comment 48

[Steve England [:stevee]]

*** Bug 288703 has been marked as a duplicate of this bug. ***

Comment 49

[Mike Connor [:mconnor]]

Comment on attachment 177004 [details] [diff] [review]
revised patch

This doesn't hurt us in the app-specific ifdef world, its still something that
needs to go, but in the meantime, lets at least do the right thing.

Comment 50

[Anbo Motohiko]

(In reply to comment #49)
Thank you for reviewing.


There are several related bugs. I know Bug 262808 (for Help window) and Bug
263433 (for Update dialog). Share this function, fixing these bugs will be easy.

I think this 'openURL()' function should be moved to another file, like
'contentAreaUtils.js'. And renaming is needed to avoid confusion. There are
functions named 'openNewTabWith' and 'openNewWindowWith'. I think
'openURIfromDialog' is good (not 'URL').


How about this?

Comment 51

[Asa Dotzler [:asa]]

Comment on attachment 177004 [details] [diff] [review]
revised patch

we're trying to wrap up alpha1. moving request out to alpha2.

Comment 52

[Worcester12345]

(In reply to comment #51)
> (From update of attachment 177004 [details] [diff] [review] [edit])
> we're trying to wrap up alpha1. moving request out to alpha2.
> 

Should there be new milestone and/or blocking flags for this? Thanks.

Comment 53

[Anbo Motohiko]

Attachment: Another patch.

This patch also fixes Bug 262808 and Bug 263433 (maybe. I cannot check update
dialog's link).

- move 'openURL' function to globalOverlay.js and rename to 'openFromDialog'.
- remove 'visitLink' function since this is no longer used from anywhere.
- add 'contentAreaClick' function to help.js (this is a simple version of
browser.js's one) and call it from browser of help viewer.
- remove XBL binding of 'link' element from updates.xml, and add event handlers
to handle opening link to update.xul.

Comment 54

[Steffen Wilberg]

Looks like you need XPCNativeWrapper anymore :)
http://lxr.mozilla.org/seamonkey/source/toolkit/content/XPCNativeWrapper.js#39
http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#4600

Comment 55

[Anbo Motohiko]

Attachment: another patch, revised

(In reply to comment #54)
> Looks like you need XPCNativeWrapper anymore :)
Thank you.
I update patch and request for review.

openFromDialog() function checks whether HTTP/HTTPS schemes is exposed or not.
Non-browser apps (at least Thunderbird and Nvu) don't expose such schemes, I
think this method can be used to detect app is browser or not.
Is this correct? Or I should change same method as attachment 177004 [details] [diff] [review]?

Comment 56

[Worcester12345]

Rerequest blocking since there is now a patch. Thanks.

Comment 57

[Mike Shaver (:shaver emeritus)]

Comment on attachment 177004 [details] [diff] [review]
revised patch

a=shaver.

Comment 58

[Justin Kerk]

Was it intentional that the patch that was approved is not the newest one?

Comment 59

[Grayson Mixon (ColdFusion650)]

im not sure. but in the id: 2005060106 build, it doesnt work. the patch was
checked in at 5:06, so the fix should be in 6 oclock build. could be because of
the xpcnativewrapper thing in comment 54.

Comment 60

[Mike Connor [:mconnor]]

Comment on attachment 184796 [details] [diff] [review]
another patch, revised

>+function openFromDialog(href)
>+{

this isn't even the style of the function you removed.	When in Rome generally
applies, especially with widely-used conventions like this.

>+  if (protocolSvc.isExposedProtocol(uri.scheme)) {
>+    var mediator = Components.classes["@mozilla.org/appshell/window-mediator;1"]

what if this is a mailto: link? this isn't quite right.

>+    // locate the most recently used window
>+    var anyWindow = mediator.getMostRecentWindow("navigator:browser");

navigator:browser may or may not be what we're looking for here.  Yes this
works for Firefox, but this breaks a mailto: link in tbird.  And if this is a
mostly-http function its poorly named, but we want a generic solution here. 
Copy/pasting darin's comments here for an idea of a potential solution.

<darin> it seems to me that we need something like ExternalProtocolService, but
a version that doesn't talk to the OS if the app can support the protocol
<darin> and if the app can support the protocol it should treat it like a URL
that entered via ShellExecute (i.e., on the command line)
<darin> app.loadUrl(foo)

>+    else {
>+      // original behaviour; browser dependent.
>+      openDialog("chrome://browser/content/browser.xul", "_blank", "chrome,all,dialog=no", href, null, null);
>+    }
>+  }

so if they're _not_ using navigator:browser, we force them to use that URL for
their browser chrome? not ok either.  The extraneous extra brackets aren't
needed.

>+  else {
>+    // non-browser
>+    protocolSvc.loadUrl(uri);
>+  }

it'd be better to do if (!exposedprotocol) loadURI(uri) and return right up
front.	Note that this should be loadURI, and should pass in an nsIPrompt, per
darin.

>@@ -155,7 +156,9 @@
>         <label id="found.app.features" hidden="true"/>
>         <vbox id="found.app.featuresList"/>
>         <separator class="thin"/>        
>-        <link id="found.app.infoLink" label="&found.app.infoLink;"/>
>+        <link id="found.app.infoLink" label="&found.app.infoLink;"
>+              onclick="if (event.button == 0) openFromDialog(this.getAttribute('href'));"
>+              onkeypress="if (event.keyCode == event.DOM_VK_ENTER || event.keyCode == event.DOM_VK_RETURN) openFromDialog(this.getAttribute('href'));"/>

if you remove the link binding, doesn't this break?

Comment 61

[Steve England [:stevee]]

*** Bug 297110 has been marked as a duplicate of this bug. ***

Comment 62

[Anbo Motohiko]

Thank you for reviewing.

(In reply to comment #60)
> (From update of attachment 184796 [details] [diff] [review] [edit])
> >+function openFromDialog(href)
> >+{
> 
> this isn't even the style of the function you removed.	When in Rome generally
> applies, especially with widely-used conventions like this.
Only 'visitURL' function is different from all of others.
http://lxr.mozilla.org/mozilla/source/toolkit/content/globalOverlay.js
Should I change the style?


> <darin> it seems to me that we need something like ExternalProtocolService, but
> a version that doesn't talk to the OS if the app can support the protocol
> <darin> and if the app can support the protocol it should treat it like a URL
> that entered via ShellExecute (i.e., on the command line)
> <darin> app.loadUrl(foo)
I agree with that.
Add 'openURI' method to nsICommandLineHandler? Or create new interface to handle
this?


> >+  else {
> >+    // non-browser
> >+    protocolSvc.loadUrl(uri);
> >+  }
> 
> it'd be better to do if (!exposedprotocol) loadURI(uri) and return right up
> front.	Note that this should be loadURI, and should pass in an nsIPrompt, per
> darin.
OK.


> >@@ -155,7 +156,9 @@
> >         <label id="found.app.features" hidden="true"/>
> >         <vbox id="found.app.featuresList"/>
> >         <separator class="thin"/>        
> >-        <link id="found.app.infoLink" label="&found.app.infoLink;"/>
> >+        <link id="found.app.infoLink" label="&found.app.infoLink;"
> >+              onclick="if (event.button == 0)
openFromDialog(this.getAttribute('href'));"
> >+              onkeypress="if (event.keyCode == event.DOM_VK_ENTER ||
event.keyCode == event.DOM_VK_RETURN) openFromDialog(this.getAttribute('href'));"/>
> 
> if you remove the link binding, doesn't this break?
I don't think so. There are no binding to link element in extensions.xml and
link works.
http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/content/extensions.xml

Comment 63

[Mike Connor [:mconnor]]

Sorry I never saw the reply here, I don't know if the right fix can be done in
the right timeframe.  Style should be left along unless you're touching the
function, and if it doesn't break, ok.

This isn't a blocker though, and maybe we should wait on 1.9 before we attempt
the "right" fix here, this bug is not really that serious.

Comment 64

[Anbo Motohiko]

Attachment: reviced patch; contains patches for reporter

- In openFromDialog() function, add support for mailto: protocol for internal
use. news: protocol seems to work, but always open new Thunderbird window.
- Add patches for reporter.
- Updated for new update service and help viewer.

In openFromDialog() function, handlers for schemes are hard-coded. This is not
good, and should be fixed by adding new generic protocol handler.


Comments are welcomed. Thank you.

Comment 65

[Grayson Mixon (ColdFusion650)]

(In reply to comment #63)
>maybe we should wait on 1.9 before we attempt the "right" fix here

not that we've branched and the 1.9a1 trunk is open, are we going to try to
attempt the "right" fix? just trying to feel things out.

Comment 66

[Dave Townsend [:mossop]]

*** Bug 306829 has been marked as a duplicate of this bug. ***

Comment 67

[Tuukka Tolvanen (sp3000)]

*** Bug 307190 has been marked as a duplicate of this bug. ***

Comment 68

[Phil Ringnalda (:philor)]

*** Bug 309519 has been marked as a duplicate of this bug. ***

Comment 69

[Grayson Mixon (ColdFusion650)]

ive tried applying the patch on trunk and it seems to have bit rotted.

Comment 70

[Worcester12345]

This is really annoying with all the new incompatibilities and updated extensions, especially since most don't update UMO any more, or at least right away. Is there and easier way (extension) to accomplish this?

I use "Duplicate Tab" extension, which has a "merge windows" feature; so I just open a blank tab, then merge windows which at least grabs all of them and consolidates to one, with a second window remaining open, that being the actual extensions window (there might be another bug to open that in the regular windows as well). This is still too many steps to accomplish what the browser claims to be one of its advantages to real world users. Any other workarounds? Thanks.

Comment 71

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #69)
> ive tried applying the patch on trunk and it seems to have bit rotted.

Indeed. The lack of movement on this bug implies that the approval+ granted by shaver was not followed through on. Will this bug be fixed for 1.5 as part of the rc1-final cleanups or will it be deferred for a 1.5 maintenance release?

Comment 72

[Scott MacGregor]

it's too late for this to make 1.5. Leaving the 2.0 nomination alone for the next train.

Comment 73

[Mike Connor [:mconnor]]

This is a nice-to-have, not a blocker.

Comment 74

[Worcester12345]

(In reply to comment #73)
> This is a nice-to-have, not a blocker.
> 

We're talking version 2 here, right, which is quite a ways away, not version 1.5. Correct? 

Comment 75

[:Gavin Sharp [email: [e-mail]]]

*** Bug 318258 has been marked as a duplicate of this bug. ***

Comment 76

[Anbo Motohiko]

Attachment: another try

This 'patch' contains patches for extension/theme manager, help viewer and text-link widget. The text-link widget is used in 'get more extensions/themes' link in EM, 'privacy policy' link in report wizard and 'details' link in software update wizard.

This 'patch' isn't tested because I don't have build environment, so I don't ask review.
Thank you.

Comment 77

[Grayson Mixon (ColdFusion650)]

since there is no c++ in the patch, there is no neeed to have a build environment. i just modified my firefox files directory, as i have on several other occassions. anyway, it could have been because i made a mistake applying the patch, but when i click "get more extensions/themes" or "visit homepage" nothing happens. but i don't get any errors. i have it show chrome errors in console and javascript in strict mode.

Comment 78

[Anbo Motohiko]

Comment on attachment 205297 [details] [diff] [review]
another try

(In reply to comment #77)
> anyway, it could have been because i made a mistake applying
> the patch, but when i click "get more extensions/themes" or "visit homepage"
> nothing happens. but i don't get any errors. i have it show chrome errors in
> console and javascript in strict mode.
Actually. One semicolon is not needed.


> since there is no c++ in the patch, there is no neeed to have a build
> environment.
Yes, I write the patch on this way.
Hmm... I think review requestor should test their patches. I don't want to waste developer's time.

Comment 79

[Anbo Motohiko]

Attachment: another try, round 2

This is mostly same as attachment 205297 [details] [diff] [review].
Sorry for bug spams.

Comment 80

[Simon Bünzli]

(In reply to comment #79)
> Created an attachment (id=206172) [edit]

Some details:
* You can get chromeURL from the pref browser.chromeURL which exists for Firefox and SeaMonkey (when they migrate to Toolkit).
* There's no need to get chromeURL if you managed to open the URI in an existing browser window.
* Instead of using loadSubScript directly, first check whether openURI is already present and use that; if it's not, add an object as a second argument to loadSubScript and get openURI from that object:

var context = {};
loader.loadSubScript("chrome://global/content/globalOverlay.js", context);
context.openURI(href);

That way, you won't unnecessary pollute the namespace.

Finally I'd like to support the fact that you consider links from Chrome as external rather than internal links. Since some users might want to ignore target attributes in websites (and set browser.link.open_newwindow to 1) but still won't be aware that they just "lost" a website through clicking a Chrome link (which is why browser.link.open_external now defaults to 3 which IMHO makes sense, since there is no obvious "current" tab in this context). Even worse, you can force website links to another tab by choice while keyboard modifiers don't work on Chrome links.

Comment 81

[Anbo Motohiko]

Thank you for replying.

> * You can get chromeURL from the pref browser.chromeURL which exists for
> Firefox and SeaMonkey (when they migrate to Toolkit).
And Thunderbird (for Mac OS X).
http://lxr.mozilla.org/mozilla/source/mail/app/profile/all-thunderbird.js#43
I think I cannot trust this pref.

> * There's no need to get chromeURL if you managed to open the URI in an
> existing browser window.
OK.

> * Instead of using loadSubScript directly, first check whether openURI is
> already present and use that; if it's not, add an object as a second argument
> to loadSubScript and get openURI from that object:
> 
> var context = {};
> loader.loadSubScript("chrome://global/content/globalOverlay.js", context);
> context.openURI(href);
> 
> That way, you won't unnecessary pollute the namespace.
Thank you for your information.


> Finally I'd like to support the fact that you consider links from Chrome as
> external rather than internal links. Since some users might want to ignore
> target attributes in websites (and set browser.link.open_newwindow to 1) but
> still won't be aware that they just "lost" a website through clicking a Chrome
> link (which is why browser.link.open_external now defaults to 3 which IMHO
> makes sense, since there is no obvious "current" tab in this context). Even
> worse, you can force website links to another tab by choice while keyboard
> modifiers don't work on Chrome links.
I think current code is wrong one when they open XUL content in a tab. In this case chrome links (xul:text-link widget) should open as the regular link.

Comment 82

[Dave Townsend [:mossop]]

*** Bug 340962 has been marked as a duplicate of this bug. ***

Comment 83

[Robert Strong (they/them - no direct email)]

*** Bug 342269 has been marked as a duplicate of this bug. ***

Comment 84

[Phil Ringnalda (:philor)]

*** Bug 343632 has been marked as a duplicate of this bug. ***

Comment 85

[Michael Wu [:mwu]]

Attachment: Quick fix for addons manager

This is a quick, clean addons manager only fix.

Comment 86

[Phil Ringnalda (:philor)]

*** Bug 344382 has been marked as a duplicate of this bug. ***

Comment 87

[Michael Wu [:mwu]]

Attachment: Quick, complete fix for addons manager

Same thing as before, except with all things in the extensions/content directory. I removed the link xbl binding from update.xml since nothing seems to use it AFAICT, so there is no need to fix that code.

Comment 88

[Robert Strong (they/them - no direct email)]

Just an fyi: the extension update ui files (e.g. update.xml) are a complete mess and will be dealt with in Bug 342606. I should have time this weekend to review the remainder of the patch.

Comment 89

[Robert Strong (they/them - no direct email)]

Comment on attachment 229211 [details] [diff] [review]
Quick, complete fix for addons manager

>Index: toolkit/mozapps/extensions/content/about.js
...
>@@ -101,17 +101,17 @@ function init()
...
>-    extensionHomepage.setAttribute("href", homepage);
>+    extensionHomepage.setAttribute('onclick', "loadHomepage(\"" + homepage + "\");");
nit: use double quotes around onclick.

>Index: toolkit/mozapps/extensions/content/extensions.js
...
>@@ -335,17 +335,17 @@ function showView(aView) {
>       getMore.setAttribute("tooltiptext", getMore.getAttribute(isThemes ? "tooltiptextthemes" :
>                                                                           "tooltiptextextensions"));
>       getMore.setAttribute("value", getMore.getAttribute(isThemes ? "valuethemes" :
>                                                                     "valueextensions"));
>       var getMoreURL = gPref.getComplexValue(isThemes ? PREF_EXTENSIONS_GETMORETHEMESURL
>                                                       : PREF_EXTENSIONS_GETMOREEXTENSIONSURL,
>                                              Components.interfaces.nsIPrefLocalizedString).data;
>       getMoreURL = getMoreURL.replace(/%APPID%/g, gAppID);
>-      getMore.setAttribute('href', getMoreURL);
>+      getMore.setAttribute('onclick', "openURL(\"" + getMoreURL + "\");");
nit: use double quotes around onclick.

>@@ -429,17 +429,22 @@ function openURL(aURL)
...
> # If we're a browser, open a new browser window instead.    
> #else
>-  openDialog("chrome://browser/content/browser.xul", "_blank", "chrome,all,dialog=no", aURL, null, null);
>+  if (window.opener && window.opener.openUILinkIn) {
>+    var newWindowPref = gPref.getIntPref("browser.link.open_newwindow");
>+    var where = newWindowPref == 3 ? "tab" : "window";
>+    window.opener.openUILinkIn(aURL, where);
>+  } else
>+    openDialog("chrome://browser/content/browser.xul", "_blank", "chrome,all,dialog=no", aURL, null, null);
Add a comment that references this bug.

>Index: toolkit/mozapps/extensions/content/list.js
...
>@@ -164,17 +164,18 @@ function init() {
>   if ("moreInfoURL" in params && params["moreInfoURL"]) {
>     message = document.getElementById("moreInfo");
>     message.value = extensionsBundle.getString("moreInfoText");
>-    message.setAttribute("href", params["moreInfoURL"]);
>+    message.setAttribute("onclick", "window.opener.openURL(\"" +
>+                                    params["moreInfoURL"] + "\");");
This ui can be opened from the EM service without the EM ui opened when a new blocklisted item is detected. Since this ui will only be shown when an item is first blocklisted let's just leave this one as is and fix this after a generic solution is created.

>Index: toolkit/mozapps/extensions/content/update.css
>Index: toolkit/mozapps/extensions/content/update.xml
Both of these will be taken care of in bug 342606 so just remove them from the patch.

r=me with those changes

Comment 90

[Michael Wu [:mwu]]

Attachment: Quick, complete fix for addons manager, v2

Comment 91

[Robert Strong (they/them - no direct email)]

Removing blocks since none of them make any sense in the context of the Extension Manager... if anything this bug should have depended on bug 263433. The patch that will land after the tree opens is just wallpaper around the actual problem of not having a generic way for toolkit ui to support this for Firefox.

Bug 263433 will be part of the real fix for this bug.

Comment 92

[Steffen Wilberg]

> Removing blocks since none of them make any sense in the context of the
> Extension Manager... if anything this bug should have depended on bug 263433.
The dependencies did make sense, see e.g. bug 262808 comment 4.

> The patch that will land after the tree opens is just wallpaper around the
> actual problem of not having a generic way for toolkit ui to support this for
> Firefox.
Attachment 206172 [details] [diff] in this bug was trying to fix this. Is there a new bug for fixing this for toolkit in general?

Comment 93

[Robert Strong (they/them - no direct email)]

(In reply to comment #92)
> > Removing blocks since none of them make any sense in the context of the
> > Extension Manager... if anything this bug should have depended on bug 263433.
> The dependencies did make sense, see e.g. bug 262808 comment 4.
Right, this bug got morphed to also include help viewer which I would be fine with if this bug got traction and was fixed. To get this fixed for Firefox 2 I am un-morphing it.

> > The patch that will land after the tree opens is just wallpaper around the
> > actual problem of not having a generic way for toolkit ui to support this for
> > Firefox.
> Attachment 206172 [details] [diff] [edit] in this bug was trying to fix this. Is there a new bug for
> fixing this for toolkit in general?
bug 263433 covers text-link which this bug should have depended on and I don't believe there is a bug to cover the other cases.

Comment 94

[Robert Strong (they/them - no direct email)]

Check in to trunk.

bug 263433 covers a generic fix for label's with class="text-link"
I don't believe a bug has been opened for supporting tabbed browsing preferences when opening urls from code... if one is filed please post the bug number in this bug.

Please file new bugs if there is any fallout from this.

Comment 95

[Worcester12345]

Version: unspecified.

Any chance of branch?

Comment 96

[Michael Wu [:mwu]]

(In reply to comment #95)
> Version: unspecified.
> 
> Any chance of branch?
> 
If nothing breaks, it should make beta2.

Comment 97

[Robert Strong (they/them - no direct email)]

Filed Bug 345001 for supporting respecting tabbed browser settings when opening web pages from code.

Comment 98

[Michael Wu [:mwu]]

Comment on attachment 229496 [details] [diff] [review]
Quick, complete fix for addons manager, v2

This patch is fairly safe and worthwhile for beta 2.

Comment 99

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This patch looks like, at the very least, it needs careful security review.  It's doing the equivalent of eval by concatenating strings to form the value of an onclick attribute.  If it's possible for those strings to have untrusted input with quotes, parentheses, or semicolons in them, then this could be a security hole.

Are the URLs all coming from trusted sources?

Comment 100

[Robert Strong (they/them - no direct email)]

For the cmd_homepage case in the add-ons mgr context menu we get the value from the extensions rdf datasource and then make sure the scheme is http or https
http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/content/extensions.js#1599

For the getMoreURL we are getting the url from getComplexValue from the locale's extensions.properties file.

For the blocklist we are getting the url from the pref extensions.blocklist.detailsURL

For the about dialog we get the value from the extensions rdf datasource.

Lots of inconsistency... I'll submit an update to this patch that verifies these are all have a scheme of either http or https

Comment 101

[Robert Strong (they/them - no direct email)]

The about dialog verifies the scheme is http or https already.

Comment 102

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 229496 [details] [diff] [review]
Quick, complete fix for addons manager, v2

We're going to minus this for now.  Feel free to renominate if you have more information about the security issues or a new patch.

Comment 103

[Michael Wu [:mwu]]

Attachment: Store URL to open in attribute

Comment 104

[Michael Wu [:mwu]]

Attachment: Store URL to open in attribute, v2

On second thought, that patch was too ugly for me bear.

Comment 105

[Michael Wu [:mwu]]

Attachment: Store URL to open in attribute, v3

Comment 106

[Michael Wu [:mwu]]

Attachment: Store URL to open in attribute, v4

Ok?

Comment 107

[Robert Strong (they/them - no direct email)]

Attachment: patch

Comment 108

[(not reading, please use [e-mail] instead)]

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

what's this:

1)

-// manages the last-selected attribute for the view buttons and richlistbox
+// manages the last-selected attribute for the view b7/21/2006uttons and richlistbox

other than that, r=sspitzer

Comment 109

[Robert Strong (they/them - no direct email)]

Checked in attachment #230195 [details] [diff] [review] to trunk. I'll submit a combined patch for 1.8.1

Comment 110

[Robert Strong (they/them - no direct email)]

Attachment: branch patch

Carrying forward review and requesting 1.8.1

Comment 111

[Nick Thomas [:nthomas] (UTC+12)]

*** Bug 345713 has been marked as a duplicate of this bug. ***

Comment 112

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 230296 [details] [diff] [review]
branch patch

a=drivers. Please go ahead and land this on the branch.

Comment 113

[Robert Strong (they/them - no direct email)]

Checked in to MOZILLA_1_8_BRANCH for Firefox 2.0

Comment 114

[J.W.]

So now Add-ons links are opened in new tab when browser.link.open_newwindow is set to 3. But I prefer browser.link.open_newwindow=1 so all window options are ignored, but then it makes Add-ons manager open links in new window.

Comment 115

[Worcester12345]

This isn't by default or even by any easy choice yet, is it? I haven't run a trunk test build in a while. Thanks.