Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11187032:
Handle resubmission of HTTP Posts. (Closed)
|
Created:
8 years, 2 months ago by sgurun-gerrit only Modified:
8 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionHandle resubmission of HTTP Posts.
Implement functionality to handle resubmission of forms in Webview.
BUG=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164014
Patch Set 1 #Patch Set 2 : #
Total comments: 14
Patch Set 3 : #Patch Set 4 : call Reload in correct thread #
Total comments: 12
Patch Set 5 : addressed code review #
Total comments: 10
Patch Set 6 : joths nits #
Total comments: 6
Patch Set 7 : final nits #
Total comments: 8
Patch Set 8 : some more indentation #Patch Set 9 : fix the dependency #Messages
Total messages: 28 (0 generated)
I noticed that there is some subtle problem related to how we create AwContentsClient in test. AwContentsClient create the WebContentsDelegateAdapter which owns an Handler, and this should be on UI thread. After our discussions, I was thinking this is a safe assumption, but in tests, we don't generally do that.
This is not ready yet -I remember there were a few things to be done. Don't review yet.
On 2012/10/17 18:01:46, sgurun wrote: > This is not ready yet -I remember there were a few things to be done. Don't > review yet. Now ready. There is some subtlety around NotificationService creation in navigation_controller_impl. Please provide feedback. Thanks,
Excellent first patch! https://codereview.chromium.org/11187032/diff/3001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/11187032/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:50: private final EventHandler mHandler = new EventHandler(); as the class is not static , it's also OK to make it anon final Handler = new Handler() { @Override public void handleMessage... (I'm ok either way) https://codereview.chromium.org/11187032/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:55: private final class EventHandler extends Handler { could be as well to pass Looper.getMainlooper() to the base-class constructor, just to be clear it should only run on main thread. (also we could "assert ThreadUtils.runningOnUiThread()" in AwContentsClient constructor.) https://codereview.chromium.org/11187032/diff/3001/android_webview/javatests/... File android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java (right): https://codereview.chromium.org/11187032/diff/3001/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java:130: byte[] postData) throws Throwable { I see this file is inconsistent, but we are trying to use android style indent in java code so no need to align with open-paren here... just put as many args as can fit per line, indenting twice like executeJavaScriptAndWaitForResult is. https://codereview.chromium.org/11187032/diff/3001/android_webview/javatests/... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java (right): https://codereview.chromium.org/11187032/diff/3001/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java:43: class MockContentsClient extends TestAwContentsClient { rather than subclass again, add the new methods directly on TestAwContentsClient. also regarding naming, I think we're trying to keep 'Mock' free for the day we pull in some mocking framework. As per http://martinfowler.com/articles/mocksArentStubs.html this would be a stub. (and TestAwContentClient is too) https://codereview.chromium.org/11187032/diff/3001/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java:93: @Feature({"Android-WebViewClient", "OnFormResubmission"}) the feature tags are more coarse gained - we're putting everything under "Android-WebView" at this time, and you could add Navigation for these tests too. (List: http://go/chrome-android-test-feature-annotations) https://codereview.chromium.org/11187032/diff/3001/content/browser/web_conten... File content/browser/web_contents/navigation_controller_impl.cc (right): https://codereview.chromium.org/11187032/diff/3001/content/browser/web_conten... content/browser/web_contents/navigation_controller_impl.cc:315: // contentviewcore? this is a damn good question: - NotificationService::Create() is only called in a couple random places on (desktop) chrome, and we'll (eventually) not go through any of those in webview. I think we probably should explicitly call it somewhere. - but the way NotificationService::current() is used through-out this file, it looks like it's mandatory to have one installed, so I'm surprised other tests etc aren't already hitting this problem? - just to check, you are calling through to this method on main (UI) thread? NotificationService is thread-local, so this would be finding null if called on wrong thread. seems related to the question about which Looper the EventHandler is constructed on. https://codereview.chromium.org/11187032/diff/3001/content/browser/web_conten... content/browser/web_contents/navigation_controller_impl.cc:323: // TODO(sgurun) change the name to onFormResubmission nits: // TODO(sgurun): change the name to OnFormResubmission also assuming the above NotificationService change isn't needed, probably easiest to leave this out of this CL as changing files here pulls in extra layers of reviews.
thanks for quick review. https://codereview.chromium.org/11187032/diff/3001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/11187032/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:50: private final EventHandler mHandler = new EventHandler(); On 2012/10/17 20:35:53, joth wrote: > as the class is not static , it's also OK to make it anon > final Handler = new Handler() { > @Override > public void handleMessage... > > (I'm ok either way) Done. https://codereview.chromium.org/11187032/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:55: private final class EventHandler extends Handler { On 2012/10/17 20:35:53, joth wrote: > could be as well to pass Looper.getMainlooper() to the base-class constructor, > just to be clear it should only run on main thread. good idea. makes sense. > > (also we could "assert ThreadUtils.runningOnUiThread()" in AwContentsClient > constructor.) I am afraid this may break some tests. Let's do it in a separate CL to prevent more fixes creeping into this one. https://codereview.chromium.org/11187032/diff/3001/android_webview/javatests/... File android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java (right): https://codereview.chromium.org/11187032/diff/3001/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java:130: byte[] postData) throws Throwable { On 2012/10/17 20:35:53, joth wrote: > I see this file is inconsistent, but we are trying to use android style indent > in java code so no need to align with open-paren here... just put as many args > as can fit per line, indenting twice like executeJavaScriptAndWaitForResult is. Done. https://codereview.chromium.org/11187032/diff/3001/android_webview/javatests/... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java (right): https://codereview.chromium.org/11187032/diff/3001/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java:43: class MockContentsClient extends TestAwContentsClient { On 2012/10/17 20:35:53, joth wrote: > rather than subclass again, add the new methods directly on > TestAwContentsClient. > > also regarding naming, I think we're trying to keep 'Mock' free for the day we > pull in some mocking framework. As per > http://martinfowler.com/articles/mocksArentStubs.html this would be a stub. (and > TestAwContentClient is too) Done. https://codereview.chromium.org/11187032/diff/3001/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java:93: @Feature({"Android-WebViewClient", "OnFormResubmission"}) On 2012/10/17 20:35:53, joth wrote: > the feature tags are more coarse gained - we're putting everything under > "Android-WebView" at this time, and you could add Navigation for these tests > too. > > (List: http://go/chrome-android-test-feature-annotations) Done. https://codereview.chromium.org/11187032/diff/3001/content/browser/web_conten... File content/browser/web_contents/navigation_controller_impl.cc (right): https://codereview.chromium.org/11187032/diff/3001/content/browser/web_conten... content/browser/web_contents/navigation_controller_impl.cc:315: // contentviewcore? I am sure this is not created at present. Even with the looper change I don't see it (but regardless, I am creating AwContentsClient in main thread, which the event handler loop is also running). On 2012/10/17 20:35:53, joth wrote: > this is a damn good question: > - NotificationService::Create() is only called in a couple random places on > (desktop) chrome, and we'll (eventually) not go through any of those in webview. > I think we probably should explicitly call it somewhere. > - but the way NotificationService::current() is used through-out this file, it > looks like it's mandatory to have one installed, so I'm surprised other tests > etc aren't already hitting this problem? > - just to check, you are calling through to this method on main (UI) thread? > NotificationService is thread-local, so this would be finding null if called on > wrong thread. seems related to the question about which Looper the EventHandler > is constructed on. https://codereview.chromium.org/11187032/diff/3001/content/browser/web_conten... content/browser/web_contents/navigation_controller_impl.cc:323: // TODO(sgurun) change the name to onFormResubmission Yep let's leave it out. On 2012/10/17 20:35:53, joth wrote: > nits: > // TODO(sgurun): change the name to OnFormResubmission > > also assuming the above NotificationService change isn't needed, probably > easiest to leave this out of this CL as changing files here pulls in extra > layers of reviews.
https://codereview.chromium.org/11187032/diff/5002/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/11187032/diff/5002/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:63: mContentViewCore.cancelPendingReload(); why do these need to go through contentViewCore? since what we really call is WebContents methods, it might make sense to make these AwContents methods, no? https://codereview.chromium.org/11187032/diff/5002/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:167: mContentViewCore = contentViewCore; I don't feel comfortable with the proliferation of ContentViewCore references in the code... Please try and make this an AwContents reference. https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/... File android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java (right): https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java:127: protected void postUrlSync(final ContentViewCore contentViewCore, please use AwContents, I've worked hard to remove as many of the ContentViewCore references in our test code as possible because we want to minimize the number of different types the test cases need to know about. https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java:148: contentViewCore.loadUrl(LoadUrlParams.createLoadHttpPostParams(url, AwContents has a .loadUrl method that does internal bookkeeping, please use that instead. https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java (right): https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java:59: protected TestAwContentsClient createContentsClientOnMainSync() throws you don't need to create the TestAwContentsClient on the main thread. https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/... File android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java (right): https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java:26: private boolean mResubmit = false; Since this will only be used in one test case (for now) it should live in a TestAwContentsClient specialization in that test case (see the AwContentsClientShouldIgnoreNavigationTest.java file for an example), not here. It would be ideal if you could wrap all this new code in a static class (OnFormResubmissionCallbackHelper extends CallbackHelper), so that if this does need to be shared between test cases it can be done easily. https://codereview.chromium.org/11187032/diff/5002/content/browser/android/co... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/11187032/diff/5002/content/browser/android/co... content/browser/android/content_view_core_impl.cc:784: web_contents_->GetController().CancelPendingReload(); Again, I don't think this should live in ContentViewCore
On 18 October 2012 02:26, <mkosiba@chromium.org> wrote: > > https://codereview.chromium.**org/11187032/diff/5002/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java<https://codereview.chromium.org/11187032/diff/5002/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java> > File > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java > (right): > > https://codereview.chromium.**org/11187032/diff/5002/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java#**newcode63<https://codereview.chromium.org/11187032/diff/5002/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java#newcode63> > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java:63: > mContentViewCore.**cancelPendingReload(); > why do these need to go through contentViewCore? since what we really > call is WebContents methods, it might make sense to make these > AwContents methods, no? > > This is in preparation for http://code.google.com/p/chromium/issues/detail?id=154198 -- we anticipate the browser would also want to use these methods. Indeed, now that we pass true in NavigationController::Reload(), the browser already must call the ContinuePendingReload. (there's a companion CL downstream for this). You bring up a good point though that ContentViewCore is a very fat API and as soon as a class gets access to it, it tends to build up more interdependencies poking around in all parts of it. But rather than incrementally duplicate all "intern" CVC functions like this onto AwContents (turning that into an equally fat API) a cleaner solution here would be to extract out the specific methods we need into a more focused interface, and only pass that where it's needed. For example: define a new interface NavigationConfirmationReceiver { proceed() ; cancel(); } and then in ShowRepostFormWarningDialog we ask ContentViewCore to give us an object that implements that interface, and pass that as a param in ShowRepostFormWarningDialog(). So then the implementor of that method doesn't have to do any work to find the object they need to call those methods on. (The sam interface could be reusable for SSL cert errors, and possibly for HTTP auth handling although that needs a route for the username and password to also be passed back through) As a smaller towards that, we could just pass ContentViewCore across as the param in ShowRepostFormWarningDialog(), which at least contains the ContentViewCore knowledge just into this one callback rather than the whole class. [there's also the point that WebContentsObserver forces the ContentViewCore to travel through here anyway. that's basically just to avoid passing a native WebContents* through as an untyped int. we can probably design a nicer way to avoid this too, but that's a job for another day] > https://codereview.chromium.**org/11187032/diff/5002/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java#**newcode167<https://codereview.chromium.org/11187032/diff/5002/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java#newcode167> > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java:167: > mContentViewCore = contentViewCore; > I don't feel comfortable with the proliferation of ContentViewCore > references in the code... Please try and make this an AwContents > reference. > > https://codereview.chromium.**org/11187032/diff/5002/** > android_webview/javatests/src/**org/chromium/android_webview/** > test/AndroidWebViewTestBase.**java<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java> > File > android_webview/javatests/src/**org/chromium/android_webview/** > test/AndroidWebViewTestBase.**java > (right): > > https://codereview.chromium.**org/11187032/diff/5002/** > android_webview/javatests/src/**org/chromium/android_webview/** > test/AndroidWebViewTestBase.**java#newcode127<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java#newcode127> > android_webview/javatests/src/**org/chromium/android_webview/** > test/AndroidWebViewTestBase.**java:127: > protected void postUrlSync(final ContentViewCore contentViewCore, > please use AwContents, I've worked hard to remove as many of the > ContentViewCore references in our test code as possible because we want > to minimize the number of different types the test cases need to know > about. > good point, I missed this. > https://codereview.chromium.**org/11187032/diff/5002/** > android_webview/javatests/src/**org/chromium/android_webview/** > test/AndroidWebViewTestBase.**java#newcode148<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java#newcode148> > android_webview/javatests/src/**org/chromium/android_webview/** > test/AndroidWebViewTestBase.**java:148: > contentViewCore.loadUrl(**LoadUrlParams.**createLoadHttpPostParams(url, > AwContents has a .loadUrl method that does internal bookkeeping, please > use that instead. > > https://codereview.chromium.**org/11187032/diff/5002/** > android_webview/javatests/src/**org/chromium/android_webview/**test/** > AwContentsClientOnFormResubmis**sionTest.java<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java> > File > android_webview/javatests/src/**org/chromium/android_webview/**test/** > AwContentsClientOnFormResubmis**sionTest.java > (right): > > https://codereview.chromium.**org/11187032/diff/5002/** > android_webview/javatests/src/**org/chromium/android_webview/**test/** > AwContentsClientOnFormResubmis**sionTest.java#newcode59<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java#newcode59> > android_webview/javatests/src/**org/chromium/android_webview/**test/** > AwContentsClientOnFormResubmis**sionTest.java:59: > protected TestAwContentsClient createContentsClientOnMainSync**() throws > you don't need to create the TestAwContentsClient on the main thread. > > https://codereview.chromium.**org/11187032/diff/5002/** > android_webview/javatests/src/**org/chromium/android_webview/** > test/TestAwContentsClient.java<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java> > File > android_webview/javatests/src/**org/chromium/android_webview/** > test/TestAwContentsClient.java > (right): > > https://codereview.chromium.**org/11187032/diff/5002/** > android_webview/javatests/src/**org/chromium/android_webview/** > test/TestAwContentsClient.**java#newcode26<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java#newcode26> > android_webview/javatests/src/**org/chromium/android_webview/** > test/TestAwContentsClient.**java:26: > private boolean mResubmit = false; > Since this will only be used in one test case (for now) it should live > in a TestAwContentsClient specialization in that test case (see the > AwContentsClientShouldIgnoreNa**vigationTest.java file for an example), > not here. > > this was my bad: It originally was in a test-specific subclass but (without actually eyeballing the code all together) I thought it was generic test helper and suggested we group it together. Now it is together it's more obvious it's not the same pattern as the rest of the test helpers are using, so agree simplest path is to put it back as you had originally. Sorry about that. > It would be ideal if you could wrap all this new code in a static class > (**OnFormResubmissionCallbackHelp**er extends CallbackHelper), so that if > this does need to be shared between test cases it can be done easily. > > if it was rewritten as a helper, would your preference still be to keep it in the a subclass anyway? I had thought in another CL you were arguing to put new helpers in the base class right away, but I may be misremembering. (I don't have strong opinion either way, other than generally doing what people find helps them write tests most easily) > https://codereview.chromium.**org/11187032/diff/5002/** > content/browser/android/**content_view_core_impl.cc<https://codereview.chromium.org/11187032/diff/5002/content/browser/android/content_view_core_impl.cc> > File content/browser/android/**content_view_core_impl.cc (right): > > https://codereview.chromium.**org/11187032/diff/5002/** > content/browser/android/**content_view_core_impl.cc#**newcode784<https://codereview.chromium.org/11187032/diff/5002/content/browser/android/content_view_core_impl.cc#newcode784> > content/browser/android/**content_view_core_impl.cc:784: > web_contents_->GetController()**.CancelPendingReload(); > Again, I don't think this should live in ContentViewCore > > https://codereview.chromium.**org/11187032/<https://codereview.chromium.org/1... >
On 2012/10/18 18:51:32, joth wrote: > On 18 October 2012 02:26, <mailto:mkosiba@chromium.org> wrote: > > > AwContentsClient.java#**newcode63<https://codereview.chromium.org/11187032/diff/5002/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java#newcode63> > > android_webview/java/src/org/**chromium/android_webview/** > > AwContentsClient.java:63: > > mContentViewCore.**cancelPendingReload(); > > why do these need to go through contentViewCore? since what we really > > call is WebContents methods, it might make sense to make these > > AwContents methods, no? > > > > > This is in preparation for > http://code.google.com/p/chromium/issues/detail?id=154198 -- we anticipate > the browser would also want to use these methods. > Indeed, now that we pass true in NavigationController::Reload(), the > browser already must call the ContinuePendingReload. (there's a companion > CL downstream for this). > All the more reason to try and solve this in a cleaner way. > > You bring up a good point though that ContentViewCore is a very fat API and > as soon as a class gets access to it, it tends to build up more > interdependencies poking around in all parts of it. > > But rather than incrementally duplicate all "intern" CVC functions like > this onto AwContents (turning that into an equally fat API) a cleaner > solution here would be to extract out the specific methods we need into a > more focused interface, and only pass that where it's needed. > For example: define a new interface NavigationConfirmationReceiver { > proceed() ; cancel(); } and then in ShowRepostFormWarningDialog we ask > ContentViewCore to give us an object that implements that interface, and > pass that as a param in ShowRepostFormWarningDialog(). So then the > implementor of that method doesn't have to do any work to find the object > they need to call those methods on. > > (The sam interface could be reusable for SSL cert errors, and possibly for > HTTP auth handling although that needs a route for the username and > password to also be passed back through) > > > As a smaller towards that, we could just pass ContentViewCore across as the > param in ShowRepostFormWarningDialog(), which at least contains the > ContentViewCore knowledge just into this one callback rather than the whole > class. > > > [there's also the point that WebContentsObserver forces the ContentViewCore > to travel through here anyway. that's basically just to avoid passing a > native WebContents* through as an untyped int. we can probably design a > nicer way to avoid this too, but that's a job for another day] I'd argue that using ContentViewCore as the 'transport' for WebContents is a flawed design (though this is not the place to solve that issue). However, it seems to me that, for the same reason you wouldn't want AwContents to duplicate parts of the ContentViewCore API, you also wouldn't want ContentViewCore to duplicate parts of the NavigationController's API. The NavigationConfirmationReceiver approach seems reasonable, although I think I'd prefer it to mimic the TabModalConfirmDialogDelegate, because that's smart enough to invalidate itself for both LOAD_START and TAB_CLOSING events (not sure what guarantees does the WebView offer if you issue a .loadUrl between getting the onFormResubmission callback and posting one of the response messages) but that's more work, so a Java-only impl is OK too. random thought: From a memory ownership perspective you would probably want to pass around an object that has a weakref (Java or native) to the WebContents. > > > > > https://codereview.chromium.**org/11187032/diff/5002/** > > android_webview/java/src/org/**chromium/android_webview/** > > > AwContentsClient.java#**newcode167<https://codereview.chromium.org/11187032/diff/5002/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java#newcode167> > > android_webview/java/src/org/**chromium/android_webview/** > > AwContentsClient.java:167: > > mContentViewCore = contentViewCore; > > I don't feel comfortable with the proliferation of ContentViewCore > > references in the code... Please try and make this an AwContents > > reference. > > > > https://codereview.chromium.**org/11187032/diff/5002/** > > android_webview/javatests/src/**org/chromium/android_webview/** > > > test/AndroidWebViewTestBase.**java<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java> > > File > > android_webview/javatests/src/**org/chromium/android_webview/** > > test/AndroidWebViewTestBase.**java > > (right): > > > > https://codereview.chromium.**org/11187032/diff/5002/** > > android_webview/javatests/src/**org/chromium/android_webview/** > > > test/AndroidWebViewTestBase.**java#newcode127<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java#newcode127> > > android_webview/javatests/src/**org/chromium/android_webview/** > > test/AndroidWebViewTestBase.**java:127: > > protected void postUrlSync(final ContentViewCore contentViewCore, > > please use AwContents, I've worked hard to remove as many of the > > ContentViewCore references in our test code as possible because we want > > to minimize the number of different types the test cases need to know > > about. > > > > good point, I missed this. > > > > https://codereview.chromium.**org/11187032/diff/5002/** > > android_webview/javatests/src/**org/chromium/android_webview/** > > > test/AndroidWebViewTestBase.**java#newcode148<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java#newcode148> > > android_webview/javatests/src/**org/chromium/android_webview/** > > test/AndroidWebViewTestBase.**java:148: > > contentViewCore.loadUrl(**LoadUrlParams.**createLoadHttpPostParams(url, > > AwContents has a .loadUrl method that does internal bookkeeping, please > > use that instead. > > > > https://codereview.chromium.**org/11187032/diff/5002/** > > android_webview/javatests/src/**org/chromium/android_webview/**test/** > > > AwContentsClientOnFormResubmis**sionTest.java<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java> > > File > > android_webview/javatests/src/**org/chromium/android_webview/**test/** > > AwContentsClientOnFormResubmis**sionTest.java > > (right): > > > > https://codereview.chromium.**org/11187032/diff/5002/** > > android_webview/javatests/src/**org/chromium/android_webview/**test/** > > > AwContentsClientOnFormResubmis**sionTest.java#newcode59<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java#newcode59> > > android_webview/javatests/src/**org/chromium/android_webview/**test/** > > AwContentsClientOnFormResubmis**sionTest.java:59: > > protected TestAwContentsClient createContentsClientOnMainSync**() throws > > you don't need to create the TestAwContentsClient on the main thread. > > > > https://codereview.chromium.**org/11187032/diff/5002/** > > android_webview/javatests/src/**org/chromium/android_webview/** > > > test/TestAwContentsClient.java<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java> > > File > > android_webview/javatests/src/**org/chromium/android_webview/** > > test/TestAwContentsClient.java > > (right): > > > > https://codereview.chromium.**org/11187032/diff/5002/** > > android_webview/javatests/src/**org/chromium/android_webview/** > > > test/TestAwContentsClient.**java#newcode26<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java#newcode26> > > android_webview/javatests/src/**org/chromium/android_webview/** > > test/TestAwContentsClient.**java:26: > > private boolean mResubmit = false; > > Since this will only be used in one test case (for now) it should live > > in a TestAwContentsClient specialization in that test case (see the > > AwContentsClientShouldIgnoreNa**vigationTest.java file for an example), > > not here. > > > > > this was my bad: It originally was in a test-specific subclass but (without > actually eyeballing the code all together) I thought it was generic test > helper and suggested we group it together. Now it is together it's more > obvious it's not the same pattern as the rest of the test helpers are > using, so agree simplest path is to put it back as you had originally. > Sorry about that. > > > > It would be ideal if you could wrap all this new code in a static class > > (**OnFormResubmissionCallbackHelp**er extends CallbackHelper), so that if > > this does need to be shared between test cases it can be done easily. > > > > > if it was rewritten as a helper, would your preference still be to keep it > in the a subclass anyway? I had thought in another CL you were arguing to > put new helpers in the base class right away, but I may be misremembering. > (I don't have strong opinion either way, other than generally doing what > people find helps them write tests most easily) > I think it makes more sense to keep it in the subclass unless you know we'll be adding more tests that make use of the helper in the near future. Since the helper is a nice static class, moving it around is trivial, so putting it in the innermost scope seems like a reasonable default strategy to me.
I like the sound of a java counterpart to content::NavigationController. I don't want to get more caught up working through the details of that in this CL, but equally if we do go that way eventually it would obsolete the need for the NavigationConfirmationReceiver anyway. So I propose we go with my "intermediate" solution: pass over the ContentViewCore as a jobject param in the WebContentsDelegateAndroid::ShowRepostFormWarningDialog call, and keep it around just to use in the message handler (e.g. stash it in the |obj| param of the messages so the handler can find it) This keeps the CVC knowledge very localized within that class, so easy to switch to a more focused interface when we have it. (small downside of going via Message.obj removes type-safety so could hide a break if we make that change... but that's why we have tests :) Selim - ContentViewCore::FromWebContents(web_contents)->GetJavaObject() should do everything you need for this. Let me know if you have other questions! On 19 October 2012 07:59, <mkosiba@chromium.org> wrote: > On 2012/10/18 18:51:32, joth wrote: > >> On 18 October 2012 02:26, <mailto:mkosiba@chromium.org> wrote: >> > >> > > AwContentsClient.java#****newcode63<https://codereview.** > chromium.org/11187032/diff/**5002/android_webview/java/src/** > org/chromium/android_webview/**AwContentsClient.java#**newcode63<https://codereview.chromium.org/11187032/diff/5002/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java#newcode63> > > > >> > android_webview/java/src/org/****chromium/android_webview/** >> > AwContentsClient.java:63: >> > mContentViewCore.****cancelPendingReload(); >> >> > why do these need to go through contentViewCore? since what we really >> > call is WebContents methods, it might make sense to make these >> > AwContents methods, no? >> > >> > >> This is in preparation for >> http://code.google.com/p/**chromium/issues/detail?id=**154198<http://code.goo... we anticipate >> the browser would also want to use these methods. >> Indeed, now that we pass true in NavigationController::Reload()**, the >> browser already must call the ContinuePendingReload. (there's a companion >> CL downstream for this). >> > > > All the more reason to try and solve this in a cleaner way. > > > > You bring up a good point though that ContentViewCore is a very fat API >> and >> as soon as a class gets access to it, it tends to build up more >> interdependencies poking around in all parts of it. >> > > But rather than incrementally duplicate all "intern" CVC functions like >> this onto AwContents (turning that into an equally fat API) a cleaner >> solution here would be to extract out the specific methods we need into a >> more focused interface, and only pass that where it's needed. >> For example: define a new interface NavigationConfirmationReceiver { >> proceed() ; cancel(); } and then in ShowRepostFormWarningDialog we ask >> ContentViewCore to give us an object that implements that interface, and >> pass that as a param in ShowRepostFormWarningDialog(). So then the >> implementor of that method doesn't have to do any work to find the object >> they need to call those methods on. >> > > (The sam interface could be reusable for SSL cert errors, and possibly for >> HTTP auth handling although that needs a route for the username and >> password to also be passed back through) >> > > > As a smaller towards that, we could just pass ContentViewCore across as >> the >> param in ShowRepostFormWarningDialog(), which at least contains the >> ContentViewCore knowledge just into this one callback rather than the >> whole >> class. >> > > > [there's also the point that WebContentsObserver forces the >> ContentViewCore >> to travel through here anyway. that's basically just to avoid passing a >> native WebContents* through as an untyped int. we can probably design a >> nicer way to avoid this too, but that's a job for another day] >> > > I'd argue that using ContentViewCore as the 'transport' for WebContents is > a flawed design (though this is not the place to solve that issue). > However, it seems to me that, for the same reason you wouldn't > want AwContents to duplicate parts of the ContentViewCore API, you also > wouldn't want ContentViewCore to duplicate parts of the > NavigationController's > API. > > The NavigationConfirmationReceiver approach seems reasonable, although > I think I'd prefer it to mimic the TabModalConfirmDialogDelegate, because > that's smart enough to invalidate itself for both LOAD_START and > TAB_CLOSING > events > (not sure what guarantees does the WebView offer if you issue a .loadUrl > between getting the onFormResubmission callback and posting one of the > response > messages) but that's more work, so a Java-only impl is OK too. > > random thought: From a memory ownership perspective you would probably > want to pass around an object that has a weakref (Java or native) to > the WebContents. > > > Yep. Using ContentViewCore "conveniently" does this for us (java and native sides both have weak refs to each other). > > > https://codereview.chromium.****org/11187032/diff/5002/** >> > android_webview/java/src/org/****chromium/android_webview/** >> > >> > > AwContentsClient.java#****newcode167<https://codereview.** > chromium.org/11187032/diff/**5002/android_webview/java/src/** > org/chromium/android_webview/**AwContentsClient.java#**newcode167<https://codereview.chromium.org/11187032/diff/5002/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java#newcode167> > > > >> > android_webview/java/src/org/****chromium/android_webview/** >> >> > AwContentsClient.java:167: >> > mContentViewCore = contentViewCore; >> > I don't feel comfortable with the proliferation of ContentViewCore >> > references in the code... Please try and make this an AwContents >> > reference. >> > >> > https://codereview.chromium.****org/11187032/diff/5002/** >> > android_webview/javatests/src/****org/chromium/android_**webview/** >> > >> > > test/AndroidWebViewTestBase.****java<https://codereview.** > chromium.org/11187032/diff/**5002/android_webview/** > javatests/src/org/chromium/**android_webview/test/** > AndroidWebViewTestBase.java<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java> > > > >> > File >> > android_webview/javatests/src/****org/chromium/android_**webview/** >> > test/AndroidWebViewTestBase.****java >> > (right): >> > >> > https://codereview.chromium.****org/11187032/diff/5002/** >> > android_webview/javatests/src/****org/chromium/android_**webview/** >> > >> > > test/AndroidWebViewTestBase.****java#newcode127<https://** > codereview.chromium.org/**11187032/diff/5002/android_** > webview/javatests/src/org/**chromium/android_webview/test/** > AndroidWebViewTestBase.java#**newcode127<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java#newcode127> > > > >> > android_webview/javatests/src/****org/chromium/android_**webview/** >> > test/AndroidWebViewTestBase.****java:127: >> >> > protected void postUrlSync(final ContentViewCore contentViewCore, >> > please use AwContents, I've worked hard to remove as many of the >> > ContentViewCore references in our test code as possible because we want >> > to minimize the number of different types the test cases need to know >> > about. >> > >> > > good point, I missed this. >> > > > > https://codereview.chromium.****org/11187032/diff/5002/** >> > android_webview/javatests/src/****org/chromium/android_**webview/** >> > >> > > test/AndroidWebViewTestBase.****java#newcode148<https://** > codereview.chromium.org/**11187032/diff/5002/android_** > webview/javatests/src/org/**chromium/android_webview/test/** > AndroidWebViewTestBase.java#**newcode148<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java#newcode148> > > > >> > android_webview/javatests/src/****org/chromium/android_**webview/** >> > test/AndroidWebViewTestBase.****java:148: >> > contentViewCore.loadUrl(****LoadUrlParams.**** >> createLoadHttpPostParams(url, >> >> > AwContents has a .loadUrl method that does internal bookkeeping, please >> > use that instead. >> > >> > https://codereview.chromium.****org/11187032/diff/5002/** >> > android_webview/javatests/src/****org/chromium/android_** >> webview/**test/** >> > >> > > AwContentsClientOnFormResubmis****sionTest.java<https://** > codereview.chromium.org/**11187032/diff/5002/android_** > webview/javatests/src/org/**chromium/android_webview/test/** > AwContentsClientOnFormResubmis**sionTest.java<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java> > > > >> > File >> > android_webview/javatests/src/****org/chromium/android_** >> webview/**test/** >> > AwContentsClientOnFormResubmis****sionTest.java >> > (right): >> > >> > https://codereview.chromium.****org/11187032/diff/5002/** >> > android_webview/javatests/src/****org/chromium/android_** >> webview/**test/** >> > >> > > AwContentsClientOnFormResubmis****sionTest.java#newcode59<http** > s://codereview.chromium.org/**11187032/diff/5002/android_** > webview/javatests/src/org/**chromium/android_webview/test/** > AwContentsClientOnFormResubmis**sionTest.java#newcode59<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java#newcode59> > > > >> > android_webview/javatests/src/****org/chromium/android_** >> webview/**test/** >> > AwContentsClientOnFormResubmis****sionTest.java:59: >> > protected TestAwContentsClient createContentsClientOnMainSync****() >> throws >> >> > you don't need to create the TestAwContentsClient on the main thread. >> > >> > https://codereview.chromium.****org/11187032/diff/5002/** >> > android_webview/javatests/src/****org/chromium/android_**webview/** >> > >> > > test/TestAwContentsClient.**java<https://codereview.** > chromium.org/11187032/diff/**5002/android_webview/** > javatests/src/org/chromium/**android_webview/test/** > TestAwContentsClient.java<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java> > > > >> > File >> > android_webview/javatests/src/****org/chromium/android_**webview/** >> > test/TestAwContentsClient.java >> > (right): >> > >> > https://codereview.chromium.****org/11187032/diff/5002/** >> > android_webview/javatests/src/****org/chromium/android_**webview/** >> > >> > > test/TestAwContentsClient.****java#newcode26<https://** > codereview.chromium.org/**11187032/diff/5002/android_** > webview/javatests/src/org/**chromium/android_webview/test/** > TestAwContentsClient.java#**newcode26<https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java#newcode26> > > > >> > android_webview/javatests/src/****org/chromium/android_**webview/** >> > test/TestAwContentsClient.****java:26: >> >> > private boolean mResubmit = false; >> > Since this will only be used in one test case (for now) it should live >> > in a TestAwContentsClient specialization in that test case (see the >> > AwContentsClientShouldIgnoreNa****vigationTest.java file for an >> example), >> >> > not here. >> > >> > >> this was my bad: It originally was in a test-specific subclass but >> (without >> actually eyeballing the code all together) I thought it was generic test >> helper and suggested we group it together. Now it is together it's more >> obvious it's not the same pattern as the rest of the test helpers are >> using, so agree simplest path is to put it back as you had originally. >> Sorry about that. >> > > > > It would be ideal if you could wrap all this new code in a static class >> > (****OnFormResubmissionCallbackHelp****er extends CallbackHelper), so >> that if >> >> > this does need to be shared between test cases it can be done easily. >> > >> > >> if it was rewritten as a helper, would your preference still be to keep it >> in the a subclass anyway? I had thought in another CL you were arguing to >> put new helpers in the base class right away, but I may be >> misremembering. >> (I don't have strong opinion either way, other than generally doing what >> people find helps them write tests most easily) >> > > > I think it makes more sense to keep it in the subclass unless you know > we'll > be adding more tests that make use of the helper in the near future. Since > the helper is a nice static class, moving it around is trivial, > so putting it in the innermost scope seems like a reasonable default > strategy > to me. > > > https://codereview.chromium.**org/11187032/<https://codereview.chromium.org/1... >
sounds reasonable. Let's not try to fix everything in one CL.
https://codereview.chromium.org/11187032/diff/5002/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/11187032/diff/5002/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:167: mContentViewCore = contentViewCore; On 2012/10/18 09:26:42, Martin Kosiba wrote: > I don't feel comfortable with the proliferation of ContentViewCore references in > the code... Please try and make this an AwContents reference. Done. https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/... File android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java (right): https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java:127: protected void postUrlSync(final ContentViewCore contentViewCore, On 2012/10/18 09:26:42, Martin Kosiba wrote: > please use AwContents, I've worked hard to remove as many of the ContentViewCore > references in our test code as possible because we want to minimize the number > of different types the test cases need to know about. Done. https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java:148: contentViewCore.loadUrl(LoadUrlParams.createLoadHttpPostParams(url, On 2012/10/18 09:26:42, Martin Kosiba wrote: > AwContents has a .loadUrl method that does internal bookkeeping, please use that > instead. Done. https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java (right): https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java:59: protected TestAwContentsClient createContentsClientOnMainSync() throws On 2012/10/18 09:26:42, Martin Kosiba wrote: > you don't need to create the TestAwContentsClient on the main thread. Done. https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/... File android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java (right): https://codereview.chromium.org/11187032/diff/5002/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java:26: private boolean mResubmit = false; On 2012/10/18 09:26:42, Martin Kosiba wrote: > Since this will only be used in one test case (for now) it should live in a > TestAwContentsClient specialization in that test case (see the > AwContentsClientShouldIgnoreNavigationTest.java file for an example), not here. > > It would be ideal if you could wrap all this new code in a static class > (OnFormResubmissionCallbackHelper extends CallbackHelper), so that if this does > need to be shared between test cases it can be done easily. Done.
Great! just small nits... this time. I'll let Martin also have a quick look too. https://codereview.chromium.org/11187032/diff/16001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/11187032/diff/16001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:57: if (contentViewCore != null) { the == null cases here should be impossible, as we do a null-check on the native side before calling showRepostFormWarningDialog(), and it can't go out of scope between there and here as the Message is holding a hard ref to the object. So I'd skio the local var and just say: (ContentViewCore)msg.obj).continuePendingReload(); https://codereview.chromium.org/11187032/diff/16001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:135: public void showRepostFormWarningDialog(Object contentViewCore) { nit: replace "Object" with "ContentViewCore" here. While we pass it over the message as an object, it will at least help catch and native-side misuse at earliest point we can. https://codereview.chromium.org/11187032/diff/16001/android_webview/javatests... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java (right): https://codereview.chromium.org/11187032/diff/16001/android_webview/javatests... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java:65: private ContentViewCore mContentViewCore; nit: as only used once, rather than hold this as a field I'd just call mAwContents.getContentViewCore().... when you need it. https://codereview.chromium.org/11187032/diff/16001/chrome/browser/component/... File chrome/browser/component/web_contents_delegate_android/java/src/org/chromium/chrome/browser/component/web_contents_delegate_android/WebContentsDelegateAndroid.java (right): https://codereview.chromium.org/11187032/diff/16001/chrome/browser/component/... chrome/browser/component/web_contents_delegate_android/java/src/org/chromium/chrome/browser/component/web_contents_delegate_android/WebContentsDelegateAndroid.java:115: public void showRepostFormWarningDialog(Object contentViewCore) { ditto pass as ContentViewCore type https://codereview.chromium.org/11187032/diff/16001/chrome/browser/component/... File chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc (right): https://codereview.chromium.org/11187032/diff/16001/chrome/browser/component/... chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc:275: ScopedJavaLocalRef<jobject> contentViewCore = nit: hacker style: content_view_core
https://codereview.chromium.org/11187032/diff/16001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/11187032/diff/16001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:57: if (contentViewCore != null) { On 2012/10/23 00:05:31, joth wrote: > the == null cases here should be impossible, as we do a null-check on the native > side before calling showRepostFormWarningDialog(), and it can't go out of scope > between there and here as the Message is holding a hard ref to the object. > So I'd skio the local var and just say: > (ContentViewCore)msg.obj).continuePendingReload(); Done. https://codereview.chromium.org/11187032/diff/16001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:135: public void showRepostFormWarningDialog(Object contentViewCore) { On 2012/10/23 00:05:31, joth wrote: > nit: replace "Object" with "ContentViewCore" here. While we pass it over the > message as an object, it will at least help catch and native-side misuse at > earliest point we can. Done. https://codereview.chromium.org/11187032/diff/16001/android_webview/javatests... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java (right): https://codereview.chromium.org/11187032/diff/16001/android_webview/javatests... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java:65: private ContentViewCore mContentViewCore; On 2012/10/23 00:05:31, joth wrote: > nit: as only used once, rather than hold this as a field I'd just call > mAwContents.getContentViewCore().... when you need it. Done. https://codereview.chromium.org/11187032/diff/16001/chrome/browser/component/... File chrome/browser/component/web_contents_delegate_android/java/src/org/chromium/chrome/browser/component/web_contents_delegate_android/WebContentsDelegateAndroid.java (right): https://codereview.chromium.org/11187032/diff/16001/chrome/browser/component/... chrome/browser/component/web_contents_delegate_android/java/src/org/chromium/chrome/browser/component/web_contents_delegate_android/WebContentsDelegateAndroid.java:115: public void showRepostFormWarningDialog(Object contentViewCore) { On 2012/10/23 00:05:31, joth wrote: > ditto pass as ContentViewCore type Done. https://codereview.chromium.org/11187032/diff/16001/chrome/browser/component/... File chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc (right): https://codereview.chromium.org/11187032/diff/16001/chrome/browser/component/... chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc:275: ScopedJavaLocalRef<jobject> contentViewCore = On 2012/10/23 00:05:31, joth wrote: > nit: hacker style: content_view_core Done.
lgtm pending martin. https://codereview.chromium.org/11187032/diff/21001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/11187032/diff/21001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:54: case CONTINUE_PENDING_RELOAD: nit: the indent doesn't match the switch on line 78 below (I don't know off hand which is correct; I let eclipse fix that for me :)
LGTM https://codereview.chromium.org/11187032/diff/21001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/11187032/diff/21001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:58: ((ContentViewCore)msg.obj).cancelPendingReload(); so this will update the most recent NavigationController entry. Not sure if we want to worry about it, but have we considered the following case? onFormResubmission(dontResend, resend) { resend.sendToTarget(); mWebView.loadUrl("asasd"); } It would be quite easy to prevent this by remembering the URL that the Messages are related to and asserting/ignoring the cancel/continue if it had changed. Obviously, if the current WebView behavior for this scenario is undefined (or equally wrong), then adding a TODO will be more than enough. https://codereview.chromium.org/11187032/diff/21001/android_webview/javatests... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java (right): https://codereview.chromium.org/11187032/diff/21001/android_webview/javatests... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java:121: onPageFinishedHelper.waitForCallback(callCount, 1, TIMEOUT, TimeUnit.SECONDS); nit: this is effectively a sleep. It's kind of unfortunate that we have to do this. Maybe add a comment explaining why this is the only way, so that the next person to look at the code doesn't waste time trying to fix it.
https://codereview.chromium.org/11187032/diff/21001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/11187032/diff/21001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:54: case CONTINUE_PENDING_RELOAD: I don't know either but looks like it needs to be indented. On 2012/10/23 01:46:03, joth wrote: > nit: the indent doesn't match the switch on line 78 below (I don't know off hand > which is correct; I let eclipse fix that for me :) https://codereview.chromium.org/11187032/diff/21001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:58: ((ContentViewCore)msg.obj).cancelPendingReload(); I don't see any mechanism in current webview to remember the URL. In any case, I will put a TODO to handle this later. On 2012/10/23 10:34:34, Martin Kosiba wrote: > so this will update the most recent NavigationController entry. Not sure if we > want to worry about it, but have we considered the following case? > > onFormResubmission(dontResend, resend) { > resend.sendToTarget(); > mWebView.loadUrl("asasd"); > } > > It would be quite easy to prevent this by remembering the URL that the Messages > are related to and asserting/ignoring the cancel/continue if it had changed. > Obviously, if the current WebView behavior for this scenario is undefined (or > equally wrong), then adding a TODO will be more than enough. https://codereview.chromium.org/11187032/diff/21001/android_webview/javatests... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java (right): https://codereview.chromium.org/11187032/diff/21001/android_webview/javatests... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java:121: onPageFinishedHelper.waitForCallback(callCount, 1, TIMEOUT, TimeUnit.SECONDS); On 2012/10/23 10:34:34, Martin Kosiba wrote: > nit: this is effectively a sleep. It's kind of unfortunate that we have to do > this. Maybe add a comment explaining why this is the only way, so that the next > person to look at the code doesn't waste time trying to fix it. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sgurun@chromium.org/11187032/4014
Presubmit check for 11187032-4014 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content/public/android
Presubmit checks took 4.4s to calculate.
On 23 October 2012 08:54, <sgurun@chromium.org> wrote: > > https://codereview.chromium.**org/11187032/diff/21001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java<https://codereview.chromium.org/11187032/diff/21001/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java> > File > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java > (right): > > https://codereview.chromium.**org/11187032/diff/21001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java#**newcode54<https://codereview.chromium.org/11187032/diff/21001/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java#newcode54> > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java:54: > case CONTINUE_PENDING_RELOAD: > I don't know either but looks like it needs to be indented. > > > On 2012/10/23 01:46:03, joth wrote: > >> nit: the indent doesn't match the switch on line 78 below (I don't >> > know off hand > >> which is correct; I let eclipse fix that for me :) >> > > https://codereview.chromium.**org/11187032/diff/21001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java#**newcode58<https://codereview.chromium.org/11187032/diff/21001/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java#newcode58> > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java:58: > ((ContentViewCore)msg.obj).**cancelPendingReload(); > I don't see any mechanism in current webview to remember the URL. In any > case, I will put a TODO to handle this later. > > > On 2012/10/23 10:34:34, Martin Kosiba wrote: > >> so this will update the most recent NavigationController entry. Not >> > sure if we > >> want to worry about it, but have we considered the following case? >> > > onFormResubmission(dontResend, resend) { >> resend.sendToTarget(); >> mWebView.loadUrl("asasd"); >> } >> > > It would be quite easy to prevent this by remembering the URL that the >> > Messages > >> are related to and asserting/ignoring the cancel/continue if it had >> > changed. > >> Obviously, if the current WebView behavior for this scenario is >> > undefined (or > >> equally wrong), then adding a TODO will be more than enough. >> > Note that chromium's NavigationController will actually handle this anyway (ignore the cancel/continue call if the repost is no-longer pending) although this is in part by chance as by design (it will actually DCHECK this scenario first, but obv. not on a release build) Anyway, I don't think we should re-implement another layer of handling for this if the underlying content API basically already supports it. actually.... adding a public NavigationController::IsReloadPending() method method would be the sweet-spot of reusing the existing handling while still allow chrome to keep the DCHECK. A TODO for that SGTM. > > https://codereview.chromium.**org/11187032/diff/21001/** > android_webview/javatests/src/**org/chromium/android_webview/**test/** > AwContentsClientOnFormResubmis**sionTest.java<https://codereview.chromium.org/11187032/diff/21001/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java> > File > android_webview/javatests/src/**org/chromium/android_webview/**test/** > AwContentsClientOnFormResubmis**sionTest.java > (right): > > https://codereview.chromium.**org/11187032/diff/21001/** > android_webview/javatests/src/**org/chromium/android_webview/**test/** > AwContentsClientOnFormResubmis**sionTest.java#newcode121<https://codereview.chromium.org/11187032/diff/21001/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java#newcode121> > android_webview/javatests/src/**org/chromium/android_webview/**test/** > AwContentsClientOnFormResubmis**sionTest.java:121: > onPageFinishedHelper.**waitForCallback(callCount, 1, TIMEOUT, > TimeUnit.SECONDS); > On 2012/10/23 10:34:34, Martin Kosiba wrote: > >> nit: this is effectively a sleep. It's kind of unfortunate that we >> > have to do > >> this. Maybe add a comment explaining why this is the only way, so that >> > the next > >> person to look at the code doesn't waste time trying to fix it. >> > > Done. > > https://codereview.chromium.**org/11187032/<https://codereview.chromium.org/1... >
+tedchoc, could you rubberstamp content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java ? TIA
Hi bulach@, satish@, Can you please review content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java when you have a chance? Thanks
lgtm w/ some fly by nits http://codereview.chromium.org/11187032/diff/4014/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): http://codereview.chromium.org/11187032/diff/4014/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:57: ((ContentViewCore)msg.obj).continuePendingReload(); space after the cast...and below http://codereview.chromium.org/11187032/diff/4014/android_webview/javatests/s... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java (right): http://codereview.chromium.org/11187032/diff/4014/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java:54: "<html><head><title>Load</title></head><body>HELLO</body></html>"; +4 indent and next bit. http://codereview.chromium.org/11187032/diff/4014/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java:73: createAwTestContainerViewOnMainSync(mContentsClient); +4 indent http://codereview.chromium.org/11187032/diff/4014/chrome/browser/component/we... File chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc (right): http://codereview.chromium.org/11187032/diff/4014/chrome/browser/component/we... chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc:276: content::ContentViewCore::FromWebContents(source)->GetJavaObject(); should be indented +2 (and below)
http://codereview.chromium.org/11187032/diff/4014/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): http://codereview.chromium.org/11187032/diff/4014/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:57: ((ContentViewCore)msg.obj).continuePendingReload(); On 2012/10/24 23:25:51, Ted C wrote: > space after the cast...and below Done. http://codereview.chromium.org/11187032/diff/4014/android_webview/javatests/s... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java (right): http://codereview.chromium.org/11187032/diff/4014/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java:54: "<html><head><title>Load</title></head><body>HELLO</body></html>"; On 2012/10/24 23:25:51, Ted C wrote: > +4 indent > > and next bit. Done. http://codereview.chromium.org/11187032/diff/4014/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnFormResubmissionTest.java:73: createAwTestContainerViewOnMainSync(mContentsClient); On 2012/10/24 23:25:51, Ted C wrote: > +4 indent Done. http://codereview.chromium.org/11187032/diff/4014/chrome/browser/component/we... File chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc (right): http://codereview.chromium.org/11187032/diff/4014/chrome/browser/component/we... chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc:276: content::ContentViewCore::FromWebContents(source)->GetJavaObject(); On 2012/10/24 23:25:51, Ted C wrote: > should be indented +2 (and below) Done.
lgtm still
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sgurun@chromium.org/11187032/32001
Retried try job too often for step(s) check_deps
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sgurun@chromium.org/11187032/31003
Change committed as 164014 |
