Skip to content
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

Pulls out speed monitoring and adds support for using cell vs. wifi if speed unavailable. #403

Merged
merged 4 commits into from
Sep 1, 2017

Conversation

garrettmoon
Copy link
Collaborator

No description provided.

@garrettmoon garrettmoon changed the title [WIP] Low res images [WIP] Pulls out speed monitoring and adds support for using cell vs. wifi if speed unavailable. Aug 31, 2017
@garrettmoon garrettmoon changed the title [WIP] Pulls out speed monitoring and adds support for using cell vs. wifi if speed unavailable. Pulls out speed monitoring and adds support for using cell vs. wifi if speed unavailable. Aug 31, 2017
@pinterest pinterest deleted a comment Sep 1, 2017
@pinterest pinterest deleted a comment Sep 1, 2017
@pinterest pinterest deleted a comment Sep 1, 2017
@pinterest pinterest deleted a comment Sep 1, 2017
Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the quick question below, LGTM!


[strongSelf lock];
BOOL shouldUpgradeLowQualityImages = [strongSelf shouldUpgradeLowQualityImages];
[strongSelf unlock];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may well be a micro-optimization, but to avoid grabbing the lock multiple times, how about taking this chance to get high and low quality thresholds and pass them to -appropriateImageIdxForURLsGivenHistoricalNetworkConditions? Then that method can be static because it no longer needs access to self.

@ghost
Copy link

ghost commented Sep 1, 2017

1 Warning
⚠️ This is a big PR, please consider splitting it up to ease code review.

Generated by 🚫 Danger

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants