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

RESOLVED FIXED in mozilla26

Status

()

Toolkit
OS.File
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: darkowlzz)

Tracking

({dev-doc-complete})

unspecified
mozilla26
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=Yoric][lang=js][engineering-friend])

Attachments

(1 attachment, 17 obsolete attachments)

4.16 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
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 !
Assignee: nobody → miteshpathak05
Created attachment 709198 [details] [diff] [review]
patch 0.5
Attachment #709198 - Flags: feedback?(dteller)
Created attachment 709200 [details] [diff] [review]
patch 0.5
Attachment #709198 - Attachment is obsolete: true
Attachment #709198 - Flags: feedback?(dteller)
Attachment #709200 - Flags: feedback?(dteller)
Created attachment 709203 [details] [diff] [review]
patch 0.51
Attachment #709200 - Attachment is obsolete: true
Attachment #709200 - Flags: feedback?(dteller)
Attachment #709203 - Flags: feedback?(dteller)
Created attachment 709204 [details] [diff] [review]
patch 0.52
Attachment #709203 - Attachment is obsolete: true
Attachment #709203 - Flags: feedback?(dteller)
Attachment #709204 - Flags: feedback?(dteller)
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|.
Attachment #709198 - Attachment is obsolete: false
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)
Attachment #709204 - Flags: feedback?(dteller)
Any news, mr_pathak?
Flags: needinfo?(miteshpathak05)
In the absence of news, I'll reset this bug. If anybody is interested in working on it, it's up for grabs.
Assignee: miteshpathak05 → nobody
Flags: needinfo?(miteshpathak05)
(Assignee)

Comment 10

5 years ago
Created attachment 730053 [details] [diff] [review]
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?
Attachment #730053 - Flags: feedback?(dteller)
Assignee: nobody → indiasuny000
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|.
Attachment #730053 - Flags: feedback?(dteller) → feedback+
Attachment #709198 - Attachment is obsolete: true
Attachment #709204 - Attachment is obsolete: true
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.
(Assignee)

Comment 13

5 years ago
Created attachment 730101 [details] [diff] [review]
Implemented Deprecated.warning for |creationDate|.

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

Unit test will appear in the next patch.

Thanks.
Attachment #730053 - Attachment is obsolete: true
Attachment #730101 - Flags: feedback?(dteller)
Comment on attachment 730101 [details] [diff] [review]
Implemented Deprecated.warning for |creationDate|.

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

Looks good, thanks.
Attachment #730101 - Flags: feedback?(dteller) → feedback+
(Assignee)

Comment 15

5 years ago
Created attachment 732447 [details] [diff] [review]
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.
Attachment #730101 - Attachment is obsolete: true
Attachment #732447 - Flags: feedback?(dteller)
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: "
Attachment #732447 - Flags: feedback?(dteller) → feedback+
(Assignee)

Comment 17

5 years ago
Created attachment 733609 [details] [diff] [review]
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.
Attachment #732447 - Attachment is obsolete: true
Attachment #733609 - Flags: feedback?(dteller)
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.
Attachment #733609 - Flags: feedback?(dteller)
Any news from this, Sunny?
Flags: needinfo?(indiasuny000)
(Assignee)

Comment 20

5 years ago
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?
Flags: needinfo?(indiasuny000)
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))|.
(Assignee)

Comment 22

5 years ago
Created attachment 744049 [details] [diff] [review]
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?
Attachment #733609 - Attachment is obsolete: true
Attachment #744049 - Flags: feedback?(dteller)
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.
Attachment #744049 - Flags: feedback?(dteller)
(Assignee)

Comment 24

5 years ago
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.
(Assignee)

Comment 26

5 years ago
Created attachment 745662 [details] [diff] [review]
Implemented Deprecated.warning for |creationDate| with test
Attachment #744049 - Attachment is obsolete: true
Attachment #745662 - Flags: review?(dteller)
(Assignee)

Comment 27

5 years ago
Created attachment 745664 [details] [diff] [review]
Implemented Deprecated.warning for |creationDate| with test

Removed whitespace :)
Attachment #745662 - Attachment is obsolete: true
Attachment #745662 - Flags: review?(dteller)
Attachment #745664 - Flags: review?(dteller)
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".
Attachment #745664 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 29

5 years ago
Created attachment 747550 [details] [diff] [review]
Implemented Deprecated.warning for |creationDate| with test
Attachment #745664 - Attachment is obsolete: true
Attachment #747550 - Flags: review?(dteller)
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?
Attachment #747550 - Flags: review?(dteller) → review+
(Assignee)

Comment 32

5 years ago
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 :)
(Assignee)

Comment 34

5 years ago
Created attachment 760178 [details] [diff] [review]
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?
Attachment #747550 - Attachment is obsolete: true
Attachment #760178 - Flags: feedback?(dteller)
(Assignee)

Comment 35

5 years ago
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.
(Assignee)

Comment 37

5 years ago
(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.
Attachment #760178 - Flags: feedback?(dteller)
(Assignee)

Comment 38

5 years ago
Created attachment 763217 [details] [diff] [review]
Implemented Deprecated.warning for |creationDate| with a test.

Fixed errors which were found in previous push to try.
Attachment #760178 - Attachment is obsolete: true
Attachment #763217 - Flags: feedback?(dteller)
(Assignee)

Updated

5 years ago
Attachment #763217 - Attachment is obsolete: true
Attachment #763217 - Flags: feedback?(dteller)
(Assignee)

Comment 40

5 years ago
Created attachment 763329 [details] [diff] [review]
Implemented Deprecated.warning for |creationDate| with a test.

Realized that I have missed something :)
Attachment #763329 - Flags: feedback?(dteller)
(Assignee)

Comment 42

5 years ago
Created attachment 763408 [details] [diff] [review]
Implemented Deprecated.warning for |creationDate| with a test.

Final patch, hopefully :)
Attachment #763329 - Attachment is obsolete: true
Attachment #763329 - Flags: feedback?(dteller)
Attachment #763408 - Flags: feedback?(dteller)
(Assignee)

Comment 43

5 years ago
Created attachment 763410 [details] [diff] [review]
Implemented Deprecated.warning for |creationDate| with a test.
Attachment #763408 - Attachment is obsolete: true
Attachment #763408 - Flags: feedback?(dteller)
Attachment #763410 - Flags: feedback?(dteller)
(Assignee)

Comment 44

5 years ago
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.
Attachment #763410 - Flags: feedback?(dteller) → review+
(Assignee)

Comment 46

5 years ago
Created attachment 765215 [details] [diff] [review]
Implemented Deprecated.warning for |creationDate| with a test.

Added commit message and comment.
Added `return this._deprecatedCreationDate` after the deprecation warning message.
Attachment #763410 - Attachment is obsolete: true
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
Attachment #765215 - Flags: review+
(Assignee)

Comment 48

5 years ago
Could someone please push it to try?
(Assignee)

Comment 50

5 years ago
Thanks Yoric
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/ef9160595f91
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [mentor=Yoric][lang=js] → [mentor=Yoric][lang=js][fixed-in-fx-team]
Whiteboard: [mentor=Yoric][lang=js][fixed-in-fx-team] → [mentor=Yoric][lang=js][fixed-in-fx-team][engineering-friend]
https://hg.mozilla.org/mozilla-central/rev/ef9160595f91
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=Yoric][lang=js][fixed-in-fx-team][engineering-friend] → [mentor=Yoric][lang=js][engineering-friend]
Target Milestone: --- → mozilla26
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?
Depends on: 808321
Flags: needinfo?(dteller)
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.
Flags: needinfo?(dteller)
Keywords: dev-doc-needed
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 916186
Depends on: 916198
You need to log in before you can comment on or make changes to this bug.