[webkit-reviews] review granted: [Bug 125431] All observable PageLoadState properties should change in an atomic fashion, with properly nested change notifications : [Attachment 218834] Add PageLoadState::commitChanges and Transaction objects to manage committing changes

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has granted mitz at webkit.org
<mitz at webkit.org>'s request for review:
Bug 125431: All observable PageLoadState properties should change in an atomic
fashion, with properly nested change notifications
https://bugs.webkit.org/show_bug.cgi?id=125431

Attachment 218834: Add PageLoadState::commitChanges and Transaction objects to
manage committing changes
https://bugs.webkit.org/attachment.cgi?id=218834&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218834&action=review


> Source/WebKit2/ChangeLog:26
> +	   (WebKit::PageLoadState::endTransaction): Added. Calles when a
Transaction is desctructed.

Desctructed.

> Source/WebKit2/ChangeLog:69
> +	   and made its consturctor initialize state and estimatedProgress.

Consturctor.

> Source/WebKit2/UIProcess/PageLoadState.h:67
> +	   Transaction(Transaction&& other)
> +	       : m_pageLoadState(other.m_pageLoadState)
> +	   {
> +	   }

I think this move constructor should “empty out” the old object. 

Otherwise you could get into situations where a moved-from transaction object
still invokes endTransaction(). One way to do this is to have a boolean flag. I
would just make m_pageLoadState a pointer instead and null it out when moving
and then check that it’s not null before calling endTransaction inside the
Transaction destructor.

> Source/WebKit2/UIProcess/PageLoadState.h:77
> +	   Transaction(PageLoadState& pageLoadState)

I think you should make this constructor explicit.