Last Comment Bug 862765 - [OS.File] OS.Shared.TEST should be defined
: [OS.File] OS.Shared.TEST should be defined
Status: RESOLVED FIXED
[mentor=Yoric][lang=js]
:
Product: Toolkit
Classification: Components
Component: OS.File (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla24
Assigned To: varuntheknight
:
: David Teller [:Yoric] (please use "needinfo")
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-17 05:48 PDT by David Teller [:Yoric] (please use "needinfo")
Modified: 2013-06-13 12:40 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Changed osfile_shared_allthreads.jsm file to define OS.Shared.TEST to false (1002 bytes, patch)
2013-04-19 10:54 PDT, varuntheknight
no flags Details | Diff | Splinter Review
Updating the bug with the correct patch (1.16 KB, patch)
2013-04-24 11:20 PDT, varuntheknight
dteller: review+
Details | Diff | Splinter Review
Fix for Bug 862765 (1.14 KB, patch)
2013-05-03 14:55 PDT, varuntheknight
no flags Details | Diff | Splinter Review
Patch for Bug 862765 (1.06 KB, patch)
2013-05-05 21:50 PDT, varuntheknight
dteller: review+
Details | Diff | Splinter Review
Fix for Bug 862675 (1.07 KB, patch)
2013-05-22 12:44 PDT, varuntheknight
dteller: review+
Details | Diff | Splinter Review

Description David Teller [:Yoric] (please use "needinfo") 2013-04-17 05:48:42 PDT
At the moment, OS.Shared.TEST starts undefined but can be magically defined by clients to activate test mode. Not very nice. It should start as |false|.

OS.Shared.TEST should be defined in file osfile_shared_allthreads.jsm.
Comment 1 varuntheknight 2013-04-19 10:54:02 PDT
Created attachment 739676 [details] [diff] [review]
Changed osfile_shared_allthreads.jsm file to define OS.Shared.TEST to false
Comment 2 Kushagra Sinha [:j4nu5] 2013-04-22 03:24:18 PDT
Varun: I think you uploaded the wrong patch ...
Comment 3 David Teller [:Yoric] (please use "needinfo") 2013-04-23 07:30:06 PDT
Comment on attachment 739676 [details] [diff] [review]
Changed osfile_shared_allthreads.jsm file to define OS.Shared.TEST to false

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

I confirm, this looks like the wrong patch.
Comment 4 varuntheknight 2013-04-23 15:05:36 PDT
Could you help me out the mistakes? I am figuring the Mozilla code out to contribute.

Thanks
Comment 5 Kushagra Sinha [:j4nu5] 2013-04-23 16:06:07 PDT
Varun: You accidentally committed the patch and then made another change (adding an extra blank line). So, the patch that you have submitted is actually a diff from your latest change and the applied patch (click the diff link in the 'Attachments' section above).

TL;DR you need to rollback your repo to the 'mozilla' revision and then send the patch ... catch someone on IRC to get a detailed explanation or you can read https://developer.mozilla.org/en-US/docs/Mercurial_Queues ... Cheers :)

P.S. Never commit your patches (unless of course when you become a core-dev)
Comment 6 varuntheknight 2013-04-24 11:20:23 PDT
Created attachment 741451 [details] [diff] [review]
Updating the bug with the correct patch
Comment 7 David Teller [:Yoric] (please use "needinfo") 2013-05-02 11:34:08 PDT
Comment on attachment 741451 [details] [diff] [review]
Updating the bug with the correct patch

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

Looks good, thanks.
Next step is ensuring that your patch respects our guidelines:
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

I believe that you only need to change the qref message.
Comment 8 varuntheknight 2013-05-03 11:09:10 PDT
Should I submit a new patch with the qref message?

Thanks
Comment 9 David Teller [:Yoric] (please use "needinfo") 2013-05-03 13:28:49 PDT
Yes, please do.
By the way, you don't need the test. Just one line |OS.Shared.TEST = false;| should be sufficient.
Comment 10 varuntheknight 2013-05-03 14:55:33 PDT
Created attachment 745370 [details] [diff] [review]
Fix for Bug 862765

I was a bit confused with the documentation regarding submitting patches with qref message. Please let me know if there is any issue with this patch.
Comment 11 Kushagra Sinha [:j4nu5] 2013-05-05 03:25:08 PDT
Varun: I think you have again made the mistake of taking the diff with your previous modification ... diffs should be taken wrt the 'original' mozilla revision
Comment 12 varuntheknight 2013-05-05 19:47:33 PDT
Hi,

I am still getting used to the docs. The repo got messed up so I had to reclone.

So, are these the steps I need to do before attaching the patch:

1. hg qnew name.patch [create the patch file]

2. make the change in source code

3. hg qref -m [to edit the message]

I am following this doc: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ

Thanks,
Comment 13 Kushagra Sinha [:j4nu5] 2013-05-05 20:39:19 PDT
Hi Varun,

Yes, those are the correct steps. Some tips:

1. Never commit your patches

2. If you decide to update your repo at some point, follow the following steps:
hg qpop -a
hg pull -u
hg qpush -a

3. Patches are stored in .hg/patches/ ... I would suggest having a look at the patch file before sending it to make sure that the diff was taken wrt the mozilla revision. To be ultra sure you can use:
hg log -l5
Make sure that your patch is marked as qtip and the mozilla revision is marked as qparent

4. Use "hg st" to make sure that your repo is clean. It does not produce an output if your repo is clean. If you see added(A) or modified(M) files, use "hg qref"

I would highly recommend reading https://developer.mozilla.org/en-US/docs/Mercurial_Queues in addition to the doc you have already mentioned.

Cheers :)
Comment 14 varuntheknight 2013-05-05 21:50:32 PDT
Created attachment 745753 [details] [diff] [review]
Patch for Bug 862765
Comment 15 varuntheknight 2013-05-05 21:51:09 PDT
Hi Kushagra,

Thanks for the helpful tips. I have followed those and hopefully this latest patch follows all of that. :)
Comment 16 Sankha Narayan Guria [:sankha] 2013-05-05 21:56:27 PDT
Comment on attachment 745753 [details] [diff] [review]
Patch for Bug 862765

Don't forget to mark the patch for review! :)
Comment 17 David Teller [:Yoric] (please use "needinfo") 2013-05-06 05:08:20 PDT
Comment on attachment 745753 [details] [diff] [review]
Patch for Bug 862765

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

Looks good to me.
Please don't forget to add ";r=yoric" to the title of your patch, though.
Comment 18 Kushagra Sinha [:j4nu5] 2013-05-18 08:17:55 PDT
Varun: I think you need to re-upload the patch with the suggested change in its title (append ";r=yoric" to it).
Comment 19 David Teller [:Yoric] (please use "needinfo") 2013-05-22 11:27:55 PDT
Varun?
Comment 20 varuntheknight 2013-05-22 12:26:19 PDT
Hi David,

Sorry, I was out for a few days. Could you please list the steps that I need to perform? I do not want to re upload the patch again later :P

Thanks,
Varun
Comment 21 David Teller [:Yoric] (please use "needinfo") 2013-05-22 12:28:06 PDT
You need to add ";r=yoric" to the title of your patch.
Comment 22 varuntheknight 2013-05-22 12:44:23 PDT
Created attachment 752900 [details] [diff] [review]
Fix for Bug 862675

Added ;r=yoric to the title of the patch.
Comment 23 Ryan VanderMeulen [:RyanVM] 2013-06-13 06:47:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b07aac272d6
Comment 24 Ryan VanderMeulen [:RyanVM] 2013-06-13 12:40:21 PDT
https://hg.mozilla.org/mozilla-central/rev/9b07aac272d6

Note You need to log in before you can comment on or make changes to this bug.