1277235 - add typed and visitCount to onVisit

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

We need these notifications to help implementing the onVisited web-extension API.

Comment 1

[Marco Bonardo [:mak]]

Clearly we cannot have a correct typedCount here until typed becomes a counter (bug 1271801), rather than a bool. I will make the api return typed as an int and for now it will just return 0 or 1. Then we should just change the idl comment and it should continue working.

I suppose visitCount should include the just added (and being notified) visit?

Comment 2

[Bob Silverberg [:bsilverberg]]

(In reply to Marco Bonardo [::mak] from comment #1)
> 
> I suppose visitCount should include the just added (and being notified)
> visit?

Yes, I have confirmed that that is what Chrome does.

Comment 3

[Marco Bonardo [:mak]]

Attachment: MozReview Request: Bug 1277235 - add typed and visitCount to onVisit. r=adw

Review commit: https://reviewboard.mozilla.org/r/57188/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57188/

Comment 4

[Marco Bonardo [:mak]]

note, both visitCount and typed will be a little bit special.
The reason for typed is bug 1271801, so it will be fixed when that bug is fixed.
the reason for typed is the fact our visit count doesn't take into account some transitions, like TRANSITION_FRAMED_LINK, TRANSITION_RELOAD or TRANSITION_DOWNLOAD. The reason is that we use this visit count for frecency and views. For those it will likely always return 1. We could evaluate changing this in future but it is complicated to ensure it won't badly regress things and will need to be careful.
On the other side, the current solution should allow to start implementing a working notification for the most common cases add-ons could be used for (I don't expect add-ons to care much about number of visits to a reload transition, for example).

Comment 5

[Marco Bonardo [:mak]]

Sorry, typo there:

(In reply to Marco Bonardo [::mak] from comment #4)
> the reason for visitCount is the fact our visit count doesn't take into account...

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8759127 [details]
MozReview Request: Bug 1277235 - add typed and visitCount to onVisit. r=adw

https://reviewboard.mozilla.org/r/57188/#review55218

Comment 7

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] - Away 9-13 June from comment #4)
> We could evaluate
> changing this in future but it is complicated to ensure it won't badly
> regress things and will need to be careful.
> On the other side, the current solution should allow to start implementing a
> working notification for the most common cases add-ons could be used for (I
> don't expect add-ons to care much about number of visits to a reload
> transition, for example).

Sounds fine to me.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/e69b8135f9e2
add typed and visitCount to onVisit. r=adw

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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