779366 - Create org.mozilla.gecko.gfx package

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: part-1-FloatUtils.patch

Part 1: Move FloatUtils to new org.mozilla.gecko.util package.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-2-create-ConfigurationUtils.patch

Part 2: Move DisplayMetrics to new org.mozilla.gecko.util.ConfigurationUtils class.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: create-DirectBufferAllocator.patch

Part 3: Move direct buffer allocation to new org.mozilla.gecko.util.DirectBufferAllocator class.

Comment 4

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

Comment on attachment 647781 [details] [diff] [review]
part-1-FloatUtils.patch

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

::: mobile/android/base/Makefile.in
@@ +16,5 @@
>  
>  include $(topsrcdir)/mobile/android/base/android-sync-files.mk
>  
> +UTIL_JAVA_FILES :=              \
> +  FloatUtils.java

The makefiles I've looked at have a convention of ending a list of files with $(NULL), maybe want to do that here too?

@@ +25,5 @@
>    CustomEditText.java           \
>    INIParser.java                \
>    INISection.java               \
> +  JarUtils.java                 \
> +  $(UTIL_JAVA_FILES)

Ditto.

Comment 5

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

Comment on attachment 647785 [details] [diff] [review]
create-DirectBufferAllocator.patch

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

Awesome!

::: mobile/android/base/DirectBufferAllocator.java
@@ +13,5 @@
> +// https://code.google.com/p/android/issues/detail?id=16941
> +//
> +
> +public final class DirectBufferAllocator {
> +    DirectBufferAllocator() {}

Make this private?

::: mobile/android/base/Tab.java
@@ +142,5 @@
>      synchronized public ByteBuffer getThumbnailBuffer() {
>          int capacity = getThumbnailWidth() * getThumbnailHeight() * 2 /* 16 bpp */;
>          if (mThumbnailBuffer != null && mThumbnailBuffer.capacity() == capacity)
>              return mThumbnailBuffer;
> +        // not calling this.freeBuffer() because it would deadlock

This should not actually deadlock, so if you want to rearrange this code to call freeBuffer go ahead. I made a note that the comment was incorrect when I reviewed that code originally (https://bugzilla.mozilla.org/show_bug.cgi?id=755070#c31) but I guess brad didn't update it.

@@ +164,1 @@
>          mThumbnailBuffer = null;

One thing I like to do with my "free" functions is have it return null. That way you can do "mThumbnailBuffer = DirectBufferAllocator.free(mThumbnailBuffer)" and avoid accidentally leaving a dangling pointer to a freed buffer. Some people don't like that idiom but I'd putting it here for consideration.

::: mobile/android/base/gfx/CheckerboardImage.java
@@ +113,5 @@
>      @Override
>      protected void finalize() throws Throwable {
>          try {
> +            DirectBufferAllocator.free(mBuffer);
> +            mBuffer = null;

Just FYI, I filed bug 779511 to move this code out of finalize()

Comment 6

[Sriram Ramasubramanian [:sriram]]

I had made a bunch of edits that doesn't need GeckoApp.mAppContext to get the displaymetrics. Is this applying on top of it?

Comment 7

[Chris Peterson [:cpeterson]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> I had made a bunch of edits that doesn't need GeckoApp.mAppContext to get
> the displaymetrics. Is this applying on top of it?

Sriram,

1. Which bug or changeset was that? My patches apply cleanly on mozilla-central. The primary motivation is to remove references from gfx classes to GeckoAppShell.

2. Is the name `ConfigurationUtils` OK? I considered `DisplayUtils` because this class is only used for DisplayMetrics, at the moment. But I thought ConfigurationUtils might be a good home for other utility functions.

Comment 8

[Chris Peterson [:cpeterson]]

kats, I have implemented your suggestions in my local patches:

1. Added $(NULL) to Makefile.

2. Made DirectBufferAllocator ctor private.

3. Removed incorrect comment about deadlock and call freeBuffer().

4. Returned null from DirectBufferAllocator.free(). I think this is a little too clever/indirect and makes for longer lines of code, but there is no harm in allowing other people to use this idiom. :)

Comment 9

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdd559da59cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb8b2b5a310
https://hg.mozilla.org/integration/mozilla-inbound/rev/4888b8395089

Comment 10

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 647783 [details] [diff] [review]
part-2-create-ConfigurationUtils.patch

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

I feel that context.getResources().getDisplayMetrics() can do the job.
Resources is closed to tied to the display and it always has an updated copy of DisplayMetrics.
Sorry :(

Comment 11

[Chris Peterson [:cpeterson]]

I accidentally landed Part 2 before Sriram r+'d the patch. Backing out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa61e5ec5612

Comment 12

[Chris Peterson [:cpeterson]]

Attachment: part-2b-pass-DisplayMetrics.patch

Part 2b: Remove ViewportMetrics' dependency on GeckoApp by passing DisplayMetrics parameter.

Comment 13

[Chris Peterson [:cpeterson]]

Attachment: part-2c-getResources-getDisplayMetrics.patch

Part 2c: Query Resources for DisplayMetrics, not the WindowManager.

Comment 14

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 648106 [details] [diff] [review]
part-2b-pass-DisplayMetrics.patch

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

I like this change of killing one more GeckoApp.mAppContext! yaaay!!
How about passing a context to ViewportMetrics() and let it deal with it, instead of spoon feeding it with DisplayMetrics?

Comment 15

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 648107 [details] [diff] [review]
part-2c-getResources-getDisplayMetrics.patch

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

This looks better. When GeckoApp.mAppContext is replaced with mActivity later, we are already ready :)

Comment 16

[Chris Peterson [:cpeterson]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #14)
> How about passing a context to ViewportMetrics() and let it deal with it,
> instead of spoon feeding it with DisplayMetrics?

Since ViewportMetrics only needs DisplayMetrics, I didn't want to add a dependency on the entire Context. I'm giving ViewportMetrics just one cookie, not the whole cookie jar. :)

Comment 17

[Sriram Ramasubramanian [:sriram]]

Makes sense :)
Already r+ ed.

Comment 18

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0ac9e6acaa5
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dc8f2dc324a

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fdd559da59cf
https://hg.mozilla.org/mozilla-central/rev/4888b8395089

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f0ac9e6acaa5
https://hg.mozilla.org/mozilla-central/rev/9dc8f2dc324a

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 647785 [details] [diff] [review]
create-DirectBufferAllocator.patch

>+    public static ByteBuffer allocate(int size) {
>+        if (size <= 0 || (size % 4) != 0) {
>+            throw new IllegalArgumentException("Invalid size " + size);
>+        }

Er, what -- why this assertion?  It's perfectly valid to allocate byte buffers that are not multiples of 4 in size!  We trigger this all over the place in today's Nightly, for some reason especially on the Nexus 7.  See https://bugzilla.mozilla.org/show_bug.cgi?id=776906#c7

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Fixed that in http://hg.mozilla.org/mozilla-central/rev/e62747d22841 (bug 779826)

Comment 23

[Chris Peterson [:cpeterson]]

These changes have landed in m-c.