Bug 1411654 (gradle-3.0)

Upgrade Android-Gradle plugin to 3.0+

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 wontfix, firefox60 fixed)

Details

Attachments

(20 attachments, 1 obsolete attachment)

59 bytes, text/x-review-board-request
maliu
: review+
Details
59 bytes, text/x-review-board-request
maliu
: review+
Details
59 bytes, text/x-review-board-request
maliu
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
maliu
: review+
Details
59 bytes, text/x-review-board-request
maliu
: review+
Details
59 bytes, text/x-review-board-request
maliu
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
maliu
: review+
Details
59 bytes, text/x-review-board-request
maliu
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
maliu
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
nalexander
: review+
Details
This is the successor to Bug 1366644.  Android-Gradle version 3.0.0 is at rc2; this tracks upgrading.

There are lots of structural changes in the new plugin version; I'm filing because I've worked through most of them and don't want somebody to repeat all that work without trying to coordinate.

One interesting wrinkle is that the new plugin pins the build-tools version.

As an aside, I got interested in this because most of the Java 8 features are gated on the new plugin version.
Android Studio 3.0 is live today: https://android-developers.googleblog.com/2017/10/android-studio-30.html.

Also, another wrinkle: the new Android-Gradle plugin parses AndroidManifest.xml at (Gradle) configuration time, not at evaluation time.  That means that our current preprocessed_manifest stuff is broken.  I think the best thing to do is:

- write a stub AndroidManifest.xml, in Gradle, at configuration time
- keep the existing preprocessed manifest stuff, but have it be a per-variant overlay on top of the stub manifest

It's possible that the tools: XML merging settings could actually make this work pretty well.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8922441 - Flags: review?(max)
Attachment #8922442 - Flags: review?(max)
Attachment #8922998 - Flags: review?(max)
Depends on: 1413006
Getting closer: I've got the android-test job updated and green in https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a3e26444568ccc30d629ec0c61817dda16cd12c.

Working on lint locally, and then we'll figure out what checkstyle is doing.
Also, it's worth noting that the motivation for working on this has changed.  The new "variant aware dependency management" is the best way to simplify the {with,without}GeckoLibraries snarl and tease out the Gradle, moz.build, and Android Studio integration points.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
OK, this is ready for the first reviews, although it's not 100% done -- there are still a few more lint issues to address or to ignore.

I've flagged maliu for the build bits and mcomella for the lint bits, but that's really not hard and fast.  snorp, you've complained about the Gecko binaries logic -- can you take a look at this?  nechen, you might be interested in reviewing this as well or instead -- discuss with maliu, please.

This builds on Bug 1417232.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Depends on: 1417232
Flags: needinfo?(snorp)
Flags: needinfo?(cnevinchen)

Comment 29

2 years ago
mozreview-review
Comment on attachment 8922998 [details]
Bug 1411654 - Pre: Don't block Google's maven repository.

https://reviewboard.mozilla.org/r/194202/#review205884
Attachment #8922998 - Flags: review?(max) → review+

Comment 30

2 years ago
mozreview-review
Comment on attachment 8928745 [details]
Bug 1411654 - Pre: Gradle is no longer experimental; IntelliJ not supported.

https://reviewboard.mozilla.org/r/200010/#review205886
Attachment #8928745 - Flags: review?(max) → review+

Comment 31

2 years ago
mozreview-review
Comment on attachment 8928746 [details]
Bug 1411654 - Pre: Update toolchain documents for Android toolchain jobs.

https://reviewboard.mozilla.org/r/200012/#review205888
Attachment #8928746 - Flags: review?(max) → review+

Comment 32

2 years ago
mozreview-review
Comment on attachment 8928747 [details]
Bug 1411654 - Part 0: Add rudimentary documentation about Gradle integration.

https://reviewboard.mozilla.org/r/200014/#review205890
Attachment #8928747 - Flags: review?(max) → review+
Looks like a step in the right direction to me, though I don't fully understand much of it.
Flags: needinfo?(snorp)
(In reply to Nick Alexander :nalexander from comment #8)
> Also, it's worth noting that the motivation for working on this has changed.
> The new "variant aware dependency management" is the best way to simplify
> the {with,without}GeckoLibraries snarl and tease out the Gradle, moz.build,
> and Android Studio integration points.

Ah, so the workaround from bug 1385695 will no longer be required, nice.
mcomella: maliu: are these patches in your queues?  I got partial review from maliu, but it's been a long time.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(max)
(In reply to Nick Alexander :nalexander (less responsive until Jan 3, 2018) from comment #35)
> mcomella: maliu: are these patches in your queues?  I got partial review
> from maliu, but it's been a long time.

Sorry, I've been PTO - I should be able to look at these today or tomorrow (catching up has been fast and afaict there are no urgent issues for me to address)
Flags: needinfo?(michael.l.comella)

Comment 37

2 years ago
mozreview-review
Comment on attachment 8922441 [details]
Bug 1411654 - Pre: Teach |mach gradle| to take GRADLE_FLAGS from the command line.

https://reviewboard.mozilla.org/r/193494/#review208690
Attachment #8922441 - Flags: review?(max) → review+
Comment on attachment 8928742 [details]
Bug 1411654 - Pre: Fix dangling resource.

https://reviewboard.mozilla.org/r/200004/#review208692

Didn't test to verify this works correctly but it looks fine to me.
Attachment #8928742 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8928743 [details]
Bug 1411654 - Pre: Remove unused PerProfileDatabaseProvider.

https://reviewboard.mozilla.org/r/200006/#review208696

::: commit-message-d2159:4
(Diff revision 1)
> +Bug 1411654 - Pre: Remove unused PerProfileDatabaseProvider. r=mcomella
> +
> +No idea what is going on with this hierarchy, but this isn't used and
> +isn't helping anything.

You can remove the reference in lint.xml too: https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/mobile/android/app/lint.xml#54
Attachment #8928743 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8928743 [details]
Bug 1411654 - Pre: Remove unused PerProfileDatabaseProvider.

https://reviewboard.mozilla.org/r/200006/#review208696

fwiw, I briefly tried to figure out when this stopped being used but it's unused since release (57).
Comment on attachment 8928744 [details]
Bug 1411654 - Pre: Clear icon disk storage in test.

https://reviewboard.mozilla.org/r/200008/#review208700

> Newer versions of Robolectric seem to have different semantics about clearing disk caches,

Didn't verify that this is true and thus this change necessary but if so, the change seems reasonable. I wonder what other caches we need to delete?
Attachment #8928744 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8928748 [details]
Bug 1411654 - Part 2: Update Robolectric to 3.5.1.

https://reviewboard.mozilla.org/r/200016/#review208702

Only skimmed this but seems reasonable: if the tests work for you, works for me!

::: mobile/android/services/src/test/java/org/mozilla/gecko/background/testhelpers/MockGlobalSession.java:31
(Diff revision 1)
>      this(new SyncConfiguration(username, new BasicAuthHeaderProvider(username, password), new MockSharedPreferences(), keyBundle), callback);
>    }
>  
>    public MockGlobalSession(SyncConfiguration config, GlobalSessionCallback callback)
>            throws SyncConfigurationException, IllegalArgumentException, IOException, NonObjectJSONException {
> -    super(config, callback, null, null);
> +    super(config, callback, RuntimeEnvironment.application,null);

nit: add ws after comma
Attachment #8928748 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8928752 [details]
Bug 1411654 - Part 6: Fix checkstyle.

https://reviewboard.mozilla.org/r/200024/#review208704
Attachment #8928752 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8928756 [details]
Bug 1411654 - Pre: Remove unused HomeExpandableListView.

https://reviewboard.mozilla.org/r/200032/#review208706
Attachment #8928756 - Flags: review?(michael.l.comella) → review+
Thanks for the info. I'll discuss with maliu !
Flags: needinfo?(cnevinchen)
Comment on attachment 8928751 [details]
Bug 1411654 - Part 5: Work through various new lint issues.

https://reviewboard.mozilla.org/r/200022/#review209126

r+ once we can resolve the pathData change and the multi-catch changes.

::: mobile/android/app/src/main/res/drawable/ic_as_trending.xml:8
(Diff revision 1)
>          android:height="24dp"
>          android:viewportWidth="12.0"
>          android:viewportHeight="12.0">
>      <path
>          android:fillColor="@color/activity_stream_icon"
> -        android:pathData="M4.97.151l-2.819,6.5A.25.25,0,0,0,2.381,7H4.029a.25.25,0,0,1,.225.359L2,12,9.4,5.437A.25.25,0,0,0,9.234,5H6.791a.25.25,0,0,1-.19-.412L10.15.412A.25.25,0,0,0,9.959,0H5.2A.25.25,0,0,0,4.97.151Z"/>
> +        android:pathData="M4.97,0.151l-2.819,6.5A0.25,0.25,0,0,0,2.381,7H4.029a0.25,0.25,0,0,1,0.225,0.359L2,12,9.4,5.437A0.25,0.25,0,0,0,9.234,5H6.791a0.25,0.25,0,0,1,-0.19,-0.412L10.15,0.412A0.25,0.25,0,0,0,9.959,0H5.2A0.25,0.25,0,0,0,4.97,0.151Z"/>

Can you explain this change? The warning mentions the problem is scientific notation, not `M4.97.1511-2.819,` but I'm not sure what that is supposed to mean – is it actually scientific notation?

::: mobile/android/base/AndroidManifest.xml.in:137
(Diff revision 1)
>              <intent-filter>
>                  <action android:name="org.mozilla.gecko.UPDATE"/>
>                  <category android:name="android.intent.category.DEFAULT" />
>              </intent-filter>
>  
> -            <intent-filter>
> +            <intent-filter tools:ignore="AppLinkUrlError">

I can't get this warning to show up locally (perhaps because the file is generated?) and there is [mysteriously very little about this warning on the internet](https://duckduckgo.com/?q=%22applinkurlerror%22&t=ffab&ia=web) so I don't know what it's for.

Does it occur when the system thinks you're looking for a filetype it thinks is an error? e.g. xpi is basically only used for add-ons so the Android system won't know about it and think it's an error.

If so, it makes sense to me why `mimeType=application/x-xpinstall` & `pathPattern=".*\\.xpi"` would throw an error but I'm not sure why this one would too.

But if there's a warning, wfm.

::: mobile/android/base/java/org/mozilla/gecko/CrashReporter.java:51
(Diff revision 1)
>  import android.widget.CompoundButton;
>  import android.widget.EditText;
>  
> -@SuppressLint("Registered") // This activity is only registered in the manifest if MOZ_CRASHREPORTER is set
> +// Registered: This activity is only registered in the manifest if MOZ_CRASHREPORTER is set.
> +// CutPasteId: This lint appears to be broken for this file.
> +@SuppressLint("Registered,CutPasteId")

I think `CutPasteId` wants us to cache the views after inflation rather than requesting findViewById on ids redundantly. From [the docs](https://sites.google.com/a/android.com/tools/tips/lint-checks):

> This lint check looks for cases where you have cut & pasted calls to
findViewById but have forgotten to update the R.id field. It's possible that
your code is simply (redundantly) looking up the field repeatedly, but lint
cannot distinguish that from a case where you for example want to initialize
fields prev and next and you cut & pasted findViewById(R.id.prev) and forgot
to update the second initialization to R.id.next.

That being said, no one ever touches this code so I'm fine use `CutPasteId` if you choose to – just update the comment mentioning that it's not worth fixing because no one touches the code.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:309
(Diff revision 1)
>                      final int faviconID = faviconIDs.get(name);
>                      iconValue.put("_id", faviconID);
>                      bookmarkValue.put(Bookmarks.FAVICON_ID, faviconID);
>                      faviconValues.add(iconValue);
>                  }
> -            } catch (IllegalAccessException | IllegalArgumentException | NoSuchFieldException e) {
> +            } catch (IllegalAccessException e) {

multi-catch should be supported if we enabled 1_7 source compatibility (iirc, it's desugared into multiple catch statements by the compiler).

This SO comment mentions it's supported – https://stackoverflow.com/a/13550632/2219998 – and it worked fine before. Why is this necessary now?

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:509
(Diff revision 1)
>              // Look for a favicon with the id R.raw.bookmarkdefaults_favicon_*.
>              Field faviconField = drawablesClass.getField(name.replace("_title_", "_favicon_"));
>              faviconField.setAccessible(true);
>  
>              return faviconField.getInt(null);
> -        } catch (IllegalAccessException | NoSuchFieldException e) {
> +        } catch (IllegalAccessException e) {

Same, multi-catch.
Comment on attachment 8928751 [details]
Bug 1411654 - Part 5: Work through various new lint issues.

https://reviewboard.mozilla.org/r/200022/#review209136

r- to clear it from my queue – please reflag me after commenting on those issues.
Attachment #8928751 - Flags: review?(michael.l.comella) → review-
Comment on attachment 8928754 [details]
Bug 1411654 - Part 5b: Work through WrongConstant lint issues.

https://reviewboard.mozilla.org/r/200028/#review209140

::: mobile/android/base/java/org/mozilla/gecko/activitystream/ActivityStreamTelemetry.java:202
(Diff revision 1)
>                          throw new IllegalStateException("Unknown highlight source: " + source);
>                  }
>                  return this;
>              }
>  
> +            @SuppressLint("WrongConstant")

Add a comment explaining this

::: mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java:96
(Diff revision 1)
>          }
>      }
>  
>      public abstract void perform(Context context, DownloadContentCatalog catalog);
>  
> +    @SuppressLint("WrongConstant")

Add a comment explaining this

::: mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java:51
(Diff revision 1)
>  
>      public DownloadAction(Callback callback) {
>          this.callback = callback;
>      }
>  
> +    @SuppressLint("WrongConstant")

Add a comment explaining this
Attachment #8928754 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8928754 [details]
Bug 1411654 - Part 5b: Work through WrongConstant lint issues.

https://reviewboard.mozilla.org/r/200028/#review209142

These int constants and annotations create weird warnings. :(

Actually, maybe we can escalate this lint warning to an error and remove the code that handles default cases for int constants, thus making suppression unnecessary?
Comment on attachment 8928755 [details]
Bug 1411654 - Part 5c: Work through ResourceUnused lint issues.

https://reviewboard.mozilla.org/r/200030/#review209146
Attachment #8928755 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8928757 [details]
Bug 1411654 - Part 5c: More work on UnusedResource lint errors.

https://reviewboard.mozilla.org/r/200034/#review209152

::: mobile/android/base/java/org/mozilla/gecko/util/UnusedResourcesUtil.java:105
(Diff revision 1)
>  
>      public static final int[] USED_IN_PAGE_ACTION = {
>              R.drawable.add_to_homescreen
>      };
> +
> +    public static final int[] USED_IN_LEANPLUM_EXPANDABLE_LIST_ACTIVITY = {

A plain text search shows this is unused:

https://searchfox.org/mozilla-central/search?q=path%3Amobile%2Fandroid+Widget.ExpandableListView&case=false&regexp=false&path=

Are you sure keeping this in UnusedResourcesUtil is necessary?

But perhaps I'm converting `Widget_ExpandableListView` -> `Widget.ExpandableListView` incorrectly...
Attachment #8928757 - Flags: review?(michael.l.comella) → review+
> As an aside, I got interested in this because most of the Java 8 features are gated on the new plugin version.

Nick, I noticed the Java 8 feature set [1] is much more comprehensive than I was expecting (e.g. lambdas, method references) and can a big difference in how we write readable code! I'm sure you were going to do it anyway but just a reminder that it'd be good to notify mobile-firefox-dev@ when these are working! :)

[1]: https://developer.android.com/studio/write/java8-support.html#supported_features

Comment 53

2 years ago
mozreview-review
Comment on attachment 8922442 [details]
Bug 1411654 - Part 1: Upgrade to Android-Gradle 3.0+ and build-tools;26.0.2.

https://reviewboard.mozilla.org/r/193500/#review208694

::: mobile/android/base/Makefile.in:251
(Diff revision 3)
>  # .gradle.deps: .aapt.deps FORCE
>  $(eval $(call gradle_command,.gradle.deps,.aapt.deps FORCE))
>  
>  classes.dex: .gradle.deps
>  	$(REPORT_BUILD)
> -	cp $(gradle_dir)/app/intermediates/transforms/dex/officialPhoton/debug/folders/1000/1f/main/classes.dex $@
> +	cp $(gradle_dir)/app/intermediates/transforms/dexMerger/officialPhoton/debug/0/classes.dex classes.dex

Will this "classes.dex" be broken if we turn ON multi-dex?
Attachment #8922442 - Flags: review?(max) → review+

Comment 54

2 years ago
mozreview-review
Comment on attachment 8928749 [details]
Bug 1411654 - Part 3: Make each variant handle source from moz.build.

https://reviewboard.mozilla.org/r/200018/#review209316
Attachment #8928749 - Flags: review?(max) → review+

Comment 55

2 years ago
mozreview-review
Comment on attachment 8928750 [details]
Bug 1411654 - Part 4: Use flavorDimensions to simplify {with,without}GeckoBinaries logic.

https://reviewboard.mozilla.org/r/200020/#review209318

::: mobile/android/base/Makefile.in:251
(Diff revision 1)
>  # .gradle.deps: .aapt.deps FORCE
>  $(eval $(call gradle_command,.gradle.deps,.aapt.deps FORCE))
>  
>  classes.dex: .gradle.deps
>  	$(REPORT_BUILD)
> -	cp $(gradle_dir)/app/intermediates/transforms/dexMerger/officialPhoton/debug/0/classes.dex classes.dex
> +	cp $(gradle_dir)/app/intermediates/transforms/dexMerger/officialWithoutGeckoBinariesNoMinApiPhoton/debug/0/classes.dex classes.dex

Will this be broken when we turn ON the multi-dex flag?

::: mobile/android/base/Makefile.in:511
(Diff revision 1)
>  $(eval $(call gradle_command,.gradle.nodeps,AndroidManifest.xml $(constants_PP_JAVAFILES) FORCE))
>  
>  .aapt.nodeps: .gradle.nodeps FORCE
>  	@$(TOUCH) $@
>  	cp $(GRADLE_ANDROID_APP_APK) gecko-nodeps.ap_
> -	cp $(gradle_dir)/app/intermediates/transforms/dex/officialPhoton/debug/folders/1000/1f/main/classes.dex classes.dex
> +	cp $(gradle_dir)/app/intermediates/transforms/dexMerger/officialWithoutGeckoBinariesNoMinApiPhoton/debug/0/classes.dex classes.dex

Will this be broken when we turn ON the multi-dex flag?
Attachment #8928750 - Flags: review?(max) → review+

Comment 56

2 years ago
mozreview-review
Comment on attachment 8928753 [details]
Bug 1411654 - Post: Cull unused variables; guard more with MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE.

https://reviewboard.mozilla.org/r/200026/#review209330
Attachment #8928753 - Flags: review?(max) → review+
Nick it looks like this is all reviewed now, do you know what's blocking the landing?
Flags: needinfo?(nalexander)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #57)
> Nick it looks like this is all reviewed now, do you know what's blocking the
> landing?

"Just" my time: I need to rebase, address the review comments, and handle the final few lint errors (likely by ignoring them).  It's on my list.
Flags: needinfo?(nalexander)
Assignee

Comment 59

2 years ago
mozreview-review-reply
Comment on attachment 8922442 [details]
Bug 1411654 - Part 1: Upgrade to Android-Gradle 3.0+ and build-tools;26.0.2.

https://reviewboard.mozilla.org/r/193500/#review208694

> Will this "classes.dex" be broken if we turn ON multi-dex?

Yes, but no more than the current situation.  Turning on multi-dex will require some version of https://bugzilla.mozilla.org/show_bug.cgi?id=1414054; this doesn't address that.
Assignee

Comment 60

2 years ago
mozreview-review-reply
Comment on attachment 8928750 [details]
Bug 1411654 - Part 4: Use flavorDimensions to simplify {with,without}GeckoBinaries logic.

https://reviewboard.mozilla.org/r/200020/#review209318

> Will this be broken when we turn ON the multi-dex flag?

See previous comment: yes, more work will be required to turn on multi-dex.
Assignee

Comment 61

2 years ago
mozreview-review-reply
Comment on attachment 8928751 [details]
Bug 1411654 - Part 5: Work through various new lint issues.

https://reviewboard.mozilla.org/r/200022/#review209126

> Can you explain this change? The warning mentions the problem is scientific notation, not `M4.97.1511-2.819,` but I'm not sure what that is supposed to mean – is it actually scientific notation?

I don't recall everything, but there's some non-standard parsing whereby ".xx.yy" is parsed like "0.xx,0.yy" and lint warns you not to rely on that (presumably since it's not entirely standard).  So I manually moved to the more normal (but slightly larger) form, verifying (using the browser) that the resulting SVG results "looked the same".

> I can't get this warning to show up locally (perhaps because the file is generated?) and there is [mysteriously very little about this warning on the internet](https://duckduckgo.com/?q=%22applinkurlerror%22&t=ffab&ia=web) so I don't know what it's for.
> 
> Does it occur when the system thinks you're looking for a filetype it thinks is an error? e.g. xpi is basically only used for add-ons so the Android system won't know about it and think it's an error.
> 
> If so, it makes sense to me why `mimeType=application/x-xpinstall` & `pathPattern=".*\\.xpi"` would throw an error but I'm not sure why this one would too.
> 
> But if there's a warning, wfm.

Yeah, I couldn't find much that seemed relevant.  I didn't want to think through the details of the XPI registration (which seems to be malformed in our manifest), so I elected to silence the warning.  I still don't want to think through all of that stuff, so if you think we need more, let's file follow-up.

> I think `CutPasteId` wants us to cache the views after inflation rather than requesting findViewById on ids redundantly. From [the docs](https://sites.google.com/a/android.com/tools/tips/lint-checks):
> 
> > This lint check looks for cases where you have cut & pasted calls to
> findViewById but have forgotten to update the R.id field. It's possible that
> your code is simply (redundantly) looking up the field repeatedly, but lint
> cannot distinguish that from a case where you for example want to initialize
> fields prev and next and you cut & pasted findViewById(R.id.prev) and forgot
> to update the second initialization to R.id.next.
> 
> That being said, no one ever touches this code so I'm fine use `CutPasteId` if you choose to – just update the comment mentioning that it's not worth fixing because no one touches the code.

I tried to understand this and didn't twig that lint wants us to cache the lookups ourselves.  I'll update the comment.

> multi-catch should be supported if we enabled 1_7 source compatibility (iirc, it's desugared into multiple catch statements by the compiler).
> 
> This SO comment mentions it's supported – https://stackoverflow.com/a/13550632/2219998 – and it worked fine before. Why is this necessary now?

This is the situation that it doesn't work -- reflection operation related things.  See https://gyulajuhasz.com/blog/dangers-of-multi-catch-in-android/.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
mcomella: I think this is ready for re-review.
Try is looking healthy: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f947c15742027ffbdb80f2f8d643cc7f19bf94a has one checkstyle issue, which I have a trivial patch to address and will push at some point.  (Not sure why it's not failing now, tbh.)
Comment on attachment 8928751 [details]
Bug 1411654 - Part 5: Work through various new lint issues.

https://reviewboard.mozilla.org/r/200022/#review216948

Nick's explanations make sense to me - wfm.
Attachment #8928751 - Flags: review?(michael.l.comella) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8928757 - Attachment is obsolete: true
Comment on attachment 8941192 [details]
Bug 1411654 - Pre: Fix checkstyle whitespace error in GeckoHlsPlayer.

This already had mcomella's r+.
Flags: needinfo?(max)
Attachment #8941192 - Flags: review?(michael.l.comella) → review+
Assignee

Comment 104

2 years ago
mozreview-review
Comment on attachment 8928746 [details]
Bug 1411654 - Pre: Update toolchain documents for Android toolchain jobs.

https://reviewboard.mozilla.org/r/200012/#review217244

This already had mcomella's r+.
Attachment #8928746 - Flags: review+
Attachment #8941192 - Flags: review+ → review?(nalexander)
Assignee

Comment 105

2 years ago
mozreview-review
Comment on attachment 8941192 [details]
Bug 1411654 - Pre: Fix checkstyle whitespace error in GeckoHlsPlayer.

https://reviewboard.mozilla.org/r/211468/#review217246
Attachment #8941192 - Flags: review?(nalexander) → review+
Assignee

Comment 106

2 years ago
mozreview-review
Comment on attachment 8928751 [details]
Bug 1411654 - Part 5: Work through various new lint issues.

https://reviewboard.mozilla.org/r/200022/#review217250
Assignee

Comment 107

2 years ago
mozreview-review
Comment on attachment 8928754 [details]
Bug 1411654 - Part 5b: Work through WrongConstant lint issues.

https://reviewboard.mozilla.org/r/200028/#review217254
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 128

2 years ago
mozreview-review
Comment on attachment 8942288 [details]
Bug 1411654 - Pre: Fix CustomTabsSecurityPopup package.

https://reviewboard.mozilla.org/r/212572/#review218274

Cherry picked from https://bugzilla.mozilla.org/show_bug.cgi?id=1426244.
Attachment #8942288 - Flags: review?(nalexander) → review+

Comment 129

2 years ago
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78576ff98660
Pre: Teach |mach gradle| to take GRADLE_FLAGS from the command line. r=maliu
https://hg.mozilla.org/integration/autoland/rev/d23f467218da
Pre: Don't block Google's maven repository. r=maliu
https://hg.mozilla.org/integration/autoland/rev/264476c77210
Pre: Fix dangling resource. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/fb15e1f54987
Pre: Remove unused PerProfileDatabaseProvider. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/108674372ef7
Pre: Clear icon disk storage in test. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/9141bbdfd13b
Pre: Gradle is no longer experimental; IntelliJ not supported. r=maliu
https://hg.mozilla.org/integration/autoland/rev/43787f4089c3
Pre: Update toolchain documents for Android toolchain jobs. r=maliu
https://hg.mozilla.org/integration/autoland/rev/bffb6a5b78d1
Pre: Fix checkstyle whitespace error in GeckoHlsPlayer. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/5c64eda20f1e
Pre: Remove unused HomeExpandableListView. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/359fadf9b3e9
Pre: Fix CustomTabsSecurityPopup package. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/eda74143389f
Part 0: Add rudimentary documentation about Gradle integration. r=maliu
https://hg.mozilla.org/integration/autoland/rev/23cbcf427745
Part 1: Upgrade to Android-Gradle 3.0+ and build-tools;26.0.2. r=maliu
https://hg.mozilla.org/integration/autoland/rev/0e493bacc5e3
Part 2: Update Robolectric to 3.5.1. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/6ff0cdf46a3d
Part 3: Make each variant handle source from moz.build. r=maliu
https://hg.mozilla.org/integration/autoland/rev/56538ed998cf
Part 4: Use flavorDimensions to simplify {with,without}GeckoBinaries logic. r=maliu
https://hg.mozilla.org/integration/autoland/rev/db978e230556
Part 5: Work through various new lint issues. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/a371f3ef4312
Part 6: Fix checkstyle. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/c2e51b70519f
Part 5b: Work through WrongConstant lint issues. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/649e7aa405ca
Part 5c: Work through ResourceUnused lint issues. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/113f1c4b9564
Post: Cull unused variables; guard more with MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE. r=maliu
Backed out changeset 113f1c4b9564 (bug 1411654) for android nightly bustages a=backout

https://hg.mozilla.org/mozilla-central/rev/c8e0679f0184f38efac317ec4bef70cff0918ef8

https://hg.mozilla.org/mozilla-central/rev/113f1c4b9564
Flags: needinfo?(nalexander)

Updated

2 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 59 → ---
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 153

2 years ago
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d276d3deba05
Pre: Teach |mach gradle| to take GRADLE_FLAGS from the command line. r=maliu
https://hg.mozilla.org/integration/autoland/rev/0850b319efd4
Pre: Don't block Google's maven repository. r=maliu
https://hg.mozilla.org/integration/autoland/rev/22a861db1573
Pre: Fix dangling resource. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/3dc3beab95f8
Pre: Remove unused PerProfileDatabaseProvider. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/373c9a71d945
Pre: Clear icon disk storage in test. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/901b304603d9
Pre: Gradle is no longer experimental; IntelliJ not supported. r=maliu
https://hg.mozilla.org/integration/autoland/rev/e7b0cc801cf1
Pre: Update toolchain documents for Android toolchain jobs. r=maliu
https://hg.mozilla.org/integration/autoland/rev/d0d513d1c379
Pre: Fix checkstyle whitespace error in GeckoHlsPlayer. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/2b37201606f5
Pre: Remove unused HomeExpandableListView. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/76085ddd5ac7
Pre: Fix CustomTabsSecurityPopup package. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/cec2b8828cc8
Part 0: Add rudimentary documentation about Gradle integration. r=maliu
https://hg.mozilla.org/integration/autoland/rev/f918500d9cf5
Part 1: Upgrade to Android-Gradle 3.0+ and build-tools;26.0.2. r=maliu
https://hg.mozilla.org/integration/autoland/rev/690e265c684c
Part 2: Update Robolectric to 3.5.1. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/730a70767743
Part 3: Make each variant handle source from moz.build. r=maliu
https://hg.mozilla.org/integration/autoland/rev/01836fd98c63
Part 4: Use flavorDimensions to simplify {with,without}GeckoBinaries logic. r=maliu
https://hg.mozilla.org/integration/autoland/rev/fde9bf9c14c3
Part 5: Work through various new lint issues. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/c270f97bb0da
Part 6: Fix checkstyle. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/c5bf85d56fed
Part 5b: Work through WrongConstant lint issues. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/55776829a744
Part 5c: Work through ResourceUnused lint issues. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/cfad693be918
Post: Cull unused variables; guard more with MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE. r=maliu
Depends on: 1430788
Depends on: 1430791
No longer depends on: 1430791
No longer depends on: 1430788
Just noticed the "Leaks" shortcut on my android phone after some recently Firefox Nightly update.

When started it displays "Leaks in org.mozilla.fennec" in the title strip.

There is no vsible content of the window.

Oh, when I moved the "Leaks" icon to the trashcan the Firefox Nightly went with it.

That's pretty inconvenient considering I had web apps with Indexed DB installed.
(In reply to Adrian Aichner [:anaran] from comment #155)
> Just noticed the "Leaks" shortcut on my android phone after some recently
> Firefox Nightly update.

Yes -- that's https://bugzilla.mozilla.org/show_bug.cgi?id=1430417, which I just landed a fix for.
> 
> When started it displays "Leaks in org.mozilla.fennec" in the title strip.
> 
> There is no vsible content of the window.
> 
> Oh, when I moved the "Leaks" icon to the trashcan the Firefox Nightly went
> with it.
> 
> That's pretty inconvenient considering I had web apps with Indexed DB
> installed.

Indeed, and we're sorry -- but that's how Android works: when you trash a launcher icon, you remove the App entirely.
Flags: needinfo?(nalexander)
Depends on: 1431140
OK, backouts for Bug 1419581, Bug 1430417, and Bug 1411654 (in that order) don't conflict.  Try pushes with full builds

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01154eb18c96610c202e6eb8bcb84355dacc1fcf

and with artifact builds

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2761199fdc0bfabf4ed2f4cf9f21f2da850224f1

RyanVM: others: I'll be pretty confident after the artifact build, since this is all on the Java/Gradle side of the house.  I'll comment on this ticket after manually verifying that the debuggable state is correct on the try builds.
No longer blocks: 1419581
> RyanVM: others: I'll be pretty confident after the artifact build, since
> this is all on the Java/Gradle side of the house.  I'll comment on this
> ticket after manually verifying that the debuggable state is correct on the
> try builds.

I checked one of the APKs from one of the try build, and it's definitely not debuggable, so I think this is good to land on m-c.  I'll push it myself.
Backed out for causing bug 1431140.

https://hg.mozilla.org/mozilla-central/rev/ef04f3ad8475
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 59 → ---
Status: REOPENED → ASSIGNED
Comment on attachment 8941192 [details]
Bug 1411654 - Pre: Fix checkstyle whitespace error in GeckoHlsPlayer.

https://reviewboard.mozilla.org/r/211468/#review219476
Attachment #8941192 - Flags: review?(michael.l.comella) → review+
Status update: there's a change to how the Android-Gradle plugin handles the underlying AndroidManifest.xml.  The plugin now overrides android:debuggable from the manifest.  That much is fine; we can always set the android:debuggable flag in Gradle.  (What's not fine: I didn't catch this, and our automated tests don't either.)

However, when debuggable is false, we don't have an external library compile dependency.  That means that we don't run a particular Gradle build step -- merging DEX files from the App and all the external libraries -- and that means classes.dex is in dex/... and not dexMerger/... and that fails our Makefile.in/classes.dex hackery.

The ugly hack is to look in two places for classes.dex.  The better approach is to _not monkey with classes.dex directly_ and instead use the APK produced by Gradle.  (And still have classes.dex handling for non-Gradle builds, may they rot in a warm place.)  This approach is basically implemented in my patches for https://bugzilla.mozilla.org/show_bug.cgi?id=1414054.

So my plan is to return to that ticket, extract the classes.dex changes, and try this again.  Tricky, tricky.
Depends on: 1433285
Alias: gradle-3.0
FYI: I made progress upgrading the Android Gradle Plugin for Firefox TV and, while I haven't finished (one test hangs), I didn't have to disable aapt2 like this fix did. fwiw, here are my WIP changes: https://github.com/mozilla-mobile/firefox-tv/pull/426

NI nalexander for info but no action needed.
Flags: needinfo?(nalexander)
OK, now that Bug 1414054 looks good in the wilds of Nightly, and with https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c66d45a187568954d2ec5b4f8fd761c9db24838 looking like it has minimal changes against mozilla-central, I think this is ready to go.

Just for the record, I am aware that the sub-packages (geckoview, thirdparty) are now compiled in release mode rather than debug mode, and to the best of my knowledge that's fine -- in fact, better than fine, desirable.  Let's hope I'm not wrong!
Flags: needinfo?(nalexander)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 184

Last year
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c86a29ef0b5b
Pre: Teach |mach gradle| to take GRADLE_FLAGS from the command line. r=maliu
https://hg.mozilla.org/integration/autoland/rev/24e4f9a8e3cd
Pre: Don't block Google's maven repository. r=maliu
https://hg.mozilla.org/integration/autoland/rev/f4ea28fdbc3e
Pre: Fix dangling resource. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/9177a22af8d0
Pre: Remove unused PerProfileDatabaseProvider. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/986da6f801e4
Pre: Clear icon disk storage in test. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/c9bff6158dd3
Pre: Gradle is no longer experimental; IntelliJ not supported. r=maliu
https://hg.mozilla.org/integration/autoland/rev/695ed77978fc
Pre: Update toolchain documents for Android toolchain jobs. r=maliu
https://hg.mozilla.org/integration/autoland/rev/96c50f164965
Pre: Fix checkstyle whitespace error in GeckoHlsPlayer. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/d5a5a3d8e6e5
Pre: Remove unused HomeExpandableListView. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/cc3b61efc9b3
Pre: Fix CustomTabsSecurityPopup package. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/2d2ce8d0b969
Part 0: Add rudimentary documentation about Gradle integration. r=maliu
https://hg.mozilla.org/integration/autoland/rev/bc4796418006
Part 1: Upgrade to Android-Gradle 3.0+ and build-tools;26.0.2. r=maliu
https://hg.mozilla.org/integration/autoland/rev/d67b952ecbe4
Part 2: Update Robolectric to 3.5.1. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/8f2c1b74bf9a
Part 3: Make each variant handle source from moz.build. r=maliu
https://hg.mozilla.org/integration/autoland/rev/589194574eff
Part 4: Use flavorDimensions to simplify {with,without}GeckoBinaries logic. r=maliu
https://hg.mozilla.org/integration/autoland/rev/1eb2f1b4c961
Part 5: Work through various new lint issues. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/cbbb957aa470
Part 6: Fix checkstyle. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/431cf29e8339
Part 5b: Work through WrongConstant lint issues. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/a4acfa51d53c
Part 5c: Work through ResourceUnused lint issues. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/ba88f9cd2669
Post: Cull unused variables; guard more with MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE. r=maliu
(In reply to Michael Comella (:mcomella) [please needinfo for a response] from comment #162)
> FYI: I made progress upgrading the Android Gradle Plugin for Firefox TV and,
> while I haven't finished (one test hangs), I didn't have to disable aapt2
> like this fix did. fwiw, here are my WIP changes:
> https://github.com/mozilla-mobile/firefox-tv/pull/426
> 
> NI nalexander for info but no action needed.

Thanks, mcomella!  Just for those interested, the aapt2 change was required to placate Robolectric, which just hadn't updated to Android-Gradle 3.0.  However, this was done a few months ago -- and we're _already_ at A-G 3.0.1 -- so it's possible that Robolectric has addressed the aapt2 change.
Blocks: 1509539
You need to log in before you can comment on or make changes to this bug.