[webkit-reviews] review granted: [Bug 135129] Assertion failure in WebPage::reload (!m_pendingNavigationID) when reloading after a same-document back navigation : [Attachment 235245] Don't create new navigations for same-document back-forward navigations

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted mitz at webkit.org <mitz at webkit.org>'s
request for review:
Bug 135129: Assertion failure in WebPage::reload (!m_pendingNavigationID) when
reloading after a same-document back navigation
https://bugs.webkit.org/show_bug.cgi?id=135129

Attachment 235245: Don't create new navigations for same-document back-forward
navigations
https://bugs.webkit.org/attachment.cgi?id=235245&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=235245&action=review


> Source/WebKit2/Shared/WebBackForwardListItem.cpp:74
> +    for (const auto& child : a.children) {
> +	   const FrameState* otherChild =
childItemWithDocumentSequenceNumber(b, child.documentSequenceNumber);
> +	   if (!otherChild || !documentTreesAreEqual(child, *otherChild))
> +	       return false;
> +    }

Too bad this is n^2 the number of children at each level of the tree, and
recursive.

It also unnecessarily compares every document sequence number during the
recursion even though it finds each child by sequence number.

I could imagine a more version that copied and sorted the vectors of children
to avoid the n^2.

And we might be able to cleanly avoid the recursion by using a Deque to keep
track of children to compare.

But I’m not sure either of these changes would actually be an improvement.

> Source/WebKit2/Shared/WebBackForwardListItem.cpp:84
> +    // The following logic must be kept in sync with
WebCore::HistoryItem::shouldDoSameDocumentNavigationTo.

I find the logic below utterly mysterious. Maybe the “why” comments are in
shouldDoSameDocumentNavigationTo?

> Source/WebKit2/Shared/WebBackForwardListItem.cpp:95
> +    WebCore::URL url = WebCore::URL(WebCore::ParsedURLString,
mainFrameState.urlString);
> +    WebCore::URL otherURL = WebCore::URL(WebCore::ParsedURLString,
otherMainFrameState.urlString);
> +
> +    if ((url.hasFragmentIdentifier() || otherURL.hasFragmentIdentifier()) &&
equalIgnoringFragmentIdentifier(url, otherURL))

Seems a real shame we have to reconstitute the URL objects just to find the "#"
characters in them. I am pretty sure this can be done more efficiently without
fully re-parsing the URLs, but it might make the code harder to maintain so I
guess we can leave it like this.