Add support for providing additional info to network image node delegate #775

Adlai-Holler · 2018-01-30T17:42:18Z

PINRemoteImage recently gained support for retrieving the raw network response from image downloads.

In order to pipe this data to the application layer, network image nodes need to retrieve it and pass it through.

This diff is minor breaking API (see changelog) but since the didLoadImage:info: method was only added recently #639 and since the migration pathway is trivial, I think we can release it in the next minor version.

Introduces class ASNetworkImageLoadInfo which includes source type, URL, download identifier, and a new id userInfo field.

Basic image downloader sends nil.
PIN image downloader sends PINRemoteImageManagerResult.

The URL is useful as well since, given that we unlock before calling out to the delegate, the image node URL could change before the user reads it in didLoadImage:.

…the ASNetworkImageNodeDelegate

Adlai-Holler · 2018-01-30T17:43:03Z

Source/ASNetworkImageNode.mm

-      UIImage *result = [[_cache synchronouslyFetchedCachedImageWithURL:_URL] asdk_image];
+    NSURL *url = _URL;
+    if (_imageLoaded == NO && url && _downloadIdentifier == nil) {
+      UIImage *result = [[_cache synchronouslyFetchedCachedImageWithURL:url] asdk_image];


We read _URL into a local variable so that it can be preserved after we unlock to call out to the delegate.

Adlai-Holler · 2018-01-30T17:43:50Z

Source/ASNetworkImageNode.mm

@@ -669,8 +670,7 @@ - (void)_lazilyLoadImageIfNecessary

        if (_delegateFlags.delegateDidLoadImageWithInfo) {
          ASDN::MutexUnlocker u(__instanceLock__);
-          ASNetworkImageNodeDidLoadInfo info = {};
-          info.imageSource = ASNetworkImageSourceFileURL;
+          auto info = [[ASNetworkImageLoadInfo alloc] initWithURL:URL sourceType:ASNetworkImageSourceFileURL downloadIdentifier:nil userInfo:nil];


Since we already checked above that URL and _URL are still equal, while we were locked, I standardized all these references to use the local variable instead of the ivar, since I had no choice but to use the local variable here (post-unlock).

maicki

LGTM

maicki · 2018-01-30T21:39:15Z

Source/ASNetworkImageLoadInfo.h

+@interface ASNetworkImageLoadInfo : NSObject <NSCopying>
+
+/// The type of source from which the image was loaded.
+@property (readonly) ASNetworkImageSourceType sourceType;


Are these all atomic on purpose?

Yep! I'm trying to figure out if there's a way to document a readonly property as atomic but get a synthesized getter that behaves like nonatomic (since we don't need a lock/barrier.)

For the time being, I figure simplicity and correct documentation are key. We won't spend much time in the spinlock/unfair-lock that Obj-C uses for these.

maicki · 2018-01-30T21:44:29Z

Source/Details/ASBasicImageDownloader.h

 *
 * This is a very basic image downloader. It does not support caching, progressive downloading and likely
 * isn't something you should use in production. If you'd like something production ready, see @c ASPINRemoteImageDownloader
 *
 * @note It is strongly recommended you include PINRemoteImage and use @c ASPINRemoteImageDownloader instead.
 */
-+ (instancetype)sharedImageDownloader;
+ (ASBasicImageDownloader *)sharedImageDownloader;


We could also change it to:

@Property (class, readonly, strong) ASBasicImageDownloader * sharedImageDownloader;

Would improve code completion.

Great call, doing it

Adlai-Holler · 2018-01-30T22:50:31Z

Our CI is backlogged to hell right now and this diff needs to be tested against Pinterest.

I ran the CI script (build.sh all) on my machine and it passed, so I'm merging.

…ate (#775) * Add support for piping arbitrary user info from ASImageDownloader to the ASNetworkImageNodeDelegate * s/source/sourceType * Fix stuff and take Michael's advice

ghost · 2018-01-30T23:57:28Z

	4 Warnings
⚠️	Please ensure license is correct for ASNetworkImageLoadInfo.h: `// // ASNetworkImageLoadInfo.h // Texture // // Copyright (c) 2018-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for ASNetworkImageLoadInfo.m: `// // ASNetworkImageLoadInfo.m // Texture // // Copyright (c) 2018-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for ASNetworkImageLoadInfo+Private.h: `// // ASNetworkImageLoadInfo+Private.h // Texture // // Copyright (c) 2018-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for PhotoCellNode.m: // // PhotoCellNode.m // Texture // // Copyright (c) 2014-present, Facebook, Inc. All rights reserved. // This source code is licensed under the BSD-style license found in the // LICENSE file in the /ASDK-Licenses directory of this source tree. An additional // grant of patent rights can be found in the PATENTS file in the same directory. // // Modifications to this file made after 4/13/2017 are: Copyright (c) through the present, // Pinterest, Inc. Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //

Generated by 🚫 Danger

nguyenhuy

LGTM!

nguyenhuy · 2018-01-31T15:55:24Z

Source/ASNetworkImageNode.mm

@@ -718,6 +718,9 @@ - (void)_lazilyLoadImageIfNecessary
          strongSelf->_downloadIdentifier = nil;
          strongSelf->_cacheUUID = nil;

+          // TODO: Why dispatch to main here?
+          // The docs say the image node delegate methods
+          // are called from background.


Context here. Per the API contract, I think it's ok to not dispatch. However, I'm not sure if it won't cause any client issues, since we have always called the delegate methods on main, and people may assume that (and many don't pay attention to the docs).

Also, from an API design point of view, delegate methods are usually called on main. We may want to do that for consistency and ease of use (i.e people don't need to worry about thread safety)?

Thanks for the context. Agree with the above, we should probably update the docs instead of updating the code.

nguyenhuy · 2018-01-31T16:12:10Z

Also, please update the file licenses when you have time.

…ate (TextureGroup#775) * Add support for piping arbitrary user info from ASImageDownloader to the ASNetworkImageNodeDelegate * s/source/sourceType * Fix stuff and take Michael's advice

Adlai Holler added 2 commits January 30, 2018 09:33

Add support for piping arbitrary user info from ASImageDownloader to …

f29e6a4

…the ASNetworkImageNodeDelegate

s/source/sourceType

29f5ebd

Adlai-Holler added Image & Video API Change labels Jan 30, 2018

Adlai-Holler commented Jan 30, 2018

View reviewed changes

Adlai-Holler requested review from maicki, garrettmoon and nguyenhuy January 30, 2018 17:44

Adlai-Holler mentioned this pull request Jan 30, 2018

Improve ASNetworkImageNode delegate callout behavior #778

Merged

Merge branch 'master' into AHProvideNetworkResponse

fce48a2

maicki approved these changes Jan 30, 2018

View reviewed changes

Fix stuff and take Michael's advice

be57d71

Adlai-Holler merged commit fef965f into master Jan 30, 2018

Adlai-Holler deleted the AHProvideNetworkResponse branch January 30, 2018 22:50

nguyenhuy reviewed Jan 31, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for providing additional info to network image node delegate #775

Add support for providing additional info to network image node delegate #775

Adlai-Holler commented Jan 30, 2018 •

edited

Loading

Adlai-Holler Jan 30, 2018

Adlai-Holler Jan 30, 2018

maicki left a comment

maicki Jan 30, 2018

Adlai-Holler Jan 30, 2018

maicki Jan 30, 2018

Adlai-Holler Jan 30, 2018

Adlai-Holler commented Jan 30, 2018

ghost commented Jan 30, 2018

nguyenhuy left a comment

nguyenhuy Jan 31, 2018 •

edited

Loading

Adlai-Holler Jan 31, 2018

nguyenhuy commented Jan 31, 2018

Add support for providing additional info to network image node delegate #775

Add support for providing additional info to network image node delegate #775

Conversation

Adlai-Holler commented Jan 30, 2018 • edited Loading

Adlai-Holler Jan 30, 2018

Choose a reason for hiding this comment

Adlai-Holler Jan 30, 2018

Choose a reason for hiding this comment

maicki left a comment

Choose a reason for hiding this comment

maicki Jan 30, 2018

Choose a reason for hiding this comment

Adlai-Holler Jan 30, 2018

Choose a reason for hiding this comment

maicki Jan 30, 2018

Choose a reason for hiding this comment

Adlai-Holler Jan 30, 2018

Choose a reason for hiding this comment

Adlai-Holler commented Jan 30, 2018

ghost commented Jan 30, 2018

nguyenhuy left a comment

Choose a reason for hiding this comment

nguyenhuy Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Adlai-Holler Jan 31, 2018

Choose a reason for hiding this comment

nguyenhuy commented Jan 31, 2018

Adlai-Holler commented Jan 30, 2018 •

edited

Loading

nguyenhuy Jan 31, 2018 •

edited

Loading