380401 - multiple uses of flags should be anded

Status: NEW

(Toolkit :: Startup and Profile System, defect)

Description

[Dave Townsend [:mossop]]

Currently if multiple appversion flags are applied to a manifest line then only one needs to match for the line to be used. This is quite unhelpful. If I wish to restrict an overlay only to Firefox 1.5.0.x for instance, I can't.

A more sensible way to go would be to need all appversion flags to match for a line to be used. Then I could do the example above. If I really needed to match a list of appversions then I can use multiple lines to achieve the same effect.

Comment 1

[Atte Kemppilä [:atte]]

Now that the plan is to release multiple major versions of Firefox this year, the situation where ANDing of appversions would be helpful is more likely occur. Especially if some extension wants to support Firefox 3.6 - 7.

Any chance this will get implemented?

Comment 2

[Dave Townsend [:mossop]]

We were talking about this the other day, glad you commented so I could find the bug. This is pretty inconsistent with how it works for multiple flags of different types so I think we should fix this.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch and tests

This patch does a simple tristate check change in the checker functions. If a flag has ever been set to "bad" it stays bad, effectively and'ing the flag checks.

Previously, the checker functions kept a flag "good" if it was ever found to be good,effectively or'ing the flag checks.

I added some tests for the and'ing behavior.

I'm not sure we want to add the and'ing to the string checks or not. I'm not sure it makes sense.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

xpcom_shell tests are passing on try server

Comment 5

[Dave Townsend [:mossop]]

(In reply to comment #3)
> Created attachment 529764 [details] [diff] [review] [review]
> patch and tests
> 
> This patch does a simple tristate check change in the checker functions. If a
> flag has ever been set to "bad" it stays bad, effectively and'ing the flag
> checks.
> 
> Previously, the checker functions kept a flag "good" if it was ever found to be
> good,effectively or'ing the flag checks.
> 
> I added some tests for the and'ing behavior.
> 
> I'm not sure we want to add the and'ing to the string checks or not. I'm not
> sure it makes sense.

I think we do just in the name of consistency. Once this is in we can say that a line is only processed if every one of its checks passes. Right now it is more confusing than that.

How about this case? http://mxr.mozilla.org/mozilla-central/source/xpcom/components/ManifestParser.cpp?force=1#339

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch and tests 2

Removes the extra check for a value in the version check. If we have no value, we always fail.

Adds tests for some and'ed string tests and an empty version value.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 529834 [details] [diff] [review]
patch and tests 2

This is kinda-sorta an api change so maybe an sr is needed, either way I'd like to make sure benjamin knows we're making this change.

We'll also need to notify the add-on devs. Wonder if we can easily search for this in mxr...

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

I looked in the addons mxr for a few flags in \.manifest files, looking for multiple uses on a single line. The only flags I found with multiple uses on a single line were:

* appversion: Found 8 matching lines in 4 files
* application: Found 14 matching lines in 6 files
* os: Found 3 matching lines in 3 files

I used regex searching and used "application#application" (for example) to look for uses.

Examples:

overlay chrome://browser/content/browser.xul chrome://venkman/content/venkman-overlay.xul application={ec8030f7-c20a-464f-9b0e-13a3a9e97384} application={a463f10c-3994-11da-9945-000d60ca027b} application=[e-mail]

overlay chrome://browser/content/browser.xul chrome://listalltabsmenu/content/browserOverlay.xul appversion=3.7a1pre appversion=3.7a1 appversion=3.7a2pre appversion=3.7a2 appversion=3.7a3pre appversion=3.7a3 

skin magicslr classic/1.0 chrome/skin/gnomestripe/ application={3550f703-e582-4d05-9a08-453d09bdfdc6} os!=WINNT os!=Darwin 

I assume this is bad enough to have second thoughts about the patch.

Comment 9

[Dave Townsend [:mossop]]

(In reply to comment #8)
> I looked in the addons mxr for a few flags in \.manifest files, looking for
> multiple uses on a single line. The only flags I found with multiple uses on a
> single line were:
> 
> * appversion: Found 8 matching lines in 4 files
> * application: Found 14 matching lines in 6 files
> * os: Found 3 matching lines in 3 files
> 
> I used regex searching and used "application#application" (for example) to look
> for uses.
> 
> Examples:
> 
> overlay chrome://browser/content/browser.xul
> chrome://venkman/content/venkman-overlay.xul
> application={ec8030f7-c20a-464f-9b0e-13a3a9e97384}
> application={a463f10c-3994-11da-9945-000d60ca027b}
> application=[e-mail]

This is an easy fix, just duplicate the line 3 times

> overlay chrome://browser/content/browser.xul
> chrome://listalltabsmenu/content/browserOverlay.xul appversion=3.7a1pre
> appversion=3.7a1 appversion=3.7a2pre appversion=3.7a2 appversion=3.7a3pre
> appversion=3.7a3 

This looks like it benefits from our fix, though since it is all for 3.7 it isn't really affected.

> skin magicslr classic/1.0 chrome/skin/gnomestripe/
> application={3550f703-e582-4d05-9a08-453d09bdfdc6} os!=WINNT os!=Darwin 

I would lay odds that this author was expecting this line to only have an effect when the os is neither WINNT or Darwin, I think we're actually fixing a problem for them!

> I assume this is bad enough to have second thoughts about the patch.

I would disagree, it looks to be a very small number of add-ons indeed, we could email those authors directly if we cared to. This I think would be a fine case for using the new AMO policy to just not automatically rev the compatibility for the add-ons identified so the developers can figure out any potential problems and solve them.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #9)

> > I assume this is bad enough to have second thoughts about the patch.
> 
> I would disagree, it looks to be a very small number of add-ons indeed, we
> could email those authors directly if we cared to. This I think would be a fine
> case for using the new AMO policy to just not automatically rev the
> compatibility for the add-ons identified so the developers can figure out any
> potential problems and solve them.

Great! Some of the add-ons are not compatible with recent Firefox version too. I certainly think we could notify and help the remaining authors.

Comment 11

[Benjamin Smedberg]

Comment on attachment 529834 [details] [diff] [review]
patch and tests 2

Yeah, I think this is ok. It doesn't affect the other flags (OS and whatnot, which are intentionally ORed together, right?)

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #11)
> Comment on attachment 529834 [details] [diff] [review] [review]
> patch and tests 2
> 
> Yeah, I think this is ok. It doesn't affect the other flags (OS and whatnot,
> which are intentionally ORed together, right?)

No. the patch removes all OR'ing, which we discussed on IRC is a bad thing. Therefore I am not going to check this code in. I am obsoleting it instead.

Mobile has a need to do range-based version selections, so I think I'll file a new bug to add a range-based operator. We discussed some approaches on IRC and I'll include those in the new bug.

Comment 13

[Todd Whiteman]

Why was OR'ing considered bad? I would of thought AND'ing is the right approach...

Also, Mark, is there a new bug for the proposed range-based selection?

Comment 14

[Kent James (:rkent)]

The recent updates of FF and TB to 7.0.1 give a perfect reason why this bug is needed (and broke my New Account Types addon that had appversion=7.0) There is no practical way for a binary app to say that it works for all possible major Mozilla versions, short of specifying a list of possible future appversions.

Comment 15

[Axel Grude [:realRaven]]

Here is a use case made necessary by recent changes to Earlybird. I am trying to support 3 different versions of Thunderbird this means I need to support the following cases:

1. versions below or equal to 3.0
2. versions between 3.0 _AND_ 10
3. versions above 10 (including 10)

For some unknown reason Earlybird (version 10.0a2) currently ignores case (3).

Currently Tb10.0a2 ignores the last 2 lines - is it related to this bug? 

overlay chrome://messenger/content/mailWindowOverlay.xul chrome://quickfolders/content/overlay.xul  application={3550f703-e582-4d05-9a08-453d09bdfdc6}
overlay chrome://messenger/content/mailWindowOverlay.xul chrome://quickfolders/content/scrollPatch/scrollMenus.xul application={3550f703-e582-4d05-9a08-453d09bdfdc6} appversion>=3.0
# TB 3-9:
overlay chrome://messenger/content/messageWindow.xul     chrome://quickfolders/content/currentFolderSingleMessageTB3.xul  application={3550f703-e582-4d05-9a08-453d09bdfdc6} appversion>=3.0 appversion<10
overlay chrome://messenger/content/mailWindowOverlay.xul chrome://quickfolders/content/overlayCurrentfolderTB3.xul  application={3550f703-e582-4d05-9a08-453d09bdfdc6} appversion>=3.0 appversion<10
# TB 10+
# with the new messagepanewrapper, one size fits all again
overlay chrome://messenger/content/mailWindowOverlay.xul chrome://quickfolders/content/overlayCurrentfolderTB10.xul  application={3550f703-e582-4d05-9a08-453d09bdfdc6} appversion>=10
overlay chrome://messenger/content/messageWindow.xul     chrome://quickfolders/content/currentFolderSingleMessageTB10.xul  application={3550f703-e582-4d05-9a08-453d09bdfdc6} appversion>=10

Comment 16

[Axel Grude [:realRaven]]

I have just checked the docs
https://developer.mozilla.org/en/Chrome_Registration#appversion
and on closer inspection, indeed it says "the line is applied if any of the flags match." - so how do you express 
minver=x + maxver=y ??

Is there a workaround by setting appversion pragmas in the XUL file itself (this would be the most convenient as I wouldn't have to supply 6 different files, where 2 would do).

Some XUL syntax like 
<box .. application=<GUID> minVer=<minimumVersion> maxVer=<maximumVersion> >

or something like 

#if application==<GUID> minVer==<minimumVersion> maxVer==<maximumVersion>   

would be the best workaround.

Comment 17

[Axel Grude [:realRaven]]

I have tried adding this to restrict the "middle branch"

overlay chrome://messenger/content/messageWindow.xul     chrome://quickfolders/content/currentFolderSingleMessageTB3.xul  application={3550f703-e582-4d05-9a08-453d09bdfdc6} appversion>=3.0  platformversion<10
overlay chrome://messenger/content/mailWindowOverlay.xul chrome://quickfolders/content/overlayCurrentfolderTB3.xul  application={3550f703-e582-4d05-9a08-453d09bdfdc6} appversion>=3.0 platformversion<10

to restrict to Thunderbird 3.* - Thunderbird 9.*  (I am assuming since Tb4 Gecko version= major version number) but unfortunately this also does not seem to work .
even appversion and platformversion use OR ? This isn't very intuitive. Do I really have to fork the whole addon to support earlybird.

Also: appversion >=10.0 doesn't work for the other branch, it isn't recognized by Tb 10.0a2. As a workaround I used appversion>9.9 but I am afraid this will fail validation when I upload it. Is there any way I can detect the platformversion from TB host?

Comment 18

[Axel Grude [:realRaven]]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> I looked in the addons mxr for a few flags in \.manifest files, looking for
> multiple uses on a single line. The only flags I found with multiple uses on
> a single line were:
> 
> * appversion: Found 8 matching lines in 4 files
> * application: Found 14 matching lines in 6 files
> * os: Found 3 matching lines in 3 files
> 
> I used regex searching and used "application#application" (for example) to
> look for uses.
> 
> Examples:
> 
> overlay chrome://browser/content/browser.xul
> chrome://venkman/content/venkman-overlay.xul
> application={ec8030f7-c20a-464f-9b0e-13a3a9e97384}
> application={a463f10c-3994-11da-9945-000d60ca027b}
> application=[e-mail]
> 
> overlay chrome://browser/content/browser.xul
> chrome://listalltabsmenu/content/browserOverlay.xul appversion=3.7a1pre
> appversion=3.7a1 appversion=3.7a2pre appversion=3.7a2 appversion=3.7a3pre
> appversion=3.7a3 
> 
> skin magicslr classic/1.0 chrome/skin/gnomestripe/
> application={3550f703-e582-4d05-9a08-453d09bdfdc6} os!=WINNT os!=Darwin 
> 
> I assume this is bad enough to have second thoughts about the patch.

trying to have a complete list of all released version numbers seems unpractical!
Doesn't this lock out any special and development builds?

Comment 19

[Axel Grude [:realRaven]]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> I looked in the addons mxr for a few flags in \.manifest files, looking for
> multiple uses on a single line. The only flags I found with multiple uses on
> a single line were:
> 
> * appversion: Found 8 matching lines in 4 files
> * application: Found 14 matching lines in 6 files
> * os: Found 3 matching lines in 3 files
> 
> I used regex searching and used "application#application" (for example) to
> look for uses.
> 
> Examples:
> 
> overlay chrome://browser/content/browser.xul
> chrome://venkman/content/venkman-overlay.xul
> application={ec8030f7-c20a-464f-9b0e-13a3a9e97384}
> application={a463f10c-3994-11da-9945-000d60ca027b}
> application=[e-mail]
> 
> overlay chrome://browser/content/browser.xul
> chrome://listalltabsmenu/content/browserOverlay.xul appversion=3.7a1pre
> appversion=3.7a1 appversion=3.7a2pre appversion=3.7a2 appversion=3.7a3pre
> appversion=3.7a3 


> 
> skin magicslr classic/1.0 chrome/skin/gnomestripe/
> application={3550f703-e582-4d05-9a08-453d09bdfdc6} os!=WINNT os!=Darwin 
> 
> I assume this is bad enough to have second thoughts about the patch.

Mark, you haven't thought this through. I think OR-ing makes sense with discrete, non-comparable values (such as application), which have a finite number of possibilities. It does make NO sense to use it for numeral, comparable values (such as appversion) as there are potentially infinite instances.

Likewise, the line 
> overlay chrome://browser/content/browser.xul
> chrome://listalltabsmenu/content/browserOverlay.xul appversion=3.7a1pre
> appversion=3.7a1 appversion=3.7a2pre appversion=3.7a2 appversion=3.7a3pre
> appversion=3.7a3 
is _obviously_ a kludge, designed to workaround the missing AND expression
   "appversion>=3.7a1pre appversion<=3.7a3"
or alternatively a tri-nary syntax
   "3.7a1pre<=appversion<=3.7a3"
which is caused precisely by this bug (default to OR for _every_ kind of argument).

This problem will now be greatly exacerbated by the new rapid release cycle and there is a major break in the UI of Thunderbird 10.0a2 (adding messagepanewrapper) which will present multiple extension developers with an unsolvable challenge. 

As stated by Kent in Comment 14 it is impossible to create an OR'ed condition that includes all future version numbers, but if you expect the authors to OR together existing version numbers for all versions between 2 release points could you please provide a _complete_ list of Thunderbird version numbers between 3.0 and 9.* ? 

I am really trying very hard to make my extensions work (spent several hours on it) without re-releasing a new forked version but so far all attempts at this have failed, because of the global "OR" rule.

I propose renaming the bug to "support combining multiple version flags via AND".
I propose that somebody writes a patch to use AND for non-discrete values such as appversion and platformversion. It would break the few scripts that were using above kludge, but it is better than breaking and forcing extension developers to abandon support for older versions of Tb/Fx.

If you think this is out of the question I suggest supporting a new backwards compatible special syntax like these three approaches (1. functional, 2.ternary, 3.bracketed) 
1. platformversion in(x,y)
or 
2. x < platformversion < y 
or 
3. (platformversion > x  platformversion < y)

Comment 20

[Axel Grude [:realRaven]]

I found the following workaround:

use "appversion>3 platformversion<9" as these are _different_ version fields; these are AND-ed together.