Closed Bug 777351 Opened 8 years ago Closed 8 years ago

Merge LayerController and GeckoLayerClient

Categories

(Firefox for Android :: Toolbar, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 17

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(6 files)

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.
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+
(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.