New issue
Advanced search Search tips

Issue 692331 link

Starred by 1 user
Status: Fixed
Owner:
eugene...@chromium.org
Closed: Mar 2017
Cc:
ios-bugs@chromium.org
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Feature

Blocking:
issue 674991



Sign in to add a comment

Implement WebStateObserver::DidFinishNavigation

Project Member Reported by eugene...@chromium.org, Feb 15 2017 
This method mirrors WebContentsObserver::DidFinishNavigation and should replace these methods:

WebStateObserver::UrlHashChanged()
WebStateObserver::HistoryStateChanged()
-[CRWWebDelegate webDidUpdateHistoryStateWithPageURL:]
-[CRWWebDelegate webWillFinishHistoryNavigationFromEntry:] for same document navigations
-[CRWWebDelegate webDidStartLoadingURL:shouldUpdateHistory:] for non-error cases

Comment 1 by eugene...@chromium.org, Feb 15 2017

Description: Show this description
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 24 2017 

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

commit 17faf252763df7fb3dbe510549ed5fd8e94e84db
Author: eugenebut <eugenebut@chromium.org>
Date: Fri Feb 24 03:13:21 2017

Implemented WebStateObserver::DidFinishNavigation(NavigationContext*).

This callback aims to replace the following methods:
 WebStateObserver::UrlHashChanged()
 WebStateObserver::HistoryStateChanged()
 -[CRWWebDelegate webDidUpdateHistoryStateWithPageURL:]
 -[CRWWebDelegate webWillFinishHistoryNavigationFromEntry:]
 -[CRWWebDelegate webDidStartLoadingURL:shouldUpdateHistory:]

These callbacks will be replaced in a separate CL. Currently NavigationContext is
created only for DidFinishNavigation callback. Long term plan is to create it
for DidStartNavigation and keep it alive until DidFinishNavigation.

This CL has small logic change by updating MIME type for Native Content
navigation, which is the right thing to do anyway.

BUG= 692331 

Review-Url: https://codereview.chromium.org/2698413004
Cr-Commit-Position: refs/heads/master@{#452732}

[modify] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/BUILD.gn
[modify] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/public/test/fakes/test_web_client.mm
[add] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/public/web_state/navigation_context.h
[modify] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/public/web_state/ui/crw_web_delegate.h
[modify] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/public/web_state/web_state_observer.h
[modify] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/test/test_url_constants.cc
[modify] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/test/test_url_constants.h
[add] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/web_state/navigation_callbacks_inttest.mm
[add] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/web_state/navigation_context_impl.h
[add] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/web_state/navigation_context_impl.mm
[add] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/web_state/navigation_context_impl_unittest.mm
[modify] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/web_state/web_state_impl.h
[modify] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/web_state/web_state_impl.mm
[modify] https://crrev.com/17faf252763df7fb3dbe510549ed5fd8e94e84db/ios/web/web_state/web_state_impl_unittest.mm

Comment 3 by eugene...@chromium.org, Feb 24 2017

Status: Fixed (was: Assigned)

Comment 4 by eugene...@chromium.org, Feb 24 2017

Status: Started (was: Fixed)
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 27 2017 

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

commit 6977a7b9c8f620c5e811a03644d4850b501675d4
Author: eugenebut <eugenebut@chromium.org>
Date: Mon Feb 27 16:52:11 2017

Use DidFinishNavigation callback in LanguageDetectionController.

This single callback replaces UrlHashChanged and HistoryStateChanged.

BUG= 692331 
TEST=page gets translated after same document navigations.

Review-Url: https://codereview.chromium.org/2711363003
Cr-Commit-Position: refs/heads/master@{#453235}

[modify] https://crrev.com/6977a7b9c8f620c5e811a03644d4850b501675d4/components/translate/ios/browser/language_detection_controller.h
[modify] https://crrev.com/6977a7b9c8f620c5e811a03644d4850b501675d4/components/translate/ios/browser/language_detection_controller.mm
[modify] https://crrev.com/6977a7b9c8f620c5e811a03644d4850b501675d4/ios/web/public/test/fakes/crw_test_web_state_observer.h
[modify] https://crrev.com/6977a7b9c8f620c5e811a03644d4850b501675d4/ios/web/public/test/fakes/crw_test_web_state_observer.mm
[modify] https://crrev.com/6977a7b9c8f620c5e811a03644d4850b501675d4/ios/web/public/web_state/web_state_observer.h
[modify] https://crrev.com/6977a7b9c8f620c5e811a03644d4850b501675d4/ios/web/public/web_state/web_state_observer_bridge.h
[modify] https://crrev.com/6977a7b9c8f620c5e811a03644d4850b501675d4/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/6977a7b9c8f620c5e811a03644d4850b501675d4/ios/web/web_state/web_state_impl.h
[modify] https://crrev.com/6977a7b9c8f620c5e811a03644d4850b501675d4/ios/web/web_state/web_state_impl.mm
[modify] https://crrev.com/6977a7b9c8f620c5e811a03644d4850b501675d4/ios/web/web_state/web_state_impl_unittest.mm
[modify] https://crrev.com/6977a7b9c8f620c5e811a03644d4850b501675d4/ios/web/web_state/web_state_observer_bridge.mm
[modify] https://crrev.com/6977a7b9c8f620c5e811a03644d4850b501675d4/ios/web/web_state/web_state_observer_bridge_unittest.mm
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 28 2017 

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

commit bd25aca5753c52eb1c0e348c0146589df14802bc
Author: eugenebut <eugenebut@chromium.org>
Date: Tue Feb 28 23:14:54 2017

Removed -[CRWWebDelegate webDidUpdateHistoryStateWithPageURL].

Replaced with webState:didFinishNavigation:, which is also added in this
CL.

BUG= 692331 

Review-Url: https://codereview.chromium.org/2718733003
Cr-Commit-Position: refs/heads/master@{#453732}

[modify] https://crrev.com/bd25aca5753c52eb1c0e348c0146589df14802bc/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/bd25aca5753c52eb1c0e348c0146589df14802bc/ios/web/public/test/fakes/crw_test_web_state_observer.h
[modify] https://crrev.com/bd25aca5753c52eb1c0e348c0146589df14802bc/ios/web/public/test/fakes/crw_test_web_state_observer.mm
[modify] https://crrev.com/bd25aca5753c52eb1c0e348c0146589df14802bc/ios/web/public/web_state/ui/crw_web_delegate.h
[modify] https://crrev.com/bd25aca5753c52eb1c0e348c0146589df14802bc/ios/web/public/web_state/web_state_observer_bridge.h
[modify] https://crrev.com/bd25aca5753c52eb1c0e348c0146589df14802bc/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/bd25aca5753c52eb1c0e348c0146589df14802bc/ios/web/web_state/web_state_observer_bridge.mm
[modify] https://crrev.com/bd25aca5753c52eb1c0e348c0146589df14802bc/ios/web/web_state/web_state_observer_bridge_unittest.mm
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 7 2017 

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

commit 9b8867c5b4b08dd484fb631fadacb6d247cdc7cf
Author: eugenebut <eugenebut@chromium.org>
Date: Tue Mar 07 19:06:33 2017

Removed -[CRWWebDelegate webDidStartLoadingURL:updateHistory:].

This callback is replaced by WebStateObserver::DidFinishNavigation.

Call OnNavigationCommitted after |webPageChanged|, because
NavigationItem is committed inside |webPageChanged|.
UpdateHttpResponseHeaders() should still be called before
SetContentsMimeType(), so |UpdateHttpResponseHeaders| was moved
out of |OnNavigationCommitted|.

BUG= 692331 , 698410 

Review-Url: https://codereview.chromium.org/2724383003
Cr-Commit-Position: refs/heads/master@{#455167}

[modify] https://crrev.com/9b8867c5b4b08dd484fb631fadacb6d247cdc7cf/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/9b8867c5b4b08dd484fb631fadacb6d247cdc7cf/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/9b8867c5b4b08dd484fb631fadacb6d247cdc7cf/ios/web/navigation/crw_session_controller.mm
[modify] https://crrev.com/9b8867c5b4b08dd484fb631fadacb6d247cdc7cf/ios/web/public/web_state/ui/crw_web_delegate.h
[modify] https://crrev.com/9b8867c5b4b08dd484fb631fadacb6d247cdc7cf/ios/web/web_state/navigation_callbacks_inttest.mm
[modify] https://crrev.com/9b8867c5b4b08dd484fb631fadacb6d247cdc7cf/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/9b8867c5b4b08dd484fb631fadacb6d247cdc7cf/ios/web/web_state/web_state_impl.h
[modify] https://crrev.com/9b8867c5b4b08dd484fb631fadacb6d247cdc7cf/ios/web/web_state/web_state_impl.mm
[modify] https://crrev.com/9b8867c5b4b08dd484fb631fadacb6d247cdc7cf/ios/web/web_state/web_state_impl_unittest.mm
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 16 2017 

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

commit 2feaa058530953a2232e6128e16deb31192ad9d2
Author: eugenebut <eugenebut@chromium.org>
Date: Thu Mar 16 16:55:17 2017

Cleaned up old navigation code that did not use pending navigation item.

Also removed PendingIndexNavigationDisabled experimental setting.

BUG= 692331 ,701717
TEST=Back forward navigation works correctly.

Review-Url: https://codereview.chromium.org/2751793002
Cr-Commit-Position: refs/heads/master@{#457459}

[modify] https://crrev.com/2feaa058530953a2232e6128e16deb31192ad9d2/ios/chrome/browser/experimental_flags.h
[modify] https://crrev.com/2feaa058530953a2232e6128e16deb31192ad9d2/ios/chrome/browser/experimental_flags.mm
[modify] https://crrev.com/2feaa058530953a2232e6128e16deb31192ad9d2/ios/chrome/browser/resources/Settings.bundle/Experimental.plist
[modify] https://crrev.com/2feaa058530953a2232e6128e16deb31192ad9d2/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/2feaa058530953a2232e6128e16deb31192ad9d2/ios/chrome/browser/web/visible_url_egtest.mm
[modify] https://crrev.com/2feaa058530953a2232e6128e16deb31192ad9d2/ios/web/interstitials/web_interstitial_impl.mm
[modify] https://crrev.com/2feaa058530953a2232e6128e16deb31192ad9d2/ios/web/public/web_state/ui/crw_web_delegate.h
[modify] https://crrev.com/2feaa058530953a2232e6128e16deb31192ad9d2/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/2feaa058530953a2232e6128e16deb31192ad9d2/ios/web/web_state/ui/crw_web_controller_unittest.mm

Comment 9 by eugene...@chromium.org, Mar 16 2017

Status: Fixed (was: Started)
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 16 2017 

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

commit ac76b4841c801b4dfe13d96a76e767d34593e63d
Author: rohitrao <rohitrao@chromium.org>
Date: Thu Mar 16 19:39:06 2017

Revert of Cleaned up old navigation code that did not use pending navigation item. (patchset #4 id:60001 of https://codereview.chromium.org/2751793002/ )

Reason for revert:
testReloadInterstitial is failing on an internal bot.  For an example, see build 4404 on the internal iphone10-simulator bot.

Original issue's description:
> Cleaned up old navigation code that did not use pending navigation item.
>
> Also removed PendingIndexNavigationDisabled experimental setting.
>
> BUG= 692331 ,701717
> TEST=Back forward navigation works correctly.
>
> Review-Url: https://codereview.chromium.org/2751793002
> Cr-Commit-Position: refs/heads/master@{#457459}
> Committed: https://chromium.googlesource.com/chromium/src/+/2feaa058530953a2232e6128e16deb31192ad9d2

TBR=kkhorimoto@chromium.org,eugenebut@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 692331 ,701717

Review-Url: https://codereview.chromium.org/2759483002
Cr-Commit-Position: refs/heads/master@{#457519}

[modify] https://crrev.com/ac76b4841c801b4dfe13d96a76e767d34593e63d/ios/chrome/browser/experimental_flags.h
[modify] https://crrev.com/ac76b4841c801b4dfe13d96a76e767d34593e63d/ios/chrome/browser/experimental_flags.mm
[modify] https://crrev.com/ac76b4841c801b4dfe13d96a76e767d34593e63d/ios/chrome/browser/resources/Settings.bundle/Experimental.plist
[modify] https://crrev.com/ac76b4841c801b4dfe13d96a76e767d34593e63d/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/ac76b4841c801b4dfe13d96a76e767d34593e63d/ios/chrome/browser/web/visible_url_egtest.mm
[modify] https://crrev.com/ac76b4841c801b4dfe13d96a76e767d34593e63d/ios/web/interstitials/web_interstitial_impl.mm
[modify] https://crrev.com/ac76b4841c801b4dfe13d96a76e767d34593e63d/ios/web/public/web_state/ui/crw_web_delegate.h
[modify] https://crrev.com/ac76b4841c801b4dfe13d96a76e767d34593e63d/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/ac76b4841c801b4dfe13d96a76e767d34593e63d/ios/web/web_state/ui/crw_web_controller_unittest.mm
Project Member

Comment 11 by bugdroid1@chromium.org, Mar 17 2017 

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

commit 3e87288e88257beea1727e67da8b69a77b7715a1
Author: eugenebut <eugenebut@chromium.org>
Date: Fri Mar 17 14:47:57 2017

Reland of "Cleaned up old navigation code that did not use pending navigation item."

Also removed PendingIndexNavigationDisabled experimental setting.

BUG= 692331 ,701717
TEST=Back forward navigation works correctly.

Original Review-Url: https://codereview.chromium.org/2751793002

Review-Url: https://codereview.chromium.org/2756753002
Cr-Commit-Position: refs/heads/master@{#457765}

[modify] https://crrev.com/3e87288e88257beea1727e67da8b69a77b7715a1/ios/chrome/browser/experimental_flags.h
[modify] https://crrev.com/3e87288e88257beea1727e67da8b69a77b7715a1/ios/chrome/browser/experimental_flags.mm
[modify] https://crrev.com/3e87288e88257beea1727e67da8b69a77b7715a1/ios/chrome/browser/resources/Settings.bundle/Experimental.plist
[modify] https://crrev.com/3e87288e88257beea1727e67da8b69a77b7715a1/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/3e87288e88257beea1727e67da8b69a77b7715a1/ios/chrome/browser/web/visible_url_egtest.mm
[modify] https://crrev.com/3e87288e88257beea1727e67da8b69a77b7715a1/ios/web/interstitials/web_interstitial_impl.mm
[modify] https://crrev.com/3e87288e88257beea1727e67da8b69a77b7715a1/ios/web/public/web_state/ui/crw_web_delegate.h
[modify] https://crrev.com/3e87288e88257beea1727e67da8b69a77b7715a1/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/3e87288e88257beea1727e67da8b69a77b7715a1/ios/web/web_state/ui/crw_web_controller_unittest.mm
►

Sign in to add a comment