620367 - Fix unused variable warning in InsertVisitedURI::AddVisit

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Since bug 599969, collision has been unused in opt builds.

Comment 1

[Marco Bonardo [:mak]]

personally I think it would be better to do
bool visited = Fetch...
if (!visited) {
  NS_NOT_REACHED()
}

Comment 2

[Marco Bonardo [:mak]]

hum, the assertion looks strange though. it should ensure that visited is true... This worries me.

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> hum, the assertion looks strange though. it should ensure that visited is
> true... This worries me.
No, it's fine; we just added it to moz_places, so we should not have a visit at this point.

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 498724 [details] [diff] [review]
Patch v1

I prefer the approach in comment 2.  r=sdwilsh with a patch like that.

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #3)
> (In reply to comment #2)
> > hum, the assertion looks strange though. it should ensure that visited is
> > true... This worries me.
> No, it's fine; we just added it to moz_places, so we should not have a visit at
> this point.

well, then the comment is nonsense
     // Now that it should be in the database, we need to obtain the id of the
     // visit we just added.

update it so it says "we need to obtain the id of the page we just added" and not the visit.

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> well, then the comment is nonsense
er, right, the comment is bogus.  s/visit/place/

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Thanks!

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 498745 [details] [diff] [review]
Patch v2

ehr, sorry you inherited my typo, the right one is NS_NOTREACHED (no underscore before reached)

Comment 9

[Marco Bonardo [:mak]]

and should be if (visited) due to comment 3 (we must follow what the assertion was doing)

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v3

Right, sorry.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch for checkin

Comment 12

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/mozilla-central/rev/046dada4fadc