Add WebView.saveWebArchive support to android_webview.

Add WebView.saveWebArchive support to android_webview.

BUG=146004
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157658

[mkosiba (inactive), 2012-09-19 12:21:39]

lgtm

https://chromiumcodereview.appspot.com/10946013/diff/1/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://chromiumcodereview.appspot.com/10946013/diff/1/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:166: //
TODO(jgreenwald): is this really possible? Is there some other default we should
nit: I don't think Jesse's going to be addressing this in the near future.

[benm (inactive), 2012-09-19 14:06:51]

Thanks Ted!

Just a set of nits and wanted to double check the change in aw_contents.cc.

Cheers,

http://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/c...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

http://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/c...
android_webview/java/src/org/chromium/android_webview/AwContents.java:114: */
Don't think we need these comments.

http://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/c...
android_webview/java/src/org/chromium/android_webview/AwContents.java:126:
return generateArchiveAutoNamePath(getOriginalUrl(), basename, ".mht");
.mht feels like it should be a const

http://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/c...
android_webview/java/src/org/chromium/android_webview/AwContents.java:136:
private void saveWebArchiveInternal(String path, final ValueCallback<String>
callback) {
Probably best to move this helper to the bottom and call out a new section

// ----------------------
// Helper methods
// ---------------------

http://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/c...
android_webview/java/src/org/chromium/android_webview/AwContents.java:153:
private static String generateArchiveAutoNamePath(
ditto move as a helper.

http://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/c...
android_webview/java/src/org/chromium/android_webview/AwContents.java:186: *
@see android.webkit.WebView#getOriginalUrl()
Don't need this comment.

http://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/c...
android_webview/java/src/org/chromium/android_webview/AwContents.java:193: }
else {
no need for an else block here.

http://codereview.chromium.org/10946013/diff/1/android_webview/native/aw_cont...
File android_webview/native/aw_contents.cc (right):

http://codereview.chromium.org/10946013/diff/1/android_webview/native/aw_cont...
android_webview/native/aw_contents.cc:102: ScopedJavaGlobalRef<jobject>*
j_message = new ScopedJavaGlobalRef<jobject>();
did you mean to change this here?

[Ted C, 2012-09-19 16:53:22]

Comments addressed.

https://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:114: */
On 2012/09/19 14:06:52, benm wrote:
> Don't think we need these comments.

Done.

https://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:126:
return generateArchiveAutoNamePath(getOriginalUrl(), basename, ".mht");
On 2012/09/19 14:06:52, benm wrote:
> .mht feels like it should be a const

Made it a constant and since generateArchiveAutoNamePath is private, I just
reference it directly without needing to pass it in.

https://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:136:
private void saveWebArchiveInternal(String path, final ValueCallback<String>
callback) {
On 2012/09/19 14:06:52, benm wrote:
> Probably best to move this helper to the bottom and call out a new section
> 
> // ----------------------
> // Helper methods
> // ---------------------

Done.

https://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:153:
private static String generateArchiveAutoNamePath(
On 2012/09/19 14:06:52, benm wrote:
> ditto move as a helper.

Done.

https://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:166: //
TODO(jgreenwald): is this really possible? Is there some other default we should
On 2012/09/19 12:21:39, Martin Kosiba wrote:
> nit: I don't think Jesse's going to be addressing this in the near future.

Done.

https://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:186: *
@see android.webkit.WebView#getOriginalUrl()
On 2012/09/19 14:06:52, benm wrote:
> Don't need this comment.

Done.

https://codereview.chromium.org/10946013/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:193: }
else {
On 2012/09/19 14:06:52, benm wrote:
> no need for an else block here.

Done.

https://codereview.chromium.org/10946013/diff/1/android_webview/native/aw_con...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/10946013/diff/1/android_webview/native/aw_con...
android_webview/native/aw_contents.cc:102: ScopedJavaGlobalRef<jobject>*
j_message = new ScopedJavaGlobalRef<jobject>();
On 2012/09/19 14:06:52, benm wrote:
> did you mean to change this here?

Yeah, when looking at refactoring the code below, I noticed that the
ScopedJavaLocalRef was there as a requirement for the constructor of the global
ref.  A few of us found it confusing so I thought I would be more explicit and
avoid creating it at all.  We already loose the local ref from message
automatically when we return from this method, so there was no need for the
scoped version.

[benm (inactive), 2012-09-19 18:03:31]

lgtm

https://codereview.chromium.org/10946013/diff/6001/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/10946013/diff/6001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContents.java:143: //
Helper methods
Ah, I meant for this section to be at the bottom of the file (maybe just above
"Native methods".

[Ted C, 2012-09-19 18:08:58]

https://codereview.chromium.org/10946013/diff/6001/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/10946013/diff/6001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContents.java:143: //
Helper methods
On 2012/09/19 18:03:31, benm wrote:
> Ah, I meant for this section to be at the bottom of the file (maybe just above
> "Native methods".

Done.

[commit-bot: I haz the power, 2012-09-19 20:16:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/10946013/4002

[commit-bot: I haz the power, 2012-09-19 20:16:48]

Failed to apply patch for
android_webview/java/src/org/chromium/android_webview/AwContents.java:
While running patch -p1 --forward --force;
patching file
android_webview/java/src/org/chromium/android_webview/AwContents.java
Hunk #1 FAILED at 4.
Hunk #2 succeeded at 23 (offset 1 line).
Hunk #3 succeeded at 105 (offset 9 lines).
Hunk #4 succeeded at 148 (offset 9 lines).
Hunk #5 FAILED at 214.
2 out of 5 hunks FAILED -- saving rejects to file
android_webview/java/src/org/chromium/android_webview/AwContents.java.rej

[commit-bot: I haz the power, 2012-09-19 21:24:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/10946013/10001

[commit-bot: I haz the power, 2012-09-19 23:54:25]

Change committed as 157658