That test makes no sense to me either.

Tracked origin of this test to the internal chrome on android fork of chromium: https://chrome-internal-review.googlesource.com/#/c/19949/9/content/android/javatests/src/org/chromium/content/browser/test/LoadDataWithBaseUrlTest.java

+mnaganov (not really on the team anymore :( but maybe can still answer questions?): remember logic behind this test? Why would calling goBack() to a loadDataWithBaseURL go back to the history url, rather than to the original data url?

I don't see any cts coverage for this behavior, so should be safe to change behavior barring compatibility issues. I think new behavior is saner, and I doubt are any incompatibility issues.

Ok, thanks!  I have one other issue to wrap up before landing https://codereview.chromium.org/1952533003/ (i.e., issue 609963), but I'll include a change to the LoadDataWithBaseUrlTest#testHistoryUrlNavigation test's expectations for when it's ready to land.  I'll add you as reviewer when the time comes.

Thanks Bo for digging out the history!

I don't actually see that it contradicts behavior in other tests though:

 -- testHistoryUrl checks that getUrl returns the history Url; it also says that if there is no history url specified, getUrl should return "about:blank";
 
 -- testHistoryUrlIgnoredWithDataSchemeBaseUrl is a special case for base URL having 'data:' scheme only -- this is when history url gets ignored.

For me, it would be then logical to expect that the history url overrides the actual url for navigation. This way it is possible for the app to "tag" the pages it loads via loadDataWithBaseUrl, and intercept when the user or the page tries to move across the history, so it can re-issue loadDataWithBaseUrl, because it's not actually the same as issuing a loadUrl with a 'data:' URL.

> For me, it would be then logical to expect that the history url overrides the actual url for navigation. This way it is possible for the app to "tag" the pages it loads via loadDataWithBaseUrl, and intercept when the user or the page tries to move across the history, so it can re-issue loadDataWithBaseUrl, because it's not actually the same as issuing a loadUrl with a 'data:' URL.

That's one edge use case..

What do you mean by "not actually the same as issuing a loadUrl with a 'data:' URL"? I would assume in this case, all the info from the loadDataWithBaseURL are saved and re-used for the back navigation, not just the data url bit.

> What do you mean by "not actually the same as issuing a loadUrl with a 'data:' URL"? I would assume in this case, all the info from the loadDataWithBaseURL are saved and re-used for the back navigation, not just the data url bit.

I must admit, I don't know for sure.

All in all, this behavior is really not specified very well, so it's probably OK to change it, unless it breaks some important app. 

'data:' URL"? I would assume in this case, all the info from the
loadDataWithBaseURL are saved and re-used for the back navigation, not just
the data url bit.

I must admit, I don't know for sure.

All in all, this behavior is really not specified very well, so it's
probably OK to change it, unless it breaks some important app.

'data:' URL"? I would assume in this case, all the info from the
loadDataWithBaseURL are saved and re-used for the back navigation, not just
the data url bit.

I must admit, I don't know for sure.

All in all, this behavior is really not specified very well, so it's
probably OK to change it, unless it breaks some important app.

Ok, I'm revisiting this so that we can turn on the new navigation path.  I also tracked down why we're behaving the way we are today, and it looks like a bug to me (which happened to get encoded into that test).  Hopefully the switch will be worthwhile.

Specifically, when we go back to a page loaded via LoadDataWithBaseURL, we actually load the page located at the history URL but report that we loaded the data URL.  To anyone observing navigation events, it looks like we went back to the state that we were in before, but now there's a different page loaded in the WebContents (the history URL's page, not the data URL's page).  This seems quite confusing.

The reason is that RenderFrameImpl::NavigateInternal calls HistoryController::GoToEntry for back/forward navigations.  The end of NavigateInternal has a bunch of logic that's aware of LoadDataWithBaseURL and uses LoadDataURL instead of WebLocalFrame::Load for it.  HistoryController::GoToEntry doesn't know about LoadDataWithBaseURL, so it just uses WebLocalFrame::Load, which incorrectly loads the history URL during back/forward navigations.  (We lie about which URL committed via MaybeGetOverriddenURL, which was added in r361481.)

When we switch to the new navigation path, we don't use HistoryController anymore, so NavigateInternal correctly uses LoadDataURL instead of WebLocalFrame::Load on back/forward navigations.  This seems like what we should do going forward, IMO.  (We may even be able to undo part or all of r361481?)

In theory, we could try to fix the bug in HistoryController before switching over, but we're almost ready to switch to the new path and delete HistoryController.  I propose just updating the test as we switch.

If that sounds good, I've written some CLs to start us down that path.  https://codereview.chromium.org/2099623002/ is a content_browsertest for this specific back/forward behavior, which switches its expectation based on whether we're using the old or new navigation logic.  I can land this now.

https://codereview.chromium.org/1952533003/ is a CL to enable the new navigation logic, and it updates the Android test while we're at it.  I can land this once I wrap up the last blocking issues with the new navigation logic.

Sound reasonable?

sgtm

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

commit fa6742e4236b8efa17c8eb60f23f566f64b1c500
Author: creis <creis@chromium.org>
Date: Fri Jun 24 18:36:23 2016

Add a content_browsertest for LoadDataWithBaseURL + Back.

The current navigation logic has a bug that loads the history URL but
lies and claims the data URL is committed.  This bug is fixed in the
new navigation logic with subframe FrameNavigationEntries.

This CL adds a test showing these two expectations, mimicing
org.chromium.android_webview.test.LoadDataWithBaseUrlTest's

It's easier to switch to the new navigation logic than fix the bug
in the old logic.

BUG= 612196 
TEST=Test passes by default and in --site-per-process.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/fa6742e4236b8efa17c8eb60f23f566f64b1c500/content/browser/frame_host/navigation_controller_impl_browsertest.cc

This was fixed in r410150, as part of completing  issue 236848 .

Apologies, applied the wrong component in bulk.