Feature/Readium sdk for lcp

[drminside]

Dear Readium SDK people,

I send a pull request for updating Readium SDK to support LCP implementation.

We have tried to minimize modification in the Readium SDK for the LCP implementation as possible.
But at least following pull request is required to be updated, we think.

Here are the points why each files are modified.

1. Platform/Android/jni/epub3.cpp
   Without the line 437, 'return nullptr' sentence, application could be crashed when it has invalid argument for LCP EPUB.
2. Platform/Apple/RDServices/Main/RDContainer.mm
   We added initialization (line 90) for the LCP and exception handling for button 'cancel' of the pass-phrase popup for iOS launcher.
   Update for the OSX is in the file in the Launcher project, and Android treats it in the JNI area, which is in the Readium DRM project.
3. ePub3/ThirdParty/libzip/zip_fseek.c
   Without the modification _zip_fseek_bytes() function, application would be crashed during seeking uncompressed media resource. It might be a bug of the libzip.
4. ePub3/ePub/container.cpp
   ContentModule for the LCP does 'future.get()' internally. So without from line 124 to 137, application doesn't work in LCP EPUB.
5. ePub3/ePub/content_module.h
   We added a pure virtual function, GetModuleName() to detect proper Content Module in the derived class.
6. ePub3/ePub/content_module_manager.cpp
   We don't know the reason why 'result.then' (at line 95 and 107) makes timeless waiting for LCP content module. But we have checked there is no problem to handle plain and encrypted resources even though we detoured these sentences.

And we added some error handling for the case in which EPUB is encrypted but not LCP.

1. ePub3/ePub/credential_request.cpp/h
   We have added some additional member functions to handle to exchange authentication information between Core and pass-phrase popup in Launcher side.
2. ePub3/ePub/encryption.cpp/h
   We have added some functions to handle additional information for the compressed and encrypted resource in the encryption.xml

[nleme]

A quick comment: in content_module_manager.cpp, around lines 116 - 120, you are leaving some code commented out. If that code is supposed to be removed for good, then please erase those lines instead. However, if instead you think that those lines of code may somehow be restored later, then please add a comment explaining that.

[nleme]

Another little comment: in encryption.cpp, around lines 55 - 78 (the new code you added) you are using Objective-C style curly brackets (also called K & R style), like so:

if (condition) {
      statement;
}

However, throughout the codebase of Readium SDK, we have stuck to the following style instead:

if (condition)
{
     statement;
}

So, to keep consistency throughout the codebase, would you mind switching to that style? Thanks.

[nleme]

In the same line as the last comment above, I noticed that in credential_request.h, around lines 147 - 165 (the definition of the new methods), the following style has been used:

return_type
function_name(arg_type arg1);

However, throughout the current codebase of the Readium SDK, we have used instead the following style:

return_type function_name(arg_type arg1);

With the return type and the function name in the same line. Would you mind changing those member function definitions to that? Thanks.

[nleme]

Just a little typo: on RDContainer.mm, around line 115, you wrote "NSLog(@"unknown exceprion");", whereas it should be "exception". By the way, since you are going to be rewriting that, why not add some extra information, like "Unknown exception during initialization of RDContainer.mm" so that somebody looking at the logs has a better idea of where the exception happened. It also makes it easier when you are searching through a log file.

[nleme]

A final question. I saw that you added the line:

#include <lcp/initialization.h>

to the file RDContainer.mm. However, where is that file added in the commits? I mean, if that file is not present, then the compilation will fail with a "header not found" error.

[nleme]

Other than the observations I made above, the change looks good. It would be nice to see the style changes that I talked about implemented, but most importantly, we need an answer to my last observation above before we can merge this into develop.

[rkwright]

Thanks for your thorough review of this, Nelson. And many thanks to DRMInside for making and submitting the changes.

[drminside]

Thanks for the comments, Nelson. All syntax related ones could be modified with easy. We will send new pull request with updated version soon.

However, regarding 'Platform/Apple/RDServices/Main/RDContainer.mm',
The original RDContainer.mm is used only for the iOS Launcher.
And modified version is used for the LCP integrated iOS Launcher.

So I suggest that it should be detached from the Readium SDK and attached into the iOS Launcher project. Then we could manage separated versions of the RDContainer.mm for LCP integrated iOS Launcher and plain iOS Launcher, respectively, which could make the Readium SDK drm agnostic.

We will present this suggestion with new 2 pull requests with updated Readium SDK and iOS Launcher.

[drminside]

We have updated the pull request project.

1. All syntax related ones are updated as Nelson recommended.
2. RDContainer.mm/h files are detached in this project and move to Launcher-iOS-LCP branch.
3. Unchanged original RDContainer.mm/h files are moved into Launcher-iOS project, which has been already sent as another pull request ePub cover #66.

For the integrity of 2 projects, I recommend that 2 pull requests should be disposed together.

[mickael-menu-mantano]

Hello,

Thank you for your contribution!

The RDServices were initially part of SDKLauncher-iOS, but were moved to readium-sdk because in real-world usage, readers won't add the launchers to their dependencies, but only readium-sdk. Thus RDServices is useful for iOS reader using readium-sdk. So I think you should revert this particular commit.

I see you only use the lcp/initialization.h dependency to initialize LCP during the opening of the container. If your LCP implementation uses ContentFilter, it might make sense to initialize your LCP lib from the new delegate method [RDContainerDelegate containerRegisterContentFilters:], in the launcher.

https://github.com/readium/readium-sdk/blob/develop/Platform/Apple/RDServices/Main/RDContainer.h

@nleme: Is there any documentation for the coding standard used in Readium?

[drminside]

Thank for your comment.

Actually, our LCP initialization needs to register ContentModule instead of ContentFilter.
And ContentModule takes a role for the ContentFilter registration internally, which is in LCP implementation part so it does not affect the Readium-SDK at all.

Anyway we'll revert RDContainer.mm/h as unchanged ones so that it does not contains LCP dependent code.

And I'll see the mechanism of the delegate method if anyone provide any document on it.
It could be helpful for us to find new place to add lcp::initialization for new ContentModule registration.

[bluefirepatrick]

@drminside As you are undoubtedly aware of, this is the pull request we are talking about
#204

I am not sure if this will be suitable for your purposes but I thought it was worth a discussion as the Launcher may not be the best place for critical registration code. @nleme is going to review this but we would be happy to schedule a call to discuss this. Overall, it is fine to edit the Launcher in any way you wish and we'll gladly mege the pull request. I just wanted to make you aware of #204 to see if it might be a better solution.

[drminside]

@mmenu-mantano and @bluefirepatrick

According to your suggestions,
We updated the RDContainer.mm/h, where following codes are added.

1. (void)containerRegisterContentModules:(RDContainer *)container
   https://github.com/drminside/readium-sdk-1/blob/feature/sdkForLcp/Platform/Apple/RDServices/Main/RDContainer.h
2. [RDContainerDelegate containerRegisterContentModules:]
   https://github.com/drminside/readium-sdk-1/blob/feature/sdkForLcp/Platform/Apple/RDServices/Main/RDContainer.mm

The reason why we didn't use [RDContainerDelegate containerRegisterContentFilters] is that we do care only ContentModule registration instead of Filter.

Each ContentModule class has a RegistrationFilters member function in it.

Please refer the CPF introduction document
https://docs.google.com/document/d/103t9HPCmGnMzFpJ3yDQXrAlJeWDQlFtJ1lLQ6UlCt10/edit

[danielweck]

Hello, and thank you for the contribution.
Where is the OpenContainerForContentModule() function implemented?
Also, could you please target the develop branch instead of master?
Regards, Daniel

[drminside]

@danielweck

OpenContainerForContentModule() is also in the container.cpp, which is untouched in the Readium SDK.
https://github.com/drminside/readium-sdk-1/blob/feature/sdkForLcp/ePub3/ePub/container.cpp
or
https://github.com/readium/readium-sdk/blob/develop/ePub3/ePub/container.cpp

And we have sent a pull request again into the develop, which is #216

[danielweck]

Superseded: #216