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

Read assets out of APK on Android #4742

Merged
merged 3 commits into from Mar 5, 2018

Conversation

Projects
None yet
5 participants
@szakarias
Copy link
Contributor

commented Mar 4, 2018

No description provided.

@szakarias szakarias requested review from jakobr-google and mravn-google Mar 4, 2018

@googlebot googlebot added the cla: yes label Mar 4, 2018

@mravn-google
Copy link
Contributor

left a comment

LGTM

#include "lib/fxl/files/unique_fd.h"
#include "lib/fxl/macros.h"
#include "lib/fxl/memory/ref_counted.h"

namespace blink {

class DirectoryAssetBundle
: public fxl::RefCountedThreadSafe<DirectoryAssetBundle> {
: public blink::AssetProvider {

This comment has been minimized.

Copy link
@mravn-google

mravn-google Mar 4, 2018

Contributor

Can't we leave out blink:: here?

@@ -312,7 +312,7 @@ void Engine::RunBundle(const std::string& bundle_path,
return;
}
std::vector<uint8_t> snapshot;
if (!GetAssetAsBuffer(blink::kSnapshotAssetKey, &snapshot))
if (!GetAssetAsBuffer(blink::kSnapshotAssetKey, &snapshot))

This comment has been minimized.

Copy link
@mravn-google

mravn-google Mar 4, 2018

Contributor

Indentation seems off here.

@@ -64,6 +65,8 @@ class PlatformView : public std::enable_shared_from_this<PlatformView> {
virtual void HandlePlatformMessage(
fxl::RefPtr<blink::PlatformMessage> message);

virtual fxl::RefPtr<blink::AssetProvider> GetAssetProvider();

This comment has been minimized.

Copy link
@mravn-google

mravn-google Mar 4, 2018

Contributor

Remove extra space after virtual

std::vector<uint8_t>* data) {
std::stringstream ss;
ss << directory_.c_str() << "/" << asset_name;
AAsset* asset = AAssetManager_open(assetManager_, ss.str().c_str(), AASSET_MODE_RANDOM);

This comment has been minimized.

Copy link
@mravn-google

mravn-google Mar 4, 2018

Contributor

I don't think we need random access to the asset bytes. We only want to fill a buffer, right?
So I'd expect this to be AASSET_MODE_BUFFER (though I haven't tried it).

https://developer.android.com/ndk/reference/group___asset.html

}

data->resize(AAsset_getLength(asset));
std::copy(buffer, buffer + data->size() , data->begin());

This comment has been minimized.

Copy link
@mravn-google

mravn-google Mar 4, 2018

Contributor

Remove space before ,

while ((count = is.read(buffer, 0, BUFFER_SIZE)) != -1) {
os.write(buffer, 0, count);

OutputStream os = null;

This comment has been minimized.

Copy link
@mravn-google

This comment has been minimized.

Copy link
@jakobr-google

jakobr-google Mar 5, 2018

Contributor

The original code looks like it did the right thing (try-with-resources). Was there a reason for changing it?

szakarias added some commits Mar 5, 2018

@szakarias szakarias force-pushed the szakarias:read-apk-assets branch from a0802de to b22f07b Mar 5, 2018

@szakarias szakarias merged commit a00f8e8 into flutter:master Mar 5, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chinmaygarde
Copy link
Member

left a comment

lgtm. You certainly don't have to address the nits. Just a few observations. Was away on holiday for a few days and lost track of reviews :)


APKAssetProvider::APKAssetProvider(JNIEnv* env, jobject jassetManager, std::string directory)
: directory_(std::move(directory)) {
assetManager_ = AAssetManager_fromJava(env, jassetManager);

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Mar 7, 2018

Member

Maybe hold a VM reference to the jobject here and release it in the destructor so that we can be sure that the object doesn't become invalid while this native asset provider is in use.

uint8_t* buffer = (uint8_t*)AAsset_getBuffer(asset);
if (!buffer) {
FXL_LOG(ERROR) << "Got null trying to acquire buffer for asset:" << asset;
return false;

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Mar 7, 2018

Member

Missing AAset_close in case of this error. The idiom is to use fxl::UniqueObject for such calls but I can certainly understand if you don't want to write that for this one-of case and just add the close call manually here.

return false;
}

uint8_t* buffer = (uint8_t*)AAsset_getBuffer(asset);

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Mar 7, 2018

Member

reinterpret_cast as specified in the style guide.

#include "lib/fxl/files/unique_fd.h"
#include "lib/fxl/macros.h"
#include "lib/fxl/memory/ref_counted.h"

namespace blink {

class DirectoryAssetBundle
: public fxl::RefCountedThreadSafe<DirectoryAssetBundle> {
: public AssetProvider {

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Mar 7, 2018

Member

Please run clang-format over this patch.

reed-at-google added a commit to reed-at-google/engine that referenced this pull request Mar 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.