777351 - Merge LayerController and GeckoLayerClient

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

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 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Move getDrawable to LayerView

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Minor cleanup in GeckoLayerClient

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Fold LayerController into GeckoLayerClient

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Remove unnecessary getBitmap function

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 5 - Scope down and inline some GLC functions

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 645838 [details] [diff] [review]
Part 4 - Remove unnecessary getBitmap function

Basically dead code removal

Comment 9

[Sriram Ramasubramanian [: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.)

Comment 10

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 645837 [details] [diff] [review]
Part 3 - Fold LayerController into GeckoLayerClient

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

Woww! This is nice :)

Comment 11

[Sriram Ramasubramanian [:sriram]]

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

Comment 12

[Sriram Ramasubramanian [:sriram]]

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.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be81e4c3d928
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb303ba52f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/b47c470168fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aa6f94160dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2ee4c12c0b3

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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.

Comment 15

[Sriram Ramasubramanian [: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.

Comment 16

[Ed Morley [:emorley]]

Backed out for native Android R1 failures, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=14039325&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=14040037&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/24e458030842

Comment 17

[Sriram Ramasubramanian [:sriram]]

I guess this is a clobbering issue. Most likely.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

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

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5461fa96ce5f
https://hg.mozilla.org/mozilla-central/rev/d7f17e94071a
https://hg.mozilla.org/mozilla-central/rev/eeebc9bf9712
https://hg.mozilla.org/mozilla-central/rev/2fb7d7b2e62d
https://hg.mozilla.org/mozilla-central/rev/09300210812a