Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
986953002:
Move implementation of load methods to chromium layer (Closed)
DescriptionMove implementation of load methods to chromium layer
Add methods in AwContents that mirrors WebView exactly. And have
WebViewChromium call through to AwContents directly.
Unfortunately the code to post to UI thread now needs to be duplicated
in WebViewChromium for each method.
BUG=464491
Committed: https://crrev.com/2ff81b49f2a9b7b029b56ab9f13b9aa823c87bf0
Cr-Commit-Position: refs/heads/master@{#319734}
Patch Set 1 #Patch Set 2 : remove more #
Total comments: 9
Patch Set 3 : return #
Messages
Total messages: 15 (3 generated)
boliu@chromium.org changed reviewers: + mnaganov@chromium.org
ptal. Sunday evening clean up :) I'll try cts tests tomorrow. https://codereview.chromium.org/986953002/diff/20001/android_webview/glue/jav... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (left): https://codereview.chromium.org/986953002/diff/20001/android_webview/glue/jav... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:543: if (url == null) { This refactor does change behavior. Eg this check will happen *after* startYourEngines or addTask now.
https://codereview.chromium.org/986953002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/986953002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1187: evaluateJavaScript(url.substring(javaScriptScheme.length()), null); Missing return statement?
torne@chromium.org changed reviewers: + torne@chromium.org
https://codereview.chromium.org/986953002/diff/20001/android_webview/glue/jav... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (left): https://codereview.chromium.org/986953002/diff/20001/android_webview/glue/jav... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:543: if (url == null) { On 2015/03/09 03:33:56, boliu wrote: > This refactor does change behavior. Eg this check will happen *after* > startYourEngines or addTask now. Do you know why we had this check in the first place? It seems like if someone was actually relying on loadUrl(null) being a no-op, then making it start chrome might well break their logic :( https://codereview.chromium.org/986953002/diff/20001/android_webview/glue/jav... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/986953002/diff/20001/android_webview/glue/jav... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:509: assert mAppTargetSdkVersion < Build.VERSION_CODES.JELLY_BEAN_MR2; This isn't a new restriction, right? I can't see where this was checked in the old code. We almost certainly shouldn't use assert for this kind of check in any case; it throws a very generic exception and can be disabled. assert is only for debugging assertions checking internal state, not for validating input/caller behaviour.
https://codereview.chromium.org/986953002/diff/20001/android_webview/glue/jav... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (left): https://codereview.chromium.org/986953002/diff/20001/android_webview/glue/jav... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:543: if (url == null) { On 2015/03/09 10:57:39, Torne wrote: > On 2015/03/09 03:33:56, boliu wrote: > > This refactor does change behavior. Eg this check will happen *after* > > startYourEngines or addTask now. > > Do you know why we had this check in the first place? It seems like if someone > was actually relying on loadUrl(null) being a no-op, then making it start chrome > might well break their logic :( http://b/10688422 I think whether to lock in the UI thread is a orthogonal issue though, and really for us to decide. https://codereview.chromium.org/986953002/diff/20001/android_webview/glue/jav... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/986953002/diff/20001/android_webview/glue/jav... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:509: assert mAppTargetSdkVersion < Build.VERSION_CODES.JELLY_BEAN_MR2; On 2015/03/09 10:57:39, Torne wrote: > This isn't a new restriction, right? I can't see where this was checked in the > old code. > > We almost certainly shouldn't use assert for this kind of check in any case; it > throws a very generic exception and can be disabled. assert is only for > debugging assertions checking internal state, not for validating input/caller > behaviour. It's not new. Copied from Line 623 from the left side. Although I'm not sure whether it did anything. I think we don't compile asserts in, right? https://codereview.chromium.org/986953002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/986953002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1187: evaluateJavaScript(url.substring(javaScriptScheme.length()), null); On 2015/03/09 09:37:39, mnaganov (cr) wrote: > Missing return statement? We generally don't return void, no?
On 2015/03/09 14:35:43, boliu wrote: > https://codereview.chromium.org/986953002/diff/20001/android_webview/glue/jav... > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java > (left): > > https://codereview.chromium.org/986953002/diff/20001/android_webview/glue/jav... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:543: > if (url == null) { > On 2015/03/09 10:57:39, Torne wrote: > > On 2015/03/09 03:33:56, boliu wrote: > > > This refactor does change behavior. Eg this check will happen *after* > > > startYourEngines or addTask now. > > > > Do you know why we had this check in the first place? It seems like if someone > > was actually relying on loadUrl(null) being a no-op, then making it start > chrome > > might well break their logic :( > > http://b/10688422 > > I think whether to lock in the UI thread is a orthogonal issue though, and > really for us to decide. Yeah, looks like as long as we don't crash on null it's probably fine. > https://codereview.chromium.org/986953002/diff/20001/android_webview/glue/jav... > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java > (right): > > https://codereview.chromium.org/986953002/diff/20001/android_webview/glue/jav... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:509: > assert mAppTargetSdkVersion < Build.VERSION_CODES.JELLY_BEAN_MR2; > On 2015/03/09 10:57:39, Torne wrote: > > This isn't a new restriction, right? I can't see where this was checked in the > > old code. > > > > We almost certainly shouldn't use assert for this kind of check in any case; > it > > throws a very generic exception and can be disabled. assert is only for > > debugging assertions checking internal state, not for validating input/caller > > behaviour. > > It's not new. Copied from Line 623 from the left side. > > Although I'm not sure whether it did anything. I think we don't compile asserts > in, right? There is no way to avoid compiling asserts in; in java asserts are toggled at runtime, not compile time. If it's not new then I guess it's okay here, but it's still gross :( > > https://codereview.chromium.org/986953002/diff/20001/android_webview/java/src... > File android_webview/java/src/org/chromium/android_webview/AwContents.java > (right): > > https://codereview.chromium.org/986953002/diff/20001/android_webview/java/src... > android_webview/java/src/org/chromium/android_webview/AwContents.java:1187: > evaluateJavaScript(url.substring(javaScriptScheme.length()), null); > On 2015/03/09 09:37:39, mnaganov (cr) wrote: > > Missing return statement? > > We generally don't return void, no?
https://codereview.chromium.org/986953002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/986953002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1187: evaluateJavaScript(url.substring(javaScriptScheme.length()), null); On 2015/03/09 14:35:43, boliu wrote: > On 2015/03/09 09:37:39, mnaganov (cr) wrote: > > Missing return statement? > > We generally don't return void, no? It is for avoiding doing loadUrl afterwards. We were not doing that before. Or perhaps I'm misunderstanding your intention here?
https://codereview.chromium.org/986953002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/986953002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1187: evaluateJavaScript(url.substring(javaScriptScheme.length()), null); On 2015/03/09 15:07:18, mnaganov (cr) wrote: > On 2015/03/09 14:35:43, boliu wrote: > > On 2015/03/09 09:37:39, mnaganov (cr) wrote: > > > Missing return statement? > > > > We generally don't return void, no? > > It is for avoiding doing loadUrl afterwards. We were not doing that before. Or > perhaps I'm misunderstanding your intention here? Oh oops. You are right of course. Added back
lgtm
cts WebViewTest, WebViewClientTest, WebChromeClientTest still pass
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/986953002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2ff81b49f2a9b7b029b56ab9f13b9aa823c87bf0 Cr-Commit-Position: refs/heads/master@{#319734} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
