cmtalbert Reporter
	Description • 13 years ago

Currently the profile will unpack any addon it encounters.  It ought to just unpack them selectively.  I have pulled the unpack attribute out of the RDF, now we just have to use it...

	Jeff Hammel
	Comment 1 • 13 years ago

see also bug 682683

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 2 • 13 years ago

Keep in mind that we still support Firefox 3.6.x which is not able to handle XPI files directly. So we can't go forward here until we drop support.

	cmtalbert Reporter
	Comment 3 • 13 years ago

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Keep in mind that we still support Firefox 3.6.x which is not able to handle
> XPI files directly. So we can't go forward here until we drop support.

There is nothing wrong with adding this support to mozprofile so that it does the right thing upon encountering the option.  We're not going to backport this to 1.5.x so, it should be fine going forward (since 1.9.2 support isn't really going to live past the 1.5.x codebase).

	Jeff Hammel
	Comment 4 • 13 years ago

Also, since mozrunner has the application version (in most cases), we could check for this

	Jeff Hammel
	Comment 5 • 13 years ago

Have we dropped FF3.6 support for mozbase going forward?  What is the plan here?

	Jeff Hammel
	Comment 6 • 13 years ago

Come to think of it, why do we care about this bug?

	Jeff Hammel
	Comment 7 • 13 years ago

I'm actually less sure why we want this patch, but here it is nonetheless

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 8 • 13 years ago

Comment on attachment 571508 [details] [diff] [review]
install packed addons for mozprofile

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

just a few questions/nits.  Either answer in bugmail or in comments.

::: mozprofile/mozprofile/addons.py
@@ +126,2 @@
>      @classmethod
> +    def get_addon_id(cls, addon_path):

why do we get rid of the self here, I still see the @classmethod attribute for this function, confusing.

@@ +184,5 @@
>                  details.update({ entry: get_text(node) })
>  
> +        # turn unpack into a true/false value
> +        if isinstance(details['unpack'], basestring):
> +            details['unpack'] = details['unpack'].lower() == 'true'

can we always assume unpack is a string and true/false?  what if it is an int or already a boolean?

@@ +236,5 @@
>                  addon = tmpdir
>  
>              # determine the addon id
> +            addon_details = AddonManager.addon_details(addon)
> +            addon_id = addon_details.get('id')

we still have get_addon_id, why are we not using it, or better yet why do we still have it?

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 9 • 13 years ago

(In reply to Jeff Hammel [:jhammel] from comment #7)
> I'm actually less sure why we want this patch, but here it is nonetheless

It's at least important for one of our test-runs when add-ons are getting installed before Mozmill starts Firefox. That way we can ensure that everything works as expected.

Also we do not have to take care of Firefox 3.6 here, because it doesn't depend on MozBase.

	Jeff Hammel
	Comment 10 • 13 years ago

(In reply to Joel Maher (:jmaher) from comment #8)
> Comment on attachment 571508 [details] [diff] [review] [diff] [details] [review]
> install packed addons for mozprofile
> 
> Review of attachment 571508 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> just a few questions/nits.  Either answer in bugmail or in comments.
> 
> ::: mozprofile/mozprofile/addons.py
> @@ +126,2 @@
> >      @classmethod
> > +    def get_addon_id(cls, addon_path):
> 
> why do we get rid of the self here, I still see the @classmethod attribute
> for this function, confusing.

The first argument you pass to a bound function -- either a class method or an instance method -- can be called anything.  By convention, cls is used for classmethods and self is used for instance methods: http://docs.python.org/library/functions.html#classmethod . 

> @@ +184,5 @@
> >                  details.update({ entry: get_text(node) })
> >  
> > +        # turn unpack into a true/false value
> > +        if isinstance(details['unpack'], basestring):
> > +            details['unpack'] = details['unpack'].lower() == 'true'
> 
> can we always assume unpack is a string and true/false?  what if it is an
> int or already a boolean?

I am mostly following what talos does already: http://hg.mozilla.org/build/talos/file/214df9ee2ea7/talos/ffsetup.py#l196  In the case where details['unpack'] is an int or a bool, the isinstance() check will return False and you won't get to the inner block.

> @@ +236,5 @@
> >                  addon = tmpdir
> >  
> >              # determine the addon id
> > +            addon_details = AddonManager.addon_details(addon)
> > +            addon_id = addon_details.get('id')
> 
> we still have get_addon_id, why are we not using it, or better yet why do we
> still have it?

Good point!  We don't need it.  I'll delete before pushing.  I wouldn't mind having the convenience function, but it is not a free calculation (unless we were to cache it), and as you point out...we don't really need it.

	Jeff Hammel
	Comment 11 • 13 years ago

(In reply to Henrik Skupin (:whimboo) from comment #9)
> (In reply to Jeff Hammel [:jhammel] from comment #7)
> > I'm actually less sure why we want this patch, but here it is nonetheless
> 
> It's at least important for one of our test-runs when add-ons are getting
> installed before Mozmill starts Firefox. That way we can ensure that
> everything works as expected.

I'm not sure what difference unpacking them makes for getting them installed. Can you elaborate?

> Also we do not have to take care of Firefox 3.6 here, because it doesn't
> depend on MozBase.

No, this is a 3.6 potentially breaking change. I added a function argument where by unpack can be forced off, though I actually messed that up and need to fix it.  In addition, it is not consumed.  If we're targeting 3.6 with mozbase trunk, then we should file some follow-up bugs, but I am content supporting 4.+

	Jeff Hammel
	Comment 12 • 13 years ago

> I added a function argument where by unpack can be forced off, though I actually messed that up and need to fix it.

I lied, my code is fine, was just reading it wrong ;)

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 13 • 13 years ago

(In reply to Jeff Hammel [:jhammel] from comment #11)
> > It's at least important for one of our test-runs when add-ons are getting
> > installed before Mozmill starts Firefox. That way we can ensure that
> > everything works as expected.
> 
> I'm not sure what difference unpacking them makes for getting them
> installed. Can you elaborate?

Well, the best would be to leave this totally up to Firefox. I have made some more thoughts about it and why can't we simply retrieve the add-on id from the install.rdf file, rename the XPI accordingly and store it under the extensions folder? Firefox >4 will only unpack if necessary, and Fx 3.6 should always unpack the extension. At least manually it works, and I haven't tested the automated way.

> > Also we do not have to take care of Firefox 3.6 here, because it doesn't
> > depend on MozBase.
> 
> No, this is a 3.6 potentially breaking change. I added a function argument
> where by unpack can be forced off, though I actually messed that up and need
> to fix it.  In addition, it is not consumed.  If we're targeting 3.6 with
> mozbase trunk, then we should file some follow-up bugs, but I am content
> supporting 4.+

Which other tools make use of mozBase currently?

	Jeff Hammel
	Comment 14 • 13 years ago

pushed: https://github.com/mozilla/mozbase/commit/e743571d485bc1988ab3017e254f79e9dff0b532

	Jeff Hammel
	Comment 15 • 13 years ago

> Which other tools make use of mozBase currently?

Slated, talos, mozregression, eideticker, mozmill, and a few others.  Sadly, I don't think we've kept a list.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 16 • 13 years ago

Jeff, so I would propose to check my above mentioned solution which should be more future proof. You should file a bug on it, and I can offer my help in whatever information you need about the add-ons manager and handling add-ons.

	Jeff Hammel
	Comment 17 • 13 years ago

Your idea is already exactly what the patch does, AIUI