Field OS.File.Info.prototype.creationDate is deprecated as it is generally ill-defined. We should use Deprecated.jsm to mark this, in the main thread version of OS.File.

I would like to work on this bug !

Comment on attachment 709198 [details] [diff] [review] patch 0.5 Review of attachment 709198 [details] [diff] [review]: ----------------------------------------------------------------- I realize that I misled you. You cannot import Deprecated.jsm in osfile_unix_front.jsm, as this file is executed in a worker. You will have to make the following changes in osfile_async_front.jsm: - in the constructor of |File.Info|, instead of returning |value|, copy all the fields of |value| *except |creationDate|* to |this| and copy |creationDate| to some object |this._deprecatedCreationDate|; - once the prototype of |File.Info| is defined, add a getter for |creationDate| that basically looks like the patch you just made, except using |this._deprecatedCreationDate| instead of |this.macBirthDate|.

Comment on attachment 709204 [details] [diff] [review] patch 0.52 Review of attachment 709204 [details] [diff] [review]: ----------------------------------------------------------------- (cancelling review, as this patch was posted while I was performing review on the previous version)

Any news, mr_pathak?

In the absence of news, I'll reset this bug. If anybody is interested in working on it, it's up for grabs.

Hi, I would like to grab this one. This patch has the changes suggested in Comment 6. I have imported |Deprecated.jsm| in |osfile_async_front.jsm| and added getter for |creationDate| in the same. Hope that's correct. :) Should some parts of |osfile_unix_front.jsm| be removed?

Comment on attachment 730053 [details] [diff] [review] Implemented changes suggested in comment 6 Review of attachment 730053 [details] [diff] [review]: ----------------------------------------------------------------- That looks good, thanks. It would be nice to also have a unit test. You can check out bug 812859 or ask yzen (generally on #developers) how he performs testing for Deprecated warnings. ::: toolkit/components/osfile/osfile_async_front.jsm @@ +21,5 @@ > > this.EXPORTED_SYMBOLS = ["OS"]; > > Components.utils.import("resource://gre/modules/osfile/osfile_shared_allthreads.jsm", this); > +Components.utils.import("resource://gre/modules/Deprecated.jsm"); You should import in |this|.

Comment on attachment 730053 [details] [diff] [review] Implemented changes suggested in comment 6 Review of attachment 730053 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/osfile/osfile_async_front.jsm @@ +645,3 @@ > }; > if (OS.Constants.Win) { > File.Info.prototype = Object.create(OS.Shared.Win.AbstractInfo.prototype); Oh, I missed that. The definition of the getter is wrong. See the documentation of Object.create for more details about defining getters: https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Object/create Also, you should probably define the getter only once, instead of once per platform, using Object.defineProperty after this |if| block.

* Imported |Deprecated.jsm| in |this|. * Refactored getter implementation. Unit test will appear in the next patch. Thanks.

Comment on attachment 730101 [details] [diff] [review] Implemented Deprecated.warning for |creationDate|. Review of attachment 730101 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks.

* Fixed the mistake made in the previous patch by using |this.Deprecated.warning|. It was throwing while running test. * Wrote a xpcshell-test. The test uses |chrome/toolkit/...| to reach file to be operated on, which is, due to some reason, not working. Says, "File doesn't exists". When used absolute path, it works. xpcshell-test works without any error with abs-path. When using |consoleListener| I am not sure if it works fine. Would be great if you leave some comment on how to use console listener to verify it's proper working.

Comment on attachment 732447 [details] [diff] [review] Implemented Deprecated.warning for |creationDate| with test Review of attachment 732447 [details] [diff] [review]: ----------------------------------------------------------------- Other than the issue below, looks good to me. ::: toolkit/components/osfile/tests/xpcshell/test_warnings.js @@ +29,5 @@ > + let d = OS.File.stat(EXISTING_FILE); > + d = d.then( > + function onSuccess(info) { > + do_print(info.creationDate); > + do_test_finished(); Nit: whitespace, in a few places in the code. (hint: use the "review" mode of Bugzilla to see them) @@ +46,5 @@ > + { > + deprecatedFunction: deprecatedFunctionCreationDate, > + expectedObservation: function (aMessage) { > + ok(aMessage.errorMessage.indexOf("DEPRECATED WARNING") > 0, > + "Field 'creationDate' is deprecated."); If this works, you have a problem: the warnings start with "DEPRECATION WARNING: "

* Removed the whitespaces. * Changed "DEPRECATED" to "DEPRECATION" but there really is some problem. The test comes out to be passes in both the cases. Any idea why? Something to do the way consoleListener is listening to it.

Comment on attachment 733609 [details] [diff] [review] Implemented Deprecated.warning for |creationDate| with a test. Review of attachment 733609 [details] [diff] [review]: ----------------------------------------------------------------- Canceling feedback until we have found why the test passes. Can you check whether |expectedObservation| is actually called? ::: toolkit/components/osfile/tests/xpcshell/test_warnings.js @@ +33,5 @@ > + do_test_finished(); > + }, > + function onFailure(reason) { > + do_print(reason); > + do_test_finished(); Nit: Please remove whitespaces. Hint: See above.

Any news from this, Sunny?

Hi, Sorry for no activity for a long time. Was busy with other bugs. I came back to the patch where I left it last time and found some blunders in the patch. The |consoleListener| was not at all working. Made some changes and made it work. Then jdm helped out in configuring |consoleListener| to start it's |observe| and get the console messages. So now |observe| receives nsIConsoleMessage, which comes out to be undefined. This maybe due to some fault in OS.File.stat implementation. yzen came to help me out with the next part. To handle the promise with task, added yield to OS.File statement. Stored the returned value in a variable. Now, here is where we got stuck again. The returned yield gives an [object object], which seems to be related to promise. When I try to check it's value, it comes out to be undefined and on checking it's properties with |getOwnPropertyNames|, again undefined. I couldn't figure why this was happening. xpcshell-test logs show proper retrieval of file info, but the yielded value seems to be undefined. Here is a paste of the code www.pastebin.mozilla.org/2307974 . The variable which is undefined above is |d|. Can you think of what is missing in the paste which is causing this problem?

I have just fixed a bug that might have been causing your problem: bug 858723. Could you try and update your mozilla-central to the latest version and see if this resolves the issue? Also, to check the keys of an object for debugging purposes, the simplest technique is generally to print |JSON.stringify(Object.keys(myObject))|.

Hi, The test in this patch is working partially correct. If you still remember from our irc discussion, the listener was listening to unnecessary messages too and the current patch does |do_check_true| on irrelevant messages which too which causes a few FAIL and 1 PASS, when the expected message arrives. To avoid the above problem, I inserted an |if| condition which checks for the availability of the expected message in every message. When the expected message is detected by |if|, a |do_check_true| is performed. But when I use this approach, the deprecation message never comes up and the expected message is never detected. Due to come reason, the relevant message is not being caught by the listener in the second approach. Any idea why is this happening?

Comment on attachment 744049 [details] [diff] [review] Implemented Deprecated.warning for |creationDate| with test Review of attachment 744049 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/osfile/tests/xpcshell/test_creationDate.js @@ +21,5 @@ > + > + let consoleListener = { > + observe: function (aMessage) { > + > + if(do_check_true(aMessage.message.indexOf("Field 'creationDate' is deprecated.") > -1)) That's not how |do_check_true()| works. This function never returns a value. It serves to stop the unit test if its argument is not |true|. That's not what you want.

Please have a look at L29-32 in test_creationDate.js. When I uncomment that, and comment out the other |if| block, the promise seems to be incomplete. Logs (do_print of aMessage.message) don't show any warning message. Perhaps it needs some delay, for it to appear or there might be something else.

Comment on attachment 744049 [details] [diff] [review] Implemented Deprecated.warning for |creationDate| with test Review of attachment 744049 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/osfile/tests/xpcshell/test_creationDate.js @@ +31,5 @@ > + deferred.resolve(); > + } > + */ > + > + Services.console.unregisterListener(consoleListener); You should only unregister the listener once the observer has found the message you are looking for.

Removed whitespace :)

Comment on attachment 745664 [details] [diff] [review] Implemented Deprecated.warning for |creationDate| with test Review of attachment 745664 [details] [diff] [review]: ----------------------------------------------------------------- Looks almost complete. Could you add ";r=yoric" to the name of the patch? ::: toolkit/components/osfile/osfile_async_front.jsm @@ +700,5 @@ > +// Deprecated > +Object.defineProperty(File.Info.prototype, "creationDate", { > + get: function creationDate() { > + Deprecated.warning("Field 'creationDate' is deprecated.", "https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File.Info#Cross-platform_Attributes"); > + } Nit: Still a whitespace here :) ::: toolkit/components/osfile/tests/xpcshell/test_creationDate.js @@ +18,5 @@ > + * of creationDate. > + */ > +add_task(function test_deprecatedCreationDate () { > + OS.Shared.DEBUG = true; > + OS.Shared.TEST = true; I have the feeling that you actually don't need lines 21 and 22. Can you check that and, if they are not necessary, remove them? @@ +23,5 @@ > + let deferred = Promise.defer(); > + let consoleListener = { > + observe: function (aMessage) { > + if(aMessage.message.indexOf("Field 'creationDate' is deprecated.") > -1) { > + do_check_true(true); Nit: Could you also print something? Something like "Deprecation message printed".

Comment on attachment 747550 [details] [diff] [review] Implemented Deprecated.warning for |creationDate| with test Review of attachment 747550 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thanks. Have you passed this through TryServer yet?

Pushed to try https://tbpl.mozilla.org/?tree=Try&rev=158380f0c58f

Yoric, I am back :) Could you please help me interpret the try server results? Anything bad?

The fact that every mochitest-other and xpcshell run failed on all platforms is bad, yes :)

The warnings in mochitests were due to not returning anything from |Info|, which was returning |value| earlier. After making proper changes, mochitests do not show warnings now, but now the xpcshell test which was written for this bug isn't working properly. The promise remains pending and the deprecation message is never received. It's strange that when I don't return anything in |Info|, the deprecation test works fine, the test passes with true==true and deprecation message could also be seen. Could you help me with finding out what's wrong with it?

Will clear the whitespaces in the next patch once the above problem is solved :)

As |Info| is a constructor, you should not |return| anything from it.

(In reply to David Rajchenbach Teller [:Yoric] from comment #36) > As |Info| is a constructor, you should not |return| anything from it. But if you see the previous code, it used to return |value|. And if nothing is returned, mochitest throws the same warnings.

Fixed errors which were found in previous push to try.

Pushed to try https://tbpl.mozilla.org/?tree=Try&rev=79da46558294

Realized that I have missed something :)

Pushed to try again https://tbpl.mozilla.org/?tree=Try&rev=c2a74c7cf455

Final patch, hopefully :)

It seems to be clean :) https://tbpl.mozilla.org/?tree=Try&rev=a0ee6a2287d8

Comment on attachment 763410 [details] [diff] [review] Implemented Deprecated.warning for |creationDate| with a test. Review of attachment 763410 [details] [diff] [review]: ----------------------------------------------------------------- We're almost there :) Also, don't forget to give a commit message to your patch. ::: toolkit/components/osfile/osfile_async_front.jsm @@ +674,5 @@ > * OS.File.prototype.stat > * > * @constructor > */ > File.Info = function Info(value) { Looks good, but you'll need to document why you do that. @@ +693,5 @@ > > +// Deprecated > +Object.defineProperty(File.Info.prototype, "creationDate", { > + get: function creationDate() { > + Deprecated.warning("Field 'creationDate' is deprecated.", "https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File.Info#Cross-platform_Attributes"); You should return this._deprecatedCreationDate.

Added commit message and comment. Added `return this._deprecatedCreationDate` after the deprecation warning message.

Comment on attachment 765215 [details] [diff] [review] Implemented Deprecated.warning for |creationDate| with a test. Review of attachment 765215 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. r=me if it passes tests

Could someone please push it to try?

Try: https://tbpl.mozilla.org/?tree=Try&rev=2cf120d20c40

Thanks Yoric

This push re-enabled test_logging.js due to misplacing the new test in the xpcshell manifest (shockingly, we noticed this because it started failing again). Fixed (and hopefully this test works on Windows and Linux, because it hasn't been running there since it was landed). https://hg.mozilla.org/integration/mozilla-inbound/rev/7e01510aa2a6

Is there a bug on file to fix FHR's use of this in providers.jsm? Seems to trigger this warning a lot in https://tbpl.mozilla.org/php/getParsedLog.php?id=26971922&tree=Fx-Team&full=1#error0. Is bug 808321 related?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #55) > Is there a bug on file to fix FHR's use of this in providers.jsm? Seems to > trigger this warning a lot in > https://tbpl.mozilla.org/php/getParsedLog.php?id=26971922&tree=Fx- > Team&full=1#error0. Is bug 808321 related? Yes, it's related; that's a prerequisite for this bug, in that we shouldn't deprecate API calls without (a) providing a replacement of some kind (that bug), and ideally (b) informing a shipping caller that it's deprecated. We're happy to use in FHR whatever alternative mechanism exists (or will exist) to find the birth times of files, but your ping is the first I've heard of this bug, so this comes as a surprise. Yoric, did you make progress on Bug 808321 under a different name, or is this just a new hole in the API that someone needs to fill?

Yes, we still need to update the documentation. However, the replacement methods (macBirthDate and winBirthDate) have been available for a long time. However, as discussed in bug 808321, FHR really shouldn't depend upon these features because they are file system dependent.