Add downloadSizeCallback before storing offline#3049
Add downloadSizeCallback before storing offline#3049michellezhuogg merged 10 commits intoshaka-project:masterfrom
Conversation
|
@joeyparrish Do you think you could review it out for v3.1? |
lib/offline/storage.js
Outdated
| } catch (e) { | ||
| throw new shaka.util.Error( | ||
| shaka.util.Error.Severity.CRITICAL, | ||
| shaka.util.Error.Category.STORAGE, | ||
| shaka.util.Error.Code.OPERATION_ABORTED); |
There was a problem hiding this comment.
This is what happens if the callback throws, correct? I think in that case, we should at least log something, and maybe even use a different error code to make it clear what happened.
There was a problem hiding this comment.
There are two causes here, the callback is asynchronous and it is I hope it returns a boolean, if it is false we should throw a "storage limit reached" error, in the case that an error is thrown (throw) I think the aborted operation is reasonable, what do you think?
There was a problem hiding this comment.
I agree with Joey, there should be at least a log, preferably a different error. Returning false already aborts the operation since we throw the STORAGE_LIMIT_REACHED error. This is more likely to happen if the app typed something wrong and we got a TypeError or something.
There was a problem hiding this comment.
Ok, I will add a new error called DOWNLOAD_SIZE_CALLBACK_ERROR
There was a problem hiding this comment.
Done. Can you review again?
|
@TheModMaker Could you review this PR? |
lib/offline/storage.js
Outdated
| } catch (e) { | ||
| throw new shaka.util.Error( | ||
| shaka.util.Error.Severity.CRITICAL, | ||
| shaka.util.Error.Category.STORAGE, | ||
| shaka.util.Error.Code.OPERATION_ABORTED); |
There was a problem hiding this comment.
I agree with Joey, there should be at least a log, preferably a different error. Returning false already aborts the operation since we throw the STORAGE_LIMIT_REACHED error. This is more likely to happen if the app typed something wrong and we got a TypeError or something.
michellezhuogg
left a comment
michellezhuogg
left a comment
There was a problem hiding this comment.
Are there any tests that need to be updated or added?
|
From my side I had no intention |
|
@michellezhuogg Is there something pending on your side? I would like this PR to be merged. |
github-actions
bot
added
the
status: archived
4 participants
Closes: #2900