New issue
Advanced search Search tips

Issue 776077 link

Starred by 1 user
Status: Fixed
Owner:
danyao@chromium.org
Closed: Nov 2017
Cc:
danyao@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Task

Blocking:
issue 734150



Sign in to add a comment

NavigationCallbacksTest.ForwardPostNavigation fails with WKBasedNavigationManager

Project Member Reported by ajuma@chromium.org, Oct 18 2017 
This is failing because TestWebStateObserver::DidStartLoading doesn't get called during the call to GoBack. So this has the same cause as  issue 775044 .

Here's the test output (the missing call to DidStartLoading is the "pre-requisites are not satisfied" error referring to line 749):
Unexpected mock function call - returning default value.
    Function call: ShouldAllowRequest(1, 16777216)
          Returns: false
Google Mock tried the following 3 expectations, but none matched:

../../ios/web/web_state/navigation_callbacks_inttest.mm:728: tried expectation #0: EXPECT_CALL(*decider_, ShouldAllowRequest(_, _))...
         Expected: the expectation is active
           Actual: it is retired
         Expected: to be called once
           Actual: called once - saturated and retired
../../ios/web/web_state/navigation_callbacks_inttest.mm:738: tried expectation #1: EXPECT_CALL(*decider_, ShouldAllowRequest(_, _))...
         Expected: the expectation is active
           Actual: it is retired
         Expected: to be called once
           Actual: called once - saturated and retired
../../ios/web/web_state/navigation_callbacks_inttest.mm:750: tried expectation #2: EXPECT_CALL(*decider_, ShouldAllowRequest(_, _))...
         Expected: all pre-requisites are satisfied
           Actual: the following immediate pre-requisites are not satisfied:
../../ios/web/web_state/navigation_callbacks_inttest.mm:749: pre-requisite #0
                   (end of pre-requisites)
         Expected: to be called once
           Actual: never called - unsatisfied and active

Comment 1 by danyao@chromium.org, Nov 10 2017

Owner: danyao@chromium.org
Status: Started (was: Available)
After crrev.com/c/762176, DidStartLoading is now triggered for back/forward navigation.

However, the test also expects DidStartLoading callback to be triggered before ShouldAllowRequest (triggered in [webView:decidePolicyForNavigationAction]. This works for LegacyNavigationManagerImpl because it implements back/forward navigation as a new load.

With WKBasedNavigation, the order of these two methods are reversed because back/forward navigation is delegated to the WKWebView directly. So [webView:decidePolicyForNavigationAction] is triggered first. DidStartLoading is triggered in WKWebView.loading KVO second.

There is no ordering contract between these two methods. I think it will be OK to update the test expectation.

Comment 2 by danyao@chromium.org, Nov 10 2017

Cc: danyao@chromium.org
 Issue 775044  has been merged into this issue.
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 14 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a8d892302a6e5532be897f92d506cdc2a02cba92

commit a8d892302a6e5532be897f92d506cdc2a02cba92
Author: Danyao Wang <danyao@chromium.org>
Date: Tue Nov 14 23:07:33 2017

[Nav Experiment] Update callback order expectations in test.

Because WKBaesdNavigationManager delegates back/forward navigation to
WKWebView, DidStartLoading for back/forward navigation is triggered as
part of WKWebView.loading KVO (similar to how fast back/forward
navigation works today). This is after the
|webView:decidePolicyForNavigationAction| callback. So update the test
expectations to reflect that.

This is OK because there is no ordering contract between DidStartLoading
and ShouldAllowRequest. The alternative way to fix the test is to use
gmock partial order. It will add a lot more gmock boilerplate to the
test so I don't think it's worth doing.

Bug:  776077 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I03a9a152cf95ffe587fa57e5e925496628965b87
Reviewed-on: https://chromium-review.googlesource.com/764792
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516487}
[modify] https://crrev.com/a8d892302a6e5532be897f92d506cdc2a02cba92/ios/web/web_state/navigation_callbacks_inttest.mm
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 16 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/30ed6ba4f0ad6eafc8e796f2fed3b18e5f2d5c61

commit 30ed6ba4f0ad6eafc8e796f2fed3b18e5f2d5c61
Author: Danyao Wang <danyao@chromium.org>
Date: Thu Nov 16 21:26:39 2017

[Nav Experiment] Fix isRendererInitiated in back/forward nav context.

WKBasedNavigationManager::AddPendingItem returns an existing nav item
for back/forward navigation. So we can't assume RENDERER_INITIATED just
because AddPendingItem() is called.

This fixes breakage in NavigationCallbacksTest.ForwardPostNavigation.

Bug:  776077 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I7b9aa10654e6073647390a021e77eefb8005794b
Reviewed-on: https://chromium-review.googlesource.com/769862
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517197}
[modify] https://crrev.com/30ed6ba4f0ad6eafc8e796f2fed3b18e5f2d5c61/ios/web/web_state/ui/crw_web_controller.mm

Comment 5 by danyao@chromium.org, Nov 16 2017

Status: Fixed (was: Started)
►

Sign in to add a comment