1203730 - Add CardView to build system

Status: RESOLVED DUPLICATE of bug 1271428

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Like bug 1171288 for recycler view.

Also, figure out how much larger it makes the build.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

43765308 bytes with Cardview (and a little code to use it, ensuring it's not removed by PG)
43677376 bytes without

Delta: ~87.932 kb. For comparison, the xxhdpi dropshadow we currently use for doorhangers is 672 bytes. :\

Nick, this seems like too much to me (for easy drop shadows and rounded corners) – what do you think?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1203730 - Add CardView to build system. r=nalexander

Bug 1203730 - Add CardView to build system. r=nalexander

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8659561 [details]
MozReview Request: Bug 1203730 - Add CardView to build system. r=nalexander

Bug 1203730 - Add CardView to build system. r=nalexander

Note that I just copied the patterns from the recycler view bug 1171288 and I
didn't closely check what I was doing.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note: it's only worth reviewing this if you think it's worth adding CardView.

Comment 5

[Chenxia Liu [:liuche]]

We'd also be able to remove RoundedCornerLayout (1k) and the associated hackery for that, but that doesn't come near recouping the 85k.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #1)
> 43765308 bytes with Cardview (and a little code to use it, ensuring it's not
> removed by PG)
> 43677376 bytes without
> 
> Delta: ~87.932 kb. For comparison, the xxhdpi dropshadow we currently use
> for doorhangers is 672 bytes. :\
> 
> Nick, this seems like too much to me (for easy drop shadows and rounded
> corners) – what do you think?

I agree, but I am quite surprised by this result.  How did you test this?

The AAR is only about ~20kb; we shouldn't go up by more than that unless we see some very surprising PG behaviour.  Push a try build, so we can see the automation PG outputs.

In general, I am for using these kinds of libraries, but the barrier to entry can't be "I asked Nick".  I suggest you solicit feedback from sebastian and perhaps one other person likely to use the library; collect pros and cons; and then surface the discussion to mobile-firefox-dev.

Comment 7

[Nick Alexander :nalexander [he/him]]

And I mid-aired liuche, who contributes exactly the kind of comment I was hoping for!  Thanks, liuche :)

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #4)
> Note: it's only worth reviewing this if you think it's worth adding CardView.

I'd rather re-visit after Bug 1108782, which would use the AAR for this.  That ticket should land shortly.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Nick Alexander :nalexander from comment #6)
> I agree, but I am quite surprised by this result.  How did you test this?

I built with the patch in this bug and the WIP patch in bug 1203345 comment 3. Then I `ls -alh` the output fennec*apk. Similarly, I updated to the fx-team changeset closest to that branch and ran the same command.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Unassigning self at least until bug 1108782 is fixed.

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #9)
> (In reply to Nick Alexander :nalexander from comment #6)
> > I agree, but I am quite surprised by this result.  How did you test this?
> 
> I built with the patch in this bug and the WIP patch in bug 1203345 comment
> 3. Then I `ls -alh` the output fennec*apk. Similarly, I updated to the
> fx-team changeset closest to that branch and ran the same command.

My guess is that you want to force PG passes -- by default, local builds do 1 pass, but automation does 6.  Just push to try :)

Comment 12

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1203730 - Add CardView to build system. r=nalexander

Bug 1203730 - Add CardView to build system. r=nalexander

Comment 13

[Chenxia Liu [:liuche]]

I fixed some merge errors and renamed some stuff - try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8738e623ab56

I didn't make any changes to the junit3 moz.build files even though I see the recyclerview libraries are included because I don't think we have any junit3 tests that run doorhangers, but I could be wrong about about them not being needed.

This would set the stage for removing RoundedCornerLayout and the shadow resources, and fixing some of the offset regressions. I did a quick test and CardView works fine with a quick replace, although there are some problems on (surprisingly) non-2.3 devices with multiple doorhangers.

Comment 14

[Chenxia Liu [:liuche]]

Comment on attachment 8679193 [details]
MozReview Request: Bug 1203730 - Add CardView to build system. r=nalexander

Bug 1203730 - Add CardView to build system. r=nalexander

Comment 15

[Nick Alexander :nalexander [he/him]]

It's worth pushing an API 11 try build, since that's more representative.  For API 9:

From https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=b404b452db2b

Size of classes.dex: 4248396 bytes
Size of fennec-44.0a1.en-US.android-arm.apk: 40912617 bytes
Size of libxul.so: 23648911 bytes
Size of omni.ja: 9735189 bytes

From the try build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=8738e623ab56

Size of classes.dex: 4344732 bytes
Size of fennec-44.0a1.en-US.android-arm.apk: 41000665 bytes
Size of libxul.so: 23640991 bytes
Size of omni.ja: 9734974 bytes

41000665 - 40912617 = 88048.  It must be that the AAR file compresses well.

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8679193 [details]
MozReview Request: Bug 1203730 - Add CardView to build system. r=nalexander

https://reviewboard.mozilla.org/r/23391/#review20849

::: mobile/android/base/Makefile.in:62
(Diff revision 2)
> +    $(ANDROID_CARDVIEW_V7_AAR_LIB) \

And https://dxr.mozilla.org/mozilla-central/source/mobile/android/b2gdroid/app/Makefile.in#32 please.  (You would see this if you pushed to try against 'android-b2gdroid'.)

If it works for you, it works for me :)