Mark Banner (:standard8) Assignee
	Description • 18 years ago

On the preferences window of suiterunner, the Smart Browsing pane has a "More Information" button which doesn't do anything. For normal SeaMonkey builds it works fine.

The problem is that smartBrowsingURL (http://lxr.mozilla.org/seamonkey/search?string=smartBrowsingURL) is currently defined in region.properties which suiterunner is dropping.

What needs to happen is that prefs should use the urlformatter to get the URL, with the URL being placed in brand.properties.

	Mark Banner (:standard8) Assignee
	Comment 1 • 18 years ago

This is work in progress. I thought I would not use the url formatter as its not really necessary/appropriate in this case, but I'm still moving the string to brand.properties.

I also thought I'd use the new openAsExternal function that Ian wrote from contentAreaUtils.js - I've had two problems:

1) pref is not defined (http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/suite/common/contentAreaUtils.js&rev=1.142&mark=100#98) - for now I've defined it locally, but should the function get the pref branch itself (the mailnews instance has pref defined globally)?

2) with the pref defined globally (as per the patch) I then get:

JavaScript error: chrome://communicator/content/contentAreaUtils.js, line 73: contentFrame has no properties

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/suite/common/contentAreaUtils.js&rev=1.142&mark=73#70

Is there a sensible way around that considering I'm in a pref window and not a browser?

	neil@parkwaycc.co.uk
	Comment 2 • 18 years ago

(In reply to comment #1)
>Is there a sensible way around that considering I'm in a pref window and not a browser?
Not currently, but I'm supposed to replace urlSecurityCheck because it has a security flaw.

	Mark Banner (:standard8) Assignee
	Comment 3 • 18 years ago

(In reply to comment #2)
> (In reply to comment #1)
> >Is there a sensible way around that considering I'm in a pref window and not a browser?
> Not currently, but I'm supposed to replace urlSecurityCheck because it has a
> security flaw.

Maybe I'll leave it as the current implementation for opening the link and we could pick it up later if we want to - especially as I think there will be other cases we need to look at.

	Robert Kaiser
	Comment 4 • 18 years ago

(In reply to comment #1)
> This is work in progress. I thought I would not use the url formatter as its
> not really necessary/appropriate in this case, but I'm still moving the string
> to brand.properties.

This _is_ a URL though, so I actually think using the urlformatter would be the correct way here.

	Mark Banner (:standard8) Assignee
	Comment 5 • 18 years ago

Revised to use the urlformatter and just use the open window function as previous implementation.

	neil@parkwaycc.co.uk
	Comment 6 • 18 years ago

Comment on attachment 249909 [details] [diff] [review]
Patch v1

Despite comment #4 I think KaiRo should officially grant l10n approval on this one.

	neil@parkwaycc.co.uk
	Comment 7 • 18 years ago

Comment on attachment 249909 [details] [diff] [review]
Patch v1

[So, what are the rules on when we use a property and when we format a localised pref?]

	Robert Kaiser
	Comment 8 • 18 years ago

I actually think the best idea is to let all URLs go through the full way of url formatter, i.e. making them localized prefs. This probably also would make that code cleaner as it wouldn't need to include the bundle directly, as that will be done by the prefs.

(I didn't even know we could feed something that's not a pref to urlformatter...)

	Robert Kaiser
	Comment 9 • 18 years ago

Comment on attachment 249909 [details] [diff] [review]
Patch v1

r- as Mark agrees with me on IRC - let's make all such URLs localized prefs when we run across them.

1) This fits better with the style used by other toolkit apps, as they use pref'd URLs everywhere (simple prefs, not localized ones like us)
2) Additionally, using prefs makes it easier for somone to tailor SeaMonkey to corporate/organisation needs by just shipping the "right" prefs file(s) to their computers.

(Of course, from that POV, we should pref all URLs we have - we may argue at some point if all of them need to be localized, but for now, I think we should take them to brand.properties)

	Mark Banner (:standard8) Assignee
	Comment 10 • 18 years ago

Revised version that uses a localized pref.

	Robert Kaiser
	Comment 11 • 18 years ago

Comment on attachment 250071 [details] [diff] [review]
Patch v2

Looks good now, r=me

	neil@parkwaycc.co.uk
	Comment 12 • 18 years ago

Comment on attachment 250071 [details] [diff] [review]
Patch v2

I think browser.related. is misleading - What's Related is a sidebar panel completely unrelated (!) to Smart Browsing. How about keyword.moreInfoURL?

	Mark Banner (:standard8) Assignee
	Comment 13 • 18 years ago

(In reply to comment #12)
> (From update of attachment 250071 [details] [diff] [review])
> I think browser.related. is misleading - What's Related is a sidebar panel
> completely unrelated (!) to Smart Browsing. How about keyword.moreInfoURL?
> 
Yep, much better, I checked this in with keyword.moreInfoURL.