Marco Bonardo [:mak] Assignee
	Description • 7 years ago

if domain1.com has icon to domain2.com/favicon.ico, we should still create a page association.

	Drew Willcoxon :adw
	Comment 2 • 7 years ago
mozreview-review

Comment on attachment 8858365 [details]
Bug 1356567 - root icons should still create a page association if the domain differs.

https://reviewboard.mozilla.org/r/130312/#review133122

LGTM

	Marco Bonardo [:mak] Assignee
	Comment 3 • 7 years ago

Looks like I may have uncovered 3 tests misusing addVisits.

	Marco Bonardo [:mak] Assignee
	Comment 5 • 7 years ago

Kit, please review the Sync change needed to properly pass tests. UpdatePlaces doesn't accept anymore "invalid" timestamps, and looks like Sync was allowing to pass floats.

	Lina Butler [:lina]
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8858365 [details]
Bug 1356567 - root icons should still create a page association if the domain differs.

https://reviewboard.mozilla.org/r/130312/#review133182

The test change looks good, but I don't think the `!Number.isInteger(visit.date)` check is necessary. r+ with that removed or explained. Thanks!

::: services/sync/modules/engines/history.js:365
(Diff revision 2)
>      // Walk through the visits, make sure we have sound data, and eliminate
>      // dupes. The latter is done by rewriting the array in-place.
>      for (i = 0, k = 0; i < record.visits.length; i++) {
>        let visit = record.visits[k] = record.visits[i];
>  
> -      if (!visit.date || typeof visit.date != "number") {
> +      if (!visit.date || typeof visit.date != "number" || !Number.isInteger(visit.date)) {

Hmm, we already round the visit date to an integer at https://searchfox.org/mozilla-central/rev/d8496d0a1f6ebef57fe39b9b204475b0eccfb94c/services/sync/modules/engines/history.js#378-379. Is this still needed?

	Marco Bonardo [:mak] Assignee
	Comment 7 • 7 years ago

(In reply to Kit Cambridge [:kitcambridge] (He/him; UTC-8) from comment #6)
> Comment on attachment 8858365 [details]
> Bug 1356567 - root icons should still create a page association if the
> domain differs.
> 
> https://reviewboard.mozilla.org/r/130312/#review133182
> 
> The test change looks good, but I don't think the
> `!Number.isInteger(visit.date)` check is necessary. r+ with that removed or
> explained. Thanks!

The fact is the history api will throw (and crash in debug builds) if it's passed a value that doesn't look like PRTime, basically a value far in the past (that means it's likely the consumer is passing milliseconds). This would prevent future intermittents.
If you want to accept these visits with a bogus visitDate (but why, since they would likely be in the past and expired pretty soon), then I could change the test to pass a large float instead of a small one.

	Lina Butler [:lina]
	Comment 8 • 7 years ago

Understood. I think we already convert floats to integers later in that function, though: https://searchfox.org/mozilla-central/rev/d8496d0a1f6ebef57fe39b9b204475b0eccfb94c/services/sync/modules/engines/history.js#378-379 That's why I was wondering if there's a reason you added the check. Just in case?

	Marco Bonardo [:mak] Assignee
	Comment 9 • 7 years ago

(In reply to Kit Cambridge [:kitcambridge] (He/him; UTC-8) from comment #8)
> Understood. I think we already convert floats to integers later in that
> function, though:
> https://searchfox.org/mozilla-central/rev/
> d8496d0a1f6ebef57fe39b9b204475b0eccfb94c/services/sync/modules/engines/
> history.js#378-379 That's why I was wondering if there's a reason you added
> the check. Just in case?

Yes you convert the float, but the final integer is small and doesn't make any sense to the history system since it would be early '70. I added the check cause, imo, doesn't make sense to accept bogus values.
So either:
1. accept bogus values, then I can leave the code as it was, change the test to pass a large float that once converter to integer will make some sense
2. what I did, stop accepting bogus values (and eventually remove the conversion code later)

	Lina Butler [:lina]
	Comment 10 • 7 years ago

Oh, I see; it's the test that's bogus. Sorry I misunderstood earlier. (2) is good, then. Let's keep what you have; we can drop the conversion code later.

	Pulsebot
	Comment 11 • 7 years ago

Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/eb94759f0fcf
root icons should still create a page association if the domain differs. r=adw,kitcambridge

	Carsten Book [:Tomcat]
	Comment 12 • 7 years ago

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=92286019&repo=autoland

	Marco Bonardo [:mak] Assignee
	Comment 13 • 7 years ago

uh, interesting, I had a Try run without failures.
Probably because I pushed before noon, and the calculation in the test is bogus.
var noon = new Date();
noon.setHours(12);
new Date(noon - (pages.length - pageIndex))
this is not supposed to end up in the future!

test_486978_sort_by_date_queries.js
Full message: TypeError: date: Tue Apr 18 2017 12:01:47 GMT-0700 (PDT) cannot be a future date

	Pulsebot
	Comment 14 • 7 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd5062f73f70
Backed out changeset eb94759f0fcf for test failures in own test

	Marco Bonardo [:mak] Assignee
	Comment 16 • 7 years ago

I changed the test to use "now" instead of "noon", it didn't really make any sense.

	Pulsebot
	Comment 17 • 7 years ago

Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/00ab2938b91c
root icons should still create a page association if the domain differs. r=adw,kitcambridge

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/00ab2938b91c