Closed Bug 1421212 Opened 2 years ago Closed 2 years ago

Remove RESULTS_AS_FULL_VISIT

Categories

(Toolkit :: Places, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: mak, Assigned: aayusht4736, Mentored)

Details

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

Attachments

(1 file, 3 obsolete files)

I am new and would like to work on this bug, and if I understand it correctly I have to remove case "RESULT_AS_FULL_VISIT" from cpp file ncNavhistory.cpp and corresponding test files, is it ok to take it work on it?
Yes, you can work on it. the references to that result type should be removed.
This has most introduction information and explains how to get a build:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
I cant find RESULT_AS_FULL_VISIT in the mozilla-central code base
(In reply to aayusht4736 from comment #4)
> I cant find RESULT_AS_FULL_VISIT in the mozilla-central code base

See the link in comment 0. Note, that it is `RESULTS_AS_FULL_VISIT` rather than `RESULT_AS_FULL_VISIT` (you're missing an 'S').
Attached patch bug1421212.patch (obsolete) — Splinter Review
I have removed reference to RESULTS_AS_FULL_VISIT in all the files mentioned and ran xpcshell and eslint and all the tests passed successfully.
Attachment #8934154 - Flags: review?(mak77)
Comment on attachment 8934154 [details] [diff] [review]
bug1421212.patch

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

Thank you, there are still a few things to fix.
Please format the title as "Bug 123456 - bug description. r=mak"

When providing a new patch, please remember to squash/merge it with this one, the patches we use are diffs from the original tree.

::: toolkit/components/places/nsINavHistoryService.idl
@@ +1045,3 @@
>     * @note Not yet implemented. See bug 409662.
>     * @note This result type is only supported by QUERY_TYPE_HISTORY.
>     */

the javadoc here above should also be removed

::: toolkit/components/places/nsNavHistory.cpp
@@ +1500,4 @@
>        break;
>  
>      case nsINavHistoryQueryOptions::RESULTS_AS_VISIT:
> +    

There are some trailing spaces here.
You should also not remove the code for the case, since it's still valid for RESULTS_AS_VISIT. you should only remove the case line.

@@ +4004,3 @@
>    switch (aOptions->ResultType())
>    {
>      case nsNavHistoryQueryOptions::RESULTS_AS_VISIT:

also here, you should only remove the case line, the code is still valid for the remaining case nsNavHistoryQueryOptions::RESULTS_AS_VISIT

::: toolkit/components/places/tests/unit/test_history.js
@@ +45,3 @@
>    options.sortingMode = options.SORT_BY_DATE_DESCENDING;
>    options.maxResults = 1;
>    // TODO: using full visit crashes in xpcshell test

this todo comment should also be removed since it refers to the removed  code

@@ +59,1 @@
>      // do_check_eq(node.transitionType, histsvc.TRANSITION_TYPED);

this commented like should also be removed, it seems to refer to the removed comment line.
Attachment #8934154 - Flags: review?(mak77) → review-
Assignee: nobody → aayusht4736
Status: NEW → ASSIGNED
Attached patch bug1421212.patch (obsolete) — Splinter Review
Removed and edited the codes as described and ran xpcshelltest and eslint to which it passed successfully.
Attachment #8935842 - Flags: review?(mak77)
Comment on attachment 8935842 [details] [diff] [review]
bug1421212.patch

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

almost good just a few things to fix

::: toolkit/components/places/nsINavHistoryService.idl
@@ +1036,4 @@
>     */
>    const unsigned short RESULTS_AS_VISIT = 1;
>  
> +  

Some trailing spaces here that should be removed, and please just leave one empty line, not 2

::: toolkit/components/places/nsNavHistory.cpp
@@ +1500,5 @@
>        break;
>  
>      case nsINavHistoryQueryOptions::RESULTS_AS_VISIT:
> +      rv=SelectAsVisit();
> +      NS_ENSURE_SUCCESS(rv,rv);

please restore whitespaces as they were

@@ +4022,1 @@
>      case nsNavHistoryQueryOptions::RESULTS_AS_URI:

why did you remove the newline from above this?
Attachment #8935842 - Flags: review?(mak77)
Attachment #8934154 - Attachment is obsolete: true
Attached patch bug1421212.patch (obsolete) — Splinter Review
Made all the changes as so specified and ran xpcshell test to which it passed successfully
Attachment #8936998 - Flags: review?(mak77)
Attachment #8935842 - Attachment is obsolete: true
Comment on attachment 8936998 [details] [diff] [review]
bug1421212.patch

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

just one small remaining glitch, but the patch looks good

::: toolkit/components/places/nsNavHistory.cpp
@@ +1503,2 @@
>        rv = SelectAsVisit();
> +      NS_ENSURE_SUCCESS(rv,rv);

you still removed one space here after the comma, please put it back.
Attachment #8936998 - Flags: review?(mak77) → review+
Mark, in case I'm on PTO, could you please help with landing this, once ready?
Flags: needinfo?(standard8)
Attached patch bug1421212.patchSplinter Review
Re-inserted the space as specified.
Attachment #8937058 - Flags: review?(mak77)
Comment on attachment 8937058 [details] [diff] [review]
bug1421212.patch

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

Thank you, I'll push your patch to Try.
Attachment #8937058 - Flags: review?(mak77) → review+
Attachment #8936998 - Attachment is obsolete: true
Flags: needinfo?(standard8)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/21bc5f64c339
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.