Closed Bug 1003336 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: OS.File, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: Yoric, Assigned: lynn_tran, Mentored)

References

Details

(Whiteboard: [lang=c++][good first bug])

Attachments

(1 file, 11 obsolete files)

No description provided.
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
Whiteboard: [mentor=Yoric][lang=c++][good first bug]
Hello, I'd like to attempt to resolve this bug. Could you please assign this bug to me ?
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
Flags: needinfo?(arjunmuk)
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?
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
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
Do I still need to call any specific header files for B2G to run Gonk? just define the macro?
Attached patch 1003336.diff (obsolete) — Splinter Review
Attachment #8440786 - Flags: review+
Attachment #8440786 - Flags: review+ → review?(dteller)
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?
Attachment #8440786 - Flags: review?(dteller)
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
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?
Mentor: dteller
Whiteboard: [mentor=Yoric][lang=c++][good first bug] → [lang=c++][good first bug]
Attached patch bug-1003336-fix.patch (obsolete) — Splinter Review
Attachment #8443243 - Flags: review?(dteller)
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?
Attachment #8443243 - Flags: review?(dteller)
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?
Attached patch bug-1003336-fix.patch (obsolete) — Splinter Review
Attachment #8444180 - Flags: feedback?(dteller)
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).
Attachment #8444180 - Flags: feedback?(dteller)
Flags: needinfo?(arjunmuk)
Attached patch bug-1003336-fix.patch (obsolete) — Splinter Review
Attachment #8444885 - Flags: feedback?
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)
Attachment #8444885 - Flags: feedback?
Attached patch New changes (obsolete) — Splinter Review
So I tested as you suggested and everything built. Here is the patch for the new changes
Attachment #8446972 - Flags: feedback?(dteller)
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?
Attachment #8446972 - Flags: feedback?(dteller) → feedback+
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?
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".
Attached patch bug-1003336-fix.patch (obsolete) — Splinter Review
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.
Attachment #8447695 - Flags: feedback?(dteller)
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);
}
Attachment #8444885 - Attachment is obsolete: true
Attachment #8444180 - Attachment is obsolete: true
Attachment #8443243 - Attachment is obsolete: true
Attachment #8440786 - Attachment is obsolete: true
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 {
Attachment #8447695 - Flags: feedback?(dteller) → feedback+
Attached patch bug-1003336-fix.patch (obsolete) — Splinter Review
Attachment #8448683 - Flags: review?(dteller)
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
Attachment #8448683 - Flags: review?(dteller) → review+
So the builds are busted and I don't think I've made any changes to the code but just the tests.
Ah, my bad. I made a typo when merging your patch.

Try: https://tbpl.mozilla.org/?tree=Try&rev=5b8b9f8c115c
Attached patch bug-1003336-fix.patch (obsolete) — Splinter Review
I made a typo in the tests, and I fixed them all now.
Attachment #8449092 - Flags: review?(dteller)
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
Attachment #8449092 - Flags: review?(dteller) → review+
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.
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.
Attached patch bug-1003336-fix.patch (obsolete) — Splinter Review
Ah thanks. I missed the line where it tells me where the file is.
Attachment #8450927 - Flags: review?(dteller)
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 ||.
Attachment #8450927 - Flags: review?(dteller) → review+
Attached patch bug-1003336-fix.patch (obsolete) — Splinter Review
Attachment #8450927 - Attachment is obsolete: true
Attachment #8450940 - Flags: review?(dteller)
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
Attachment #8450940 - Flags: review?(dteller) → review+
Attached patch bug-1003336-fix.patch (obsolete) — Splinter Review
I double checked other tests to see if there are any that have yet been tested and requires Gonk/Android.
Attachment #8450940 - Attachment is obsolete: true
Attachment #8451179 - Flags: review?(dteller)
Attachment #8448683 - Attachment is obsolete: true
Attachment #8447695 - Attachment is obsolete: true
Attachment #8446972 - Attachment is obsolete: true
Attachment #8449092 - Attachment is obsolete: true
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?
Attachment #8451179 - Flags: review?(dteller) → review+
Attached patch Fixing the issueSplinter Review
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
Attachment #8451179 - Attachment is obsolete: true
Attachment #8452287 - Flags: review+
Tests look good. Unless you wish to make other changes, we can land this patch.
Assignee: nobody → lynn_tran
Flags: needinfo?(lynn_tran)
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.
I think I'm good with this and I will create an account on mozillans.org
Flags: needinfo?(lynn_tran)
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.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/3042cb4e521e
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=c++][good first bug] → [lang=c++][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3042cb4e521e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [lang=c++][good first bug][fixed-in-fx-team] → [lang=c++][good first bug]
Target Milestone: --- → mozilla33
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!
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.
You need to log in before you can comment on or make changes to this bug.