Closed Bug 553890 Opened 15 years ago Closed 14 years ago

App installed extensions should have the appManaged attribute

Categories

(SeaMonkey :: General, defect)

Product:
SeaMonkey
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: robert.strong.bugs, Unassigned)

References

Details

As follows
<em:appManaged>true</em:appManaged>

If an application provides an extension it should have the appManaged attribute otherwise app update cannot tell that an app upgrade will include a compatible version of the extension.

I can workaround this deficiency that has caused Bug 553763 but Seamonkey should also fix it on its end as well.

Filing under general since it isn't obvious what component this should be filed under.
IIRC, we intentionally decided that we don't use appManaged for our shipped extensions, so users can upgrade to newer versions in their profile. AFAIK, this was even a request from the ChatZilla and venkman teams. We should not lightheartedly change that. And we probably can't so easily anyhow, as the install.rdf are in their repos and not ours.
Perhaps Seamonkey could then provide an environment for running tests where these idiosyncrasies won't cause the tests to fail? For example, not including these extensions in the builds that are running the tests. Another option is to ifndef these tests for Seamonkey.
For all tests (but maybe 'extension' ones), SeaMonkey wants to run the test suites with its extensions enabled, to match what the user gets out of the box ;-)

Fwiw, see https://wiki.mozilla.org/ChatZilla:Suiterunner#appManaged.

I know little about <em:appManaged>, but some ideas (if they are possible):
*Enhance Toolkit to handle the case these SeaMonkey extensions are in.
*Have 'extension' tests temporarily disable the offending extensions.
 (or consider them as "appManaged".)
*...

NB: Disabling the tests seems like a good workaround in the short term! But I wouldn't want that to last if it could be avoided...
(In reply to comment #2)
> Perhaps Seamonkey could then provide an environment for running tests where
> these idiosyncrasies won't cause the tests to fail? For example, not including
> these extensions in the builds that are running the tests. Another option is to
> ifndef these tests for Seamonkey.

I know it's suboptimal (and it is in multiple ways but we found no really good solution), is there a way to either put something in those tests that they detect SeaMonkey and do some things differently there or ifndef them in toolkit and put forks of them into SeaMonkey-specific code to have the mechanisms tested on our side as well?
I will be able to fix bug 553763 so it passes on SeaMonkey sometime next month. I filed this bug so when tests that expect app provided extensions to have the appManaged attribute the test writer wouldn't have to rework or one-off the tests for SeaMonkey each time a new test is added. It would be much better if there was a single fix on the SeaMonkey side to handle this instead.
(In reply to comment #5)
> I will be able to fix bug 553763 so it passes on SeaMonkey sometime next month.

OK, should we ifndef them with a comment temporarily so we can get them off the radar for the moment?

> I filed this bug so when tests that expect app provided extensions to have the
> appManaged attribute the test writer wouldn't have to rework or one-off the
> tests for SeaMonkey each time a new test is added. It would be much better if
> there was a single fix on the SeaMonkey side to handle this instead.

I fully understand what you are saying but I understood that the reasoning and goals in https://wiki.mozilla.org/ChatZilla:Suiterunner (and the same is true for venkman and inspector) pointed to not using that flag. Was that decision wrong? What would we gain/lose by setting it?
(In reply to comment #6)
> (In reply to comment #5)
> > I will be able to fix bug 553763 so it passes on SeaMonkey sometime next month.
> 
> OK, should we ifndef them with a comment temporarily so we can get them off the
> radar for the moment?
I somehow doubt that all of the bugs in bug 452942 will be fixed by the time I have the time to fix bug 553763 but I wouldn't be adverse to a patch to disable these tests for SeaMonkey if you are going to do the same for the other bugs in bug 452942. Personally, I wouldn't bother.

> > I filed this bug so when tests that expect app provided extensions to have the
> > appManaged attribute the test writer wouldn't have to rework or one-off the
> > tests for SeaMonkey each time a new test is added. It would be much better if
> > there was a single fix on the SeaMonkey side to handle this instead.
> 
> I fully understand what you are saying but I understood that the reasoning and
> goals in https://wiki.mozilla.org/ChatZilla:Suiterunner (and the same is true
> for venkman and inspector) pointed to not using that flag. Was that decision
> wrong? What would we gain/lose by setting it?
I have no problem with SeaMonkey choosing to not use appManaged but that choice shouldn't affect the tests and the fix for this IMO should be on the SeaMonkey side of things especially since it is a single point where this can be fixed vs. having to take this idiosyncrasy into account when new tests are added. The pros / cons of using appManaged are for the most part listed on the page you cited. One thing that isn't clear though it does mention is that the extension manager doesn't check for updates for an extension that is installed into a location that can't be updated so any users not running as admin or running as admin on Vista / Win7 with UAC turned on will not get updates to the extension... I wouldn't be surprised if this is the case for the vast majority of SeaMonkey users. Specifically:
"The EM will check for updates only if the app directory is writable. However, any accepted updates will go to the profile anyway."
would be better written as
"The EM will check for updates only if the app directory is writable. If the directory is writable then any updates will be updated/installed into the profile."

btw: I just talked with Mossop and the extension manager rewrite will likely remove appManaged as well as no longer update extensions installed into the install directory since there is no way of updating extensions installed into this location. For the case where an app would like to provide extensions that can be updated new functionality similar to what was implemented for distributions would likely be added that allows providing extensions that would be installed into the profile along with the ability to add new extensions for existing profiles. For tests he is considering adding the ability to not install these extensions as well as not include extensions from other locations since it can be a PITA when running tests as well as standalone talos.
So, from what you say about the EM rewrite, it doesn't really make sense to coerce the extensions to add appManaged to their install.rdf files (we pull and build directly from their repos, so we'd need to get the change done in those three repos) as it will not be in the 1.9.3 release we are targeting with the next SeaMonkey release.
Turning those tests off for SeaMonkey with an ifndef or such is probably the best idea until that EM rewrite lands and then we should figure out how to move forward then. The current solution is suboptimal in multiple ways, but it won't get better with any change in the current system - and though I doubt there will be a really good solution for our case in the rewritten system, I see it at least being different, so we need to rework things then anyhow.

I know it will probably be some time until all our trunk oranges are fixed but I'd like to move forward on that front.
I understand not wanting to spend resources / time on adding appManaged and thereby using app provided extensions the way they are meant to be used and I hope you understand that I and others don't want to spend resources / time on working around idiosyncracies caused by not using app provided extension the way they are meant to be used. In the future, it would be better if SeaMonkey filed bugs for and help add functionality that is desired from components in toolkit, etc. instead of choosing to do things differently than what is supported / recommended.

Regarding ifndef, my previous comment still applies. Perhaps it is possible for SeaMonkey to patch the test harness(es) so the ifdef's / ifndef's for tests can be managed on the SeaMonkey side?
(In reply to comment #9)
> In the future, it would be better if SeaMonkey
> filed bugs for and help add functionality that is desired from components in
> toolkit, etc. instead of choosing to do things differently than what is
> supported / recommended.

We will surely look into that in the future, but I don't think it makes too much sense to put work into this (across three repositories that we don't control ourselves) as long as the EM rework hasn't landed, as AFAIK it's planned to land for 1.9.3, which is what our next major release is targeted at.
We'll look into how to get things going when this work is in.

Back when we made those decisions, we had to deal with getting SeaMonkey in shape on toolkit at all, we weren't too familiar with what EM wants us to do, and from what Mossop said and what we discussed with the authors of those add-ons we ship, esp. ChatZilla, we agreed to go with what we have today, which we know has its problems as well, and bugs are even filed for them, see bug 300967 and bug 413153. Let's see what the new EM backend brings, perhaps it makes what we want to do completely impossible, wouldn't be the first time toolkit does that to us despite better knowledge. OTOH, perhaps it follows completely different precedents and makes everything possible that we need. Or none of both. I just don't want to revert decisions we made a long time ago for dealing with an EM backend that we won't ship in our next release as it's being replaced.

> Regarding ifndef, my previous comment still applies. Perhaps it is possible for
> SeaMonkey to patch the test harness(es) so the ifdef's / ifndef's for tests can
> be managed on the SeaMonkey side?

I don't think that is reasonably possible, but I don't really know test harness code (other than the fact that most of that is python which usually makes me come to the edge of throwing up).

All I proposed was to temporarily exclude those tests for SeaMonkey until the new EM backend lands and we need to re-think and possibly rework a number of things here anyhow.
Can we re-purpose this bug to be about doing things correctly once the new EM work has been landed, and make it dependent on whatever bug is landing that rework, while we circumvent the current failures temporarily until that new work lands?

Mossop, can you set that dependency? Does this plan sound good to you?
I'd prefer not to morph this since it really is about something quite a bit different and the new work will only involve me on the periphery. The bug as filed is wontfix per product decisions stated in the comments so resolved -> wontfix.

I've already submitted a patch for review for bug 553763... I was able to get to it in between reviews. I don't see any point in disabling the tests bug 553763 references unless you can't wait a few of days for the review.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
(In reply to comment #12)
> I'd prefer not to morph this since it really is about something quite a bit
> different and the new work will only involve me on the periphery.

Well, I'd have thought it's about just the same thing in the new system, but I have no clue either where to track the new system or what we'll need to do there. I guess I'll just have to clone/file a new bug for "whatever we'll need to do when the patch to whatever bug is landed (or its branch merged) on trunk".

> I've already submitted a patch for review for bug 553763... I was able to get
> to it in between reviews. I don't see any point in disabling the tests bug
> 553763 references unless you can't wait a few of days for the review.

Thanks for creating that patch, it surely helps. I was more talking about what we need to do in the future and the other bug we have up there, then. Bug I guess that's all up to other bugs now.
Blocks: 555615
(In reply to comment #13)
> (In reply to comment #12)
> > I'd prefer not to morph this since it really is about something quite a bit
> > different and the new work will only involve me on the periphery.
> 
> Well, I'd have thought it's about just the same thing in the new system, but I
> have no clue either where to track the new system or what we'll need to do
> there. I guess I'll just have to clone/file a new bug for "whatever we'll need
> to do when the patch to whatever bug is landed (or its branch merged) on
> trunk".
This bug is about adding the appManaged attribute for app provided extensions which applies to the release branches and current (possibly future in some form as well) trunk. The new system which I described in comment #7 has not been implemented yet (notice 'will likely' in comment #7) and it only applies to trunk some time after the rewrite. They *are* different in several ways and will likely be substantially different if the implementation in comment #7 is realized in some form.

> > I've already submitted a patch for review for bug 553763... I was able to get
> > to it in between reviews. I don't see any point in disabling the tests bug
> > 553763 references unless you can't wait a few of days for the review.
> 
> Thanks for creating that patch, it surely helps. I was more talking about what
> we need to do in the future and the other bug we have up there, then. Bug I
> guess that's all up to other bugs now.
I see... that sounds like a comment for bug Bug 452942 then.
Blocks: SMAddonMgr
No longer blocks: 555615
You need to log in before you can comment on or make changes to this bug.