Dictionary add-ons should be compatible by default

RESOLVED FIXED in mozilla17

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesper Kristensen, Assigned: Jesper Kristensen)

Tracking

unspecified
mozilla17
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 651189 [details] [diff] [review]
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>.
Attachment #651189 - Flags: review?(dtownsend+bugmail)
(Assignee)

Comment 1

5 years ago
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.
(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 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.
(Assignee)

Comment 4

5 years ago
(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.
Duplicate of this bug: 667262
(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 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)| ?
(Assignee)

Comment 8

5 years ago
Created attachment 651556 [details] [diff] [review]
patch v2

I am not sure who of you I should request review from.
Attachment #651189 - Attachment is obsolete: true
Attachment #651189 - Flags: review?(dtownsend+bugmail)
Attachment #651556 - Flags: review?(dtownsend+bugmail)
Attachment #651556 - Flags: review?(dtownsend+bugmail) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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
Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=2ec9ffb953d7

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0f3d35933b
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4c0f3d35933b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.