1256922 - Remove unused HardwareUtils.isLowMemoryPlatform()

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

The isLowMemoryPlatform() method of the HardwareUtils class is unused.

To start, set up a build environment - you can see the instructions here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

To fix this bug, you need to remove this method:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java#89-100

At least LOW_MEMORY_THRESHOLD_MB will be unused after that. Remove it too.

Then, you'll need to create a patch to upload - see
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 1

[bzrd_Sdn]

I'd really like to fix this bug, It's will my first bug

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to bzrd_Sdn from comment #1)
> I'd really like to fix this bug, It's will my first bug

Great! Let me know if you need any help. As soon as you'll upload your first patch I'll assign the bug to you.

Comment 3

[bzrd_Sdn]

I have a problem to push the patch,I don't understand this section 
"To push to mozilla-central and other mozilla-hosted repositories you need committer access, and you must edit the file (your-local-hg-root)/.hg/hgrc (note, this is NOT your ~/.hgrc)" 
thank you. !

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

You can't push to mozilla-central without permission. Instead you'll need to create a patch file with "hg export" and upload it to this bug.

Comment 5

[bzrd_Sdn]

after having done this command hg export . > bug1256922.diff
what should I do ?

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attach the file to this bug. There should be a button at the top of this page.

Comment 7

[bzrd_Sdn]

Attachment: bug1256922.patch

attachment for bug 1256922

Comment 8

[bzrd_Sdn]

sorry this patch is not valid

Comment 9

[bzrd_Sdn]

Attachment: bug1256922.diff

Comment 10

[bzrd_Sdn]

please review the new  patch !
Thank you

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8731233 [details] [diff] [review]
bug1256922.diff

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

Just use the "review?" flag on the attachment if you want the patch to be reviewed. :)

::: mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java
@@ +23,5 @@
>      // has be in sync with Gecko's equivalent threshold (defined in
>      // xpcom/base/nsMemoryImpl.cpp) and should only be used in cases
>      // where we can't depend on Gecko to be up and running e.g. show/hide
>      // reading list capabilities in HomePager.
> +    

NIT: There are some empty spaces here.

@@ +85,5 @@
>      public static int getMemSize() {
>          return SysInfo.getMemSize();
>      }
>  
> +   

NIT: Here too.

::: testing/mozharness/test/hgrc
@@ +2,5 @@
> +username = bzrd_sdn <[e-mail]>
> +
> +[defaults]
> +qnew = -Ue
> +

This should go into your local .hrc file and should not be in the tree or part of your patch. :)

Comment 12

[bzrd_Sdn]

ok! I'll do another patch

Comment 13

[bzrd_Sdn]

Attachment: bug1256922.diff

attachment for bug1256922

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8731323 [details] [diff] [review]
bug1256922.diff

Oh, I did miss this patch. Just set the "review" flag and I'll see it. :)

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8731323 [details] [diff] [review]
bug1256922.diff

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

The change itself looks good. But this patch still touches testing/mozharness/test/hgrc (even though there are no "visible" changed).

Comment 16

[bzrd_Sdn]

Attachment: patch.diff

new patch for bug1256922

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8735803 [details] [diff] [review]
patch.diff

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

Almost! :-)

The hgrc is now gone but there are some new trailing whitespaces.

::: mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java
@@ +23,5 @@
>      // has be in sync with Gecko's equivalent threshold (defined in
>      // xpcom/base/nsMemoryImpl.cpp) and should only be used in cases
>      // where we can't depend on Gecko to be up and running e.g. show/hide
>      // reading list capabilities in HomePager.
> +    

NIT: Trailing whitespaces

@@ +85,5 @@
>      public static int getMemSize() {
>          return SysInfo.getMemSize();
>      }
>  
> +   

NIT: Trailing whitespaces

Comment 18

[bzrd_Sdn]

Attachment: patch.diff

new attachment for bug 1256922

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8735812 [details] [diff] [review]
patch.diff

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

LGTM. Thank you! :)

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

I tried to import this patch in order to push it to try and the repo but it seems to be a broken patch somehow:

> TheSilence:fx-team sebastian$ hg import 'https://bug1256922.bmoattachments.org/attachment.cgi?id=8735812'
> applying https://bug1256922.bmoattachments.org/attachment.cgi?id=8735812
> abort: bad hunk #1 @@ -19,17 +19,17 @@ public final class HardwareUtils {
> (18 17 17 17)

Is this a patch exported from mercurial?

Comment 21

[bzrd_Sdn]

yes

Comment 22

[bzrd_Sdn]

what should I do?

Comment 23

[bzrd_Sdn]

Attachment: bug1256922.diff

I understand what is the problem, normally this new patch is good

Comment 24

[bzrd_Sdn]

Attachment: bug1256922-0104.diff

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8737202 [details] [diff] [review]
bug1256922-0104.diff

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

::: mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java
@@ -23,5 @@
>      // has be in sync with Gecko's equivalent threshold (defined in
>      // xpcom/base/nsMemoryImpl.cpp) and should only be used in cases
>      // where we can't depend on Gecko to be up and running e.g. show/hide
>      // reading list capabilities in HomePager.
> -    private static final int LOW_MEMORY_THRESHOLD_MB = 384;

You should remove the comments too.

Comment 26

[bzrd_Sdn]

Attachment: patchBug1256922.diff

new patch

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8737736 [details] [diff] [review]
patchBug1256922.diff

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

::: mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java
@@ +19,4 @@
>      private static final String LOGTAG = "GeckoHardwareUtils";
>  
>      // Minimum memory threshold for a device to be considered
>      // a low memory platform (see isLowMemoryPlatform). This value

Is this the correct patch? The comment is still there.

Comment 28

[bzrd_Sdn]

Attachment: patch_bug1256922.diff

sorry ! forgot some comments effectively

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8740376 [details] [diff] [review]
patch_bug1256922.diff

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

Did you remove the comments from the patch?

You'll need to remove the comments from the code and then update your commit and create a new patch.

The file you uploaded can't be applied:
> TheSilence:fx-team sebastian$ hg import 'https://bug1256922.bmoattachments.org/attachment.cgi?id=8740376'
> applying https://bug1256922.bmoattachments.org/attachment.cgi?id=8740376
> patching file mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java
> Hunk #1 FAILED at 18
> 1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/java/org/mozilla/gecko/util/HardwareUtils.java.rej
> abort: patch failed to apply

Also: I just saw that the patch doesn't contain a commit message and author line. Is this a patch exported from Mercurial?

Comment 30

[bzrd_Sdn]

Attachment: mypatch_bug125622.diff

new patch!

Comment 31

[bzrd_Sdn]

Attachment: newpatch_bug1256922.diff

new patch

Comment 32

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8740399 [details] [diff] [review]
newpatch_bug1256922.diff

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

LGTM.

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/ef8a688b8b72f266782ed728071dd82dca1acf76
Bug 1256922 - Remove unused HardwareUtils.isLowMemoryPlatform(). r=sebastian

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ef8a688b8b72