I don't think test-only code should exist in this patch, especially considering they would all be removed in next patch.

We have done , so we are already able to access and other classes inside , right?

nit : Would we consider to use only instead?

nit : reorder those by alphabet order.

No need to do that, you can capture into the lambda which would allow you to use directly inside the lambda.

Why we load the thumbnail after calling ? And would you consider move into ? Because that seems a part of setting metadata.

Same here.

nit : we have done so we can use directly without having a scope name.

I wonder if we need to assert both and are not null, because they are not set in this function. Adding that assertion can ensure we always call in the expected situation.

capture then you are able to use .

why need ?

Don't mess the test-only code with the production code. If this method would only be complete when applying the change in next patch, then they should be merged together. Otherwise, you should consider using another way to separate them.

Would we modify ? If not, use .

Use following format in order to follow other checks in this function.

if (FAILED(hr)) {
 LOG("XXXX");
 return;
 }

nit : would you consider to separate those comments and put them just above the line where the operation is going to be executed?

nit : to reduce redundant scope description, because this type has been used mutiple times.

The purpose of embedding the test-only stuff is to demonstrate that the needs the raw binary data of an image file, including its file header.

The test-only part in this patch could definitely be removed in this patch once the usage of is illustrated explicitly.

yeah, good catch.

Sure.

1. I guess one of the reason for having is to set . Thumbnail is not a property within so it shouldn't be there.
2. will load the image asynchronously. The display interface will updated when upon image is loaded.

would be created/reset in . I'll add an assertion to make sure would only be called after is executed.

On the other hand, would be created from inside so it would always be non-null.

The intention of whole test stuff is to make this clear: The needs the raw binary data of an image file, including its file header.

I'll add a log if it fails.

I'll move from .cpp to .h

Because it would be done asynchrously, at the time this function run, is it possible that we've closed the ?

If so, we could add a check to abort following steps if is already closed.

Or, is it possible to tell that we want to cancel the callback?

nit : I would prefer only use in the file, which can prevent from polluting other files which include this file. So that is why I proposed to use , not to use namespace directly.

it's unclear to me through code inspection that these two calls are threadsafe. but, just want to call out that these may end up being called on another thread and want to confirm that you thought about it.

same concern here-- it might be possible to addref/release in multiple threads. is this refcounting threadsafe?

The only function I found is [1]. Another way to do so is to drop/reset since would be re-initialized every time is called.

However, I am not sure if it's safe to call and (or ) at the same time. Those APIs are poorly documented. I am not sure if they are thread-safe. Also, it's hard to test them manually, to see if they are thread-safe, since I cannot find a place to insert a delay when the data is being written or flushed.

I guess it's fine to keep writing data even is closed. Chrome does the same way, without canceling the writing task when the whole class is deconstructed [2].

If the image would be cached, then it might be reused if the first image upon is the same as the cached one.

[1] https://docs.microsoft.com/en-us/uwp/api/windows.storage.streams.idatawriter.flushasync?view=winrt-19041#Windows_Storage_Streams_IDataWriter_FlushAsync
[2] https://source.chromium.org/chromium/chromium/src/+/master:components/system_media_controls/win/system_media_controls_win.cc;l=53;drc=b3af28c015f96bbd3c17d39b16b4e5778510578e?originalUrl=https:%2F%2Fcs.chromium.org%2F

This is the only place that can be called, so it's safe.

For , I am not really sure. The [2] inherits from , which is safe to call from user interface threads [3]. It doesn't state it can be called from different threads at the same time. However, chrome implements this in the same way [1] so I guess it's ok to follow.

[1] https://docs.microsoft.com/en-us/previous-versions/windows/desktop/mediatransport/isystemmediatransportcontrolsdisplayupdater
[2] https://docs.microsoft.com/en-us/windows/win32/api/inspectable/nn-inspectable-iinspectable?redirectedfrom=MSDN#remarks
[3] https://source.chromium.org/chromium/chromium/src/+/master:components/system_media_controls/win/system_media_controls_win.cc;l=257,302?originalUrl=https:%2F%2Fcs.chromium.org%2F

I'll see if I can make it .

Ah, the image won't need to be cached. We need to disconnect the image fetching process if we have one.

If chrome's implementation is right, I guess it's ok to call in .