Merge LayerController and GeckoLayerClient

RESOLVED FIXED in Firefox 17

Status

()

Firefox for Android
Toolbar
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
Firefox 17
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

These two classes are very tightly coupled, have pretty much the same lifetime, and it's very difficult to provide a specific role for either class in its current state. They should be merged into one class, which should simplify the code greatly.
Created attachment 645835 [details] [diff] [review]
Part 1 - Move getDrawable to LayerView
Created attachment 645836 [details] [diff] [review]
Part 2 - Minor cleanup in GeckoLayerClient
Created attachment 645837 [details] [diff] [review]
Part 3 - Fold LayerController into GeckoLayerClient
Created attachment 645838 [details] [diff] [review]
Part 4 - Remove unnecessary getBitmap function
Created attachment 645839 [details] [diff] [review]
Part 5 - Scope down and inline some GLC functions
Blocks: 777468
Comment on attachment 645835 [details] [diff] [review]
Part 1 - Move getDrawable to LayerView

I have a rebased version of this locally, which includes the s/background/tabs_tray_whatever/ change you made.
Attachment #645835 - Flags: review?(sriram)
Attachment #645836 - Flags: review?(sriram)
Comment on attachment 645837 [details] [diff] [review]
Part 3 - Fold LayerController into GeckoLayerClient

Figured you might want to review this since it will affect that patch you created to invert the LayerController <-> LayerView dependency
Attachment #645837 - Flags: review?(sriram)
Comment on attachment 645838 [details] [diff] [review]
Part 4 - Remove unnecessary getBitmap function

Basically dead code removal
Attachment #645838 - Flags: review?(sriram)
Attachment #645839 - Flags: review?(sriram)
Comment on attachment 645835 [details] [diff] [review]
Part 1 - Move getDrawable to LayerView

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

This looks good to me. Do we really want to write the getBitmap() ourselves?
I know the view doesn't support it. However, R.drawable.something refers the non-scaled version, if it's in drawable-nodpi/ folder -- which is what we have.
Also, I feel R.drawable.something is better than "something". This can be caught at compile time than at runtime.
r+ (please try using getResources().getDrawable() if that would serve the purpose.)
Attachment #645835 - Flags: review?(sriram) → review+
Attachment #645836 - Flags: review?(sriram) → review+
Comment on attachment 645837 [details] [diff] [review]
Part 3 - Fold LayerController into GeckoLayerClient

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

Woww! This is nice :)
Attachment #645837 - Flags: review?(sriram) → review+
Comment on attachment 645838 [details] [diff] [review]
Part 4 - Remove unnecessary getBitmap function

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

Are you serious? getBitmap() was returning just "null" all these days? :O
Attachment #645838 - Flags: review?(sriram) → review+
Comment on attachment 645839 [details] [diff] [review]
Part 5 - Scope down and inline some GLC functions

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

This is a nice cleanup.
Attachment #645839 - Flags: review?(sriram) → review+
Created attachment 647954 [details] [diff] [review]
Part 6 - Follow up to use R.drawable.xxx instead of strings

(In reply to Sriram Ramasubramanian [:sriram] from comment #9)
> This looks good to me. Do we really want to write the getBitmap() ourselves?
> I know the view doesn't support it. However, R.drawable.something refers the
> non-scaled version, if it's in drawable-nodpi/ folder -- which is what we
> have.
> Also, I feel R.drawable.something is better than "something". This can be
> caught at compile time than at runtime.
> r+ (please try using getResources().getDrawable() if that would serve the
> purpose.)

Whoops, I forgot to do this. Attached is a follow-up patch that does it.
Attachment #647954 - Flags: review?(sriram)
Comment on attachment 647954 [details] [diff] [review]
Part 6 - Follow up to use R.drawable.xxx instead of strings

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

Look good to me.
Attachment #647954 - Flags: review?(sriram) → review+
I guess this is a clobbering issue. Most likely.
The R1 failures were caused by my previous patches in bug 777075. See https://bugzilla.mozilla.org/show_bug.cgi?id=777075#c15 for details. I did a try run with the fixed patches from bug 777075 and the patches on this bug, and the R1 test was green: https://tbpl.mozilla.org/?tree=Try&rev=abd4947bf028

I rebased these patches and re-landed them on inbound (and took advantage of the opportunity to fold the follow-up patch #6 into patch #1 where it belongs).

https://hg.mozilla.org/integration/mozilla-inbound/rev/5461fa96ce5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7f17e94071a
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeebc9bf9712
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fb7d7b2e62d
https://hg.mozilla.org/integration/mozilla-inbound/rev/09300210812a
You need to log in before you can comment on or make changes to this bug.