Closed Bug 1085428 Opened 6 years ago Closed 6 years ago

Replace do_log_info with do_print in Places

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: mak, Assigned: swaprp, Mentored)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 3 obsolete files)

do_log_info is basically the same as do_print nowadays, so we should remove our own function and use do_print everywhere.

This is the mxr query of entries to replace (it also includes the function definition that should be removed, not replaced)

http://mxr.mozilla.org/mozilla-central/search?string=do_log_info&find=places
Attached patch mypatch.diff (obsolete) — Splinter Review
(In reply to jcollins.95 from comment #1)
> Created attachment 8508399 [details] [diff] [review]
> mypatch.diff

Raise the review flag on your patch. Or this will go unnoticed.
Comment on attachment 8508399 [details] [diff] [review]
mypatch.diff

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

yep, you should ask for review to the mentor when attaching a patch if it's ready for review. otherwise you can use the feedback flag. if you have questions but not a patch you can use the needinfo flag.

Btw, the patch is mostly good, apart that you renamed the do_log_info function to do_print, but this way we'll overwrite the global do_print

::: toolkit/components/places/tests/head_common.js
@@ +777,3 @@
>  {
>    print("TEST-INFO | " + _TEST_FILE + " | " + aMessage);
>  }

you should REMOVE this function, not rename it
Assignee: nobody → jcollins.95
Status: NEW → ASSIGNED
Flags: needinfo?(jcollins.95)
It seems this issue has not been updated since long. Is it OK for me to take it?

Thanks.
considered there has been no action in months, yes.
Assignee: jcollins.95 → nobody
Flags: needinfo?(jcollins.95)
Thanks Marco. Once I am done with code changes, how can I verify that my changes are fine? Do I need to run any tests?
I have renamed calls to do_log_info to do_print and also removed the do_log_info function.
Attachment #8544702 - Flags: review?(mak77)
Comment on attachment 8544702 [details] [diff] [review]
Patch_Bug_1085428_replace_do_log_info.diff

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

Thank you, it looks good, I will push this to the Try server for you to run tests.

In the meanwhile, please fix the following comments, and add r=mak to the commit message

::: toolkit/components/places/tests/unit/test_update_frecency_after_delete.js
@@ +134,2 @@
>                "of bookmark's URI should not be zero if URI is still " +
>                "bookmarked.");

please reindent this
Attachment #8544702 - Flags: review?(mak77) → review+
Also, looks like the patch doesn't apply cleanly to the current code, please pull a most recent version of the tree and update it.

I see rejections in head_autocomplete.js, test_418257.js, test_special_search.js and test_word_boundary_search.js
Thank you. Okay, I will update it. 

Should the above indentation change go in the same patch?

Sorry, I didn't get "add r=mak to the commit message". Could you please explain?

Thanks.
(In reply to Swapnil R Patil [:swaprp] from comment #11)
> Thank you. Okay, I will update it. 
> 
> Should the above indentation change go in the same patch?

Sure.

> Sorry, I didn't get "add r=mak to the commit message". Could you please
> explain?

the commit message for the patch usually should be in the form
"Bug # - description. r=reviewer"
Comment on attachment 8544702 [details] [diff] [review]
Patch_Bug_1085428_replace_do_log_info.diff

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

::: toolkit/components/places/tests/unit/test_update_frecency_after_delete.js
@@ +70,4 @@
>  
>  add_test(function remove_bookmark_still_bookmarked()
>  {
> +  do_print("After removing bookmark, frecency of bookmark's URI should ",

the final comma should be changed to a + here, since it was broken and do_print accepts multiple arguments
I did pull the latest code and made changes. But I didnt find any change to do in head_autocomplete.js, test_418257.js, test_special_search.js and test_word_boundary_search.js, for which there were rejection. Please verify.
Attachment #8548130 - Flags: review?(mak77)
This is what I get applying to the current source

patching file toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
Hunk #4 FAILED at 166
1 out of 5 hunks FAILED -- saving rejects to file toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js.rej
patching file toolkit/components/places/tests/unifiedcomplete/test_418257.js
Hunk #1 FAILED at 29
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/places/tests/unifiedcomplete/test_418257.js.rej
patching file toolkit/components/places/tests/unifiedcomplete/test_special_search.js
Hunk #3 succeeded at 70 with fuzz 1 (offset 0 lines).
Hunk #10 succeeded at 145 with fuzz 1 (offset 0 lines).
Hunk #11 succeeded at 158 with fuzz 1 (offset 0 lines).
Hunk #20 succeeded at 255 with fuzz 1 (offset 0 lines).
Hunk #23 succeeded at 296 with fuzz 1 (offset 0 lines).
Hunk #24 succeeded at 305 with fuzz 1 (offset 0 lines).
Hunk #25 FAILED at 312
Hunk #30 succeeded at 400 with fuzz 1 (offset 0 lines).
Hunk #31 succeeded at 411 with fuzz 1 (offset 0 lines).
Hunk #32 succeeded at 426 with fuzz 1 (offset 0 lines).
1 out of 32 hunks FAILED -- saving rejects to file toolkit/components/places/tests/unifiedcomplete/test_special_search.js.rej
patching file toolkit/components/places/tests/unifiedcomplete/test_word_boundary_search.js
Hunk #3 succeeded at 65 with fuzz 1 (offset 0 lines).
Hunk #4 succeeded at 74 with fuzz 1 (offset 0 lines).
after pulling, be sure you update to the new tip
Flags: needinfo?(nobody)
Assignee: nobody → swapnil.rp15
Flags: needinfo?(nobody)
Attachment #8508399 - Attachment is obsolete: true
Attachment #8544702 - Attachment is obsolete: true
Comment on attachment 8548130 [details] [diff] [review]
I pulled the latest code and made changes. Also did indentation changes wherever necessary.

need a patch that applies cleanly, if you have difficulties with that I can help doing it.
Attachment #8548130 - Flags: review?(mak77)
Hi Marco,

I ran the command "hg pull -u". So this I think updates to new tip. Am I right? And then created the patch. But I had my changes already in place while pulling the latest code. Do I need to undo my changes and then pull the latest code and then make required changes?  

So basically my doubt is - if I have changed any file locally, then while pulling the latest code doesn't that file gets updated with the latest code?
Flags: needinfo?(mak77)
(In reply to Swapnil R Patil [:swaprp] from comment #18)
> Hi Marco,
> 
> I ran the command "hg pull -u". So this I think updates to new tip. Am I
> right? And then created the patch. But I had my changes already in place
> while pulling the latest code. Do I need to undo my changes and then pull
> the latest code and then make required changes?  
> 
> So basically my doubt is - if I have changed any file locally, then while
> pulling the latest code doesn't that file gets updated with the latest code?

if you have changes committed to the tree, the update will likely generate a new head and you'll end up with 2 heads in your repository, then what you should do is to rebase your changeset on top of the repository head.

try checking with hg heads.

If you used mq (mercurial queues) to create your patch, you should just be able to hg qpop -a (pop all the patches), hg update default, then hg qpush (you should then see the conflicts)

if you didn't use mq, you can try 2 different approaches:

1. hg update to the head with your change, then hg pull --rebase (see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Using_hg_rebase)
2. hg update to the head with your change, then hg qimport -r tip (will convert the top changeset to a mq patch), hg qpop -a, hg update default, hg qpush

both methods will send you to your preferred merge tool, as set in the Mercurial config, or the internal merge with manual markers in the files. The second method allows you to also do the changes manually comparing the .rej files left in the tree after you qpush the patch (might be easier).
Flags: needinfo?(mak77)
Hi Marco,

Apologies for the delay. I tried your suggestions. But there was one integrity error. So I am cloning the repository again and giving it a try.

Thanks.
Cloned latest repository and made changes. Please verify.
Attachment #8548130 - Attachment is obsolete: true
Attachment #8554620 - Flags: review?(mak77)
Comment on attachment 8554620 [details] [diff] [review]
Bug_1085428_Replace_do_log_info_with_do_print.diff

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

Since this bitrots easily, I just pushed it

For the next time, please setup your Mercurial.ini so that patches have you set as author. the mdn tutorials have such information.


https://hg.mozilla.org/integration/fx-team/rev/bc7023dd8130
Attachment #8554620 - Flags: review?(mak77) → review+
Target Milestone: --- → mozilla38
https://hg.mozilla.org/mozilla-central/rev/bc7023dd8130
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.