Closed Bug 862765 Opened 11 years ago Closed 11 years ago

[OS.File] OS.Shared.TEST should be defined

Categories

(Toolkit Graveyard :: OS.File, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla24

People

(Reporter: Yoric, Assigned: varuntheknight)

Details

(Whiteboard: [mentor=Yoric][lang=js])

Attachments

(1 file, 4 obsolete files)

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.
Varun: I think you uploaded the wrong patch ...
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.
Attachment #739676 - Flags: review?(dteller)
Could you help me out the mistakes? I am figuring the Mozilla code out to contribute.

Thanks
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)
Attachment #739676 - Attachment is obsolete: true
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.
Attachment #741451 - Flags: review+
Should I submit a new patch with the qref message?

Thanks
Yes, please do.
By the way, you don't need the test. Just one line |OS.Shared.TEST = false;| should be sufficient.
Attached patch Fix for Bug 862765 (obsolete) — Splinter Review
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.
Attachment #741451 - Attachment is obsolete: true
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
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,
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 :)
Attached patch Patch for Bug 862765 (obsolete) — Splinter Review
Attachment #745370 - Attachment is obsolete: true
Hi Kushagra,

Thanks for the helpful tips. I have followed those and hopefully this latest patch follows all of that. :)
Comment on attachment 745753 [details] [diff] [review]
Patch for Bug 862765

Don't forget to mark the patch for review! :)
Attachment #745753 - Flags: review?(dteller)
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.
Attachment #745753 - Flags: review?(dteller) → review+
Varun: I think you need to re-upload the patch with the suggested change in its title (append ";r=yoric" to it).
Varun?
Flags: needinfo?(varuntheknight)
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
Flags: needinfo?(varuntheknight)
You need to add ";r=yoric" to the title of your patch.
Added ;r=yoric to the title of the patch.
Attachment #745753 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/9b07aac272d6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: