2007-02-13

Content Preferences - take 2

I released an updated version of the Content Preferences extension today.

This update features significant changes under the hood to improve the API for extensions that want to use it to store their own site-specific prefs (about which more shortly).

It also offers the following two improvements for end-users who use it for its text-zoom functionality:
  1. Text zoom changes via the mousewheel are now supported, so no matter how you change your text zoom (via the View menu, keyboard shortcuts, the sidebar slider provided by this extension, or mousewheel gestures), this extension will save your setting for each site you visit.

  2. An experimental mode lets you set preferences for a domain and all its subdomains. (f.e. *.mozilla.org). To enable this mode, use about:config to set the extensions.cpref.siteMode preference to the number 1.

    If you're trying out the Gran Paradiso Alpha 2 release (or trunk nightlies), the extension uses the new "effective TLD service" to correctly determine the base domain for each site (f.e. mozilla.org for www.mozilla.org and bbc.co.uk for www.bbc.co.uk). Otherwise, it falls back to the "top-level domain plus one segment" rule (i.e. mozilla.org for www.mozilla.org and co.uk for www.bbc.co.uk).
Give it a try, and let me know what you think.

14 comments:

Anonymous said...

http://weblogs.mozillazine.org/weirdal/archives/016751.html

Brian King said...

Is the slider widget part of the toolkit, or one you rolled yourself? I know Neil Deakin was working on one on the trunk, not sure of the status:

http://wiki.mozilla.org/XUL:Slider_Tag

I also suggest an icon in the sidebar and a white background to make it look prettier, but I am sure you have more important things to do!

Myk said...

Anonymous: I've looked at that link, and I don't see the connection between that page and this one. Can you elaborate?

Brian: the slider widget is Neil's, and he's checked it into the trunk already, but it's not on the branch, so I've bundled it with this extension to make sure the extension works on the branch as well.

john p baker said...

I have just noticed a bug in getSiteForURI which means that it never uses the TLD:

if (result)
site = result[0];
throw("failed to obtain site using effective TLD service");
always throws!!

PS .reverse().slice(0,2).reverse(). is equivalent to .slice(-2).

John

john.p.baker said...

I have to say that I find the key assignment for the xul:scale widget counter-intuitive; Up/PgUp decreases value and Dn/PgDn increases it!
The rationale behind this seems to be that in the vertical orientation the maximum value is at the bottom; This also seems the opposite to the real world where bottom is minimum (eg sound mixing desk, air conditioner temperature controls...).

I would expect the minimum value to be bottom/left and up increasing the value towards the maximum at the top/right.

If this is the first use of the widget in the wild than it may be worth you starting a discussion in a usability/accessibility newsgroup, or even filing a bug.

john

Myk said...

John,

Thanks for catching that persistent throw! Looks like I forgot the "else" before it.

And thanks for the tip on slice! I thought it should be possible to pass a negative value for the "begin" argument to slice, but when I checked the JS 1.5 reference, I didn't find mention of it on the Array.slice page. I guess I should have just tried it out. I've updated the reference with this info.

As for the behavior of the slider widget, I agree that it is counter-intuitive. I'll check to make sure it isn't a bug in my code and file a bug on it if not.

john.p.baker said...

The scale widget seems to be closely tied into scroll-bars in which case the bug is probably that PgUp/PgDn/Up/Dn have any effect at all on horizontal scales and should either do nothing or preferably pass
on to an associated vertical control.

This may mean that a different control is needed for origin bottom/left sliders - especially as there is no PgLeft/PgRight.

I am not sure what the TLD is supposed to do with numeric IP addresses passed to it - but getSiteForURI probably wants to return the four elements rather than just the last two.

You may need a tidy-up routine that removes items from the sites table if they have no entry in the prefs table - but no rush for that.

john

john.p.baker said...

Ah! Numeric IP addresses are covered by bugs 367446 - which you will presumably use when available, and
364129.

I am having a problem on the trunk build: File/NewTab is not giving me a LocationChange which, on invetsigation, seems to be because your ProgressListener is getting null for the uri.

john

john.p.baker said...

This seems to fix it:

cpref_log(" -> " + (site ? site : uri ? uri.spec : null));

john

Myk said...

Numeric IP addresses are covered by bugs 367446 - which you will presumably use when available, and
364129.


Yup, I'm looking forward to having those helper methods available.

Thanks for the fix for the File > New Tab problem! I'll incorporate it into the next version.

john p baker said...

Some jottings that need to be somewhere better than a scrap of paper on my desk:

Controller

As there is the possibility of only the controller and service as core, I would like to see an early exit from onLocationChange if there are no observers registered (ie before the database queries).

I think that the call to the individual observers needs a try/catch so that, if there is a problem in one of them, it doesn't prevent the others from being called.

Service

In removePref, after the pref is deleted, I think you should check if the site has any other prefs set and if not delete the site from the sites table. [This is the tidy-up that I mentioned earlier]

john

流水线,生产线-重任流水线 said...
This comment has been removed by a blog administrator.
john p baker said...

I think that the call to the individual observers needs a try/catch so that, if there is a problem in one of them, it doesn't prevent the others from being called.

This now looks true, and in fact should possibly apply to Firefox's controller.

There would appear to be a problem in the text-zoom that prevents the other handlers operating:
o Open more than one tab
o Open and close preferences sidebar
o Select each tab in turn and the window title does not change.
o Open the sidebar
o ... window title does change

changing to this.observers[key].forEach(function(observer) { try {observer.onLocationChange(sitePrefs[key] || globalPrefs[key])} catch (e) {}); fixes it [and adding an alert(e) shows this.docShell has no properties]

john

john p baker said...

...and in fact should possibly apply to Firefox's controller.

I have logged the Firefox controller bug as https://bugzilla.mozilla.org/show_bug.cgi?id=376222 using similar code to the suggestion for your controller.

john