Drop even more redundant field initialisers

RESOLVED FIXED in Firefox 34

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ckitching, Assigned: ckitching)

Tracking

unspecified
Firefox 34
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

4 years ago
See bug 1041836.

Two things occurred to me:

1) I found a regex bug that caused some of the problems detected by the analyser to be omitted from the patch, so I'm providing a patch here to finish the job.

2) I gave the analyser the ability to detect equivalently-redundant assignments given explicitly in constructors (also not fixable by proguard, as this is to what the other sort of initialiser is converted at compile-time anyway).

It turns out we have quite a few of these: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#96
(Assignee)

Comment 1

4 years ago
Created attachment 8466754 [details] [diff] [review]
Part 1/3: Delete redundant initialisers-to-default-values from constructors
Attachment #8466754 - Flags: review?(rnewman)
(Assignee)

Comment 2

4 years ago
Created attachment 8466755 [details] [diff] [review]
Part 2/3: Delete redundant initialisers-to-default-values from constructors (android-sync)
Attachment #8466755 - Flags: review?(rnewman)
(Assignee)

Comment 3

4 years ago
Created attachment 8466756 [details] [diff] [review]
Part 3/3: Delete even more redundant field initialisers
Attachment #8466756 - Flags: review?(rnewman)
Attachment #8466756 - Flags: review?(rnewman) → review+
Comment on attachment 8466754 [details] [diff] [review]
Part 1/3: Delete redundant initialisers-to-default-values from constructors

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

r+ except for TwoWayView.

::: mobile/android/base/widget/TwoWayView.java
@@ -316,5 @@
>      public TwoWayView(Context context, AttributeSet attrs) {
>          this(context, attrs, 0);
>      }
>  
>      public TwoWayView(Context context, AttributeSet attrs, int defStyle) {

Please split out the TwoWayView changes into a separate patch, and ask lucasr for review; it's effectively a third-party dependency.
Attachment #8466754 - Flags: review?(rnewman) → review+
OS: All → Android
Hardware: ARM → All
Comment on attachment 8466755 [details] [diff] [review]
Part 2/3: Delete redundant initialisers-to-default-values from constructors (android-sync)

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

Don't forget uplift (either by sending a pull request, or asking me as you land this).
Attachment #8466755 - Flags: review?(rnewman) → review+
(Assignee)

Comment 7

4 years ago
Created attachment 8466866 [details] [diff] [review]
Part 4/3: Delete redundant initialisers-to-default-values from constructors in TwoWayView

Splitting out changes to TwoWayView.

Lucas: Explicit initialisation of fields to zero, null, or false are redundant and not stripped during any compilation or optimisation phase. This patch removes them, slightly reducing binary size and cleaning up code.
In general, Java guarantees that all non-final fields are initialised to zero, null, or false.
Attachment #8466866 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8466866 [details] [diff] [review]
Part 4/3: Delete redundant initialisers-to-default-values from constructors in TwoWayView

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

Looks good. For future patches, you should submit changes to TwoWayView directly to the upstream project in: https://github.com/lucasr/twoway-view/

I've already pushed these changes there: https://github.com/lucasr/twoway-view/commit/de9082db9824c38c7c7e52947170e6312d0a594b
Attachment #8466866 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/c092f9e4310d
https://hg.mozilla.org/mozilla-central/rev/4bd09430f063
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
(In reply to Lucas Rocha (:lucasr) from comment #8)
> Comment on attachment 8466866 [details] [diff] [review]
> Part 4/3: Delete redundant initialisers-to-default-values from constructors
> in TwoWayView
> 
> Review of attachment 8466866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. For future patches, you should submit changes to TwoWayView
> directly to the upstream project in: https://github.com/lucasr/twoway-view/
> 
> I've already pushed these changes there:
> https://github.com/lucasr/twoway-view/commit/
> de9082db9824c38c7c7e52947170e6312d0a594b

Just to be clear: I pushed the patch to the upstream repo but you still have to land it in m-c.
Flags: needinfo?(chriskitching)
(Assignee)

Comment 13

4 years ago
Ah.
Flags: needinfo?(chriskitching)
(In reply to Chris Kitching [:ckitching] from comment #9)
> https://hg.mozilla.org/integration/fx-team/rev/c092f9e4310d
> https://hg.mozilla.org/integration/fx-team/rev/4bd09430f063

One of these two caused bug 1037617 to go perma-orange - attempts to back these out lead to conflicts - could you either fix the failure or back this out? :-)
Status: RESOLVED → REOPENED
Flags: needinfo?(chriskitching)
Resolution: FIXED → ---
The failures can also be seen in the try run:
https://tbpl.mozilla.org/?tree=Try&rev=23795b78bd58
(though since they match a known intermittent it's easily missed)
(Assignee)

Comment 17

4 years ago
Righto: working on it now.
Flags: needinfo?(chriskitching)
https://hg.mozilla.org/mozilla-central/rev/cf087beb1f9f
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(In reply to Chris Kitching [:ckitching] from comment #17)
> Righto: working on it now.

Thank you :-)
Depends on: 1052177
You need to log in before you can comment on or make changes to this bug.