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

RESOLVED FIXED in mozilla24

Status

()

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

People

(Reporter: Yoric, Assigned: varuntheknight)

Tracking

Trunk
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

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

Comment 1

4 years ago
Created attachment 739676 [details] [diff] [review]
Changed osfile_shared_allthreads.jsm file to define OS.Shared.TEST to false
Attachment #739676 - Flags: review?(dteller)
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)
(Assignee)

Comment 4

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

Comment 6

4 years ago
Created attachment 741451 [details] [diff] [review]
Updating the bug with the correct patch
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+
(Assignee)

Comment 8

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

Comment 10

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

Comment 12

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

Comment 14

4 years ago
Created attachment 745753 [details] [diff] [review]
Patch for Bug 862765
Attachment #745370 - Attachment is obsolete: true
(Assignee)

Comment 15

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

Comment 20

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

Comment 22

4 years ago
Created attachment 752900 [details] [diff] [review]
Fix for Bug 862675

Added ;r=yoric to the title of the patch.
Attachment #745753 - Attachment is obsolete: true
Attachment #752900 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b07aac272d6
Assignee: nobody → varuntheknight
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9b07aac272d6
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.