Closed Bug 1356567 Opened 3 years ago Closed 3 years ago

root icons should still create a page association if the domain differs

Categories

(Toolkit :: Places, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(1 file)

if domain1.com has icon to domain2.com/favicon.ico, we should still create a page association.
Blocks: 1356079
Blocks: 1356525
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
Attachment #8858365 - Flags: review?(adw) → review+
Looks like I may have uncovered 3 tests misusing addVisits.
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.
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?
Attachment #8858365 - Flags: review?(kit) → review+
(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.
Flags: needinfo?(kit)
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?
Flags: needinfo?(kit)
(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)
Flags: needinfo?(kit)
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.
Flags: needinfo?(kit)
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
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=92286019&repo=autoland
Flags: needinfo?(mak77)
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
Flags: needinfo?(mak77)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd5062f73f70
Backed out changeset eb94759f0fcf for test failures in own test
I changed the test to use "now" instead of "noon", it didn't really make any sense.
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
https://hg.mozilla.org/mozilla-central/rev/00ab2938b91c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.