Bug 221303 - PCM: earliestTimeToSend should be treated as an independent time value, not relative to timeOfAdClick
Summary: PCM: earliestTimeToSend should be treated as an independent time value, not r...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-02 15:43 PST by Kate Cheney
Modified: 2021-02-03 15:58 PST (History)
2 users (show)

See Also:


Attachments
Patch (19.24 KB, patch)
2021-02-02 16:53 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch for landing (19.24 KB, patch)
2021-02-03 15:15 PST, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2021-02-02 15:43:07 PST
Currently the persistent PCM implementation updates earliestTimeToSend after the session is closed. This is not necessary, because earliestTimeToSend is an independent time value and should not need to be updated after a session end/start.
Comment 1 Radar WebKit Bug Importer 2021-02-02 15:43:20 PST
<rdar://problem/73902668>
Comment 2 Kate Cheney 2021-02-02 16:53:26 PST
Created attachment 419080 [details]
Patch
Comment 3 John Wilander 2021-02-03 11:35:10 PST
Comment on attachment 419080 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419080&action=review

> Source/WebKit/ChangeLog:14
> +        need any adjustment after a new session.

Just to make sure, we are now going to send the report 24-48 hours after the triggering event, not after the click, right?

> Source/WebKit/ChangeLog:16
> +        No new tests, this will be covered by 

Will be or is?
Comment 4 Kate Cheney 2021-02-03 11:54:51 PST
Comment on attachment 419080 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419080&action=review

>> Source/WebKit/ChangeLog:14
>> +        need any adjustment after a new session.
> 
> Just to make sure, we are now going to send the report 24-48 hours after the triggering event, not after the click, right?

Yes, the time calculation happens when we insert an attributed report into the DB. We call attributeAndGetEarliestTimeToSend() which calculates now + 24-48 hours.

>> Source/WebKit/ChangeLog:16
>> +        No new tests, this will be covered by 
> 
> Will be or is?

Is. This test now marks an attribution as having an earliest time to send of (now - 1hr) and tests that it sends immediately after a session start. I can update the wording.
Comment 5 John Wilander 2021-02-03 12:25:17 PST
(In reply to katherine_cheney from comment #4)
> Comment on attachment 419080 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419080&action=review
> 
> >> Source/WebKit/ChangeLog:14
> >> +        need any adjustment after a new session.
> > 
> > Just to make sure, we are now going to send the report 24-48 hours after the triggering event, not after the click, right?
> 
> Yes, the time calculation happens when we insert an attributed report into
> the DB. We call attributeAndGetEarliestTimeToSend() which calculates now +
> 24-48 hours.

Good.

> >> Source/WebKit/ChangeLog:16
> >> +        No new tests, this will be covered by 
> > 
> > Will be or is?
> 
> Is. This test now marks an attribution as having an earliest time to send of
> (now - 1hr) and tests that it sends immediately after a session start. I can
> update the wording.

OK, please point out that this patch makes the test check this. Otherwise it sounds like you will have a follow-up patch with a test.
Comment 6 John Wilander 2021-02-03 12:25:38 PST
Comment on attachment 419080 [details]
Patch

r=me with minor comment on change log.
Comment 7 Kate Cheney 2021-02-03 15:15:38 PST
Created attachment 419188 [details]
Patch for landing
Comment 8 EWS 2021-02-03 15:58:51 PST
Committed r272346: <https://trac.webkit.org/changeset/272346>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419188 [details].