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

Feature/content filter error architecture #239

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

nleme
Copy link
Contributor

@nleme nleme commented May 20, 2016

Creating this pull request to track the work to merge the new architecture for ContentFilter errors into develop.

This is a rather old feature branch, so the first thing I have to do is to merge develop into this feature branch, and solve any possible merge conflicts, so that this code is up to date.

nleme and others added 3 commits June 29, 2015 16:26
ContentFilters are one of the mechanisms that the Readium SDK
uses for extensibility. However, up to this point, there was not a
clear way to report an error inside a ContentFilter. For example,
if there is any error inside the FilterData() function, there would be
no clear way to report that back to the app. This change creates
a new architecture through which the code inside a given
ContentFilter will be able to report an error back to the app, and
the app will be able to do something about that (like displaying
the error to the user).
Recently, I implemented a change in the Readium SDK that allows
ContentFilters to report errors. That is important because the
ContentFilters are the basic mechanism for extending the Readium
SDK, and it is expected that people all over the world will start
writing their own ContentFilters. Given that, it is necessary that
there is a mechanism for reporting when there is an error in a given
ContentFilter. The basic mechanism in the Readium SDK has already
been implemented, but this change adds the extra glue code in the
JNI layer so that Android apps can receive those error messages.
@nleme
Copy link
Contributor Author

nleme commented May 20, 2016

Let me attach the conversation that was already going on about the changes in this feature branch:

Email from Daniel Weck:

I am a little concerned about the NoLicenseForDecryption type of ContentFilterError, which seems quite specific.
See whole code diff:
develop...feature/contentFilterErrorArchitecture
For example, the LCP ContentModule integration handles license-related DRM "bootstrapping" (e.g. certificate + signature checks, user passphrase input) early on, outside of the ContentFilter chain.
See:
develop...feature/content-module
readium/SDKLauncher-Android@develop...feature/lcp
readium/SDKLauncher-iOS@develop...feature/lcp
readium/SDKLauncher-OSX@develop...feature/lcp
https://github.com/readium/readium-lcp-client/tree/android

Email from me:

First, about the enumeration value "NoLicenseForDecryption". Would you prefer something more generic like "InternalError"?

Second, about the error processing in ContentModule. That's absolutely correct, but that should happen in parallel to the error processing in the ContentFilter. It doesn't matter if you have a ContentModule set up or not, or even if you have any DRM going or not, you can always have a chain of ContentFilter objects, for whatever reasons, and those objects when filtering bytes can have an error. The idea here is how to report when those errors happen.

We should continue the discussion here on the pull request, from now on.

@danielweck
Copy link
Member

Reminder: we need to review this in depth, comparative analysis with LCP's own error handling additions.
e.g. throw ePub3::ContentModuleException in
https://github.com/readium/readium-lcp-client/blob/develop/src/lcp-content-filter/LcpContentModule.cpp
See: https://github.com/readium/readium-sdk/blob/feature/content-module/ePub3/ePub/content_module_exception.h
defined in feature/content-module branch of readium-sdk

@danielweck
Copy link
Member

CC @clebeaupin

*/
static void HandleContentFilterError(const std::string &filterId, unsigned int errorCode, const std::string &message)
{
s_contentFilterErrorHandler(filterId, errorCode, message);
Copy link
Member

Choose a reason for hiding this comment

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

I think there should be NULL check here, just in case ResetContentFilterErrorHandler() was never invoked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants