Closed Bug 1203730 Opened 6 years ago Closed 5 years ago

Add CardView to build system

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1271428

People

(Reporter: mcomella, Assigned: liuche)

References

Details

Attachments

(1 file, 1 obsolete file)

Like bug 1171288 for recycler view.

Also, figure out how much larger it makes the build.
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?
Flags: needinfo?(nalexander)
Bug 1203730 - Add CardView to build system. r=nalexander
Attachment #8659561 - Flags: review?(nalexander)
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.
Note: it's only worth reviewing this if you think it's worth adding CardView.
Assignee: nobody → michael.l.comella
We'd also be able to remove RoundedCornerLayout (1k) and the associated hackery for that, but that doesn't come near recouping the 85k.
(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.
And I mid-aired liuche, who contributes exactly the kind of comment I was hoping for!  Thanks, liuche :)
Flags: needinfo?(nalexander)
(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.
Depends on: 1108782
Attachment #8659561 - Flags: review?(nalexander)
(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.
Unassigning self at least until bug 1108782 is fixed.
Assignee: michael.l.comella → nobody
(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 :)
Bug 1203730 - Add CardView to build system. r=nalexander
Attachment #8679193 - Flags: review?(nalexander)
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.
Assignee: nobody → liuche
Attachment #8659561 - Attachment is obsolete: true
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
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 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 :)
Attachment #8679193 - Flags: review?(nalexander) → review+
Blocks: 1219318
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1271428
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.