[Android WebAPK] Make webapk_installer.cc return proto as base64 string

[Android WebAPK] Make webapk_installer.cc return proto as base64 string

This CL makes webapk_installer.cc return the proto as a base64 encoded string
instead of as a byte array. This change is because SharedPreferences does not
support array extras. We can't set the serialized proto as an extra on
com.google.android.gms.gcm.Task because the size of the extras on Tasks is
capped at 10Kb.

BUG=713655

[pkotwicz, 2017-06-19 01:40:55]

pkotwicz@chromium.org changed reviewers:
+ hanxi@chromium.org

[pkotwicz, 2017-06-19 01:40:56]

Xi, can you please take a look?

[Xi Han, 2017-06-19 13:48:58]

Most of the CL looks pretty good, just one question about the encoding/decoding.

https://codereview.chromium.org/2943913002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2943913002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_installer.cc:486:
base::Base64Decode(*serialized_proto, &decoded_proto);
A dumb question: since we eventually send the "decoded" proto, why bother
encoding them in the background task?

[pkotwicz, 2017-06-19 14:54:06]

Xi, can you please take a look?

https://codereview.chromium.org/2943913002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2943913002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_installer.cc:486:
base::Base64Decode(*serialized_proto, &decoded_proto);
Because it is bad practice to store binary data into Java Strings.
ConvertUTF16ToJavaString() does not properly convert binary strings to Java
Strings. (I did this initially)

Maybe there is a java.nio.charset.Charset which works well. I am not very
optimistic because the os.Bundle for Android Tasks is marshalled and
unmarshalled by Android and and the marshalling and unmarshalling likely ignores
the Charset.

[Xi Han, 2017-06-19 17:14:14]

lgtm with nits.

https://codereview.chromium.org/2943913002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapk/webapk_install_service.h (right):

https://codereview.chromium.org/2943913002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_install_service.h:12: #include <vector>
Nit: you probably don't need this.

https://codereview.chromium.org/2943913002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2943913002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_installer.cc:486:
base::Base64Decode(*serialized_proto, &decoded_proto);
On 2017/06/19 14:54:06, pkotwicz wrote:
> Because it is bad practice to store binary data into Java Strings.
> ConvertUTF16ToJavaString() does not properly convert binary strings to Java
> Strings. (I did this initially)
> 
> Maybe there is a java.nio.charset.Charset which works well. I am not very
> optimistic because the os.Bundle for Android Tasks is marshalled and
> unmarshalled by Android and and the marshalling and unmarshalling likely
ignores
> the Charset.

Ah, I see, it is for the code path that WebApkUpdateManager calls BuildProto().
Thanks for the explanation!

https://codereview.chromium.org/2943913002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapk/webapk_update_manager.cc (right):

https://codereview.chromium.org/2943913002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_update_manager.cc:40:
base::android::ScopedJavaLocalRef<jstring> j_proto =
Nit: rename j_proto to java_proto, since we already use java_callback.

[pkotwicz, 2017-06-20 00:35:32]

Description was changed from

==========
[Android WebAPK] Make webapk_installer.cc return proto as base64 string

This CL makes webapk_installer.cc return the proto as a base64 encoded string
instead of as a byte array. This change is because
com.google.android.gms.gcm.Task does not support array extras.

BUG=713655
==========

to

==========
[Android WebAPK] Make webapk_installer.cc return proto as base64 string

This CL makes webapk_installer.cc return the proto as a base64 encoded string
instead of as a byte array. This change is because SharedPreferences does not
support array extras. We can't set the serialized proto as an extra on
com.google.android.gms.gcm.Task because the size of the extras on Tasks is
capped at 10Kb.

BUG=713655
==========

[pkotwicz, 2017-06-20 00:35:32]

pkotwicz@chromium.org changed reviewers:
+ dominickn@chromium.org

[pkotwicz, 2017-06-20 00:36:05]

Dominick, can you please take a look?

[dominickn, 2017-06-20 01:07:21]

Where does the SharedPreference come into play? Is that a follow up CL?

https://codereview.chromium.org/2943913002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2943913002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:179: std::string
serialized_proto;
Base64Encode can be run in place, which will save a string allocation. Can you
do:

auto serialized_proto = base::MakeUnique<std::string>();
webapk->SerializeToString(&serialized_proto.get());
 base::Base64Encode(serialized_proto.get(), serialized_proto.get());

[pkotwicz, 2017-06-20 22:18:26]

Dominick, can you please take another look? The SharedPreference is introduced
in https://codereview.chromium.org/2948713002/ which is a follow up CL

https://codereview.chromium.org/2943913002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2943913002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:179: std::string
serialized_proto;
I can do this change. However, this does not decrease the number of string
allocations since base::Base64Encode() does a copy internally

[dominickn, 2017-06-21 04:13:49]

How big is this proto going to be? SharedPreferences are held entirely in memory
and are backed by an XML file. The proto has serialized images in it. I don't
know if it's a good idea to be storing that in SharedPreferences (particularly
if the images are large).

https://codereview.chromium.org/2943913002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2943913002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:179: std::string
serialized_proto;
On 2017/06/20 22:18:26, pkotwicz wrote:
> I can do this change. However, this does not decrease the number of string
> allocations since base::Base64Encode() does a copy internally

I meant that you'll save one of |serialized_proto| and |base64_serialized_proto|
from being allocated when they're default-constructed (we only use one instead
of both). I'm not overly fussed though.