Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2642303002:
PlzNavigate: Chrome UI changes for new methods of WebContentsObserver (Closed)
|
Created:
3 years, 11 months ago by shaktisahu Modified:
3 years, 10 months ago CC:
chromium-reviews, dominickn+watch_chromium.org, mdjones+watch_chromium.org, donnd+watch_chromium.org, jam, lizeb+watch-custom-tabs_chromium.org, pkotwicz+watch_chromium.org, dfalcantara+watch_chromium.org, darin-cc_chromium.org, zpeng+watch_chromium.org, agrieve+watch_chromium.org, android-webview-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Chrome UI changes for new methods of WebContentsObserver
This CL hooks up the new WebContentsObserver and TabObserver methods
to the clank front-end and removes the old methods.
API cheat sheet: https://docs.google.com/document/d/1HTLT1PI2LiS7rh29eW2OSh2zG-xqFEynv-T7vn39OLc
Content Navigation design doc: https://docs.google.com/document/d/1ICLLQoC9EsZ-bWH4ZKRhPCIoZKn6pOj02SlGl6SKH6Y
BUG=676139
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2642303002
Cr-Commit-Position: refs/heads/master@{#449562}
Committed: https://chromium.googlesource.com/chromium/src/+/53e1c483fe44e3544dbd872911c2914777e992e6
Patch Set 1 #Patch Set 2 #Patch Set 3 #Patch Set 4 : merge origin/master #Patch Set 5 : NavigationThrottle fix for returning correct error code #Patch Set 6 : AWCO change with didStopLoading #Patch Set 7 : AWCO fix for fragments #Patch Set 8 : AW fragment check #Patch Set 9 : rebase after PostMessageHandler #Patch Set 10 : Added HTTP status code to the param list #Patch Set 11 : AW fix #Patch Set 12 : isFragmentNavigation #
Total comments: 29
Patch Set 13 : some comments from Ted #
Total comments: 10
Patch Set 14 : boliu@ comments #Patch Set 15 : PreviousURL Fix #
Total comments: 30
Patch Set 16 : jam@ comments #
Total comments: 10
Patch Set 17 : comments #
Total comments: 1
Patch Set 18 : Added error description #Patch Set 19 : rebase origin/master #Patch Set 20 : errorDescription in WCOProxy #
Total comments: 3
Patch Set 21 : comments #Patch Set 22 : undo changes for error description #Messages
Total messages: 119 (85 generated)
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Description was changed from ========== PlzNavigate: Java changes for WebContentsObserver new methods BUG= ========== to ========== PlzNavigate: Chrome UI changes for new methods of WebContentsObserver This CL hooks up the new WebContentsObserver and TabObserver methods to the clank front-end and removes the old methods. API cheat sheet: https://docs.google.com/document/d/1HTLT1PI2LiS7rh29eW2OSh2zG-xqFEynv-T7vn39OLc Content Navigation design doc: https://docs.google.com/document/d/1ICLLQoC9EsZ-bWH4ZKRhPCIoZKn6pOj02SlGl6SKH6Y BUG=676139 ==========
Description was changed from ========== PlzNavigate: Chrome UI changes for new methods of WebContentsObserver This CL hooks up the new WebContentsObserver and TabObserver methods to the clank front-end and removes the old methods. API cheat sheet: https://docs.google.com/document/d/1HTLT1PI2LiS7rh29eW2OSh2zG-xqFEynv-T7vn39OLc Content Navigation design doc: https://docs.google.com/document/d/1ICLLQoC9EsZ-bWH4ZKRhPCIoZKn6pOj02SlGl6SKH6Y BUG=676139 ========== to ========== PlzNavigate: Chrome UI changes for new methods of WebContentsObserver This CL hooks up the new WebContentsObserver and TabObserver methods to the clank front-end and removes the old methods. API cheat sheet: https://docs.google.com/document/d/1HTLT1PI2LiS7rh29eW2OSh2zG-xqFEynv-T7vn39OLc Content Navigation design doc: https://docs.google.com/document/d/1ICLLQoC9EsZ-bWH4ZKRhPCIoZKn6pOj02SlGl6SKH6Y BUG=676139 ==========
shaktisahu@chromium.org changed reviewers: + tedchoc@chromium.org
tedchoc@ - PTAL
Description was changed from ========== PlzNavigate: Chrome UI changes for new methods of WebContentsObserver This CL hooks up the new WebContentsObserver and TabObserver methods to the clank front-end and removes the old methods. API cheat sheet: https://docs.google.com/document/d/1HTLT1PI2LiS7rh29eW2OSh2zG-xqFEynv-T7vn39OLc Content Navigation design doc: https://docs.google.com/document/d/1ICLLQoC9EsZ-bWH4ZKRhPCIoZKn6pOj02SlGl6SKH6Y BUG=676139 ========== to ========== PlzNavigate: Chrome UI changes for new methods of WebContentsObserver This CL hooks up the new WebContentsObserver and TabObserver methods to the clank front-end and removes the old methods. API cheat sheet: https://docs.google.com/document/d/1HTLT1PI2LiS7rh29eW2OSh2zG-xqFEynv-T7vn39OLc Content Navigation design doc: https://docs.google.com/document/d/1ICLLQoC9EsZ-bWH4ZKRhPCIoZKn6pOj02SlGl6SKH6Y BUG=676139 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by shaktisahu@chromium.org
The CQ bit was checked by shaktisahu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Description was changed from ========== PlzNavigate: Chrome UI changes for new methods of WebContentsObserver This CL hooks up the new WebContentsObserver and TabObserver methods to the clank front-end and removes the old methods. API cheat sheet: https://docs.google.com/document/d/1HTLT1PI2LiS7rh29eW2OSh2zG-xqFEynv-T7vn39OLc Content Navigation design doc: https://docs.google.com/document/d/1ICLLQoC9EsZ-bWH4ZKRhPCIoZKn6pOj02SlGl6SKH6Y BUG=676139 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Chrome UI changes for new methods of WebContentsObserver This CL hooks up the new WebContentsObserver and TabObserver methods to the clank front-end and removes the old methods. API cheat sheet: https://docs.google.com/document/d/1HTLT1PI2LiS7rh29eW2OSh2zG-xqFEynv-T7vn39OLc Content Navigation design doc: https://docs.google.com/document/d/1ICLLQoC9EsZ-bWH4ZKRhPCIoZKn6pOj02SlGl6SKH6Y BUG=676139 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
shaktisahu@chromium.org changed reviewers: - tedchoc@chromium.org
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by shaktisahu@chromium.org
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
shaktisahu@chromium.org changed reviewers: + tedchoc@chromium.org
https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:210: recordErrorInPolicyAuditor(url, "", errorCode); I didn't find a way to retrieve the error description. Mostly it is generated from ContentRendererClient::GetNavigationErrorStrings that requires a WebURLError to pass in which has more details like error domain etc. https://codereview.chromium.org/2642303002/diff/220001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2642303002/diff/220001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:152: urls_same_ignoring_fragment && navigation_handle->IsSamePage(); This is the only param so far that didn't come from NavigationHandle class which implies if we reorganize the params into a Java NavigationHandle class, this would still be needed to be passed separately. https://codereview.chromium.org/2642303002/diff/220001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:163: : 200); May be we need a better default value other than 200. I tried using 0, but for some tests (ConextualSearchManager#testTapContentVisibility and few more), it gets flaky when the device is running in offline mode. https://codereview.chromium.org/2642303002/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2642303002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:578: OnRequestFailed(false, net::ERR_ABORTED); Changes in this file will be replaced by fix of crbug.com/681029 https://codereview.chromium.org/2642303002/diff/220001/extensions/browser/ext... File extensions/browser/extension_navigation_throttle.cc (right): https://codereview.chromium.org/2642303002/diff/220001/extensions/browser/ext... extensions/browser/extension_navigation_throttle.cc:2: // Use of this source code is governed by a BSD-style license that can be Changes in this file will be replaced by fix of crbug.com/681029
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
overall this change looks quite good...I think it would be ok to start roping in the other reviewers you'll need for webview and the like https://codereview.chromium.org/2642303002/diff/220001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/220001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:95: didFailLoad(isInMainFrame, errorCode, "", url); do you need to return here? https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java (right): https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:476: if (!TextUtils.equals(url, DomDistillerUrlUtils.getOriginalUrlFromDistillerUrl( try to not mix style changes with code changes...will make blame harder to reason about. https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java (right): https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:97: setIsObscuredByOtherView(false); this can likely all fit on a single line https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:210: recordErrorInPolicyAuditor(url, "", errorCode); On 2017/01/31 01:57:53, shaktisahu wrote: > I didn't find a way to retrieve the error description. Mostly it is generated > from ContentRendererClient::GetNavigationErrorStrings that requires a > WebURLError to pass in which has more details like error domain etc. I wonder if we could just do net::ErrorToShortString here. I believe bauerb@ added this though, so they'll likely know what should be logged here. https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:211: } Should we return in the errorCode case? https://codereview.chromium.org/2642303002/diff/220001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2642303002/diff/220001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:113: DidFailLoadInternal(!render_frame_host->GetParent(), error_code, let's delete DidFailLoadInternal and inline it here as it should only be the one caller now right? https://codereview.chromium.org/2642303002/diff/220001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:141: CR_DEFINE_STATIC_LOCAL(GURL, last_committed_url, (GURL())); Hmm...I'd ask clamy@ or jam@ whether there is a way to get this information from NavigationHandle (likely transitively). I'm curious if we look at how the previous LoadCommittedDetails was being generated to see if we can pull the previous_url here in the same way. https://codereview.chromium.org/2642303002/diff/220001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:152: urls_same_ignoring_fragment && navigation_handle->IsSamePage(); On 2017/01/31 01:57:53, shaktisahu wrote: > This is the only param so far that didn't come from NavigationHandle class which > implies if we reorganize the params into a Java NavigationHandle class, this > would still be needed to be passed separately. As far as I can tell, this seems to only be used in WebView. If needed, I wonder if we could push this caching to them. https://codereview.chromium.org/2642303002/diff/220001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:163: : 200); On 2017/01/31 01:57:53, shaktisahu wrote: > May be we need a better default value other than 200. I tried using 0, but for > some tests (ConextualSearchManager#testTapContentVisibility and few more), it > gets flaky when the device is running in offline mode. I think we likely want a unset value here much like -1/null for page transition. These are only valid if the page has committed right? I do think 200 implies success when it is not appropriate. How do the contextual search tests fail? https://codereview.chromium.org/2642303002/diff/220001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2642303002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:138: determinedProcessVisibility(); should we do hasCommitted || errorCode != 0 to keep the logic the same as before for this?
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
boliu@ - PTAL at android_webview/* https://codereview.chromium.org/2642303002/diff/220001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/220001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:95: didFailLoad(isInMainFrame, errorCode, "", url); On 2017/01/31 18:57:47, Ted C wrote: > do you need to return here? Done. https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java (right): https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:476: if (!TextUtils.equals(url, DomDistillerUrlUtils.getOriginalUrlFromDistillerUrl( On 2017/01/31 18:57:47, Ted C wrote: > try to not mix style changes with code changes...will make blame harder to > reason about. Done. It was unexpected, due to git cl format. https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:211: } On 2017/01/31 18:57:47, Ted C wrote: > Should we return in the errorCode case? Done. Yes. In that case I would move the block having observers.next().onDidFinishNavigation() before this block as tab observers still need to be notified. https://codereview.chromium.org/2642303002/diff/220001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2642303002/diff/220001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:113: DidFailLoadInternal(!render_frame_host->GetParent(), error_code, On 2017/01/31 18:57:48, Ted C wrote: > let's delete DidFailLoadInternal and inline it here as it should only be the one > caller now right? Done. https://codereview.chromium.org/2642303002/diff/220001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:141: CR_DEFINE_STATIC_LOCAL(GURL, last_committed_url, (GURL())); On 2017/01/31 18:57:48, Ted C wrote: > Hmm...I'd ask clamy@ or jam@ whether there is a way to get this information from > NavigationHandle (likely transitively). I'm curious if we look at how the > previous LoadCommittedDetails was being generated to see if we can pull the > previous_url here in the same way. I checked with jam@ and they will be adding a getter in NavigationHandle to get the previous URL. I will wait for that change. https://codereview.chromium.org/2642303002/diff/220001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:163: : 200); On 2017/01/31 18:57:48, Ted C wrote: > On 2017/01/31 01:57:53, shaktisahu wrote: > > May be we need a better default value other than 200. I tried using 0, but for > > some tests (ConextualSearchManager#testTapContentVisibility and few more), it > > gets flaky when the device is running in offline mode. > > I think we likely want a unset value here much like -1/null for page transition. > These are only valid if the page has committed right? > > I do think 200 implies success when it is not appropriate. How do the > contextual search tests fail? The test fails only for Nexus 5 whenever the device goes offline. I think the contextual search tests depend on the internet connectivity, which causes this flakiness. Prior to this CL, in didNavigateMainFrame, even though an error occurs, we would still get 200 since the error page loads successfully. But now since we switched to didFinishNavigation, that is not the case. If an error occurs, then navigation_handle->GetResponseHeaders() will return null. It doesn't fail all the time though in offline mode. I think I should make this default value to 200 and file a bug to fix those 5 test cases. Let me know. https://codereview.chromium.org/2642303002/diff/220001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2642303002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:138: determinedProcessVisibility(); On 2017/01/31 18:57:48, Ted C wrote: > should we do hasCommitted || errorCode != 0 to keep the logic the same as before > for this? From the comments, I think determinedProcessVisibility must to be called for once for every navigation no matter if it fails or succeeds (I might be mistaken though). In that case we wouldn't need to check as didFinishNavigation is guaranteed to be called exactly once.
shaktisahu@chromium.org changed reviewers: + boliu@chromium.org
+boliu@
+boliu@
+boliu@
+boliu@
you should get a navigatio owner to review the non-android parts of content also.. why is the CC list empty? https://codereview.chromium.org/2642303002/diff/240001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/240001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:95: didFailLoad(isInMainFrame, errorCode, "", url); under what circumstances will errorCode be non-zero, but didFailLoad would not be called? (I'm wonder if didFailLoad could be called twice) https://codereview.chromium.org/2642303002/diff/240001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:107: nit: could early out if (!isInMainFrame) here https://codereview.chromium.org/2642303002/diff/240001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwWebContentsObserverTest.java (right): https://codereview.chromium.org/2642303002/diff/240001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwWebContentsObserverTest.java:24: private static final String EXAMPLE_URL_WITH_HASH = "http://www.example.com/#anchor"; _WITH_FRAGMENT? https://codereview.chromium.org/2642303002/diff/240001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2642303002/diff/240001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:149: CR_DEFINE_STATIC_LOCAL(GURL, last_committed_url, (GURL())); why static?
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
shaktisahu@chromium.org changed reviewers: + jam@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! just some comments to maintain the same behavior as before https://codereview.chromium.org/2642303002/diff/280001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/280001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:95: didFailLoad(isInMainFrame, errorCode, "", url); i'm not sure I follow why this change is made? didFailLoad is still called by the C++ side (it's not deprecated) https://codereview.chromium.org/2642303002/diff/280001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:99: if (hasCommitted) mCommittedNavigation = true; to maintain the same behavior as before, this line should be if (!hasCommitted) return; mCommittedNavigation = true; https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ActivityTabTaskDescriptionHelper.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ActivityTabTaskDescriptionHelper.java:96: if (isSamePage) return; to maintain the same behavior, you also need to check hasCommitted and isInMainFrame https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:304: if (isInMainFrame) { same comment ToolbarManager.java, I think yo ualso want to check !isSamePage to maintain same behavior as before https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:88: if (mNavigatedOnce && isInMainFrame && !isSamePage && mChannel != null) { also hasCommitted to maintain the same behavior https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:444: if (!isInMainFrame) return; same comment ToolbarManager.java, I think yo ualso want to check !isSamePage to maintain same behavior as before https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabStateBrowserControlsVisibilityDelegate.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabStateBrowserControlsVisibilityDelegate.java:90: if (!isInMainFrame) return; i think you also want hasCommitted to maintain the same behavior https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:211: if (errorCode != 0) { where is this block coming from? https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:427: // Update URL as soon as it becomes available when it's a new tab. the old code was triggered by WCO::DidStartNavigationToPendingEntry which is only called for main frame navigations. so this should be moved after the !isInMainFrame check? https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:437: // This event is used as the primary trigger for the progress bar because it to maintain same behavior as before, we need to early out if isSamePage() is true (since WCO::DidStartProvisionalLoadForFrame isn't called for same page navigations) https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:386: if (isInMainFrame) updateUrlBar(); same comment ToolbarManager.java, I think yo ualso want to check !isSamePage to maintain same behavior as before https://codereview.chromium.org/2642303002/diff/280001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2642303002/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:140: if (!isInMainFrame || isSamePage) return; also: || !hasCommitted to maintain the same behavior
jam@ - A few questions about the behavior of the functions. See my replies. https://codereview.chromium.org/2642303002/diff/240001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/240001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:107: On 2017/02/06 15:16:34, boliu wrote: > nit: could early out if (!isInMainFrame) here Done. https://codereview.chromium.org/2642303002/diff/240001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwWebContentsObserverTest.java (right): https://codereview.chromium.org/2642303002/diff/240001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwWebContentsObserverTest.java:24: private static final String EXAMPLE_URL_WITH_HASH = "http://www.example.com/#anchor"; On 2017/02/06 15:16:34, boliu wrote: > _WITH_FRAGMENT? Done. https://codereview.chromium.org/2642303002/diff/240001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2642303002/diff/240001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:149: CR_DEFINE_STATIC_LOCAL(GURL, last_committed_url, (GURL())); On 2017/02/06 15:16:34, boliu wrote: > why static? I wanted to capture the value of the URL last time this function was called. But any way, I am removing this variable as navigation_handle->GetPreviousURL() has been added as a better way of handling this. https://codereview.chromium.org/2642303002/diff/280001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/280001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:95: didFailLoad(isInMainFrame, errorCode, "", url); On 2017/02/07 05:10:32, jam wrote: > i'm not sure I follow why this change is made? didFailLoad is still called by > the C++ side (it's not deprecated) This is to account for the fact that DidFailProvisionalLoad was deprecated and was replaced by DidFinishNavigation. In the eariler WCOProxy, both DidFailProvisionalLoad and DidFailLoad were piped to didFailLoad. I have changed it in this CL. So didFailLoad still needs to be called also from didFinishNavigation. https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:211: if (errorCode != 0) { On 2017/02/07 05:10:32, jam wrote: > where is this block coming from? Same comment as AndroidWebContentsObserver.java https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:427: // Update URL as soon as it becomes available when it's a new tab. On 2017/02/07 05:10:32, jam wrote: > the old code was triggered by WCO::DidStartNavigationToPendingEntry which is > only called for main frame navigations. so this should be moved after the > !isInMainFrame check? Oh, I thought it is called for all frames. If you look into NavigatorImpl::NavigateToEntry, it seems so since it has some checks for IsMainFrame, but I might be incorrect. https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_imp... https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:437: // This event is used as the primary trigger for the progress bar because it On 2017/02/07 05:10:32, jam wrote: > to maintain same behavior as before, we need to early out if isSamePage() is > true (since WCO::DidStartProvisionalLoadForFrame isn't called for same page > navigations) Okay, didn't know that, again not obvious from the code :). I will add isSamePage() here and other call sites.
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:210: recordErrorInPolicyAuditor(url, "", errorCode); On 2017/01/31 18:57:48, Ted C wrote: > On 2017/01/31 01:57:53, shaktisahu wrote: > > I didn't find a way to retrieve the error description. Mostly it is generated > > from ContentRendererClient::GetNavigationErrorStrings that requires a > > WebURLError to pass in which has more details like error domain etc. > > I wonder if we could just do net::ErrorToShortString here. I believe bauerb@ > added this though, so they'll likely know what should be logged here. I think error_page::LocalizedError::GetErrorDetails() with a domain of net::kErrorDomain should work.
https://codereview.chromium.org/2642303002/diff/280001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/280001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:95: didFailLoad(isInMainFrame, errorCode, "", url); On 2017/02/07 07:35:51, shaktisahu wrote: > On 2017/02/07 05:10:32, jam wrote: > > i'm not sure I follow why this change is made? didFailLoad is still called by > > the C++ side (it's not deprecated) > > This is to account for the fact that DidFailProvisionalLoad was deprecated and > was replaced by DidFinishNavigation. In the eariler WCOProxy, both > DidFailProvisionalLoad and DidFailLoad were piped to didFailLoad. I have changed > it in this CL. So didFailLoad still needs to be called also from > didFinishNavigation. ah, ok thanks for the explanation, I missed that. https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:427: // Update URL as soon as it becomes available when it's a new tab. On 2017/02/07 07:35:51, shaktisahu wrote: > On 2017/02/07 05:10:32, jam wrote: > > the old code was triggered by WCO::DidStartNavigationToPendingEntry which is > > only called for main frame navigations. so this should be moved after the > > !isInMainFrame check? > > Oh, I thought it is called for all frames. If you look into > NavigatorImpl::NavigateToEntry, it seems so since it has some checks for > IsMainFrame, but I might be incorrect. > https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_imp... for current behavior (i.e. without site isolation/out of process iframes), it's only called for main frame navigations. https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:437: // This event is used as the primary trigger for the progress bar because it On 2017/02/07 07:35:51, shaktisahu wrote: > On 2017/02/07 05:10:32, jam wrote: > > to maintain same behavior as before, we need to early out if isSamePage() is > > true (since WCO::DidStartProvisionalLoadForFrame isn't called for same page > > navigations) > > Okay, didn't know that, again not obvious from the code :). I will add > isSamePage() here and other call sites. yeah it's definitely not obvious or consistent from the other methods! that's another reason why the new callbacks are better :)
https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:210: recordErrorInPolicyAuditor(url, "", errorCode); On 2017/02/07 13:19:16, Bernhard Bauer wrote: > On 2017/01/31 18:57:48, Ted C wrote: > > On 2017/01/31 01:57:53, shaktisahu wrote: > > > I didn't find a way to retrieve the error description. Mostly it is > generated > > > from ContentRendererClient::GetNavigationErrorStrings that requires a > > > WebURLError to pass in which has more details like error domain etc. > > > > I wonder if we could just do net::ErrorToShortString here. I believe bauerb@ > > added this though, so they'll likely know what should be logged here. > > I think error_page::LocalizedError::GetErrorDetails() with a domain of > net::kErrorDomain should work. Hmm... but error_page is in components and we may not be able to use it here.
more corner case questions from me, since I don't know navigation stuff all that well.. https://codereview.chromium.org/2642303002/diff/300001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2642303002/diff/300001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:151: if (navigation_handle->HasCommitted()) { is it safe to skip this check if HasCommitted is not true? https://codereview.chromium.org/2642303002/diff/300001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2642303002/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:140: if (hasCommitted && isInMainFrame && !isSamePage) { is the hasCommitted check necessary? if this is skipped due to hasCommitted, do we need to listen to load events then?
https://codereview.chromium.org/2642303002/diff/240001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/240001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:95: didFailLoad(isInMainFrame, errorCode, "", url); On 2017/02/06 15:16:34, boliu wrote: > under what circumstances will errorCode be non-zero, but didFailLoad would not > be called? (I'm wonder if didFailLoad could be called twice) I believe this is orthogonal. If this is failed here (i.e. provisional load has failed), the didFailLoad will not be called. https://codereview.chromium.org/2642303002/diff/300001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2642303002/diff/300001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:151: if (navigation_handle->HasCommitted()) { On 2017/02/07 18:38:18, boliu wrote: > is it safe to skip this check if HasCommitted is not true? Yes, if the HasCommitted is false, then no successful navigation happened, not even a successful error page. User was left in the previous page. So in that case, we don't need to worry about if it was for a fragment navigation. https://codereview.chromium.org/2642303002/diff/300001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2642303002/diff/300001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:140: if (hasCommitted && isInMainFrame && !isSamePage) { On 2017/02/07 18:38:18, boliu wrote: > is the hasCommitted check necessary? if this is skipped due to hasCommitted, do > we need to listen to load events then? Yes, hasCommitted is true for successful commits or error pages that replace the previous page. It can be false for downloads (since download URL needn't be committed to history), and HTTP 204/205 errors. So it depends on the usage, and in this case, it makes sense to skip the next line if hasCommitted is false.
https://codereview.chromium.org/2642303002/diff/240001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/240001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:95: didFailLoad(isInMainFrame, errorCode, "", url); On 2017/02/07 20:09:49, shaktisahu wrote: > On 2017/02/06 15:16:34, boliu wrote: > > under what circumstances will errorCode be non-zero, but didFailLoad would not > > be called? (I'm wonder if didFailLoad could be called twice) > > I believe this is orthogonal. Can you double check to make sure then? Changing callback behavior here has potential to break apps. > If this is failed here (i.e. provisional load has > failed), the didFailLoad will not be called. https://codereview.chromium.org/2642303002/diff/300001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2642303002/diff/300001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:151: if (navigation_handle->HasCommitted()) { On 2017/02/07 20:09:49, shaktisahu wrote: > On 2017/02/07 18:38:18, boliu wrote: > > is it safe to skip this check if HasCommitted is not true? > > Yes, if the HasCommitted is false, then no successful navigation happened, not > even a successful error page. User was left in the previous page. So in that > case, we don't need to worry about if it was for a fragment navigation. What does IsSamePage return in that case? Always true because navigation didn't happen, or depends on where the intended destimation of failed navigation? Whatever the case, probably worth spelling it out in the documentation on WebContentsObserver
https://codereview.chromium.org/2642303002/diff/240001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/240001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:95: didFailLoad(isInMainFrame, errorCode, "", url); On 2017/02/07 22:08:23, boliu wrote: > On 2017/02/07 20:09:49, shaktisahu wrote: > > On 2017/02/06 15:16:34, boliu wrote: > > > under what circumstances will errorCode be non-zero, but didFailLoad would > not > > > be called? (I'm wonder if didFailLoad could be called twice) > > > > I believe this is orthogonal. > > Can you double check to make sure then? Changing callback behavior here has > potential to break apps. > > > If this is failed here (i.e. provisional load has > > failed), the didFailLoad will not be called. > I double checked. This is not called twice. https://codereview.chromium.org/2642303002/diff/300001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2642303002/diff/300001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:151: if (navigation_handle->HasCommitted()) { On 2017/02/07 22:08:23, boliu wrote: > On 2017/02/07 20:09:49, shaktisahu wrote: > > On 2017/02/07 18:38:18, boliu wrote: > > > is it safe to skip this check if HasCommitted is not true? > > > > Yes, if the HasCommitted is false, then no successful navigation happened, not > > even a successful error page. User was left in the previous page. So in that > > case, we don't need to worry about if it was for a fragment navigation. > > What does IsSamePage return in that case? Always true because navigation didn't > happen, or depends on where the intended destimation of failed navigation? > > Whatever the case, probably worth spelling it out in the documentation on > WebContentsObserver IsSamePage is set to false for the navigations that don't commit.
my parts lgtm, but wait for approval from jam@ https://codereview.chromium.org/2642303002/diff/300001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2642303002/diff/300001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:151: if (navigation_handle->HasCommitted()) { On 2017/02/08 00:05:07, shaktisahu wrote: > On 2017/02/07 22:08:23, boliu wrote: > > On 2017/02/07 20:09:49, shaktisahu wrote: > > > On 2017/02/07 18:38:18, boliu wrote: > > > > is it safe to skip this check if HasCommitted is not true? > > > > > > Yes, if the HasCommitted is false, then no successful navigation happened, > not > > > even a successful error page. User was left in the previous page. So in that > > > case, we don't need to worry about if it was for a fragment navigation. > > > > What does IsSamePage return in that case? Always true because navigation > didn't > > happen, or depends on where the intended destimation of failed navigation? > > > > Whatever the case, probably worth spelling it out in the documentation on > > WebContentsObserver > > IsSamePage is set to false for the navigations that don't commit. I'm guessing NetError and response code are gonna be "normal", or somewhat meaningless as well? Please put that in a comment on WebContentsObserver.java https://codereview.chromium.org/2642303002/diff/300001/extensions/browser/ext... File extensions/browser/extension_navigation_throttle.cc (right): https://codereview.chromium.org/2642303002/diff/300001/extensions/browser/ext... extensions/browser/extension_navigation_throttle.cc:93: return content::NavigationThrottle::BLOCK_REQUEST; is this change relevant to this CL? android doesn't support extensions afaik?
shaktisahu@chromium.org changed reviewers: + dfalcantara@chromium.org
jam@, tedchoc@, dfalcantara@ - PTAL https://codereview.chromium.org/2642303002/diff/300001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2642303002/diff/300001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:151: if (navigation_handle->HasCommitted()) { On 2017/02/08 00:10:38, boliu wrote: > On 2017/02/08 00:05:07, shaktisahu wrote: > > On 2017/02/07 22:08:23, boliu wrote: > > > On 2017/02/07 20:09:49, shaktisahu wrote: > > > > On 2017/02/07 18:38:18, boliu wrote: > > > > > is it safe to skip this check if HasCommitted is not true? > > > > > > > > Yes, if the HasCommitted is false, then no successful navigation happened, > > not > > > > even a successful error page. User was left in the previous page. So in > that > > > > case, we don't need to worry about if it was for a fragment navigation. > > > > > > What does IsSamePage return in that case? Always true because navigation > > didn't > > > happen, or depends on where the intended destimation of failed navigation? > > > > > > Whatever the case, probably worth spelling it out in the documentation on > > > WebContentsObserver > > > > IsSamePage is set to false for the navigations that don't commit. > > I'm guessing NetError and response code are gonna be "normal", or somewhat > meaningless as well? Please put that in a comment on WebContentsObserver.java Done. NetError will still have the meaningful error code obtained from the net layer. https://codereview.chromium.org/2642303002/diff/300001/extensions/browser/ext... File extensions/browser/extension_navigation_throttle.cc (right): https://codereview.chromium.org/2642303002/diff/300001/extensions/browser/ext... extensions/browser/extension_navigation_throttle.cc:93: return content::NavigationThrottle::BLOCK_REQUEST; On 2017/02/08 00:10:38, boliu wrote: > is this change relevant to this CL? android doesn't support extensions afaik? I will remove this before commit. This is a bug fix from https://codereview.chromium.org/2657813004/ which was required to pass the CQ dry run.
https://codereview.chromium.org/2642303002/diff/280001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/280001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java:99: if (hasCommitted) mCommittedNavigation = true; On 2017/02/07 05:10:32, jam wrote: > to maintain the same behavior as before, this line should be > > if (!hasCommitted) > return; > > mCommittedNavigation = true; Done. https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ActivityTabTaskDescriptionHelper.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ActivityTabTaskDescriptionHelper.java:96: if (isSamePage) return; On 2017/02/07 05:10:32, jam wrote: > to maintain the same behavior, you also need to check hasCommitted and > isInMainFrame Done. https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:304: if (isInMainFrame) { On 2017/02/07 05:10:32, jam wrote: > same comment ToolbarManager.java, I think yo ualso want to check !isSamePage to > maintain same behavior as before Done. https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:88: if (mNavigatedOnce && isInMainFrame && !isSamePage && mChannel != null) { On 2017/02/07 05:10:32, jam wrote: > also hasCommitted to maintain the same behavior Done. https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:444: if (!isInMainFrame) return; On 2017/02/07 05:10:32, jam wrote: > same comment ToolbarManager.java, I think yo ualso want to check !isSamePage to > maintain same behavior as before Done. https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabStateBrowserControlsVisibilityDelegate.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabStateBrowserControlsVisibilityDelegate.java:90: if (!isInMainFrame) return; On 2017/02/07 05:10:32, jam wrote: > i think you also want hasCommitted to maintain the same behavior Done. https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:211: if (errorCode != 0) { On 2017/02/07 07:35:51, shaktisahu wrote: > On 2017/02/07 05:10:32, jam wrote: > > where is this block coming from? > > Same comment as AndroidWebContentsObserver.java Done. https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:427: // Update URL as soon as it becomes available when it's a new tab. On 2017/02/07 17:12:38, jam wrote: > On 2017/02/07 07:35:51, shaktisahu wrote: > > On 2017/02/07 05:10:32, jam wrote: > > > the old code was triggered by WCO::DidStartNavigationToPendingEntry which is > > > only called for main frame navigations. so this should be moved after the > > > !isInMainFrame check? > > > > Oh, I thought it is called for all frames. If you look into > > NavigatorImpl::NavigateToEntry, it seems so since it has some checks for > > IsMainFrame, but I might be incorrect. > > > https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_imp... > > for current behavior (i.e. without site isolation/out of process iframes), it's > only called for main frame navigations. Done. https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:437: // This event is used as the primary trigger for the progress bar because it On 2017/02/07 17:12:38, jam wrote: > On 2017/02/07 07:35:51, shaktisahu wrote: > > On 2017/02/07 05:10:32, jam wrote: > > > to maintain same behavior as before, we need to early out if isSamePage() is > > > true (since WCO::DidStartProvisionalLoadForFrame isn't called for same page > > > navigations) > > > > Okay, didn't know that, again not obvious from the code :). I will add > > isSamePage() here and other call sites. > > yeah it's definitely not obvious or consistent from the other methods! that's > another reason why the new callbacks are better :) Done. https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2642303002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:386: if (isInMainFrame) updateUrlBar(); On 2017/02/07 05:10:32, jam wrote: > same comment ToolbarManager.java, I think yo ualso want to check !isSamePage to > maintain same behavior as before Done. https://codereview.chromium.org/2642303002/diff/280001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2642303002/diff/280001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:140: if (!isInMainFrame || isSamePage) return; On 2017/02/07 05:10:32, jam wrote: > also: || !hasCommitted > to maintain the same behavior Done.
https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:210: recordErrorInPolicyAuditor(url, "", errorCode); On 2017/02/07 18:08:22, shaktisahu wrote: > On 2017/02/07 13:19:16, Bernhard Bauer wrote: > > On 2017/01/31 18:57:48, Ted C wrote: > > > On 2017/01/31 01:57:53, shaktisahu wrote: > > > > I didn't find a way to retrieve the error description. Mostly it is > > generated > > > > from ContentRendererClient::GetNavigationErrorStrings that requires a > > > > WebURLError to pass in which has more details like error domain etc. > > > > > > I wonder if we could just do net::ErrorToShortString here. I believe > bauerb@ > > > added this though, so they'll likely know what should be logged here. > > > > I think error_page::LocalizedError::GetErrorDetails() with a domain of > > net::kErrorDomain should work. > > Hmm... but error_page is in components and we may not be able to use it here. Why wouldn't we able to use it here? We're in chrome/ so we have access to all of the components? Is it just because we haven't expose GetErrorDetails to java? If that is the case, we could do a few different things. 1.) we could add a new param to didFinishNavigation that is errorDetails that is populated when errorCode != 0. This is somewhat wasteful as only one call site needs this, but it goes through the observer proxy so we're only paying the jni cost once. But no place in webview needs it, so it is a relatively chrome specific need in terms of embedder. 2.) Put a android utility class in components/error_page that allows this to call into it. 3.) Put a native method "somewhere" in the tab directory that allows you do get this (easiest solution but probably least clean as you have an odd dangling method somewhere...but I'd rather do this now with a TODO to find a better home than to have a TODO/ship a build to stable where this isn't working anymore...unless bauerb@ says it is ok to have broken for a while). https://codereview.chromium.org/2642303002/diff/220001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2642303002/diff/220001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:163: : 200); On 2017/02/03 06:56:17, shaktisahu wrote: > On 2017/01/31 18:57:48, Ted C wrote: > > On 2017/01/31 01:57:53, shaktisahu wrote: > > > May be we need a better default value other than 200. I tried using 0, but > for > > > some tests (ConextualSearchManager#testTapContentVisibility and few more), > it > > > gets flaky when the device is running in offline mode. > > > > I think we likely want a unset value here much like -1/null for page > transition. > > These are only valid if the page has committed right? > > > > I do think 200 implies success when it is not appropriate. How do the > > contextual search tests fail? > > The test fails only for Nexus 5 whenever the device goes offline. I think the > contextual search tests depend on the internet connectivity, which causes this > flakiness. > Prior to this CL, in didNavigateMainFrame, even though an error occurs, we would > still get 200 since the error page loads successfully. > But now since we switched to didFinishNavigation, that is not the case. If an > error occurs, then navigation_handle->GetResponseHeaders() will return null. It > doesn't fail all the time though in offline mode. > I think I should make this default value to 200 and file a bug to fix those 5 > test cases. Let me know. File a bug, and put a TODO above this line with a reference to the bug number. https://codereview.chromium.org/2642303002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java (right): https://codereview.chromium.org/2642303002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:466: if (!hasCommitted && isInMainFrame && !hasPendingNonNtpNavigation(tab)) { should this be && errorCode != 0 to match the previous didFailLoad behavior?
https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:210: recordErrorInPolicyAuditor(url, "", errorCode); On 2017/02/08 05:31:15, Ted C wrote: > On 2017/02/07 18:08:22, shaktisahu wrote: > > On 2017/02/07 13:19:16, Bernhard Bauer wrote: > > > On 2017/01/31 18:57:48, Ted C wrote: > > > > On 2017/01/31 01:57:53, shaktisahu wrote: > > > > > I didn't find a way to retrieve the error description. Mostly it is > > > generated > > > > > from ContentRendererClient::GetNavigationErrorStrings that requires a > > > > > WebURLError to pass in which has more details like error domain etc. > > > > > > > > I wonder if we could just do net::ErrorToShortString here. I believe > > bauerb@ > > > > added this though, so they'll likely know what should be logged here. > > > > > > I think error_page::LocalizedError::GetErrorDetails() with a domain of > > > net::kErrorDomain should work. > > > > Hmm... but error_page is in components and we may not be able to use it here. > > Why wouldn't we able to use it here? We're in chrome/ so we have access to > all of the components? Is it just because we haven't expose GetErrorDetails > to java? > > If that is the case, we could do a few different things. > 1.) we could add a new param to didFinishNavigation that is errorDetails that is > populated when errorCode != 0. This is somewhat wasteful as only one call > site needs this, but it goes through the observer proxy so we're only paying > the jni cost once. But no place in webview needs it, so it is a relatively > chrome > specific need in terms of embedder. > 2.) Put a android utility class in components/error_page that allows this to > call > into it. > 3.) Put a native method "somewhere" in the tab directory that allows you do > get this (easiest solution but probably least clean as you have an odd dangling > method somewhere...but I'd rather do this now with a TODO to find a better > home than to have a TODO/ship a build to stable where this isn't working > anymore...unless bauerb@ says it is ok to have broken for a while). Oh, I was trying to add this to web_contents_observer_proxy.cc which lives in contents/, so components/error_page cannot be used there. This would rule out option 1. Since chrome is the only place where this is being used, I can go with option 3 for now with a TODO to file a better place later.
lgtm
https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:210: recordErrorInPolicyAuditor(url, "", errorCode); On 2017/02/08 06:32:10, shaktisahu wrote: > On 2017/02/08 05:31:15, Ted C wrote: > > On 2017/02/07 18:08:22, shaktisahu wrote: > > > On 2017/02/07 13:19:16, Bernhard Bauer wrote: > > > > On 2017/01/31 18:57:48, Ted C wrote: > > > > > On 2017/01/31 01:57:53, shaktisahu wrote: > > > > > > I didn't find a way to retrieve the error description. Mostly it is > > > > generated > > > > > > from ContentRendererClient::GetNavigationErrorStrings that requires a > > > > > > WebURLError to pass in which has more details like error domain etc. > > > > > > > > > > I wonder if we could just do net::ErrorToShortString here. I believe > > > bauerb@ > > > > > added this though, so they'll likely know what should be logged here. > > > > > > > > I think error_page::LocalizedError::GetErrorDetails() with a domain of > > > > net::kErrorDomain should work. > > > > > > Hmm... but error_page is in components and we may not be able to use it > here. > > > > Why wouldn't we able to use it here? We're in chrome/ so we have access to > > all of the components? Is it just because we haven't expose GetErrorDetails > > to java? > > > > If that is the case, we could do a few different things. > > 1.) we could add a new param to didFinishNavigation that is errorDetails that > is > > populated when errorCode != 0. This is somewhat wasteful as only one call > > site needs this, but it goes through the observer proxy so we're only paying > > the jni cost once. But no place in webview needs it, so it is a relatively > > chrome > > specific need in terms of embedder. > > 2.) Put a android utility class in components/error_page that allows this to > > call > > into it. > > 3.) Put a native method "somewhere" in the tab directory that allows you do > > get this (easiest solution but probably least clean as you have an odd > dangling > > method somewhere...but I'd rather do this now with a TODO to find a better > > home than to have a TODO/ship a build to stable where this isn't working > > anymore...unless bauerb@ says it is ok to have broken for a while). > > Oh, I was trying to add this to web_contents_observer_proxy.cc which lives in > contents/, so components/error_page cannot be used there. This would rule out > option 1. > > Since chrome is the only place where this is being used, I can go with option 3 > for now with a TODO to file a better place later. That sounds good to me, thanks!
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm
lgtm w/ one final comment https://codereview.chromium.org/2642303002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java (right): https://codereview.chromium.org/2642303002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:466: if (!hasCommitted && isInMainFrame && !hasPendingNonNtpNavigation(tab)) { I had a comment in an early patch set that I think this needs errorCode != 0 to be consistent with the old behavior of being triggered only in didFailLoad
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, jam@chromium.org, tedchoc@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2642303002/#ps420001 (title: "undo changes for error description")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1486712232823350,
"parent_rev": "59f7c65c351f8fd43229710e65413df8f3da7c72", "commit_rev":
"53e1c483fe44e3544dbd872911c2914777e992e6"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Chrome UI changes for new methods of WebContentsObserver This CL hooks up the new WebContentsObserver and TabObserver methods to the clank front-end and removes the old methods. API cheat sheet: https://docs.google.com/document/d/1HTLT1PI2LiS7rh29eW2OSh2zG-xqFEynv-T7vn39OLc Content Navigation design doc: https://docs.google.com/document/d/1ICLLQoC9EsZ-bWH4ZKRhPCIoZKn6pOj02SlGl6SKH6Y BUG=676139 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Chrome UI changes for new methods of WebContentsObserver This CL hooks up the new WebContentsObserver and TabObserver methods to the clank front-end and removes the old methods. API cheat sheet: https://docs.google.com/document/d/1HTLT1PI2LiS7rh29eW2OSh2zG-xqFEynv-T7vn39OLc Content Navigation design doc: https://docs.google.com/document/d/1ICLLQoC9EsZ-bWH4ZKRhPCIoZKn6pOj02SlGl6SKH6Y BUG=676139 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2642303002 Cr-Commit-Position: refs/heads/master@{#449562} Committed: https://chromium.googlesource.com/chromium/src/+/53e1c483fe44e3544dbd872911c2... ==========
Message was sent while issue was closed.
Committed patchset #22 (id:420001) as https://chromium.googlesource.com/chromium/src/+/53e1c483fe44e3544dbd872911c2...
Message was sent while issue was closed.
https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2642303002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:210: recordErrorInPolicyAuditor(url, "", errorCode); On 2017/02/08 10:26:38, Bernhard Bauer wrote: > On 2017/02/08 06:32:10, shaktisahu wrote: > > On 2017/02/08 05:31:15, Ted C wrote: > > > On 2017/02/07 18:08:22, shaktisahu wrote: > > > > On 2017/02/07 13:19:16, Bernhard Bauer wrote: > > > > > On 2017/01/31 18:57:48, Ted C wrote: > > > > > > On 2017/01/31 01:57:53, shaktisahu wrote: > > > > > > > I didn't find a way to retrieve the error description. Mostly it is > > > > > generated > > > > > > > from ContentRendererClient::GetNavigationErrorStrings that requires > a > > > > > > > WebURLError to pass in which has more details like error domain etc. > > > > > > > > > > > > I wonder if we could just do net::ErrorToShortString here. I believe > > > > bauerb@ > > > > > > added this though, so they'll likely know what should be logged here. > > > > > > > > > > I think error_page::LocalizedError::GetErrorDetails() with a domain of > > > > > net::kErrorDomain should work. > > > > > > > > Hmm... but error_page is in components and we may not be able to use it > > here. > > > > > > Why wouldn't we able to use it here? We're in chrome/ so we have access to > > > all of the components? Is it just because we haven't expose GetErrorDetails > > > to java? > > > > > > If that is the case, we could do a few different things. > > > 1.) we could add a new param to didFinishNavigation that is errorDetails > that > > is > > > populated when errorCode != 0. This is somewhat wasteful as only one call > > > site needs this, but it goes through the observer proxy so we're only paying > > > the jni cost once. But no place in webview needs it, so it is a relatively > > > chrome > > > specific need in terms of embedder. > > > 2.) Put a android utility class in components/error_page that allows this to > > > call > > > into it. > > > 3.) Put a native method "somewhere" in the tab directory that allows you do > > > get this (easiest solution but probably least clean as you have an odd > > dangling > > > method somewhere...but I'd rather do this now with a TODO to find a better > > > home than to have a TODO/ship a build to stable where this isn't working > > > anymore...unless bauerb@ says it is ok to have broken for a while). > > > > Oh, I was trying to add this to web_contents_observer_proxy.cc which lives in > > contents/, so components/error_page cannot be used there. This would rule out > > option 1. > > > > Since chrome is the only place where this is being used, I can go with option > 3 > > for now with a TODO to file a better place later. > > That sounds good to me, thanks! Seems like error_page::LocalizedError::GetErrorDetails is not able to find the string resource for many net error codes. Filed crbug/690784 to investigate separately and left the error description empty for now. https://codereview.chromium.org/2642303002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java (right): https://codereview.chromium.org/2642303002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:466: if (!hasCommitted && isInMainFrame && !hasPendingNonNtpNavigation(tab)) { On 2017/02/10 00:36:01, Ted C wrote: > I had a comment in an early patch set that I think this needs errorCode != 0 to > be consistent with the old behavior of being triggered only in didFailLoad Done.
Message was sent while issue was closed.
https://codereview.chromium.org/2642303002/diff/380001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2642303002/diff/380001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:14: #include "components/error_page/common/localized_error.h" Okay, you were calling into components/ from content/, which is only allowed in very specific circumstances. There is a DEPS rule that is trying to prevent that (which you changed 😃), but even with that -- as you can see -- it failed to find the localized string at runtime, because that is presumably in a resource file that isn't loaded into content/. The solution would be to look the error description up in a static method somewhere in chrome/ and call that via JNI from Java. That is what I assumed you would do in https://codereview.chromium.org/2642303002/#msg88 (Ted's option 3); apologies if I misunderstood.
Message was sent while issue was closed.
On 2017/02/10 08:07:41, shaktisahu wrote: > Seems like error_page::LocalizedError::GetErrorDetails is not able to find the > string resource for many net error codes. Filed crbug/690784 to investigate > separately and left the error description empty for now. Chrome should always be in a shippable state. If this is causing a real breakage, revert asap. |
