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

Add default HTTP User-Agent for specific system #2409

Merged
merged 2 commits into from Aug 8, 2018

Conversation

@zhongwuzw
Copy link
Member

zhongwuzw commented Jul 31, 2018

New Pull Request Checklist

  • I have read and understood the CONTRIBUTING guide

  • I have read the Documentation

  • I have searched for a similar pull request in the project and found none

  • I have updated this branch with the latest master to avoid conflicts (via merge from master or rebase)

  • I have added the required tests to prove the fix/feature I am adding

  • I have updated the documentation (if necessary)

  • I have run the tests and they pass

  • I have run the lint and it passes (pod lib lint)

This merge request fixes / reffers to the following issues: ...

Pull Request Description

Let's tell the server what we are. Server maybe forbidden us if he think it's not the valid request.

Don't needs to reinventing the wheel, borrow snippet from AFNetWorking.

Fixed #2408 .

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 31, 2018

Codecov Report

Merging #2409 into master will decrease coverage by 0.08%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2409      +/-   ##
=========================================
- Coverage   75.09%     75%   -0.09%     
=========================================
  Files          36      36              
  Lines        3895    3905      +10     
=========================================
+ Hits         2925    2929       +4     
- Misses        970     976       +6
Impacted Files Coverage Δ
SDWebImage/SDWebImageDownloader.m 78.09% <78.57%> (-0.45%) ⬇️
SDWebImage/SDWebImageDownloaderOperation.m 85.43% <0%> (-1.95%) ⬇️
SDWebImage/SDImageCache.m 63.84% <0%> (-0.42%) ⬇️
Tests/Tests/SDWebImageDownloaderTests.m 83.75% <0%> (+0.9%) ⬆️
SDWebImage/SDWebImageManager.m 75.22% <0%> (+1.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f18747...36cd96a. Read the comment docs.

@dreampiggy

This comment has been minimized.

Copy link
Contributor

dreampiggy commented Jul 31, 2018

User-Agent it's just a HTTP header.

For 4.x, use headersFilter instead.

For 5.x, use requestModifier instead.

We should not bind complicated business code to a common library. If the user desired to use something, we just provide a abstract protocol or way to let them provide the information.

@zhongwuzw

This comment has been minimized.

Copy link
Member Author

zhongwuzw commented Jul 31, 2018

@dreampiggy It has no any business code, it just add the device and system version. just like all common user agent. you can see UIWebView/WKWebView or Safari.

Like below, you can see how Apple uses the user-agent to identify the device.

image

@bpoplauschi

This comment has been minimized.

Copy link
Member

bpoplauschi commented Jul 31, 2018

@zhongwuzw I agree we need to set the User-Agent. I would also consider the comment from @dreampiggy and actually make use of our existing features to do so.
Just that we need this User-Agent configured properly for all our users. We can't expect them to do it. We only offer ways to alter this behavior, if they need that.

@zhongwuzw zhongwuzw mentioned this pull request Aug 1, 2018
3 of 8 tasks complete
@zhongwuzw

This comment has been minimized.

Copy link
Member Author

zhongwuzw commented Aug 2, 2018

@bpoplauschi the propose @dreampiggy suggested is used for custom for each request, so any opinion about this PR?

@bpoplauschi

This comment has been minimized.

Copy link
Member

bpoplauschi commented Aug 4, 2018

I've looked at this PR again. Guys, I think an API like requestModifier is too advanced for the users that simply use our UIImageView (or similar) categories. I see AFNetworking also sets the User-Agent in the same way, so I say we do the same.
This is not a case where some users want some custom behaviour. From my point of view, this is the User-Agent set for each request of all our users, so we need to set it properly.
The only feature I say we could add (but it's just nice to have) is another field in the DownloaderConfig that allows passing in the User-Agent.

@zhongwuzw

This comment has been minimized.

Copy link
Member Author

zhongwuzw commented Aug 6, 2018

@bpoplauschi Emm 🤔 , User-Agent is only a field of Headers, user can override these values by (void)setValue:(nullable NSString *)value forHTTPHeaderField:(nullable NSString *)field of SDWebImageDownloader or requestModifier, I think it's maybe enough. Would you think we must add the Boolean in DownloaderConfig?

@@ -94,11 +94,32 @@ - (nonnull instancetype)initWithSessionConfiguration:(nullable NSURLSessionConfi
_downloadQueue.maxConcurrentOperationCount = 6;
_downloadQueue.name = @"com.hackemist.SDWebImageDownloader";
_URLOperations = [NSMutableDictionary new];
SDHTTPHeadersMutableDictionary *headerDictionary = [SDHTTPHeadersMutableDictionary dictionary];
NSString *userAgent = nil;
#if SD_IOS

This comment has been minimized.

Copy link
@dreampiggy

dreampiggy Aug 6, 2018

Contributor

This seems does not handle tvOS, so it's better to use SD_UIKIT instead.

This comment has been minimized.

Copy link
@zhongwuzw

zhongwuzw Aug 6, 2018

Author Member

Nice catch.

@dreampiggy

This comment has been minimized.

Copy link
Contributor

dreampiggy commented Aug 6, 2018

@zhongwuzw Don't need to add one extra config in SDWebImageDownloadConfig or somewhere. It shuold be one of built-in HTTP headers like Accept-Content-Type. User can use current APIs like setValue:forHTTPHeader: or headersFilter to change or override it. It's not suitable to add any property just for User-Agent this specify headers.

@bpoplauschi

This comment has been minimized.

Copy link
Member

bpoplauschi commented Aug 6, 2018

You are right guys, I forgot about setValue:forHTTPHeader:. That is sufficient for the users to customize. So we just need to calculate the User-Agent for the default case, which is most of the users (that will never deal with that). Basically this PR + the minor change request by @dreampiggy

@bpoplauschi bpoplauschi added this to the 4.4.3 milestone Aug 8, 2018
@bpoplauschi

This comment has been minimized.

Copy link
Member

bpoplauschi commented Aug 8, 2018

I see no reason to keep this un-merged. Good job @zhongwuzw

@bpoplauschi bpoplauschi merged commit 205e376 into SDWebImage:master Aug 8, 2018
3 checks passed
3 checks passed
codecov/patch 78.57% of diff hit (target 75.09%)
Details
codecov/project Absolute coverage decreased by -0.08% but relative coverage increased by +3.47% compared to 8f18747
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zhongwuzw zhongwuzw deleted the zhongwuzw:add-default-user-agent branch Aug 8, 2018
@bpoplauschi bpoplauschi mentioned this pull request Sep 7, 2018
52 of 52 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.