Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Clean up FileSource #3033

Merged
merged 6 commits into from
Nov 16, 2015
Merged

[core] Clean up FileSource #3033

merged 6 commits into from
Nov 16, 2015

Conversation

jfirebaugh
Copy link
Contributor

👀 @tmpsantos @kkaefer @mikemorris

Clean up FileSource by taking advantage of the safe callback and cancellation semantics provided by RunLoop::invokeWithCallback.

Fixes #2827, eliminates the complex logic around Resource destruction, eliminates ResourceHolder, moves us toward #2909.

Also allows simplification of the node implementation of FileSource: we can use RunLoop to handle dispatch back to the node event loop. I removed JS-level cancellation via the optional "cancel" property; it was untested and unused. (Note that RunLoop::invokeWithCallback handles cancellation at the C++ level; i.e. not calling the callback if the request has already been cancelled.)

@jfirebaugh jfirebaugh force-pushed the cleanup-file-source branch 4 times, most recently from 958ce9c to 1f67471 Compare November 14, 2015 02:44
// because if the request was cancelled, then R might have been destroyed. L2 needs to check
// the flag because the request may have been cancelled after L2 was invoked but before it
// began executing.
auto after = [flag, current = RunLoop::current.get(), callback1 = std::move(callback)] (auto&&... results1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch.

@tmpsantos
Copy link
Contributor

Kudos @jfirebaugh, I really liked this.

@mikemorris
Copy link
Contributor

Aside from the following minor naming issue this is working great for me, and fixes the segfaults I was seeing previously in #2334.

../../src/mbgl/storage/default_file_source.cpp: In member function ‘void mbgl::DefaultFileSource::cancel(const mbgl::Resource&, mbgl::FileRequest*)’:
../../src/mbgl/storage/default_file_source.cpp:74:78: error: declaration of ‘request’ shadows a member of 'this' [-Werror=shadow]
 void DefaultFileSource::cancel(const Resource& resource, FileRequest* request) {
                                                                              ^

@miccolis Is removing the JavaScript cancel interface an issue at the implementation level?

@miccolis
Copy link
Contributor

@mikemorris removing cancel is fine.

@jfirebaugh jfirebaugh force-pushed the cleanup-file-source branch 5 times, most recently from 6d92558 to 6fb713d Compare November 16, 2015 20:23
@jfirebaugh jfirebaugh merged commit 43c6e2f into master Nov 16, 2015
@jfirebaugh jfirebaugh deleted the cleanup-file-source branch November 16, 2015 20:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants