1085428 - Replace do_log_info with do_print in Places

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 1

[jcollins.95]

Attachment: mypatch.diff

Comment 2

[Akshendra Pratap Singh]

(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 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 4

[Swapnil R Patil [:swaprp]]

It seems this issue has not been updated since long. Is it OK for me to take it?

Thanks.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

considered there has been no action in months, yes.

Comment 6

[Swapnil R Patil [:swaprp]]

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?

Comment 7

[Swapnil R Patil [:swaprp]]

Attachment: Patch_Bug_1085428_replace_do_log_info.diff

I have renamed calls to do_log_info to do_print and also removed the do_log_info function.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=77b10a2f4390

Comment 11

[Swapnil R Patil [:swaprp]]

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.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(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 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 14

[Swapnil R Patil [:swaprp]]

Attachment: I pulled the latest code and made changes. Also did indentation changes wherever necessary.

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.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

after pulling, be sure you update to the new tip

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 18

[Swapnil R Patil [:swaprp]]

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?

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 20

[Swapnil R Patil [:swaprp]]

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.

Comment 21

[Swapnil R Patil [:swaprp]]

Attachment: Bug_1085428_Replace_do_log_info_with_do_print.diff

Cloned latest repository and made changes. Please verify.

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bc7023dd8130