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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

13 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.
Assignee

Comment 1

13 years ago
Posted 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.
Assignee

Comment 3

13 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.

Comment 4

13 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.
Assignee

Comment 5

13 years ago
Posted 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+

Comment 8

13 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...)

Comment 9

13 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)
Attachment #249909 - Flags: review?(kairo) → review-
Assignee

Comment 10

13 years ago
Posted patch Patch v2Splinter Review
Revised version that uses a localized pref.
Attachment #249909 - Attachment is obsolete: true
Attachment #250071 - Flags: review?(kairo)

Comment 11

13 years ago
Comment on attachment 250071 [details] [diff] [review]
Patch v2

Looks good now, r=me
Attachment #250071 - Flags: review?(kairo) → review+
Assignee

Updated

13 years ago
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+
Assignee

Comment 13

13 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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.