1261313 - Remove RemoveVisitsByTimeframe API

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

RemoveVisitsByTimeframe has been deprecated in Firefox 47, we should remove it from Firefox 50 on.

http://mxr.mozilla.org/mozilla-central/search?string=removevisitsbytimeframe

Comment 1

[Feng Guo]

Hi, I am very new in the community, can I work on this bug? Thanks

Comment 2

[Feng Guo]

Attachment: remove RemoveVisitsByTimeframe API

Comment 3

[Feng Guo]

(In reply to Marco Bonardo [::mak] from comment #0)
> RemoveVisitsByTimeframe has been deprecated in Firefox 47, we should remove
> it from Firefox 50 on.
> 
> http://mxr.mozilla.org/mozilla-central/search?string=removevisitsbytimeframe

Hi Marco,I removed the API and can you assign it to me so I can upload my patch. Thanks

Comment 4

[Marco Bonardo [:mak]]

hi, yes you can work on this, but note we cannot land the patch before Firefox 50, that according to https://wiki.mozilla.org/RapidRelease/Calendar means June.

Comment 5

[Feng Guo]

(In reply to Marco Bonardo [::mak] from comment #4)
> hi, yes you can work on this, but note we cannot land the patch before
> Firefox 50, that according to https://wiki.mozilla.org/RapidRelease/Calendar
> means June.

sure,thanks for letting me know

Comment 6

[Marco Bonardo [:mak]]

This is now actionable, are you still interested into fixing the bug?
You should actually remove the code, rather than just commenting it out.

http://searchfox.org/mozilla-central/search?q=removevisitsByTimeframe&path=

Comment 7

[habeebahma1]

I want to work on this bug.
Please assign to me.
Thank You.

Comment 8

[Marco Bonardo [:mak]]

if you have patch ready in the next 2 weeks, please ask for review to Drew Wilcoxon (:adw), since I'm away

Comment 9

[habeebahma1]

Do I have to simply remove the function or do something else

Comment 10

[habeebahma1]

Thanks

Comment 11

[habeebahma1]

Attachment: Removed RemoveVisitByTimeFrame api at 3 places

In this patch I removed the funtion RemoveVisitByTimeFrame. Please suggest if I have to do something else.
Thank You

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 8778565 [details] [diff] [review]
Removed RemoveVisitByTimeFrame api at 3 places

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

Thanks for the patch.  You've removed all the right code, but there are a couple of code comments that need proper fixing.  Could you please fix them and post a new patch?  Let me know if you have any questions.

::: toolkit/components/places/nsNavHistory.cpp
@@ -2423,4 @@
>  /**
>   * Performs cleanup on places that just had all their visits removed, including
>   * deletion of those places.  This is an internal method used by
> - * RemovePagesInternal and RemoveVisitsByTimeframe.  This method does not

You're removing too much here.  Please remove only the reference to RemoveVisitsByTimeframe.

@@ +2745,3 @@
>   * @param aEndTime
>   *        The end of the timeframe, inclusive
>   */

This comment here above the nsNavHistory::RemoveVisitsByTimeframe method definition needs to be removed too, since it describes the method.

Comment 13

[habeebahma1]

Attachment: Removed comments and code properly as needed

Comment 14

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a251cb35cc1

Comment 15

[Drew Willcoxon :adw]

Comment on attachment 8779297 [details] [diff] [review]
Removed comments and code properly as needed

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

Thanks, this looks good.  I pushed your patch to tryserver to make sure it passes all our tests.  Once that looks good, we can check in your patch.

Comment 16

[Drew Willcoxon :adw]

Attachment: for check in

tryserver results look good, so I'll mark this as checkin-needed, which means one of the sheriffs will check in your patch soon.

This new patch just makes some whitespace formatting changes and adds a note to the commit message.

Thanks again, Mohammed.

Comment 17

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/df29bfbb6d10d6c5451abd2bfed40c6470ab7161
Bug 1261313 - Remove RemoveVisitsByTimeframe API. r=adw

Comment 18

[Drew Willcoxon :adw]

Actually I checked it in myself just now.

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Sylvestre Ledru [:Sylvestre]]

I guess this can ride the trains.