1003336 - OS.Constants.Sys.Name should not be "Android" for Firefox OS

Status: RESOLVED FIXED

(Toolkit :: OS.File, defect)

Comment 1

[David Teller [:Yoric] (please use "needinfo")]

JavaScript code can use OS.Constants.Sys.Name to determine the name of the operating system upon which it is executed. However, at the moment, this value is "Android" for Firefox OS, since the Firefox OS kernel is based on the Android kernel.

However, there are differences between Gonk (the Firefox OS kernel) and Android, so we should make sure to return something else. The objective of this bug is to replace "Android" with "Gonk" on Firefox OS.

The file defining OS.Constants.Sys.Name is dom/system/OSFileConstants.cpp
To determine whether the operating system is Gonk, use nsIXULRunTime's property `widgetToolkit`. If this property is equal to "gonk", we are running GOnk.

See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIXULRuntime

Comment 2

[Arjun Mukherjee [:illume]]

Hello, I'd like to attempt to resolve this bug. Could you please assign this bug to me ?

Comment 3

[David Teller [:Yoric] (please use "needinfo")]

Hi illume and thanks for working on this bug. I'll be glad to assign the bug once I see a first patch – this is the (recent) official policy on mentored bugs, by the way.

If you have any question, do not hesitate to ask either here or on irc http://client00.chat.mibbit.com/?server=irc.mozilla.org&channel=%23introduction&nick=illume

Comment 4

[Lynn Tran]

Hi, I'm working on this, but I'm wondering how could I tell the compiler/makefile to run that specific OS after being detected by GetWidgetToolkit?

Comment 5

[David Teller [:Yoric] (please use "needinfo")]

Actually, I found out a simpler way. You can simply use
  #ifdef MOZ_WIDGET_GONK
  // Things that will be executed only on B2G
  #else
  // Things that will be executed anywhere else
  #endif // MOZ_WIDGET_GONK

Comment 6

[Lynn Tran]

Oh nice. I thought about that, but wasn't sure if the macro would work because it wasn't something exist by default like mac and windows. Cool. I will make the changes now

Comment 7

[Lynn Tran]

Do I still need to call any specific header files for B2G to run Gonk? just define the macro?

Comment 8

[Lynn Tran]

Attachment: 1003336.diff

Comment 9

[David Teller [:Yoric] (please use "needinfo")]

Comment on attachment 8440786 [details] [diff] [review]
1003336.diff

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

I'm afraid that this patch doesn't do anything.

::: dom/system/OSFileConstants.cpp
@@ +21,5 @@
>  #include <spawn.h>
>  #endif // defined(ANDROID)
> +#if defined MOZ_WIDGET_GONK
> +#else
> +#endif //defined (MOZ_WIDGET_GONK)

These lines are not useful.

@@ +733,5 @@
>  
>    // GetFileAttributes error constant
>    INT_CONSTANT(INVALID_FILE_ATTRIBUTES),
>  
> +  // GetrvNamedSecurityInfo and SetNamedSecurityInfo constants

Typo?

Comment 10

[Lynn Tran]

Can you give me some guides here? I know after using the predecessors, I need to include files that GONK will execute, but I don't know what those would be otherwise, the predecessor is a bit useless and not sure about the last line. It could be I hit something by accident with vim. Will put it back to how it was originally for my next patch

Comment 11

[David Teller [:Yoric] (please use "needinfo")]

As I mentioned, you don't need to include a anything.

You need to use 

#if defined(MOZ_WIDGET_GONK)
 // this will be executed on B2G
#else
 // this will be executed everywhere else
#endif // defined(MOZ_WIDGET_GONK)

to replace the following lines with something that will be better suited for B2G:

http://dxr.mozilla.org/mozilla-central/source/dom/system/OSFileConstants.cpp?from=OSFileConstants.cpp#858-873

As you can see, these lines perform the following tasks:
1. locate the XUL runtime service (which contains informations about the OS);
2. request the name of the OS;
3. convert the name of the OS to a JS string;
4. convert that JS string into a JS value;
5. place the JS value as field `Name` of the object `objSys`.

Instead, on B2G, we should do
1. convert the name "Gonk" to a JS string;
2. convert that JS string into a JS value;
3. place the JS value as a field `Name` of the object `objSys`.

Does this answer your questions?

Comment 12

[Lynn Tran]

Attachment: bug-1003336-fix.patch

Comment 13

[David Teller [:Yoric] (please use "needinfo")]

Comment on attachment 8443243 [details] [diff] [review]
bug-1003336-fix.patch

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

::: dom/system/OSFileConstants.cpp
@@ +28,5 @@
> +
> +JS:Rooted<JS::Value> valVersion(cx, STRING_TO_JSVAL(strVersion));
> +if (!JS_SetProperty(cx, objSys, "Name", valVersion)){
> +    return false;
> +    }

Well, this block looks correct but it's in the wrong place.
Do you believe that this should compile?

Comment 14

[Lynn Tran]

I don't think it should compile, but it somehow compiled on my machine. I think it's missing context if I put it there, and not really sure where I should put it. I'm think of may be moving it to where we declaring the OS.Constant.Sys.Name but using with the #if define perhaps?

Comment 15

[Lynn Tran]

Attachment: bug-1003336-fix.patch

Comment 16

[David Teller [:Yoric] (please use "needinfo")]

Comment on attachment 8444180 [details] [diff] [review]
bug-1003336-fix.patch

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

Could you repost the same patch without changing the indentation of the file?

Generally, changing indentation is a bad idea as:
- it introduces misleading information in the logs (people who try to find who write a line will believe you wrote it and ask you questions or believe you caused the bugs, whereas you only changed the whitespace);
- it makes the life of your reviewer harder (I need to check every single line that you changed, and that's hundreds).

Comment 17

[Lynn Tran]

Attachment: bug-1003336-fix.patch

Comment 18

[David Teller [:Yoric] (please use "needinfo")]

Comment on attachment 8444885 [details] [diff] [review]
bug-1003336-fix.patch

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

::: dom/system/OSFileConstants.cpp
@@ +856,5 @@
>    JS::Rooted<JSObject*> objSys(cx);
>    if (!(objSys = GetOrCreateObjectProperty(cx, objConstants, "Sys"))) {
>      return false;
>    }
> +#if defined MOZ_WIDGET_GONK

Does that build?

@@ +857,5 @@
>    if (!(objSys = GetOrCreateObjectProperty(cx, objConstants, "Sys"))) {
>      return false;
>    }
> +#if defined MOZ_WIDGET_GONK
> +  JSS* strVErsion = JS_NewStringCopyN(cx, "Gonk");

This line looks suspicious. To check whether everything between the #if and #endif builds, you could revert the condition and see what it gives.
e.g.
#if !defined(MOZ_WIDGET_GONK)

Comment 19

[Lynn Tran]

Attachment: New changes

So I tested as you suggested and everything built. Here is the patch for the new changes

Comment 20

[David Teller [:Yoric] (please use "needinfo")]

Comment on attachment 8446972 [details] [diff] [review]
New changes

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

Ok, that looks good, with a few changes below.
The next step will be to test if this breaks anything and either fix the patch or adapt the tests.

Also, you should add a commit message to your patch, along the lines of:

Bug 1003336 - OS.Constants.Sys.Name should not be "Android" for Firefox OS;r=yoric

::: dom/system/OSFileConstants.cpp
@@ +857,5 @@
>    if (!(objSys = GetOrCreateObjectProperty(cx, objConstants, "Sys"))) {
>      return false;
>    }
>  
>    nsCOMPtr<nsIXULRuntime> runtime = do_GetService(XULRUNTIME_SERVICE_CONTRACTID);

This line should go in the #else branch.

@@ +858,5 @@
>      return false;
>    }
>  
>    nsCOMPtr<nsIXULRuntime> runtime = do_GetService(XULRUNTIME_SERVICE_CONTRACTID);
> +#if defined MOZ_WIDGET_GONK  

Nit: Can you remove the whitespace at the end of the line?
Also, for consistency with the rest of the file,
#if defined(MOZ_WIDGET_GONK)

@@ +860,5 @@
>  
>    nsCOMPtr<nsIXULRuntime> runtime = do_GetService(XULRUNTIME_SERVICE_CONTRACTID);
> +#if defined MOZ_WIDGET_GONK  
> +  if (runtime){
> +     JSString* strVersion = JS_NewStringCopyZ(cx, "Gonk");

Nit: In this file, we are using two spaces for indentation. Please rework your layout for consistency.

@@ +862,5 @@
> +#if defined MOZ_WIDGET_GONK  
> +  if (runtime){
> +     JSString* strVersion = JS_NewStringCopyZ(cx, "Gonk");
> +     if (!strVersion){
> +        return false;    

Nit: Can you remove the whitespace at the end of the line?

@@ +866,5 @@
> +        return false;    
> +     }
> +     JS::Rooted<JS::Value> valVersion(cx, STRING_TO_JSVAL(strVersion));
> +     if (!JS_SetProperty(cx, objSys, "Name", valVersion)){
> +        return false;    

Nit: Can you remove the whitespace at the end of the line?

@@ +869,5 @@
> +     if (!JS_SetProperty(cx, objSys, "Name", valVersion)){
> +        return false;    
> +     }
> +  }
> +#else  

Nit: Can you remove the whitespace at the end of the line?

@@ +885,5 @@
>      if (!JS_SetProperty(cx, objSys, "Name", valVersion)) {
>        return false;
>      }
>    }
> +#endif //defined (MOZ_WIDGET_GONK)  

Nit: Can you remove the whitespace at the end of the line?

Comment 21

[David Teller [:Yoric] (please use "needinfo")]

Let's look at test results:
https://tbpl.mozilla.org/?tree=Try&rev=e8c2fd0585da

Comment 22

[Lynn Tran]

So I fixed as you suggested, but it seems like the build failed some tests. I tried to look at them, but I think they are more about styling options when it is using under Gonk instead of Android.  Are we still waiting for 1 more to build or that's all the result?

Comment 23

[David Teller [:Yoric] (please use "needinfo")]

That's alright, it failed the tests that counted on OS.Constants.Sys.Name to be "Android". Now you need to fix these tests so that they work whether OS.Constants.Sys.Name is "Android" or "Gonk".

Comment 24

[Lynn Tran]

Attachment: bug-1003336-fix.patch

So I've fixed the tests that failed the buid, however, there is one that I'm not sure, which is this: https://tbpl.mozilla.org/php/getParsedLog.php?id=42626925&tree=Try&full=1#error6

The test do_check_eq relies checks the service and the constant, so I assume that once you get other tests fixed then the test_constant will return the right result and will fix that error as well.

Comment 25

[David Teller [:Yoric] (please use "needinfo")]

Ok, that's normal. This test is here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/tests/xpcshell/test_constants.js?from=test_constants.js&case=true#18

Services.appinfo.OS is "Android" on B2G and we are not going to fix it.

So you need to do

if (OS.Constants.Sys.Name == "Gonk") {
  // Services.appinfo.OS doesn't know the difference between Gonk and Android
  do_check_eq(Services.appinfo.OS, "Android");
} else {
  do_check_eq(Services.appinfo.OS, OS.Constants.Sys.Name);
}

Comment 26

[David Teller [:Yoric] (please use "needinfo")]

Comment on attachment 8447695 [details] [diff] [review]
bug-1003336-fix.patch

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

Looks good. You just need to fix the last test.

::: dom/system/OSFileConstants.cpp
@@ +857,5 @@
>    if (!(objSys = GetOrCreateObjectProperty(cx, objConstants, "Sys"))) {
>      return false;
>    }
>  
> +#if defined MOZ_WIDGET_GONK

Please add the parentheses, as I asked you.

@@ +859,5 @@
>    }
>  
> +#if defined MOZ_WIDGET_GONK
> +    JSString* strVersion = JS_NewStringCopyZ(cx, "Gonk");
> +    if (!strVersion){

Nit: whitespace between ) and {

Comment 27

[Lynn Tran]

Attachment: bug-1003336-fix.patch

Comment 28

[David Teller [:Yoric] (please use "needinfo")]

Comment on attachment 8448683 [details] [diff] [review]
bug-1003336-fix.patch

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

That looks good to me.
Let's test it.
Try: https://tbpl.mozilla.org/?tree=Try&rev=aa2e22622284

Comment 29

[Lynn Tran]

So the builds are busted and I don't think I've made any changes to the code but just the tests.

Comment 30

[David Teller [:Yoric] (please use "needinfo")]

Ah, my bad. I made a typo when merging your patch.

Try: https://tbpl.mozilla.org/?tree=Try&rev=5b8b9f8c115c

Comment 31

[Lynn Tran]

Attachment: bug-1003336-fix.patch

I made a typo in the tests, and I fixed them all now.

Comment 32

[David Teller [:Yoric] (please use "needinfo")]

Comment on attachment 8449092 [details] [diff] [review]
bug-1003336-fix.patch

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

Looks good to me.
Try: https://tbpl.mozilla.org/?tree=Try&rev=149c8cf36045

Comment 33

[Lynn Tran]

So now there is only one test fails, but I can't find the OS.File.prototype.setDates anywhere. I think that's what causes the fail of the test cases. Does that mean that it takes the date from the prototype OS and make comparison if it's undefined or not? I'm not quite sure where that OS.File.prototype is.

Comment 34

[David Teller [:Yoric] (please use "needinfo")]

Method `OS.File.prototypes.setDates` is set here: http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_async_front.jsm#787

As you can see, we do not set it for Android, because this function does not exist in the Android libc. Since Android and B2G use the same libc, you should make sure that the method also not set on B2G.

Regarding your question, `OS.File.prototype` is the prototype of constructor `OS.File`, which are the objects that represent files in this library.

Comment 35

[Lynn Tran]

Attachment: bug-1003336-fix.patch

Ah thanks. I missed the line where it tells me where the file is.

Comment 36

[David Teller [:Yoric] (please use "needinfo")]

Comment on attachment 8450927 [details] [diff] [review]
bug-1003336-fix.patch

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

r=me with the change below

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +826,5 @@
>    }
>  };
>  
>  
> +if (SharedAll.Constants.Sys.Name != "Android" || SharedAll.Constants.Sys.Name != "Gonk") {

It's && rather than ||.

Comment 37

[Lynn Tran]

Attachment: bug-1003336-fix.patch

Comment 38

[David Teller [:Yoric] (please use "needinfo")]

Comment on attachment 8450940 [details] [diff] [review]
bug-1003336-fix.patch

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

https://tbpl.mozilla.org/?tree=Try&rev=1eea5d3e6836

Comment 39

[Lynn Tran]

Attachment: bug-1003336-fix.patch

I double checked other tests to see if there are any that have yet been tested and requires Gonk/Android.

Comment 40

[David Teller [:Yoric] (please use "needinfo")]

Comment on attachment 8451179 [details] [diff] [review]
bug-1003336-fix.patch

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

By the way, could you mark your previous versions as Obsolete as you upload new versions?

Comment 41

[David Teller [:Yoric] (please use "needinfo")]

Attachment: Fixing the issue

Since there was a merge issue, attaching a fixed version.
You should consider pulling a more recent version of mozilla-central, by the way.

Try: https://tbpl.mozilla.org/?tree=Try&rev=9a7d9d322a40

Comment 42

[David Teller [:Yoric] (please use "needinfo")]

Tests look good. Unless you wish to make other changes, we can land this patch.

Comment 43

[David Teller [:Yoric] (please use "needinfo")]

By the way, once the patch has landed, you should register an account on mozillians.org if you don't have one yet. I will vouch for you.

Comment 44

[Lynn Tran]

I think I'm good with this and I will create an account on mozillans.org

Comment 45

[David Teller [:Yoric] (please use "needinfo")]

Great :)

Let me mark this patch as `checkin-needed`, so that sheriffs can land it. This will trigger all the tests we have to ensure that we do not break something accidentally. Once this is done, the patch will be added to mozilla-central.

If you are interested in working on a related bug, you can take a look at Bug 999748 - Add the user trash directory to OS.Constants.Path. Otherwise, if you want to contribute to another part of the code, don't hesitate to reach out, I will try and help you find something that will interest you.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3042cb4e521e

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3042cb4e521e

Comment 48

[Lynn Tran]

Here is my Mozillan profile: https://mozillians.org/en-US/u/lynn_tran/. I think I will take that bug, but in the mean time, I'm wondering if there is any easy bug for the Javascript compiler, which is IonMonkey I believe to work on? 

Thank you for your help a lot especially for someone, who is completely new to contributing to open source project!

Comment 49

[David Teller [:Yoric] (please use "needinfo")]

For IonMonkey, you should take a look at bug 1003801. Look at the field "Depends on". Most of these bugs are good beginner bugs for IonMonkey.