-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[DRAFT] Impression observer logic added to tiles #29919
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I got this logic right... but I would love a sanity check here as I think I've stared at this code for too long
🧹 Tidy commitJust 3 file(s) touched. Thanks for keeping it clean and review-friendly! ❌ Per-file test coverage gateThe following changed file(s) are below 35.0% coverage:
Client.app: Coverage: 37.15
Generated by 🚫 Danger Swift against 728d1e0 |
...lt in a second dwell fire during lifetime
fixed formatting formatting formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably also be a guard !rect.isNull else { return 0 } (usually guard is used for this type of early-returning)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah. Nice call!
firefox-ios/Client/Frontend/Home/TopSites/Cell/ObservableCollectionViewCell.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you can use Timer's built-in initializer instead of instantiating it with its constructor directly so you don't need to manually add it to the run loop like:
visibilityTimer = Timer.scheduledTimer(withTimeInterval: visibleTimeThresholdSeconds, repeats: false) { [weak self] _ in
// do stuff...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so this was throwing me for a loop (pardon the pun).
https://developer.apple.com/documentation/foundation/timer
The docs seem to indicate that using scheduledTimer spawns the timer on the current run loop in default mode. But then these docs seem to indicate that we want to be in .common mode so that the timer can still run and fire even when the user is interacting with the UI. (If I'm understanding it correctly)
https://developer.apple.com/documentation/corefoundation/common-mode-flag
I wasn't able to obviously find a way to force the scheduledTimer into the .common mode but it does seem like there should be a way. I'll look into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh maybe this line actually will work either way.
RunLoop.main.add(t, forMode: .common). Trying to see if there is an easy way to test this but I think it should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should add this tolerance. In practice, this will just ensure that it almost always fires later than you intend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm curious of your thoughts here. I was reading this:
And there is a section on recommending that we set tolerances to allow timers to be batched. Maybe this is an over optimization though... I don't really know.
Just did a pass on this, and also asked some of the other iOS leads for feedback as well. Here's some things to think about:
- this is going to fail on some Swiftlint stuff. For example, single letter variables, are kinda meaningless to read, and are a nono in our codebase. I'd recommend installing Swiftlint and building just to address those concerns straight out
- missing
deinit; we should have one, to guarantee observers are removed even ifprepareForReuseisn’t called due to deallocation timing - we're observing all parent scrollviews under
startObservingIfNeeded; should we only observe the parent scrollview? - do we need to have
layoutSubviewscallingstartObservingIfNeededeach time? do layout updates change the scrollview hierarchy - there's a concern about
layoutSubviewslogic still firing sinceprepareForReusedoesn't resetisVisibilityMonitoringEnabled. The cell could theoretically be reused for a non-sponsored top site...- Maybe
TopSiteCellin this scenario should be the one to resetisVisibilityMonitoringEnabled = falsein its prepareForResuse, since it started the monitoring? - Logic is also duplicated between the
didSetfor false and theprepareForReuse
- Maybe
- nit:
isVisibilityMonitoringEnabledseems largely used for side effects as well as guard checks. We could think about using functions instead of variable setting side effects to initiate the visibility monitoring - minor performance concern: KVO fires with pixel per scroll. We colud think about doing a micro-debounce or something, perhaps?
- nit: we could also use modern KVO tokens (
NSKeyValueObservation) instead of string key paths + overrideobserveValueto prevent crashes from mismatched add/remove and gives type safety - inheriting from
UICollectionViewCell- what if the future version of Discover More, that we already have designs for, and will be building towards, is not in a collection view? Do we want to make this more generic, or, do we want to have a cell & a view version of this? - finally, and this is a bit bit of contention that's really hard to answer at the moment: we currently use Redux in the app to be doing things. So view stuff is in the view, but logic should be happening in some middleware. However, we're also currently assessing (especially next week during the work week we have), whether or not we want to continue to use Redux, and whether it's solve the problems we introduced it to solve and such. So I'm hesitating to say, "hey, this needs to go into our Redux pattern" but I'm also hesitating to say "this is fine" lol. To be discussed!?!? What's the urgency of getting this landed?
Uh oh!
There was an error while loading. Please reload this page.
DO NOT MERGE: This is a draft PR and is still a work in progress.
The goal of this PR is to include a new impression observer class that handles its own view state using a custom UICollectionViewCell base class.
This draft has been opened for comments on the approach. Any release of this code must be done behind a feature flag.
📝 Checklist