Closed
Bug 781346
Opened 12 years ago
Closed 12 years ago
Expose local profile directory
Categories
(Toolkit Graveyard :: OS.File, defect)
Toolkit Graveyard
OS.File
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla20
People
(Reporter: Yoric, Assigned: AMoz)
References
Details
(Whiteboard: [mentor=Yoric][lang=c++])
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
Bug 775588 exposes the profile directory (aka ProfD), which may be remote under Windows. We should expose the local profile directory (aka ProfLD).
Reporter | ||
Comment 1•12 years ago
|
||
By the way, what do ProfD and ProfLD yield on Android?
Reporter | ||
Updated•12 years ago
|
Whiteboard: [mentor=Yoric][lang=c++]
Comment 2•12 years ago
|
||
can I be assigned this project???
Reporter | ||
Comment 3•12 years ago
|
||
With pleasure. Note, however, that this project will have to wait a few days until we have found the problem with bug 775588.
Assignee: nobody → mailto.rajesh05
Depends on: 775588
Comment 4•12 years ago
|
||
Thanks a lot. Can you please tell approx time???
Reporter | ||
Comment 5•12 years ago
|
||
I think that I have found a fix for bug 775588. If testing confirms that my fix is correct, I hope that the patch will have landed by Wednesday.
Reporter | ||
Comment 6•12 years ago
|
||
(In reply to rajesh kumar from comment #4)
> Thanks a lot. Can you please tell approx time???
It has landed.
Reporter | ||
Comment 7•12 years ago
|
||
Rajesh, are you still planning to work on this?
Reporter | ||
Updated•12 years ago
|
Assignee: mailto.rajesh05 → nobody
Assignee | ||
Comment 8•12 years ago
|
||
I am interested to contribute to this bug. As explained by Mr. Yoric in irc, i got to know the concepts but i want to know what modifications need to be done in the corresponding files.
Reporter | ||
Comment 9•12 years ago
|
||
@Amod, the relevant code is in OSFileConstants.cpp.
As you may see in that code, we fetch the special directory NS_APP_USER_PROFILE_50_DIR to define a JavaScript constant |profileDir|. This is the global profile directory.
The objective of this bug is to also fetch the special directory NS_APP_USER_PROFILE_LOCAL_50_DIR to define a JavaScript constant |localProfileDir|. This will be the local profile directory.
You may find the documentation of most of the functions used in this file at https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/
You will then need to write a test. For this purpose, extend function |test_path| from main_test_osfile_async.js with a check almost identical to that of |profileDir|, but using |localProfileDir| and "ProfLD" instead of |profileDir| and "ProfD".
Assignee | ||
Comment 10•12 years ago
|
||
i have just replicated the stmnts with few minor changes. Also i have to write the test function..but before that let me know whether the initial part sounds good or not.
Assignee | ||
Comment 11•12 years ago
|
||
i dont understand why the diff view looks so strange when i have changed only few lines.
Reporter | ||
Comment 12•12 years ago
|
||
(In reply to Amod from comment #11)
> i dont understand why the diff view looks so strange when i have changed
> only few lines.
The reference version seems to be a very old version. Have you pulled recently?
Reporter | ||
Comment 13•12 years ago
|
||
Comment on attachment 673373 [details] [diff] [review]
this is a very crude patch...may contain many errors
Review of attachment 673373 [details] [diff] [review]:
-----------------------------------------------------------------
The result looks good. You will need to write a test, of course.
Could you first resolve the hg-related issue, though? Maybe you forgot to call |hg update| after |hg pull|?
Attachment #673373 -
Flags: feedback+
Assignee | ||
Comment 14•12 years ago
|
||
Actually i didn’t use any hg command to get the code. I browsed to the file stored on the server and downloaded that (since it was only a matter of single file).
Reporter | ||
Comment 15•12 years ago
|
||
Amod, as I mentioned over IRC, you really should start using hg. Otherwise, you will end up with issues like this one.
https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Mercurial
Assignee | ||
Comment 16•12 years ago
|
||
i tried a lot but on the command..
hg pull
the outcome is
abort: no default repository found.
i tried this being in mozilla-central folder and once in mozilla-central/src folder.
Initially there was no src folder so i created it also i created the files /.hg/hgrc and wrote :
[paths]
default = http://hg.mozilla.org/mozilla-central/
now what else should i do ?
I even tried contacting in irc but didn’t get any solution.
Assignee | ||
Comment 17•12 years ago
|
||
has solved the issue thanks to jdm.
But now the connection times out a lot while executing the hg cmd.
that will be solve soon.
thanks !
Assignee | ||
Comment 18•12 years ago
|
||
Solving the problem !
Currently i have exams till 3rd, so will continue the work after that. Thanks !
Assignee | ||
Comment 19•12 years ago
|
||
i used hg pull over other system and copied the entire folder on my system...Still that is giving the same patch..
Reporter | ||
Comment 20•12 years ago
|
||
That sounds surprising. Did you pull from mozilla-central?
Assignee | ||
Comment 21•12 years ago
|
||
yes.
But still one thing is probing my mind. if the patch is getting compared with the old version on the server, then how come the client is at fault ?
Reporter | ||
Comment 22•12 years ago
|
||
The server is used only to download the latest version to the client. The patch is always compared with the version on the client. Consequently, the issue must be on the client – or in the command used to generate the patch.
* Normally, after your pull, the version of OSFileConstants.cpp that you have on your computer is identical to the following: dxr.mozilla.org/mozilla-central/dom/system/OSFileConstants.cpp.html – check the definition of |typedef struct { ... } Paths| to ensure that this is correct. If the version that you have has a different version of |Paths|, then you have the wrong version on your computer, and you should fix this.
* Create a new patch with |hg qnew|, e.g. |hg qnew localProfileDirectory|.
* Make your changes to OSFileConstants.cpp .
* Record your changes to the patch with |hg qref|.
* Attach the patch to this bug.
Assignee | ||
Comment 23•12 years ago
|
||
the typedef struct{ //stmnts } is very much similar to that of the dxr path mentioned above but when i use diff /path of the file > patch_name, it compares with the older version.
i have PASTEBINed the typedef struct {} on the following link.
http://pastebin.mozilla.org/1911255
(In reply to David Rajchenbach Teller [:Yoric] from comment #22)
> The server is used only to download the latest version to the client. The
> patch is always compared with the version on the client. Consequently, the
> issue must be on the client – or in the command used to generate the patch.
>
> * Normally, after your pull, the version of OSFileConstants.cpp that you
> have on your computer is identical to the following:
> dxr.mozilla.org/mozilla-central/dom/system/OSFileConstants.cpp.html – check
> the definition of |typedef struct { ... } Paths| to ensure that this is
> correct. If the version that you have has a different version of |Paths|,
> then you have the wrong version on your computer, and you should fix this.
>
> * Create a new patch with |hg qnew|, e.g. |hg qnew localProfileDirectory|.
>
> * Make your changes to OSFileConstants.cpp .
>
> * Record your changes to the patch with |hg qref|.
>
> * Attach the patch to this bug.
Reporter | ||
Comment 24•12 years ago
|
||
Good, that's the right definition of |Path|.
Now, I don't know how you use |diff|, but you should use |hg qnew|/|hg qref|, as I mentioned above. This should solve the issue.
I forgot to mention that you will find the file containing the patch in directory .hg/patches
Reporter | ||
Comment 25•12 years ago
|
||
Ok, this bug is now officially blocking me from progressing with bug 753768. Amod, our discussion on IRC seem to indicate that you are basically done with this bug. Is there anything still missing?
Severity: enhancement → blocker
Assignee | ||
Comment 26•12 years ago
|
||
i have to upload the final patch which will extend the | test | function which i will upload within a day.
If you require it very urgently then you may proceed.
Thanks !
Assignee | ||
Comment 27•12 years ago
|
||
Hello.
You earlier mentioned that i need to | extend | function from the js file. I thought there is something | inheritance | involved since i took the word "extend" in that manner (as used in c++ to extend a class).
So far these days i had been searching google and MDN in and out on how to do this until i saw your comment and finally asked on IRC.
jdm told me that what i was interpreting totally different than what you wanted to say.
So was the delay...i would have uploaded the patch far back had i not got confused. You also know in the previous patch that i made the appropriate changes.
Attachment #673373 -
Attachment is obsolete: true
Reporter | ||
Comment 28•12 years ago
|
||
Sorry about the confusion. Thanks for the patch, I will review it.
Reporter | ||
Comment 29•12 years ago
|
||
Comment on attachment 684499 [details] [diff] [review]
The code
Review of attachment 684499 [details] [diff] [review]:
-----------------------------------------------------------------
C++ looks good, but I am still waiting for the tests.
Attachment #684499 -
Flags: review+
Assignee | ||
Comment 30•12 years ago
|
||
which automated test should i opt for ?
Reporter | ||
Comment 31•12 years ago
|
||
The test I asked you to patch is main_test_osfile_async.js, function |test_path|. This test is a chrome mochitest. To launch it:
./mach mochitest-chrome toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
Assignee | ||
Comment 32•12 years ago
|
||
i have exectured the cmd which you mentioned at irc.
It gave following results. I have pastebined upto what the terminal in ubuntu could hold.
Have a look at it.
http://pastebin.mozilla.org/1967164
Assignee | ||
Comment 33•12 years ago
|
||
i have executed the cmd which you mentioned at irc.
It gave following results. I have pastebined upto what the terminal in ubuntu could hold.
Have a look at it.
http://pastebin.mozilla.org/1967164
Reporter | ||
Comment 34•12 years ago
|
||
Amod, I asked you to patch the file, so that we can add it to our test suite. This will allow us to execute the test automatically, on all platforms, with each change to Firefox, to ensure not only that the feature works now but also that it keeps working despite successive changes to Firefox.
So, could you please provide the patch I am asking for?
Assignee | ||
Comment 35•12 years ago
|
||
I have already provided you the patch earlier...then you had told me to do the test which i did just now.
I had modified only two files: one is OSConstants.cpp and main_test_osfile_async.js out of which i have provided the patch of OSConstants.cpp
Assignee | ||
Comment 36•12 years ago
|
||
i added a stmnt to the test_path as said by you.
Reporter | ||
Comment 37•12 years ago
|
||
Comment on attachment 686487 [details] [diff] [review]
test patch.
Review of attachment 686487 [details] [diff] [review]:
-----------------------------------------------------------------
That patch just adds a blank line. Are you sure it's the right patch?
Assignee | ||
Comment 38•12 years ago
|
||
apologizes..this is with the required changges !
Attachment #686487 -
Attachment is obsolete: true
Reporter | ||
Comment 39•12 years ago
|
||
Comment on attachment 686495 [details] [diff] [review]
The test
Review of attachment 686495 [details] [diff] [review]:
-----------------------------------------------------------------
Ok, that one looks good.
Attachment #686495 -
Flags: review+
Assignee | ||
Comment 40•12 years ago
|
||
what work is remaining now regarding this bug ?
Reporter | ||
Updated•12 years ago
|
Attachment #684499 -
Attachment description: contains all the changes you have mentioned ! → The code
Reporter | ||
Updated•12 years ago
|
Attachment #686495 -
Attachment description: test patch. → The test
Reporter | ||
Comment 41•12 years ago
|
||
At this stage, we still need to:
- merge with the latest versions of m-c;
- launch tests on all platforms;
- format the patches correctly.
As I need this patch, I will take over from this point.
Reporter | ||
Comment 42•12 years ago
|
||
Same patches, consolidated as one patch, and following the conventions of http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Attachment #684499 -
Attachment is obsolete: true
Attachment #686495 -
Attachment is obsolete: true
Attachment #686544 -
Flags: review+
Reporter | ||
Comment 43•12 years ago
|
||
Pushed to Try: https://tbpl.mozilla.org/?tree=Try&rev=dde507f262ac
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → amod.narvekar
Comment 45•12 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 46•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Updated•1 years ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•