[webkit-reviews] review denied: [Bug 52248] Back/forward list recovery after a WebProcess crash is crashy itself. : [Attachment 78595] Patch v1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Brady Eidson <beidson at apple.com>'s
request for review:
Bug 52248: Back/forward list recovery after a WebProcess crash is crashy
itself.
https://bugs.webkit.org/show_bug.cgi?id=52248

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

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

Looks like this failed to build on Windows.

> WebKit2/Shared/SessionState.cpp:62
> +    // This might change.

This comment is cryptic. I think what you mean to say is:

    // Because this might change later, callers should use this instead of
calling list().isEmpty() directly themselves.

> WebKit2/UIProcess/WebPageProxy.cpp:212
> +    reattachToWebProcess(true);

The boolean here is not good. It’s unclear what “true” means. I’d rather see an
enum used to make call sites like this one understandable.

> WebKit2/UIProcess/WebPageProxy.cpp:238
> -    process()->send(Messages::WebProcess::CreateWebPage(m_pageID,
creationParameters()), 0);
> +    process()->send(Messages::WebProcess::CreateWebPage(m_pageID,
creationParameters(reloadCurrentSessionHistoryItem)), 0);

Instead of sending a boolean, could we just send a second message to trigger
the reloading?

> WebKit2/UIProcess/WebPageProxy.cpp:312
> +	   reattachToWebProcess(false);

The boolean here is not good. It’s unclear what “false” means. I’d rather see
an enum used to make call sites like this one understandable.

> WebKit2/UIProcess/WebPageProxy.cpp:322
> +	   reattachToWebProcess(false);

The boolean here is not good. It’s unclear what “false” means. I’d rather see
an enum used to make call sites like this one understandable.

> WebKit2/UIProcess/WebPageProxy.cpp:365
> +	   reattachToWebProcess(true);

The boolean here is not good. It’s unclear what “true” means. I’d rather see an
enum used to make call sites like this one understandable.

> WebKit2/UIProcess/WebPageProxy.cpp:377
> +    if (!isValid()) {
> +	   reattachToWebProcessWithItem(m_backForwardList->forwardItem());
>	   return;
> +    }

Might be over-engineering to support this. How did you test it?

> WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:74
> +void WebBackForwardListProxy::setHighestItemIDFromUIProcess(uint64_t itemID)


Do we have a race condition here? What if an ID is created before receiving the
new highest item?

> WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:84
> +    uniqueHistoryItemID = itemID;
> +
> +    // If the highest used ID is even, increment it to the next odd ID as
that is what should be used
> +    // for items generated by the WebProcess. 
> +    if (uniqueHistoryItemID % 2 == 0)
> +	   ++uniqueHistoryItemID;

I think this would read better if you set uniqueHistoryItemID only once. Maybe
like this:

    if (itemID % 2)
	uniqueHistoryItemID = itemID;
    else
	uniqueHistoryItemID = itemID + 1;

Or with a function:

    static uint64_t makeOdd(uint64_t number)
    {
	return number + number % 2;
    }

And:

    uniqueHistoryItemID = makeOdd(itemID);