WebView loadDataWithBaseURL() was determined as a same page navigation

third_party/WebKit/Source/core/loader/FrameLoader.cpp

Index: third_party/WebKit/Source/core/loader/FrameLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/FrameLoader.cpp b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
index fde5f299ac1e539ba4f62f6f845d6b91866f77a7..163731d513ee6f3d309887370cf157b4efa6c00d 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
@@ -939,7 +939,12 @@ FrameLoadType FrameLoader::determineFrameLoadType(
        equalIgnoringCase(m_frame->document()->url(), blankURL())))
     return FrameLoadTypeReplaceCurrentItem;
 
-  if (request.resourceRequest().url() == m_documentLoader->urlForHistory()) {
+  // TODO(crbug.com/662823): WebFrameLoaderImpl::loadData() sets baseUrl for

[Charlie Reis, 2016/11/28 20:07:27]

Can you clarify what still needs to be done here?  I would also suggest filing a
separate bug for it, since I expect that we'll be closing 662823 when this CL
lands.

[Takashi Toyoshima, 2016/11/29 09:39:36]

Filed a new bug, crbug.com/669408.

Actually, this comment wasn't enough to explain everything. Sorry for that.

As I told torne@, WebView spec looks a little unclear to me. The document does
not explain what should be the page URL for this API. Since we need the page URL
to detect the same page navigation correctly, I just tried to exclude all
loadDataWithBaseURL() cases from the same page navigation in this patch as a
workaround.

If the URL should be the same URL with baseUrl, current behavior should be a
right expected one.

If the URL should be encoded content in a data URL format, we may need
exceptional code to send the baseURL as a referer for this WebView API.

IMO, assuming baseUrl as a page URL sounds more reasonable.

+  // ResourceRequest's URL, but we need to compare Data URL encoded contents
+  // to determine the same page navigation for Data URLs.
+  bool isDataUrlWithEmulatedBaseUrl = request.substituteData().isValid();

[Charlie Reis, 2016/11/28 20:07:27]

nit: isDataUrlWithBaseUrl seems like it would be more consistent with the API
name.

I'm mildly concerned that checking for this via substituteData might affect more
cases than just loadDataWithBaseURL.  I'm not familiar with what SubstituteData
is used for, so I can't say. 

[dcheng, 2016/11/28 20:12:55]

I echo Charlie's concern here: is this actually selective to only the data URL
with base URL case? I see SubstituteData() created in a number of other places
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Sub...),
so it's not clear to me that this filtering is selective enough (or maybe it is,
but needs comments to explain why)

[Takashi Toyoshima, 2016/11/29 09:39:36]

Agreed.
Probably, we should discuss what is the right behavior first.

Then, if we decided to handle loadDataWithBaseURL() cases in a special way like
this, I'd introduce an explicit flag in the SubstituteData() to say this content
is generated by loadDataWithBaseURL().

+  if (!isDataUrlWithEmulatedBaseUrl &&
+      request.resourceRequest().url() == m_documentLoader->urlForHistory()) {
     if (!request.originDocument())
       return FrameLoadTypeReloadMainResource;
     return request.resourceRequest().httpMethod() == HTTPNames::POST