Issue 29342994: Issue 2490 - Investigate another way to disable the crash reporter (Closed)
|
Created:
May 24, 2016, 2:34 p.m. by diegocarloslima Modified:
Oct. 25, 2016, 3:40 p.m. CC:
René Jeschke Visibility:
Public. |
DescriptionIssue 2490 - Investigate another way to disable the crash reporter
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removing comments #
Total comments: 2
Patch Set 3 : Force disable crashreporter without removing upstream code #
Total comments: 2
Patch Set 4 : Adding empty line #MessagesTotal messages: 14
https://codereview.adblockplus.org/29342994/diff/29342995/configure.in File configure.in (right): https://codereview.adblockplus.org/29342994/diff/29342995/configure.in#newcod... configure.in:6043: # case $target in should not we remove the lines instead of commenting out?
https://codereview.adblockplus.org/29342994/diff/29342995/configure.in File configure.in (right): https://codereview.adblockplus.org/29342994/diff/29342995/configure.in#newcod... configure.in:6043: # case $target in On 2016/09/12 11:26:57, anton wrote: > should not we remove the lines instead of commenting out? Yeah, you're right. I'll fix that.
On 2016/09/14 23:31:24, diegocarloslima wrote: > https://codereview.adblockplus.org/29342994/diff/29342995/configure.in > File configure.in (right): > > https://codereview.adblockplus.org/29342994/diff/29342995/configure.in#newcod... > configure.in:6043: # case $target in > On 2016/09/12 11:26:57, anton wrote: > > should not we remove the lines instead of commenting out? > > Yeah, you're right. I'll fix that. Any update?
On 2016/09/30 06:27:09, anton wrote: > On 2016/09/14 23:31:24, diegocarloslima wrote: > > https://codereview.adblockplus.org/29342994/diff/29342995/configure.in > > File configure.in (right): > > > > > https://codereview.adblockplus.org/29342994/diff/29342995/configure.in#newcod... > > configure.in:6043: # case $target in > > On 2016/09/12 11:26:57, anton wrote: > > > should not we remove the lines instead of commenting out? > > > > Yeah, you're right. I'll fix that. > > Any update? I've just submitted a patch
On 2016/10/20 10:06:41, diegocarloslima wrote: > On 2016/09/30 06:27:09, anton wrote: > > On 2016/09/14 23:31:24, diegocarloslima wrote: > > > https://codereview.adblockplus.org/29342994/diff/29342995/configure.in > > > File configure.in (right): > > > > > > > > > https://codereview.adblockplus.org/29342994/diff/29342995/configure.in#newcod... > > > configure.in:6043: # case $target in > > > On 2016/09/12 11:26:57, anton wrote: > > > > should not we remove the lines instead of commenting out? > > > > > > Yeah, you're right. I'll fix that. > > > > Any update? > > I've just submitted a patch LGTM
https://codereview.adblockplus.org/29342994/diff/29358371/configure.in File configure.in (right): https://codereview.adblockplus.org/29342994/diff/29358371/configure.in#newcod... configure.in:6045: MOZ_CRASHREPORTER= Could we also empty this without removing all this code? We typically try to keep changes to upstream code minimal to avoid conflicts.
On 2016/10/20 13:27:03, Felix Dahlke wrote: > https://codereview.adblockplus.org/29342994/diff/29358371/configure.in > File configure.in (right): > > https://codereview.adblockplus.org/29342994/diff/29358371/configure.in#newcod... > configure.in:6045: MOZ_CRASHREPORTER= > Could we also empty this without removing all this code? We typically try to > keep changes to upstream code minimal to avoid conflicts. Oh, and when we don't need --disable-crashreporter anymore, we can: 1. Remove that part from the build instructions in README.md. 2. Remove the check from adblockbrowser-build/build.py 3. Remove the option from adblockbrowser-build/mozconfig-* Feel free to create a follow-up issue for #2 and #3 since it's in a different repository, but #1 we should tackle while at it I'd say.
On 2016/10/20 13:31:52, Felix Dahlke wrote: > On 2016/10/20 13:27:03, Felix Dahlke wrote: > > https://codereview.adblockplus.org/29342994/diff/29358371/configure.in > > File configure.in (right): > > > > > https://codereview.adblockplus.org/29342994/diff/29358371/configure.in#newcod... > > configure.in:6045: MOZ_CRASHREPORTER= > > Could we also empty this without removing all this code? We typically try to > > keep changes to upstream code minimal to avoid conflicts. > > Oh, and when we don't need --disable-crashreporter anymore, we can: > > 1. Remove that part from the build instructions in README.md. > 2. Remove the check from adblockbrowser-build/build.py > 3. Remove the option from adblockbrowser-build/mozconfig-* > > Feel free to create a follow-up issue for #2 and #3 since it's in a different > repository, but #1 we should tackle while at it I'd say. For the record, Diego pointed me to a dedicated issue/review for the README change (https://issues.adblockplus.org/ticket/3767), so a follow-up issue for #2 and #3 would make me happy. (That and a response to my inline comment.)
On 2016/10/21 11:26:55, Felix Dahlke wrote: > On 2016/10/20 13:31:52, Felix Dahlke wrote: > > On 2016/10/20 13:27:03, Felix Dahlke wrote: > > > https://codereview.adblockplus.org/29342994/diff/29358371/configure.in > > > File configure.in (right): > > > > > > > > > https://codereview.adblockplus.org/29342994/diff/29358371/configure.in#newcod... > > > configure.in:6045: MOZ_CRASHREPORTER= > > > Could we also empty this without removing all this code? We typically try to > > > keep changes to upstream code minimal to avoid conflicts. > > > > Oh, and when we don't need --disable-crashreporter anymore, we can: > > > > 1. Remove that part from the build instructions in README.md. > > 2. Remove the check from adblockbrowser-build/build.py > > 3. Remove the option from adblockbrowser-build/mozconfig-* > > > > Feel free to create a follow-up issue for #2 and #3 since it's in a different > > repository, but #1 we should tackle while at it I'd say. > > For the record, Diego pointed me to a dedicated issue/review for the README > change (https://issues.adblockplus.org/ticket/3767), so a follow-up issue for #2 > and #3 would make me happy. (That and a response to my inline comment.) I have just submitted a patch that keeps the upstream code. Also, created the follow up issues: https://issues.adblockplus.org/ticket/4566 and https://issues.adblockplus.org/ticket/4567
https://codereview.adblockplus.org/29342994/diff/29358371/configure.in File configure.in (right): https://codereview.adblockplus.org/29342994/diff/29358371/configure.in#newcod... configure.in:6045: MOZ_CRASHREPORTER= On 2016/10/20 13:27:03, Felix Dahlke wrote: > Could we also empty this without removing all this code? We typically try to > keep changes to upstream code minimal to avoid conflicts. Acknowledged.
LGTM, up to you whether you want to address my nit. https://codereview.adblockplus.org/29342994/diff/29359876/configure.in File configure.in (right): https://codereview.adblockplus.org/29342994/diff/29359876/configure.in#newcod... configure.in:6076: # Force disable crashreporter Ultra nit: I guess I'd add an empty line on top of this in the general spirit of the file, but wouldn't insist :)
https://codereview.adblockplus.org/29342994/diff/29359876/configure.in File configure.in (right): https://codereview.adblockplus.org/29342994/diff/29359876/configure.in#newcod... configure.in:6076: # Force disable crashreporter On 2016/10/25 15:15:03, Felix Dahlke wrote: > Ultra nit: I guess I'd add an empty line on top of this in the general spirit of > the file, but wouldn't insist :) Ok, I'll add that 
|
