Closed Bug 1261313 Opened 4 years ago Closed 4 years ago

Remove RemoveVisitsByTimeframe API

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox51 --- fixed

People

(Reporter: mak, Assigned: habeebahma1, Mentored)

References

Details

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

Attachments

(1 file, 3 obsolete files)

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, I am very new in the community, can I work on this bug? Thanks
Flags: needinfo?(mak77)
(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
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.
Assignee: nobody → edyguo0926
Flags: needinfo?(mak77)
(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
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=
Flags: needinfo?(edyguo0926)
I want to work on this bug.
Please assign to me.
Thank You.
if you have patch ready in the next 2 weeks, please ask for review to Drew Wilcoxon (:adw), since I'm away
Assignee: edyguo0926 → habeebahma1
Flags: needinfo?(edyguo0926)
Do I have to simply remove the function or do something else
Thanks
In this patch I removed the funtion RemoveVisitByTimeFrame. Please suggest if I have to do something else.
Thank You
Attachment #8778565 - Flags: review?(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.
Attachment #8778565 - Flags: review?(adw)
Attachment #8779297 - Flags: review?(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.
Attachment #8779297 - Flags: review?(adw) → review+
Attached patch for check inSplinter Review
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.
Attachment #8739723 - Attachment is obsolete: true
Attachment #8778565 - Attachment is obsolete: true
Attachment #8779297 - Attachment is obsolete: true
Attachment #8779578 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Actually I checked it in myself just now.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/df29bfbb6d10
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
I guess this can ride the trains.
You need to log in before you can comment on or make changes to this bug.