Closed Bug 285653 Opened 19 years ago Closed 19 years ago

add reporter tool to nightly builds

Categories

(Other Applications Graveyard :: Reporter, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: jay, Assigned: raccettura)

References

Details

Attachments

(8 files, 7 obsolete files)

13.04 KB, patch
raccettura
: review+
bryner
: superreview+
Details | Diff | Splinter Review
1.86 KB, patch
Details | Diff | Splinter Review
24.80 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
1.36 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
5.46 KB, patch
Details | Diff | Splinter Review
2.11 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
5.16 KB, patch
Details | Diff | Splinter Review
946 bytes, patch
chase
: review+
Details | Diff | Splinter Review
Just like dom inspector, we want to enable the reporter tool in nightly builds.
 We probably want to disable it for release builds, but we can figure out how to
do that later.  For now, I'm just going to do what bryner has outlined and see
if I can get it working.

From my understanding, this is what needs to happen:

1. Get extensions/reporter hooked up to the build system
   - Create Makefile.in and jar.mn to package up reporter.jar
   - Add entry for reporter in mozilla/allmakefiles.sh
2. Make necessary changes in browser/installer/windows and /unix
   - Create new reporter.jst files
   - Add reporter.jar to packages-static
   - Add reporter to ComponentList in installer.cfg
   - Add new reporter section in config.it

I'm completely new to this, so that is all I have for now.  If I'm missing
something, please let me know.  I'm going to attach a rough patch soon.
Sounds good to me.

Just to clarify:  if changes are made within the reporter component (data
collection, transmission, etc), I want to + anything on that level.  Things
regarding build, file organization etc... no problem.

Just want to make sure there are no unexpected changes regarding data.

Other than that... go for the gold.

Regarding disabling in release builds:  should of course consider keeping it, so
we can tell end users to keep out of bugzilla and notify us through this and
talkback.  That's another battle of course.
You rock Jay!  (Hey, I have to be able to express useless-but-supporting
Bugzilla comments somewhere, right?)
+1

/sorry for bug spam
The changes from my original comment have been made and I was able to run my
build and see the reporter item in the help menu.  I could not get the id from
the server, so we might need to look into that.
Attachment #177084 - Flags: superreview?(bryner)
Attachment #177084 - Flags: review?(robert)
Server is busted at the moment after some config changes justdave made a few
weeks ago.  I haven't bothered to fix it since one of the things apparantly
effected, I'm going to be replacing soon (new DB abstraction class)... see bug
285536.  I'm planning to update the reporter-test instance at some point early
next week (if not sooner).

Comment on attachment 177084 [details] [diff] [review]
patch to get reporter working in nightlies

--- browser/installer/unix/config.it	8 Mar 2005 06:39:29 -0000	1.8
+++ browser/installer/unix/config.it	11 Mar 2005 02:12:15 -0000
@@ -631,6 +633,16 @@
 Force Upgrade File0=[SETUP PATH]\chrome\@AB_CD@.jar
 FileCount=$FileCount$

+[Component RPT]
+Description Short=@RPT_SHORT@
+Description Long=@RPT_SHORT@ 
+Archive=reporter.xpi
+$InstallSize$:reporter
+$InstallSizeArchive$:reporter.xpi
+Attributes=SELECTED|VISIBLE|FORCE_UPGRADE
+Force Upgrade File0=[SETUP PATH]\chrome\reporter.jar

The unix installer files need forward slashes in the path.

>--- browser/installer/unix/packages-static	24 Feb 2005 20:11:03 -0000	1.28
>+++ browser/installer/unix/packages-static	11 Mar 2005 02:12:15 -0000
>@@ -314,3 +314,6 @@
> bin/components/libqfaservices.so
> bin/components/qfaservices.xpt
> bin/components/talkback/*
>+
>+[reporter]
>+bin\chrome\reporter.jar

Same here.

Looks ok otherwise, sr=me with those changes.
Attachment #177084 - Flags: superreview?(bryner) → superreview+
Comment on attachment 177084 [details] [diff] [review]
patch to get reporter working in nightlies

Oh yea.

r=me

I'm not reviewing the actual build files.  Only that it doesn't harm the system
as a whole (that's all I'm comfortable doing).

if you want a better r, feel free to ask someone else.	I'm only concered with
data being fubar'd.

I trust bryner's experience.
Attachment #177084 - Flags: review?(robert) → review+
question for bryner (relevent to this bug).

Want to store the reporter URL (currently:
http://reporter-test.mozilla.org/service/) in the prefs, rather than hardcoded,
so it's like update, and other services.

How would that work with the build system?  Would we just hard code it in place
(so regardless of if reporter is enabled it goes in?

Anyway... before this lands, this should be in the prefs if possible:

extension.reporter.url = http://reporter-test.mozilla.org/service

I'll come up with the appropriate reporter patch.
Robert:  Any update on the prefs change?  I'm ready to check in my patch...so
just give me the word.  Thanks.
A few notes here (in particular for Jay):

1.  This patch will make reporter use 2 new prefs (not included in patch:
pref("extensions.reporter.privacyURL",
"http://reporter-test.mozilla.org/privacy/?plain=true");

pref("extensions.reporter.serviceURL",
"http://reporter-test.mozilla.org/service/");

(will change to reporter.mozilla.org when ready)

These are *not* included in the patch (I'm not sure where they go so that they
are only included when building with reporter).  I'm hoping you can take care
of that.

2.  reporter should still be off until for now (you can checkin with it
disabled).  I've got a few kinks I'd rather workout before people start hitting
the dev instance.

3.  Thanks
This patch will break Firefox localizablity.
We should use extensions/reporter/locales/chrome/en-US for locale files probably.
...and speaking of localization, the patch turned tbox orange because you forgot
to define RPT_SHORT in browser/locales/en-US/installer/installer.inc somewhere.
 Add it somewhere in there within the rest of the defines and that'll fix at
least the current bustage (probably all of it, actually, but I didn't actually
test a fix).
OS: Windows XP → All
Hardware: PC → All
Just checked in changes to fix the bustage.  I'll work with Robert to get his
prefs changes in tomorrow.
As it is now Reporter is not enabled by default...you need to specify the
extension in mozconfig.  Once we're ready to enable it by default, I can edit
the configure.in to make it a default extension.
This is apparantly enabled (it's appearing in builds):
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

this needs to be disabled until we get a few other things done first (I have 2
patches that really need to land first).

This should disable the reporter extension until we're ready for prime time.
I'll work with Robert to get the other two checkins in before enabling it
again.
r=bryner, sr=ben ...just checked in the changes.  We shouldn't see Reporter
during the custom install and it should not be installed by default.  Please let
me know if it doesn't work...I'm still new to this, so learning as I go.
Thanks Jay!

I think were going to freeze this for a second until we decide how localization
will be dealt with (officially).  At that point we'll pick up.
source l10n patch for reporter tool. It contains fallback to en-US
Attachment #179880 - Flags: review?(benjamin)
Attachment #179880 - Flags: approval1.8b2?
Comment on attachment 179880 [details] [diff] [review]
source l10n for reporter

a=asa for landing as soon as this has review.
Attachment #179880 - Flags: approval1.8b2? → approval1.8b2+
Attachment #179880 - Flags: review?(benjamin) → review+
I recommend doing this for the URLs, to make for easier testing:

All URLs in the client should be accessed through prefs, not via stringbungle
strings directly.

For l10n reasons you need to do the following:

add a pref to your default prefs file in this format:

pref("foo.bar", "chrome://reporter/locale/blah.properties");

and in blah.properties:

foo.bar=http://reporter.mozilla.org/?blah=%S

or whatever you have.

Then in the chrome code you want to do:

var str = prefBranch.getComplexValue("foo.bar",
Components.interfaces.nsIPrefLocalizedString).data;

Now, QA can change the values of these prefs easily to test servers via
about:config, we can easily deploy update patches that change them if necessary,
etc. 

Comment on attachment 179880 [details] [diff] [review]
source l10n for reporter

checked in
Thanks for that patch.

If anyone wants to tackle Ben's suggestion (comment 21), feel free to do so (and
accept my thanks).  I don't see myself getting to it for a few days due to
school.  I've got a few other little patches lurking as well.
Robert:  I can take a hack at it.  Are there any other changes you don't have
checked in yet for reporter?  If you want to email me your patches, I can make
the changes to my tree and submit one huge patch for review when/if I finish
before you get time to do it.
Jay: if you can take care of that one:  great.  I guess we still need a pref or
two to point to the correct .properties file as well.

I've got a few little things going... but don't worry about them.  I'm merging
the "generate unique ID" screen into the privacy policy and a few little things.

My #1 concern at the moment is localization.  So if that can be taken care of, I
think we are in decent shape.

I'm going to work with justdave on the last server side stuff (production
instance).  Once that's setup and tested I'll patch so we point to production
not dev.

So that's the game plan at this moment:
1.  Localization
2.  Quirks (in the works)
3.  Production

I've got 2 going, and 3 is in que.  If someone can wrap up 1, were covered.
Didn't we already fix l10n?
Attached patch Use preferences (obsolete) — Splinter Review
This is what I originally had in mind.
Removing fallback bits because it doesn't work. L10N teams are forced to
localize those bits to get their builds.
Attachment #180366 - Flags: review?(benjamin)
Comment on attachment 180366 [details] [diff] [review]
remove fallback bits

Add a newline to the end, please.
Attachment #180366 - Flags: review?(benjamin) → review+
Comment on attachment 180366 [details] [diff] [review]
remove fallback bits

r=bsmedberg;a=asa

Checking in extensions/reporter/locales/jar.mn;
/cvsroot/mozilla/extensions/reporter/locales/jar.mn,v  <--  jar.mn
new revision: 1.2; previous revision: 1.1
done
What's left to do here? Reporter is a 1.8b2 requirement (I hope) and we're
running short on time for the release?
Flags: blocking1.8b2+
What's left is figuring out this pref thing (see comment 21).  Attachment 180002 [details] [diff]
was my attempt... though not sure how great that is.

I'm swamped for at least the next week or two, and can't guarantee any time to
work on this (time may of course appear magically).  

Jay sent an email on 4/7 that he had something going as well... perhaps he can
take care of this last essential reporter side patch.

I'll work with justdave on getting a production instance setup.  Then we'll
start pointing builds over to that.


(this timing isn't ideal for me, so pardon my delays and times of disappearance).
Have we communicated to the L10n teams that this needs to be localized? Do we
know what the status is there - how many of them have localized Reporter?
(In reply to comment #33)
> Have we communicated to the L10n teams that this needs to be localized?

Yes.

> Do we
> know what the status is there - how many of them have localized Reporter?

Currently we have 13 locales in CVS trunk, and 9 of them have reporter localized.
Yes, this has been communicated to npm.l10n, and since it's part of the build
system, once bug 287262 is resolved the builds will show red/green/orange
whether it has been localized (along with everything else).
We also need a finalized list of problem types.

*this effects localization*

I need to personally add them as they need to be setup in the server as well. 
At that time I'm going to clear the reporter-test db so we can do some final
testing on a clean slate.
The finalized list of problem types is this:

Browser not supported
Can't log in
Plugin not shown
Other content missing
Behavior wrong
Appearance wrong
Other problem 
Attached patch Add new TypesSplinter Review
Requesting approval to land this
Attachment #181341 - Flags: approval-l10n?
Attachment #181341 - Flags: approval-aviary1.1a?
Comment on attachment 181341 [details] [diff] [review]
Add new Types

a=asa
Attachment #181341 - Flags: approval-l10n?
Attachment #181341 - Flags: approval-l10n+
Attachment #181341 - Flags: approval-aviary1.1a?
Attachment #181341 - Flags: approval-aviary1.1a+
Checking in extensions/reporter/locales/en-US/chrome/reportWizard.dtd;
/cvsroot/mozilla/extensions/reporter/locales/en-US/chrome/reportWizard.dtd,v 
<--  reportWizard.dtd
new revision: 1.2; previous revision: 1.1
done
Checking in extensions/reporter/resources/content/reporter/reportWizard.js;
/cvsroot/mozilla/extensions/reporter/resources/content/reporter/reportWizard.js,v
 <--  reportWizard.js
new revision: 1.5; previous revision: 1.4
done
Checking in extensions/reporter/resources/content/reporter/reportWizard.xul;
/cvsroot/mozilla/extensions/reporter/resources/content/reporter/reportWizard.xul,v
 <--  reportWizard.xul
new revision: 1.3; previous revision: 1.2
done
I've tried to get the prefs stuff working, but have had no luck.  Talkback
related issues are keeping me busy these days, so if we want to get this thing
going, someone else needs to step up and finish this off.  I'll attach the patch
I have (not working), so feel free to work off that.

Once we have the extension tested and working, I'll flip the switch so that it
is in the nightly builds.
Assignee: jay → robert
Attached patch Prefs Patch (obsolete) — Splinter Review
Use the following prefs for url's to reporter server.

pref("extensions.reporter.privacyURL",
"chrome://reporter/locale/reportWizard.properties");
pref("extensions.reporter.serviceURL",
"chrome://reporter/locale/reportWizard.properties");
Attachment #179239 - Attachment is obsolete: true
Attachment #180002 - Attachment is obsolete: true
Attachment #181441 - Attachment is obsolete: true
Comment on attachment 181710 [details] [diff] [review]
Prefs Patch

ben, does this look good?
Attachment #181710 - Flags: review?(bugs)
OK, what patches here need to be checked in?

Also, where are the entries to the default prefs files for the reporter that
connect the pref values to the .properties file in the Prefs Patch? I don't see
them....
I really don't think the URLs ought to be in localizable properties files...
let's just make it a pref and substitute the locale at runtime. There's a
history of localizations making major goofs trying to "translate" URLs.
this patch fixes reporter for seamonkey
Attachment #181949 - Flags: review?(benjamin)
Attachment #181949 - Flags: approval1.8b2?
*** Bug 291695 has been marked as a duplicate of this bug. ***
Can we land this now? Is there anything left to do? Time is getting _very_ short
for 1.8b2 and I _really_ want to see this make the 1.1 alpha.
Comment on attachment 181949 [details] [diff] [review]
Patch for seamonkey (Checked in)

gandalf, feel free to land the seamonkey change as soon as possible.
Attachment #181949 - Flags: approval1.8b2? → approval1.8b2+
Attached patch Use Preferences (obsolete) — Splinter Review
Attachment #181710 - Attachment is obsolete: true
Attachment #182111 - Flags: review?
Attachment #182111 - Flags: approval-l10n?
Attachment #182111 - Flags: approval-aviary1.1a?
Attachment #182111 - Flags: approval-aviary1.1a? → approval1.8b2?
Attachment #181710 - Flags: review?(bugs)
Comment on attachment 182111 [details] [diff] [review]
Use Preferences

doesn't require further l10n approval, a=asa for 1.8b2. 

Bens are not available at the moment. Who else can review these two simple
patches so we can land this thing?
Attachment #182111 - Flags: approval1.8b2?
Attachment #182111 - Flags: approval1.8b2+
Attachment #182111 - Flags: approval-l10n?
Comment on attachment 182111 [details] [diff] [review]
Use Preferences


>+ var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                        .getService(Components.interfaces.nsIPrefService)
>+                     	.getBranch("extensions.reporter.");

nit: spacing, but that's common in this file

>   var soapCall = new SOAPCall();
>-  soapCall.transportURI = RMOURI;
>+  soapCall.transportURI = prefs.getCharPref("serviceURL");

getCharPref can throw here.  What breaks if it does?  Unless failure here is
handled, try/catch this part of the code please, and the other getCharPref
(falling back to the hardcoded values is ok for this case)

any call to prefs can fail, there should be a fallback case always.

r=me with those fixed.
Attachment #182111 - Flags: review? → review+
Attached patch Prefs Patch (obsolete) — Splinter Review
Address mconnor's comments
Attachment #182111 - Attachment is obsolete: true
Attached patch Use preferencesSplinter Review
Attachment #182121 - Attachment is obsolete: true
Comment on attachment 182123 [details] [diff] [review]
Use preferences

a=asa
Attachment #182123 - Flags: approval1.8b2+
Status: NEW → ASSIGNED
(In reply to comment #50) 
> (From update of attachment 181949 [details] [diff] [review] [edit]) 
> gandalf, feel free to land the seamonkey change as soon as possible.  
 
Can someone do this on behalf of me? I'm not on my box now. 
If not, I'll check this in tommorow. 
 
Comment on attachment 182123 [details] [diff] [review]
Use preferences

+  $(INSTALL) $(srcdir)/resources/content/prefs/reporter.js
$(DIST)/bin/defaults/pref

PREF_JS_EXPORTS?
Attachment #181949 - Flags: review?(benjamin) → review+
Comment on attachment 181949 [details] [diff] [review]
Patch for seamonkey (Checked in)

Checking in jar.mn;
/cvsroot/mozilla/extensions/reporter/locales/jar.mn,v  <--  jar.mn
new revision: 1.4; previous revision: 1.3
done
Checking in generic/chrome/contents.rdf;
/cvsroot/mozilla/extensions/reporter/locales/generic/chrome/contents.rdf,v  <--
 contents.rdf
new revision: 1.3; previous revision: 1.2
done
Attachment #181949 - Attachment description: Patch for seamonkey → Patch for seamonkey (Checked in)
Would someone verify the gandalf's patch (comment 47) for seamonkey worked.

This bug should stay open until we enable.
Can we enable today? Time is getting short on 1.1a.
(In reply to comment #60)
> Would someone verify the gandalf's patch (comment 47) for seamonkey worked.

Well, at least there is no parse error displayed in browser windows anymore (I
applied the patch manually since CVS mirrors seem to be updating slowly).

Anything else to be tested?
Robert:  We should probably get it enabled today or tomorrow and see if
everything is working.  If you need me to check in the changes, let me know. 
Otherwise you can just revert back the changes I made in
https://bugzilla.mozilla.org/attachment.cgi?id=179810
Jay: re-enable, and close this bug.

All regressions from here on out go in new bugs under the reporter component.

We will point to reporter-test for at least the first builds until we see things
are going ok.  Please test during htis period.  If all goes well I'll point
builds from there after to the production database.  The move to production will
be bug 292696.

Thanks to all who have contributed to this bug.  Were extremely close.  Once
were building, and test against reporter-test we'll make the switch.
Is this supposed to really be on in firefox for real now?  It's not (at least on
linux).

1. it's not being built on linux according to the ocean build logs.  reporter is
not in the list of default extensions and is not explicitly listed in
about:buildconfig

2. it doesn't seem to be built on windows based on
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1115350920.17763.gz&fulltext=1

3. the unix config.it is missing a line.  I'll attach a patch for that.
Attached patch fix config.it (obsolete) — Splinter Review
Attachment #182750 - Flags: review?(benjamin)
Andrew: we are *not* enabled as of yet pending a fix for bug 292804.

When that is, we will enable it.
Depends on: 292804
Attachment #182750 - Flags: review?(benjamin) → review+
Attachment #182750 - Flags: approval-aviary1.1a?
if we need this config.it fix, a=asa.
Attachment #182750 - Flags: approval-aviary1.1a? → approval-aviary1.1a+
Comment on attachment 182750 [details] [diff] [review]
fix config.it

this is in
Attachment #182750 - Attachment is obsolete: true
We're coming up quickly on 1.1a1/1.8b2 and if this doesn't make it, then we'll
try again for 1.1a2/1.8b3. 
Flags: blocking1.8b2+ → blocking1.8b3+
I've checked in bsmedberg's suggested fix in bug 292804.  It worked for him, so
let's pray that Reporter is working with tomorrow's build. If not, it's back to
the drawing board and we'll have to find some help in getting this thing done.
> I've checked in bsmedberg's suggested fix in bug 292804.  It worked for him, so
> let's pray that Reporter is working with tomorrow's build. If not, it's back to
> the drawing board and we'll have to find some help in getting this thing done.

I'm not sure what you're expecting. If I try to install one of the hourly
installer builds from
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/
(http://tinderbox.mozilla.org/Firefox/ tells you what platform the machines are
building), and I do not deselect the reporter option, it displays some "can't
find reporter.jar" error message.

That's because we're still not building reporter by default. So if you want to
build Reporter in your own tree, you have to add this line to your mozconfig:
  ac_add_options --enable-extensions=default,reporter

With that, "make installer" in mozilla/browser/installer creates an installer
which includes Reporter. If you don't deselect the Reporter option, the build
displays the Report item in the Help menu.
Just in case it isn't clear how to build an extension by default, you have to
add it to configure.in these days.

For 1.8a2, we need either this patch, or disable Reporter again in the
installer. Otherwise, the installer will display an error because it can't
find/register Reporter.
Attachment #183724 - Flags: review?(mconnor)
Flags: blocking1.8b2?
Correct, I believe we also need to re-enable attachment 179810 [details] [diff] [review]

Jay:  go ahead and turn it on.
Jay "enabled" Reporter on 2005-05-03 12:49. But unlike before, he only added it
to the custom install, but not to the standard install, and also disobeyed the
comment about QFA having to be the last in the list:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/installer/windows/config.it&mark=209-210,220-222,225,236-239#209
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/installer/unix/config.it&mark=215-216,226-228,231,242-245#215
Steffen: You are a lifesaver!  I was wondering if I had to add reporter to some
other build config file.  Thanks for your input and the patch. Once you get a
review for your patch, do you want to check it in?  Or should I?  

Also, I asked Asa about enabling reporter for Default installs, and we decided
to only include it in Custom installs.  Regarding the order of the QFA and RPT,
I was going to update that comment to include RPT because both Talkback and
Reporter are tools that others might not want to include in their builds (so
those two items are fine where they are...at the end of the list).

Asa: If this is the last change we need to get this working, let's get it in for
1.8b2.
For 1.8b2/1.1a1 I'd like to have this be a custom option. For 1.1a2 if
everything works out win 1.1a1 testing, then we'll enable by default. Let's get
this sucker in! :D
Steffen: I had a question about the configure.in change.  When I first hooked up
Reporter, I did add "reporter" to the MOZ_EXTENSIONS_ALL list in configure.in. 
Was that not the right place to put it?
the MOZ_EXTENSIONS_ALL only gets used if you do --enable-extensions=all.  If you
don't do that it uses the DEFAULT for whatever product you're building.  It's
also possible to leave configure alone and do
--enable-extensions=default,reporter (on the tinderbox) if you think other
people who build from source should not build reporter by default.
(In reply to comment #76)
> I was going to update that comment to include RPT because both Talkback and
> Reporter are tools that others might not want to include in their builds (so
> those two items are fine where they are...at the end of the list).

Since that's not code, it's not as urgent, but lets get that done sooner than later.
Attachment #183724 - Flags: review?(mconnor) → review?(chase)
Feel free to check in the patch once it has review and approval. And we should
really try to get that one-liner in before 1.8b2.

If we want Reporter to be an option in custom installs, as Asa said, no more
changes should be needed - apart from correcting that comment.
Comment on attachment 183724 [details] [diff] [review]
build reporter by default by adding it to configure.in

a=asa pending review.
Attachment #183724 - Flags: approval1.8b2+
Attachment #183724 - Flags: approval-aviary1.1a1+
Comment on attachment 183724 [details] [diff] [review]
build reporter by default by adding it to configure.in

r=chase
Attachment #183724 - Flags: review?(chase) → review+
Comment on attachment 183724 [details] [diff] [review]
build reporter by default by adding it to configure.in

Checked in.
removing blocking flags since this is landed.
Flags: blocking1.8b3+
Flags: blocking1.8b2?
Resolved Fixed.  After I made my last checkin, I was able to do a fresh pull and
get a build with Reporter working.  Please test with 5/17 builds to verify that
we are good to go.  Thanks to everyone that helped me and Robert out in getting
this in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Looks good on Firefox trunk builds from 05/17
Status: RESOLVED → VERIFIED
Product: Other Applications → Other Applications Graveyard
You need to log in before you can comment on or make changes to this bug.