NavigationRequest creation on DidCommit, without DidCommitProvisionalLoad_Params

[Rakina Zata Amni]

Hi navigation-dev,

There are some cases where the browser might receive DidCommit calls without having the NavigationRequest for the commit, forcing us to create a NavigationRequest for them with info from DidCommitProvisionalLoad_Params, which complicates our effort to migrate fields out of DidCommitProvisionalLoad_Params.

One of the cases is error page commit, which is discussed in another thread. This time I'm taking a look at the 3 remaining cases (initial commit, same-document, tests):

• For the first two cases I think we still won't let the browser know about the commit and create a NavigationRequest until DidCommit time, to avoid additional roundtrip latency. But, it's possible to remove dependency to DidCommit params entirely because I suspect we might only need to create an "empty" NavigationRequest for the initial commit case, and reuse the previous commit's NavigationRequest for the same-document case.
• For the test, I think it just needs some rewriting (although it might be quite complex :().

There are more details on this doc, PTAL! These are still early thoughts so I might have misunderstood things, missing important details, etc etc, but sending them early to get more insight :)

[Arthur Sonzogni]

Thanks for writing this!
I commented on the doc.

Among the 4 cases remaining, only the same-document navigation is a legitimate case. The other ones should all be removed.

A while ago, I had started a patch for #4 (DomSerializerTests), but never finished it. I will try to finish it soon.

The only real unknown to me is removing the "fake" initial navigation. I think I want this to disappear, I would be curious to see what would prevent us from doing it.

[Nasko Oskov]

Thanks Rakina for the doc!

I really like that we are exploring removing some of the edge cases, as it has been something we have desired for a while but never had the time to tackle. I've left some comments on the doc and it seems mostly tractable.

[Camille Lamy]

Thanks for the doc! I would be happy to see all of the cases go away, except for renderer-initiated same-document navigation which are legitimate.

[Charlie Reis]

Thanks for looking into this!  Agreed about synchronous same-document commits being needed.

I'm less clear that it's safe to remove commit messages from the initial empty document when no URL (or about:blank) was specified for the frame, since other documents and observers may be expecting an onload / DidFinishNavigation in that case.  If those events can be synthesized, or if we can show that it isn't a problem to remove the events for compatibility (e.g., by comparing with other browsers' behavior), then maybe it could be ok to skip the commit in that case.

Charlie

--
You received this message because you are subscribed to the Google Groups "navigation-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to navigation-de...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CAMKsNvoNhAp%2B-%2BO_nq1shaSQJ3_GpirtA9BH_CjNTGiZO85XQg%40mail.gmail.com.

[Rakina Zata Amni]

Hi all,

Thanks for the comments and help! I've updated the doc with the current plans for the remaining cases, here's a summary:

• Initial about:blank commit 
• To fully remove this, we need a lot more investigation like Charlie and others mentioned.
• For now, I propose we just keep the current behavior (create a NavigationRequest at DidCommit) but remove the DidCommitProvisionalLoad_Params dependency. We don't need any info from the renderer except the URL (at least 1 WPT is expecting window.open(about.blank#something) to be synchronous - not sure if it's OK to change this). So, we can make these commits notify the browser through DidCommitInitialEmptyDocument(url), and not go through DidCommitProvisionalLoad at all. 
• Same-document navigation 
• (this case has its own doc that explains the changes in more detail. PTAL :) )
• Our initial plan was to make DidCommitSameDocumentNavigationParams for renderer-initiated same-document navigations, and make browser-initiated same-document navigations go through DidCommitPerNavigationMojoInterface to remove dependency to navigation_token.
• However, we can actually keep browser-initiated same-document navigations as it was before and just move navigation_token to DidCommitSameDocumentNavigationParams, removing the DidCommitProvisionalLoad_Params version.
• But we still might want to consider doing the browser-initiated vs renderer-initiated separation part in the future, as it removes some special handling in the current code (cross-document navigation restarts, aborting commits, same_document_navigation_requests_, etc).
• Error page case: will have a final decision and fix soon (see thread).
• DomSerializerTest case is gone thanks to arthursonzogni@ \o/

PTAL and let me know your thoughts! I'll start sending out CLs soon.

[Arthur Sonzogni]

Thanks Rakina for the update!

• For now, I propose we just keep the current behavior (create a NavigationRequest at DidCommit) but remove the DidCommitProvisionalLoad_Params dependency. We don't need any info from the renderer except the URL (at least 1 WPT is expecting window.open(about.blank#something) to be synchronous - not sure if it's OK to change this). So, we can make these commits notify the browser through DidCommitInitialEmptyDocument(url), and not go through DidCommitProvisionalLoad at all. 

window.open(url) usually means:

1. Create a new window/frame, the frame starts from the initial empty document.
2. Navigate to url (the navigation is asynchronous and might fail)

If url is considered empty, (2) is unnecessary, we just stay on the initial empty document.

url = about:blank#something is considered "empty". This is an interesting edge case indeed ;-)

Some potential ideas to explore:

1. (As you said), use DidCommitInitialEmptyDocument(url). This sounds fine for now.
2. The url is known from 1 via CreateNewWindowParams.url. So it is still possible in theory not to rely on the DidCommit / DidCommitInitialEmptyDocument(url). We can remove them at some point.
3. Break it into: (1) open the initial empty document (about:blank) + (2) same-document navigation toward #something.