299040 - Package talkback and reporter as global extensions

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P1)

Description

[Darin Fisher]

Package talkback and reporter as global extensions

This will greatly simply the implementation of bug 299021.  It will also improve
the user experience IMO since users will observe a one-to-one relation between
stuff they optionally chose to install and stuff listed in the extension manager.

I discussed moving talkback with bryner, and he seemed to think that it
shouldn't be an issue.  talkback apparently just needs all of its components to
reside alongside the qfaservices component library.

Moving reporter should be a breeze since it is just a chrome package.

bsmedberg asked whether we wanted to have app update udpate these components or
have them update via UMO.  IMO, it makes better sense to update them via app
update since users who never install an extension would be confused to see UI
about extensions being out of date if we update them to a new major version of
the product.  Also, since we ship these extensions with firefox, it just makes
sense to update them along with the rest of the app.

Comment 1

[Darin Fisher]

Attachment: Make reporter a global extension, v1

Comment 2

[Benjamin Smedberg]

Comment on attachment 187742 [details] [diff] [review]
Make reporter a global extension, v1

This is not enough. We need to move the localized files from en-US.jar to
reporter.jar and register them that way. It should be a fairly simple change in
extensions/reporter/jar.mn removing the @AB_CD@.jar line.

Comment 3

[Darin Fisher]

why is this not enough?  the reporter still works?  and i can disable it.  i
understand that it is not completely separated, but it is sufficient to enable
us to not add reporter to someones build (via app update) if they didn't want
it.  see the bug that is blocked by this bug.

Comment 4

[Benjamin Smedberg]

Comment on attachment 187742 [details] [diff] [review]
Make reporter a global extension, v1

ok, r=me with the assurance that we'll do updateURL="about:blank" or something
so that extension update will not try to upgrade this.

Comment 5

[Darin Fisher]

I set an updateURL property with value about:blank on the install.rdf included
in this patch to prevent the reporter hosted on UMO from overwriting the one
shipped with Firefox.  If we did not prevent this, then partial app update would
be broken if a different version of reporter was installed on top of the one
that shipped with Firefox.

Comment 6

[Darin Fisher]

reporter changes are in with updateURL = about:blank

Comment 7

[Ryan VanderMeulen [:RyanVM]]

With the currently checked-in patch, a reporter directory is still created
inside the chrome folder. The subdirectories within are empty, but they still
shouldn't be there.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

(In reply to comment #7)
> With the currently checked-in patch, a reporter directory is still created
> inside the chrome folder. The subdirectories within are empty, but they still
> shouldn't be there.

OK, apparently my post was a bit premature. I've noticed that all the
directories corresponding to the various JAR files are empty. Whether or not
that's intended behavior, I don't know.

Anyway, sorry for the bugspam. Move along, nothing to see here.

Comment 9

[Darin Fisher]

It is intentional and benign :-)

Comment 10

[Robert Strong (they/them - no direct email)]

(In reply to comment #5)
> I set an updateURL property with value about:blank on the install.rdf included
> in this patch to prevent the reporter hosted on UMO from overwriting the one
> shipped with Firefox.  If we did not prevent this, then partial app update would
> be broken if a different version of reporter was installed on top of the one
> that shipped with Firefox.
This is seen as an error in the extension update code and is reported as such
when the update check completes. Perhaps special casing a string value (e.g.
"none") which has an obvious meaning, etc.

Comment 11

[Jim Jeffery not reading bug-mail 1/2/11]

The 'Report Web Site' option is missing under 'Help' since moving this to an
extension. 

See bug: https://bugzilla.mozilla.org/show_bug.cgi?id=299573

Comment 12

[Chase Phillips]

Attachment: Move references to Talkback files into extensions/ directory - v1 [browser/mail parts checked in]

This should cover Firefox, Thunderbird (on Windows), and the Suite.  Let me
know if this patch doesn't cover them accurately.

I have the corresponding Talkback client build changes queued up and ready to
go so we can land these changes in parallel.

Comment 13

[Darin Fisher]

Comment on attachment 188376 [details] [diff] [review]
Move references to Talkback files into extensions/ directory - v1 [browser/mail parts checked in]

looks good, r=darin

Comment 14

[Benjamin Smedberg]

Comment on attachment 188376 [details] [diff] [review]
Move references to Talkback files into extensions/ directory - v1 [browser/mail parts checked in]

Seamonkey does not know anything about bin/extensions and so it makes no sense
to package talkback as an extension for seamonkey. We should only be changing
the packaging files for browser/mail (those parts are r=me)

Comment 15

[Darin Fisher]

good catch benjamin!

Comment 16

[Chase Phillips]

If Suite Talkback client needs to stay in bin/components, more changes will be
needed to the way the Talkback client is built since there's currently no easy
way to toggle how its built based on what product it's being built into.

(For example, the Makefiles for Talkback don't reuse mozilla/ build
infrastructure.  There's a Makefile per platform.)

Talkback build should be reworked so it's easier to modify in the future, but we
don't have time for that atm.  Let me take another look to see if there's a fast
way to do this.

Comment 17

[Chase Phillips]

With bsmedberg's help found a quick way to do this.  Landing browser/mail parts...

Comment 18

[Chase Phillips]

Comment on attachment 188376 [details] [diff] [review]
Move references to Talkback files into extensions/ directory - v1 [browser/mail parts checked in]

Checking in browser/installer/windows/packages-static;
/cvsroot/mozilla/browser/installer/windows/packages-static,v  <-- 
packages-static
new revision: 1.51; previous revision: 1.50
done
Checking in browser/installer/unix/packages-static;
/cvsroot/mozilla/browser/installer/unix/packages-static,v  <--	packages-static
new revision: 1.48; previous revision: 1.47
done
Checking in mail/installer/windows/basemail-win;
/cvsroot/mozilla/mail/installer/windows/basemail-win,v	<--  basemail-win
new revision: 1.43; previous revision: 1.42
done

Comment 19

[Darin Fisher]

OK, then I think we can mark this bug fixed :-)

Comment 20

[Chase Phillips]

After reviewing the build logs on Windows I saw three files were missing:
  qfaservices.dll
  qfaservices.xpt
  master.ini

I also altered the build process for these three items so they would be placed
into the appropriate Talkback directory.  Local tests for the first two files
appeared to work.  master.ini should also be resolved but to be certain I will
verify in tomorrow morning's release build.

Still need to verify Talkback works as expected on Windows, Mac, Linux from
tomorrow's builds, too.

Comment 21

[Chase Phillips]

Based on feedback from bryner and examining the Talkback module, files for Mac
need to be placed in a talkback/ subdir inside components.  After bryner made
this change locally Talkback launched as expected.  I've made this change in the
module and am respinning the Fx Mac build.

Comment 22

[Robert Strong (they/them - no direct email)]

(In reply to comment #21)
Chase - could you add <appManaged>true</appManaged> to the talkback install.rdf?
Now that bug 299887 has landed this will prevent it from being checked for
updates. Here is an example of what I am referring to:
http://lxr.mozilla.org/seamonkey/source/extensions/inspector/install.rdf#35

Comment 23

[Chase Phillips]

(In reply to comment #22)
> (In reply to comment #21)
> Chase - could you add <appManaged>true</appManaged> to the talkback install.rdf?
> Now that bug 299887 has landed this will prevent it from being checked for
> updates. Here is an example of what I am referring to:
> http://lxr.mozilla.org/seamonkey/source/extensions/inspector/install.rdf#35

Done.