416680 - Mozilla Crash Reporter should use branding

Status: NEW

(Toolkit :: Crash Reporting, defect)

Description

[Andrés Delfino]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008021003 Minefield/3.0b4pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008021003 Minefield/3.0b4pre

Mozilla Crash Reporter should use branding.

See attached screenshot.

Reproducible: Always

Comment 1

[Andrés Delfino]

Attachment: Screen shot.

Note that Firefox should say Minefield.

Comment 2

[(not currently active) Ted Mielczarek]

It's a known issue, but it's difficult to fix.  I'm not overly concerned about it, given the unlikeliness of anyone actually using the crash reporter + unofficial branding.

Comment 4

[Reed Loden [:reed]]

Attachment: patch - v1

Comment 5

[Benjamin Smedberg]

Comment on attachment 380398 [details] [diff] [review]
patch - v1

I don't think run-time substitution is the correct answer. We should use build-time substitution if anything.

Comment 6

[Reed Loden [:reed]]

(In reply to comment #5)
> (From update of attachment 380398 [details] [diff] [review])
> I don't think run-time substitution is the correct answer. We should use
> build-time substitution if anything.

build-time how? AnnotateCrashReport() stores it in memory, aiui... are you saying I should store the ProductName in crashreporter.ini/crashreporter-override.ini and do it that way?

Comment 7

[Benjamin Smedberg]

Just use the preprocessor to substitute MOZ_APP_DISPLAYNAME wherever it currently says literal Firefox.

Comment 8

[Reed Loden [:reed]]

Attachment: patch - v2

Something like this?

Comment 9

[Reed Loden [:reed]]

Comment on attachment 381727 [details] [diff] [review]
patch - v2

>+    CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("ProductName"),
>+                                       NS_STRINGIFY(MOZ_APP_DISPLAYNAME));

Pretend the NS_STRINGIFY is just NS_LITERAL_CSTRING. NS_STRINGIFY doesn't compile in this case. ;)

Comment 10

[Benjamin Smedberg]

Comment on attachment 381727 [details] [diff] [review]
patch - v2

It looks like you're still adding an %s... why? And why the change to appData.name? That code is in XULrunner and really must load the data dynamically.

Comment 11

[Reed Loden [:reed]]

(In reply to comment #10)
> (From update of attachment 381727 [details] [diff] [review])
> It looks like you're still adding an %s... why? And why the change to
> appData.name? That code is in XULrunner and really must load the data
> dynamically.

The original crashreporter.ini uses two %s string replacements (http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/crashreporter/crashreporter.ini#13). The crash reporter code has checks built-in for this weirdness (http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/crashreporter.cpp#402). I'm just bringing Firefox's crashreporter-override.ini file back into agreement with the original so replacement for the app name is done based on the branding used instead of a hardcoded "Firefox".

As for appData.name, my previous patch took it into account.

So, you still want to do what you mentioned in comment #7 then and just leave the XULRunner code alone? Maybe I was just trying to complicate this too much...

Comment 12

[Benjamin Smedberg]

Yes, a build-time substitution of @MOZ_APP_DISPLAYNAME@ and don't touch the XULRunner code.

Comment 13

[(not currently active) Ted Mielczarek]

This actually turns out to be important for Fennec, because there the Name from application.ini is always Fennec, but the releases are "Firefox".

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.