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

Update ContentDataSource.java #3428

Closed
wants to merge 1 commit into from
Closed

Update ContentDataSource.java #3428

wants to merge 1 commit into from

Conversation

axet
Copy link

@axet axet commented Nov 5, 2017

Checking File size makes more sense then asking for InputStream cached bytes.

#3426 explanation

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@@ -89,7 +89,7 @@ public long open(DataSpec dataSpec) throws ContentDataSourceException {
long assetFileDescriptorLength = assetFileDescriptor.getLength();
if (assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH) {
// The asset must extend to the end of the file.
bytesRemaining = inputStream.available();
bytesRemaining = ((FileInputStream) inputStream).getChannel().size();
if (bytesRemaining == 0) {
// FileInputStream.available() returns 0 if the remaining length cannot be determined,
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment doesn't seem to make sense given then change being made? Does the if block itself still make sense?

Copy link
Author

Choose a reason for hiding this comment

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

bytesRemaining = inputStream.available()

  • for the momen when you open Pipe this call can return 0.

bytesRemaining = ((FileInputStream) inputStream).getChannel().size();

  • FileDescription for correct file in file system, this call will return correct file size.

Again. I do not understand, why you asking this. Propbably need more testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the previous code we actually called inputStream.available() and then did something conditionally on the value being 0. We're not making an inputStream.available() call any more, so the comment doesn't make sense. Can size() return 0? If not why is this if block still here? If so the comment needs changing.

@@ -89,7 +89,7 @@ public long open(DataSpec dataSpec) throws ContentDataSourceException {
long assetFileDescriptorLength = assetFileDescriptor.getLength();
if (assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH) {
// The asset must extend to the end of the file.
bytesRemaining = inputStream.available();
bytesRemaining = ((FileInputStream) inputStream).getChannel().size();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's possible for multiple assets to be in a single file. In which case is size() the size of just this one, or of all of them? I think you might want to subtract position() of the FileChannel to get correct behavior?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure what do you mean, FileChannel one per file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the uri for which the source is being opened necessarily corresponds to the whole file though. Else this code wouldn't need to be using assetFileDescriptor.getStartOffset as above? If the uri only corresponds to a part of the file, that needs to be accounted for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if the uri does correspond to the whole file, you'd need to subtract skipped if it's non-zero.

Copy link
Author

Choose a reason for hiding this comment

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

Then correct line is:

bytesRemaining = ((FileInputStream) inputStream).getChannel().size() - assetFileDescriptor.getStartOffset

@ojw28
Copy link
Contributor

ojw28 commented Nov 5, 2017

  • We wont be able to merge this without a signed CLA.
  • Pull requests need to go to the dev-v2 branch, not release-v2. Please address the comments and make a new request to that branch.

@axet
Copy link
Author

axet commented Nov 5, 2017

  1. Will wont be able to merge this without a signed CLA.

less then 5 lines of code need to sign anything? Eclipse treat small bug fixes wihtout any paper work. I do not wana bother to sing anything.

  1. Ok, I close this one, but keep reffered issue open.

@axet axet closed this Nov 5, 2017
@ojw28
Copy link
Contributor

ojw28 commented Nov 5, 2017

Please do not send a second pull request if you don't intend to sign the CLA. We wont be able to accept it. Thanks.

@axet
Copy link
Author

axet commented Nov 5, 2017

@ojw28 I didnt know you trying to use "documents" to protect you from the world. I would'nt pull again.

Anyway you can't protect yourself using this technique sign - and enter. I can prove I made this code having it before the code come to the repository, and since it get there without my sign it be equals staling code. So, sign or no sign - always possible to make you/google guilty and violating copyright slavery... Just make it simple, opensource and fun. BTW i'm not going to sue anyway. Just show you how stupid "CLA" is.

@google google locked and limited conversation to collaborators Mar 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants