782115 - Dictionary add-ons should be compatible by default

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Jesper Kristensen]

Attachment: Dictionaries default to compatible v1

Dictionary add-ons with <em:type>64</em:type> introduced by bug 591780 should be compatible by default. Since the introduction of dictionaries (I think it was Firefox 2) they have seen one incompatible change. That was bug 533038 in Firefox 4 which introduced <em:unpack>true</em:unpack>.

Comment 1

[Jesper Kristensen]

I just discovered Bug 667262 after filing this, but that bug was from before Bug 692664, so I think its conclusion is no longer relevant.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jesper Kristensen from comment #1)
> I just discovered Bug 667262 after filing this, but that bug was from before
> Bug 692664, so I think its conclusion is no longer relevant.

Not entirely irrelevant. For add-ons, any incompatible change in the application can break some add-ons, and those broken versions can be marked incompatible by AMO. 

However, for dictionaries it's my understanding that any incompatible change in hunspell is likely to break *all* dictionaries. 

I think we need some mitigation strategy against that. Compatible-by-default add-ons have a minimum version they need to declare they're compatible with, controlled by the extensions.minCompatibleAppVersion an extensions.minCompatiblePlatformVersion prefs (for Firefox it's 4.0, and 2.0 for Toolkit). We could do the same for dictionaries, specifying different app/platform minimum versions - and those can be bumped whenever there's an incompatible hunspell change.

Would like to hear Dave's thoughts though - I think he's thought about dictionaries more than I have.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 651189 [details] [diff] [review]
Dictionaries default to compatible v1

Review of attachment 651189 [details] [diff] [review]:
-----------------------------------------------------------------

Quick late-night drive-by:

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +5508,5 @@
>        version = aPlatformVersion
>  
>      // Only extensions can be compatible by default; themes and language packs
>      // always use strict compatibility checking.
> +    if ((this.type == "extension" || this.type == "dictionary") &&

These if-statements for strict compatibility are getting a bit unwieldy. Define these two types as a const array at the top of the file, as see if this.type is in that array?

Also, the comment needs updated.

Comment 4

[Jesper Kristensen]

(In reply to Blair McBride (:Unfocused) from comment #2)
It might be a good idea to make a separate preference for minCompatible*Version for dictionary add-ons. But I think we would need to weigh the risk of an incompatible change ever occurring against added complexity by having two prefs. I will wait for Dave's opinion before implementing something like that. Besides, compatible by default for non-restartless dictionaries are already handled by minCompatible*Version, so adding a new preference could be a separate enhancement after this bug.

Comment 6

[Dave Townsend [:mossop]]

(In reply to Blair McBride (:Unfocused) from comment #2)
> (In reply to Jesper Kristensen from comment #1)
> > I just discovered Bug 667262 after filing this, but that bug was from before
> > Bug 692664, so I think its conclusion is no longer relevant.
> 
> Not entirely irrelevant. For add-ons, any incompatible change in the
> application can break some add-ons, and those broken versions can be marked
> incompatible by AMO. 
> 
> However, for dictionaries it's my understanding that any incompatible change
> in hunspell is likely to break *all* dictionaries. 
> 
> I think we need some mitigation strategy against that. Compatible-by-default
> add-ons have a minimum version they need to declare they're compatible with,
> controlled by the extensions.minCompatibleAppVersion an
> extensions.minCompatiblePlatformVersion prefs (for Firefox it's 4.0, and 2.0
> for Toolkit). We could do the same for dictionaries, specifying different
> app/platform minimum versions - and those can be bumped whenever there's an
> incompatible hunspell change.
> 
> Would like to hear Dave's thoughts though - I think he's thought about
> dictionaries more than I have.

I don't think there's any benefit to adding the different prefs speculatively, it's just as easy to do it later if we ever encounter the need I think.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 651189 [details] [diff] [review]
Dictionaries default to compatible v1

Review of attachment 651189 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +756,5 @@
> +
> +    if (addon.type == "dictionary")
> +      addon.strictCompatibility = getRDFProperty(ds, root, "strictCompatibility") == "true";
> +    else
> +      addon.strictCompatibility = true;

Should pull this code out of the if and share it for both types, using whatever test we agree upon elsewhere.

@@ +5508,5 @@
>        version = aPlatformVersion
>  
>      // Only extensions can be compatible by default; themes and language packs
>      // always use strict compatibility checking.
> +    if ((this.type == "extension" || this.type == "dictionary") &&

How about instead a function like |supportsCompatibleByDefault(aType)| ?

Comment 8

[Jesper Kristensen]

Attachment: patch v2

I am not sure who of you I should request review from.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Jesper, to make life easier for those checking in on your behalf, please make sure your patches include all of the necessary metadata needed. I've already taken care of this one locally, but it'll make things easier next time. Thanks!
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=2ec9ffb953d7

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0f3d35933b

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4c0f3d35933b