827805 - [OS.File] Use Deprecated.jsm to mark deprecation of OS.File.Info.prototype.creationDate

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 1

[Mitesh Pathak [:mr_pathak]]

I would like to work on this bug !

Comment 2

[Mitesh Pathak [:mr_pathak]]

Attachment: patch 0.5

Comment 3

[Mitesh Pathak [:mr_pathak]]

Attachment: patch 0.5

Comment 4

[Mitesh Pathak [:mr_pathak]]

Attachment: patch 0.51

Comment 5

[Mitesh Pathak [:mr_pathak]]

Attachment: patch 0.52

Comment 6

[David Teller [:Yoric] - still alive but not very active]

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 7

[David Teller [:Yoric] - still alive but not very active]

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)

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Any news, mr_pathak?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

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

Comment 10

[Sunny [:darkowlzz]]

Attachment: Implemented changes suggested in comment 6

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 11

[David Teller [:Yoric] - still alive but not very active]

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 12

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 13

[Sunny [:darkowlzz]]

Attachment: Implemented Deprecated.warning for |creationDate|.

* Imported |Deprecated.jsm| in |this|.
* Refactored getter implementation.

Unit test will appear in the next patch.

Thanks.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 730101 [details] [diff] [review]
Implemented Deprecated.warning for |creationDate|.

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

Looks good, thanks.

Comment 15

[Sunny [:darkowlzz]]

Attachment: Implemented Deprecated.warning for |creationDate| with test

* 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 16

[David Teller [:Yoric] - still alive but not very active]

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: "

Comment 17

[Sunny [:darkowlzz]]

Attachment: Implemented Deprecated.warning for |creationDate| with a test.

* 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 18

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Any news from this, Sunny?

Comment 20

[Sunny [:darkowlzz]]

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?

Comment 21

[David Teller [:Yoric] - still alive but not very active]

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))|.

Comment 22

[Sunny [:darkowlzz]]

Attachment: Implemented Deprecated.warning for |creationDate| with test

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 23

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 24

[Sunny [:darkowlzz]]

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 25

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 26

[Sunny [:darkowlzz]]

Attachment: Implemented Deprecated.warning for |creationDate| with test

Comment 27

[Sunny [:darkowlzz]]

Attachment: Implemented Deprecated.warning for |creationDate| with test

Removed whitespace :)

Comment 28

[David Teller [:Yoric] - still alive but not very active]

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 29

[Sunny [:darkowlzz]]

Attachment: Implemented Deprecated.warning for |creationDate| with test

Comment 30

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 31

[Sunny [:darkowlzz]]

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

Comment 32

[Sunny [:darkowlzz]]

Yoric, 

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

Comment 33

[Josh Matthews [:jdm]]

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

Comment 34

[Sunny [:darkowlzz]]

Attachment: Implemented Deprecated.warning for |creationDate| with a test.

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?

Comment 35

[Sunny [:darkowlzz]]

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

Comment 36

[David Teller [:Yoric] - still alive but not very active]

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

Comment 37

[Sunny [:darkowlzz]]

(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.

Comment 38

[Sunny [:darkowlzz]]

Attachment: Implemented Deprecated.warning for |creationDate| with a test.

Fixed errors which were found in previous push to try.

Comment 39

[Sunny [:darkowlzz]]

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

Comment 40

[Sunny [:darkowlzz]]

Attachment: Implemented Deprecated.warning for |creationDate| with a test.

Realized that I have missed something :)

Comment 41

[Sunny [:darkowlzz]]

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

Comment 42

[Sunny [:darkowlzz]]

Attachment: Implemented Deprecated.warning for |creationDate| with a test.

Final patch, hopefully :)

Comment 43

[Sunny [:darkowlzz]]

Attachment: Implemented Deprecated.warning for |creationDate| with a test.

Comment 44

[Sunny [:darkowlzz]]

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

Comment 45

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 46

[Sunny [:darkowlzz]]

Attachment: Implemented Deprecated.warning for |creationDate| with a test.

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

Comment 47

[David Teller [:Yoric] - still alive but not very active]

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

Comment 48

[Sunny [:darkowlzz]]

Could someone please push it to try?

Comment 49

[David Teller [:Yoric] - still alive but not very active]

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

Comment 50

[Sunny [:darkowlzz]]

Thanks Yoric

Comment 51

[Ryan VanderMeulen [:RyanVM]]

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

Comment 52

[Ryan VanderMeulen [:RyanVM]]

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

Comment 53

[Ryan VanderMeulen [:RyanVM]]

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

Comment 54

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/7e01510aa2a6

Comment 55

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 56

[Richard Newman [:rnewman]]

(In reply to :Gavin Sharp (use [e-mail] 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?

Comment 57

[David Teller [:Yoric] - still alive but not very active]

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.