Closed Bug 361203 Opened 14 years ago Closed 14 years ago

Suiterunner: More Information button doesn't work on Smart Browsing pane.

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch WIP Patch (obsolete) — Splinter Review
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?
(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.
(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.
(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.
Attached patch Patch v1 (obsolete) — Splinter Review
Revised to use the urlformatter and just use the open window function as previous implementation.
Assignee: prefs → bugzilla
Attachment #249038 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #249909 - Flags: superreview?(neil)
Attachment #249909 - Flags: review?(neil)
Comment on attachment 249909 [details] [diff] [review]
Patch v1

Despite comment #4 I think KaiRo should officially grant l10n approval on this one.
Attachment #249909 - Flags: review?(neil) → review?(kairo)
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?]
Attachment #249909 - Flags: superreview?(neil) → superreview+
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...)
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)
Attachment #249909 - Flags: review?(kairo) → review-
Attached patch Patch v2Splinter Review
Revised version that uses a localized pref.
Attachment #249909 - Attachment is obsolete: true
Attachment #250071 - Flags: review?(kairo)
Comment on attachment 250071 [details] [diff] [review]
Patch v2

Looks good now, r=me
Attachment #250071 - Flags: review?(kairo) → review+
Attachment #250071 - Flags: superreview?(neil)
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?
Attachment #250071 - Flags: superreview?(neil) → superreview+
(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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.