[Android WebView] Fix error description setting for provisional load

[Android WebView] Fix error description setting for provisional load

When provisional load fails, there is no HTML error page, so
the change I've made in https://codereview.chromium.org/15979017/
wasn't working properly. Now fixed.

R=benm@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204272

[mnaganov (inactive), 2013-06-05 09:27:35]

Thanks to joth's insightful comment, I've discovered that the code wasn't
working for provisional loads, since RenderViewImpl passes a NULL 'error_html'
pointer in this case.

[benm (inactive), 2013-06-05 09:35:08]

nice catch!

https://codereview.chromium.org/16441002/diff/1/android_webview/javatests/src...
File
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedErrorTest.java
(right):

https://codereview.chromium.org/16441002/diff/1/android_webview/javatests/src...
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedErrorTest.java:138:
assertFalse(onReceivedErrorHelper.getDescription().isEmpty());
nit: Could remove the not null assert above I guess?

https://codereview.chromium.org/16441002/diff/1/android_webview/renderer/aw_c...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/16441002/diff/1/android_webview/renderer/aw_c...
android_webview/renderer/aw_content_renderer_client.cc:66: std::string err =
UTF16ToUTF8(error.localizedDescription);
I think you could leave this inside the error_html block and use the
error.localizedDescription directly in the block you added below?

In fact I think we only need to create the std::string variable in the case that
the string16 is non-empty, ie. we are replacing the %e in the error page
content.

[mnaganov (inactive), 2013-06-05 09:46:21]

https://codereview.chromium.org/16441002/diff/1/android_webview/javatests/src...
File
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedErrorTest.java
(right):

https://codereview.chromium.org/16441002/diff/1/android_webview/javatests/src...
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedErrorTest.java:138:
assertFalse(onReceivedErrorHelper.getDescription().isEmpty());
On 2013/06/05 09:35:08, benm wrote:
> nit: Could remove the not null assert above I guess?

Yes, if the description will happen to be null, the test will crash anyway.

https://codereview.chromium.org/16441002/diff/1/android_webview/renderer/aw_c...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/16441002/diff/1/android_webview/renderer/aw_c...
android_webview/renderer/aw_content_renderer_client.cc:66: std::string err =
UTF16ToUTF8(error.localizedDescription);
On 2013/06/05 09:35:08, benm wrote:
> I think you could leave this inside the error_html block and use the
> error.localizedDescription directly in the block you added below?
> 
> In fact I think we only need to create the std::string variable in the case
that
> the string16 is non-empty, ie. we are replacing the %e in the error page
> content.

Sorry, I've made this change in a very mechanical manner. Surely we don't
actually need 'err' for setting error_description. Fixed.

[benm (inactive), 2013-06-05 10:00:11]

lgtm

thanks!

[commit-bot: I haz the power, 2013-06-05 10:06:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/16441002/5001

[commit-bot: I haz the power, 2013-06-05 10:30:16]

Retried try job too often on linux_chromeos for step(s) components_unittests,
content_unittests, interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[mnaganov (inactive), 2013-06-05 15:59:23]

Committed patchset #2 manually as r204272 (presubmit successful).

[joth, 2013-06-05 16:10:37]

lgtm

https://codereview.chromium.org/16441002/diff/5001/android_webview/renderer/a...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/16441002/diff/5001/android_webview/renderer/a...
android_webview/renderer/aw_content_renderer_client.cc:85: *error_description =
error.localizedDescription;
nit: need to use { } when there's an else.

Maybe easier:

*error_description = error.localizedDescription;
if (error_description->isEmpty()
  *error_description = ASCIIToUTF16(net::ErrorToString(error.reason));

[mnaganov (inactive), 2013-06-05 16:36:17]

On 2013/06/05 16:10:37, joth wrote:
> lgtm
> 
>
https://codereview.chromium.org/16441002/diff/5001/android_webview/renderer/a...
> File android_webview/renderer/aw_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/16441002/diff/5001/android_webview/renderer/a...
> android_webview/renderer/aw_content_renderer_client.cc:85: *error_description
=
> error.localizedDescription;
> nit: need to use { } when there's an else.
> 
> Maybe easier:
> 
> *error_description = error.localizedDescription;
> if (error_description->isEmpty()
>   *error_description = ASCIIToUTF16(net::ErrorToString(error.reason));

Thanks! I'll fix with some other change.