Currently, |OS.Constants.libc| and under windows |OS.Constants.win| are initialized eagerly whenever a chrome worker is opened, regardless of whether they are used. We could turn these objects into lazy getters.

Sir, I would like to contribute to this bug. I would be highly obliged if u assign the bug to me..

For reference: - about getters in JavaScript https://developer.mozilla.org/en/JavaScript/Guide/Working_with_Objects#Defining_Getters_and_Setters - about getters in JSAPI https://developer.mozilla.org/En/SpiderMonkey/JSAPI_Cookbook#Defining_a_property_with_a_getter_and_setter

(In reply to Amod from comment #1) > Sir, I would like to contribute to this bug. I would be highly obliged if u > assign the bug to me.. Here you go. Don't hesitate to contact me on IRC if you have any question.

Sure...I will get back soon after my JS getters and setters...I have decided to shift to Linux..so downloading the source code..

Resetting ownership of this bug in the meantime.

Any news?

I talked to a few people on the IRC, and I think I will need to use resolve hooks to fix this. Its my first time with resolve hooks and it might take some time before I probably get it right.

This is just a WIP patch. Can you tell me whether I am going in the right direction? I am creating a function to return the values for the first time, and in that function I'll initialize the constants for future use.

I will try and review this today. If I don't, this will have to wait about two weeks, so please accept my apologies for the delay.

Comment on attachment 725246 [details] [diff] [review] WIP patch Review of attachment 725246 [details] [diff] [review]: ----------------------------------------------------------------- Good start. However, what you are doing would define something along the lines of: OS.Constants.libc = function libc() { OS.Constants.libc.CONSTANT_1 = VALUE_1; OS.Constants.libc.CONSTANT_2 = VALUE_2; // ... } ::: dom/system/OSFileConstants.cpp @@ +646,5 @@ > + > + // now initialize the objects for future calls > + JSObject *objLibc; > + if (!(objLibc = GetOrCreateObjectProperty(cx, objConstants, "libc"))) { > + return false; Instead of doing |GetOrCreateObjectProperty|, you should: - create a new object |objLibc| using https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JS_NewObject - use |objLibc| to overwrite the existing property libc using https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JS_DefineProperty @@ +684,5 @@ > } > > // Build OS.Constants.libc > > + JS_DefineFunction(cx, objConstants, "libc", lazyLibcGetter, 0, 0); Instead of defining |libc| as a function, you should define it as a getter - see https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JS_DefineProperty

I have made the suggested changes. But I am getting these build errors: 0:40.27 /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp: In function ‘bool mozilla::DefineOSFileConstants(JSContext*, JSObject*)’: 0:40.27 /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp:691:90: error: could not convert ‘0’ from ‘int’ to ‘jsval {aka JS::Value}’ 0:40.37 /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp: In function ‘bool mozilla::lazyLibcGetter(JSContext*, unsigned int, jsval*)’: 0:40.37 /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp:658:1: error: control reaches end of non-void function [-Werror=return-type]

(In reply to Sankha Narayan Guria [:sankha93] from comment #11) > Created attachment 729678 [details] [diff] [review] > WIP patch v2 > > I have made the suggested changes. But I am getting these build errors: > > 0:40.27 > /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp: In > function ‘bool mozilla::DefineOSFileConstants(JSContext*, JSObject*)’: > 0:40.27 > /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp:691: > 90: error: could not convert ‘0’ from ‘int’ to ‘jsval {aka JS::Value}’ This `0` is actually the |NULL| you pass as fourth argument. You should pass a jsval. Since you are actually not going to use that jsval, |JSVAL_VOID| (i.e. |undefined|) should do the trick. https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Cookbook?redirectlocale=en-US&redirectslug=SpiderMonkey%2FJSAPI_Phrasebook#Defining_a_read-only_property_with_only_a_getter > 0:40.37 > /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp: In > function ‘bool mozilla::lazyLibcGetter(JSContext*, unsigned int, jsval*)’: > 0:40.37 > /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp:658: > 1: error: control reaches end of non-void function [-Werror=return-type] You forgot to return a boolean from that function. It should be |true| in case of success, |false| in case of error.

Working patch. All tests are passing on machine.

This patch lazy initializes both |OS.Constants.libc| and |OS.Constants.Win|.

Comment on attachment 730520 [details] [diff] [review] patch v2 Review of attachment 730520 [details] [diff] [review]: ----------------------------------------------------------------- That looks good. Now, I would like two things: - confirmation from khuey that we are producing something useful; - a test that ensures that OS.Constants.libc is defined, initially as a getter, and then as an object - a test that ensures that OS.Constants.Win is defined only under Windows - a test that ensures that, if OS.Constants.Win is defined, it is defined as a getter, and then as an object. To determine whether a property is defined as a getter, you may use |Object.getOwnPropertyDescriptor|. ::: dom/system/OSFileConstants.cpp @@ +652,5 @@ > + } > + if(!JS_DefineProperty(cx, objConstants, "libc", OBJECT_TO_JSVAL(objLibc), NULL, NULL, JSPROP_READONLY)) { > + return false; > + } > + vp.setObject(*objLibc); That's the first time I see |vp.setObject|. I suspect that the API has changed while I was looking away. Did you have confirmation from someone on the JSAPI team that this is the right way to do things? @@ +661,5 @@ > + * Function to lazy initialize |OS.Constants.Win| > + */ > +JSBool lazyWinGetter(JSContext *cx, JSHandleObject objConstants, JSHandleId id, JSMutableHandleValue vp) > +{ > +#if defined(XP_WIN) You should move this |#if defined|/|#endif| around the function.

Comment on attachment 730520 [details] [diff] [review] patch v2 Review of attachment 730520 [details] [diff] [review]: ----------------------------------------------------------------- Replace NULL with nullptr everywhere. This isn't a problem since you're just defining a property, but for the future you should be aware that lazyFooGetter can be invoked on objects other than OS. I'm giving an r+ despite the comments because I trust Yoric to ensure that they are addressed before checking this in ;-) ::: dom/system/OSFileConstants.cpp @@ +645,5 @@ > + // initialize |OS.Constants.libc| for future calls > + JSObject *objLibc; > + if (!(objLibc = JS_NewObject(cx, NULL, NULL, objConstants))) { > + return false; > + } Please add newlines between if blocks. if (foo) { doThing(); } if (bar) { doOtherThing(); } etc

sankha93, could you apply khuey's review?

I have made the changes as khuey said. I also shifted the tests to XPC shell tests. But somehow I am having problems with executing them. Maybe I am missing something. Can you please check?

Comment on attachment 737484 [details] [diff] [review] Patch with XPCShell test Review of attachment 737484 [details] [diff] [review]: ----------------------------------------------------------------- Looks close to completion. ::: dom/system/tests/unit/test_lazyConstants.js @@ +4,5 @@ > +function run_test() { > + Components.classes["@mozilla.org/net/osfileconstantsservice;1"]. > + getService(Components.interfaces.nsIOSFileConstantsService). > + init(); > + Components.utils.import("resource://gre/modules/ctypes.jsm"); I don't think you need to import ctypes.jsm @@ +10,5 @@ > +} > + > +function test_lazyLibc() { > + var desc = Object.getOwnPropertyDescriptor(OS.Constants, "libc"); > + isnot(undefined, desc.get, "OS.Constants.libc is defined as a getter function"); At this point, you should probably force evaluation of OS.Constants.libc. ::: dom/system/tests/unit/xpcshell.ini @@ +1,2 @@ > +[DEFAULT] > +head = head_channels.js head_cache.js Your patch doesn't contain head_channels.js or head_cache.js. If you do not need these files, remove them from the head. If you need these files, could you please add them to the patch?

jwalden suggests that the properties should also have JSPROP_NATIVE_ACCESSORS

This patch is just the changes in OSFileConstants.cpp. I am having trouble with the tests, will be doing something about it.

Comment on attachment 756909 [details] [diff] [review] patch without tests Review of attachment 756909 [details] [diff] [review]: ----------------------------------------------------------------- A review won't be very useful until we get the tests to pass.

Sankha, did you make any progress on that bug?

Yes, I will be uploading a patch within 2 days.

What's up, sankha93?

I am sorry, I may not have time to work on this at the moment.

Is this bus still available to solve? Can I take it up?

Thanks for offering to help. That bug doesn't sound terribly useful these days, so I'm going to close it.