Closed Bug 262575 Opened 15 years ago Closed 14 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta2

People

(Reporter: g.teunis, Assigned: mwu)

References

Details

(Keywords: fixed1.8.1)

Attachments

(6 files, 18 obsolete files)

3.23 KB, patch
mconnor
: review-
Details | Diff | Splinter Review
3.23 KB, patch
Details | Diff | Splinter Review
2.33 KB, patch
mconnor
: review+
Details | Diff | Splinter Review
5.56 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
9.08 KB, patch
moco
: review+
Details | Diff | Splinter Review
9.55 KB, patch
rstrong
: review+
beltzner
: approval1.8.1+
Details | Diff | Splinter Review
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.
Forgot to mention:
"Get more themes" and "Get more extensions" links in the theme/extension
managers also don't use the Tabbed Browsing preferences.
Attached patch patch (obsolete) — Splinter Review
This patch looks pref "browser.link.open_newwindow.ui", not
"browser.link.open_newwindow". Firefox does not have UI for this pref.
(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)
Will this patch also fix bug 262685? That might even be a dupe?
*** Bug 262685 has been marked as a duplicate of this bug. ***
I reported some time ago this one :
https://bugzilla.mozilla.org/show_bug.cgi?id=251181 which max marked as WONTFIX...
(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.

I also think the patch has to use the browser.link.open_newwindow
Is more consistent and expected behavior to the Tabbed Browsing settings.
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?
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.
(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.
Attachment #160905 - Attachment is obsolete: true
Attached patch updated patch (obsolete) — Splinter Review
This patch still ignores link modifiers. Without it, this patch works well for
me.
My opinion is that the 'link modifiers' can be ignored on UI links.
Attached patch ugly hack (obsolete) — Splinter Review
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).
Attachment #161364 - Attachment is obsolete: true
(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.
(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?
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.
(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 on attachment 161465 [details] [diff] [review]
ugly hack

(In reply to comment #18)
Sorry, I misunderstood comment #13.
Attachment #161465 - Attachment is obsolete: true
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...
Attachment #161467 - Attachment is obsolete: true
Same as before, only openURL() uses browser.tabs.loadDivertedInBackground for
tab selection.
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 on attachment 161476 [details] [diff] [review]
teach openURL() about browser.link.open_newwindow  (alternate)

Obsoleted; real patch against latest Aviary snapshot coming up.
Attachment #161476 - Attachment description: teach openURL() about browser.link.open_newwindow and browser.tabs.loadDivertedInBackground → teach openURL() about browser.link.open_newwindow (alternate)
Attachment #161476 - Attachment is obsolete: true
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 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.
Attachment #162067 - Flags: review?(bugs)
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'.
> 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 on attachment 162067 [details] [diff] [review]
teach openURL() about browser.link.open_newwindow (final)

Canceling review and obsoleting. New patch coming up.
Attachment #162067 - Attachment is obsolete: true
Attachment #162067 - Flags: review?(bugs)
Attached patch fixed patchSplinter Review
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 on attachment 162072 [details] [diff] [review]
fixed patch

Re-requesting review? from Ben.
Attachment #162072 - Flags: review?(bugs)
> +    // 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
(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?
(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...
*** Bug 266305 has been marked as a duplicate of this bug. ***
*** Bug 266467 has been marked as a duplicate of this bug. ***
*** Bug 266671 has been marked as a duplicate of this bug. ***
*** Bug 267380 has been marked as a duplicate of this bug. ***
*** Bug 268737 has been marked as a duplicate of this bug. ***
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.
Attachment #162072 - Flags: review?(bugs) → review-
(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?
*** Bug 283835 has been marked as a duplicate of this bug. ***
Attached patch patch based on attachment 162072 (obsolete) — Splinter Review
This patch uses openURI method of browser window.
This behaves as URI from 'external app', not 'internal link'.

How about this one?
(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.
Attachment #175711 - Attachment is obsolete: true
Attached patch revised patchSplinter Review
Attachment #177004 - Flags: review?
Flags: blocking-aviary1.1?
Comment on attachment 177004 [details] [diff] [review]
revised patch

don't expect this to be reviewed without asking a particular browser/toolkit
peer.
(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.
Attachment #177004 - Flags: review? → review?(mconnor)
*** Bug 288703 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1? → blocking-aviary1.1-
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.
Attachment #177004 - Flags: review?(mconnor) → review+
(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 on attachment 177004 [details] [diff] [review]
revised patch

we're trying to wrap up alpha1. moving request out to alpha2.
Attachment #177004 - Flags: approval-aviary1.1a2?
Attachment #177004 - Flags: approval-aviary1.1a1?
Attachment #177004 - Flags: approval-aviary1.1a1-
(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.
Attached patch Another patch. (obsolete) — Splinter Review
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.
Attached patch another patch, revised (obsolete) — Splinter Review
(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]?
Attachment #184172 - Attachment is obsolete: true
Attachment #184796 - Flags: review?(mconnor)
Blocks: 262808
Rerequest blocking since there is now a patch. Thanks.
Flags: blocking-aviary1.1- → blocking-aviary1.1?
Comment on attachment 177004 [details] [diff] [review]
revised patch

a=shaver.
Attachment #177004 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Was it intentional that the patch that was approved is not the newest one?
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.
Flags: blocking1.8b3?
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?
Attachment #184796 - Flags: review?(mconnor) → review-
*** Bug 297110 has been marked as a duplicate of this bug. ***
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
Flags: blocking1.8b3? → blocking1.8b3-
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.
Severity: normal → minor
Flags: blocking-aviary1.1? → blocking-aviary1.1-
- 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.
Attachment #184796 - Attachment is obsolete: true
Flags: blocking-aviary2.0?
Attachment #189287 - Attachment is obsolete: true
(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.
*** Bug 306829 has been marked as a duplicate of this bug. ***
*** Bug 307190 has been marked as a duplicate of this bug. ***
*** Bug 309519 has been marked as a duplicate of this bug. ***
Assignee: bugs → nobody
OS: Windows XP → All
QA Contact: bugs → extension.manager
Hardware: PC → All
Summary: Visit Homepage in Extension and Theme manager should repect Tabbed Browsing preferences → Visit Homepage in Extension and Theme manager should respect Tabbed Browsing preferences
ive tried applying the patch on trunk and it seems to have bit rotted.
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.
(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?
Flags: blocking1.8rc2?
it's too late for this to make 1.5. Leaving the 2.0 nomination alone for the next train.
Flags: blocking1.8rc2? → blocking1.8rc2-
This is a nice-to-have, not a blocker.
Flags: blocking-aviary2? → blocking-aviary2-
(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? 
Summary: Visit Homepage in Extension and Theme manager should respect Tabbed Browsing preferences → "Visit Homepage" and "Get More Extensions/Themes" in Extension and Theme manager should respect tabbed browsing preferences
*** Bug 318258 has been marked as a duplicate of this bug. ***
Attached patch another try (obsolete) — Splinter Review
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.
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 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.
Attachment #205297 - Attachment is obsolete: true
Attached patch another try, round 2 (obsolete) — Splinter Review
This is mostly same as attachment 205297 [details] [diff] [review].
Sorry for bug spams.
(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.
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.
Blocks: 320628
Blocks: 263433
*** Bug 340962 has been marked as a duplicate of this bug. ***
Attachment #206172 - Attachment is obsolete: true
*** Bug 342269 has been marked as a duplicate of this bug. ***
*** Bug 343632 has been marked as a duplicate of this bug. ***
Attached patch Quick fix for addons manager (obsolete) — Splinter Review
This is a quick, clean addons manager only fix.
*** Bug 344382 has been marked as a duplicate of this bug. ***
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.
Assignee: nobody → michael.wu
Attachment #228848 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #229211 - Flags: review?(robert.bugzilla)
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 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
Attachment #229211 - Flags: review?(robert.bugzilla) → review+
Attachment #229211 - Attachment is obsolete: true
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.
No longer blocks: 262808, 263433, 320628
> 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?
(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.
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Version: unspecified.

Any chance of branch?
(In reply to comment #95)
> Version: unspecified.
> 
> Any chance of branch?
> 
If nothing breaks, it should make beta2.
Target Milestone: --- → Firefox 2 beta2
Filed Bug 345001 for supporting respecting tabbed browser settings when opening web pages from code.
Comment on attachment 229496 [details] [diff] [review]
Quick, complete fix for addons manager, v2

This patch is fairly safe and worthwhile for beta 2.
Attachment #229496 - Flags: approval1.8.1?
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?
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
The about dialog verifies the scheme is http or https already.
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.
Attachment #229496 - Flags: approval1.8.1? → approval1.8.1-
Attached patch Store URL to open in attribute (obsolete) — Splinter Review
Attachment #230019 - Flags: review?(robert.bugzilla)
On second thought, that patch was too ugly for me bear.
Attachment #230019 - Attachment is obsolete: true
Attachment #230021 - Flags: review?(robert.bugzilla)
Attachment #230019 - Flags: review?(robert.bugzilla)
Attachment #230021 - Attachment is obsolete: true
Attachment #230030 - Flags: review?(robert.bugzilla)
Attachment #230021 - Flags: review?(robert.bugzilla)
Ok?
Attachment #230030 - Attachment is obsolete: true
Attachment #230032 - Flags: review?(robert.bugzilla)
Attachment #230030 - Flags: review?(robert.bugzilla)
Attached patch patchSplinter Review
Attachment #230032 - Attachment is obsolete: true
Attachment #230195 - Flags: review?(sspitzer)
Attachment #230032 - Flags: review?(robert.bugzilla)
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
Attachment #230195 - Flags: review?(sspitzer) → review+
Attached patch branch patchSplinter Review
Carrying forward review and requesting 1.8.1
Attachment #230296 - Flags: review+
Attachment #230296 - Flags: approval1.8.1?
*** Bug 345713 has been marked as a duplicate of this bug. ***
Comment on attachment 230296 [details] [diff] [review]
branch patch

a=drivers. Please go ahead and land this on the branch.
Attachment #230296 - Flags: approval1.8.1? → approval1.8.1+
Checked in to MOZILLA_1_8_BRANCH for Firefox 2.0
Keywords: fixed1.8.1
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.
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.
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.