[webkit-reviews] review granted: [Bug 68278] http/tests/history/back-with-fragment-change.php fails on non-Chromium bots : [Attachment 107747] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Mihai Parparita
<mihaip at chromium.org>'s request for review:
Bug 68278: http/tests/history/back-with-fragment-change.php fails on
non-Chromium bots
https://bugs.webkit.org/show_bug.cgi?id=68278

Attachment 107747: Patch
https://bugs.webkit.org/attachment.cgi?id=107747&action=review

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


> Source/WebCore/ChangeLog:11
> +	   navigated-to history item was cloberred by the same-document
navigation.

typo: cloberred should be clobbered

> Source/WebCore/loader/HistoryController.cpp:549
> +void HistoryController::updateForProvisionalLoadStopped()

Should this function also set m_provisionalItem to 0? If so, you could remove
that call from the one call site that does it separately and explicitly.

> Source/WebCore/loader/HistoryController.cpp:555
> +    bool isTargetItem = m_provisionalItem ?
m_provisionalItem->isTargetItem() : false;

You just moved this line of code, but I suggest writing it with && instead of
the ternary operator.

    bool isTargetItem = m_provisionalItem && m_provisionalItem->isTargetItem();


> Source/WebCore/loader/HistoryController.cpp:558
> +	   if (HistoryItem* resetItem =
mainFrame->loader()->history()->currentItem()) {

I find the variable name “reset item” to be a bit confusing. It sounds like a
verb phrase to me.

> Source/WebCore/loader/HistoryController.h:73
> +    void updateForProvisionalLoadStopped();

A better name for this would be updateForStoppedProvisionalLoad. For the
record, I also think the function above should be renamed
updateForCompletedFrameLoad. I think I understand why someone gave that a
different name before, but it’s unnecessarily non-grammatical naming.