[webkit-reviews] review denied: [Bug 118892] Use TemporaryChange where possible : [Attachment 207064] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Kwang Yul Seo
<skyul at company100.net>'s request for review:
Bug 118892: Use TemporaryChange where possible
https://bugs.webkit.org/show_bug.cgi?id=118892

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=207064&action=review


The idea of this patch is great, it's so much safer to use RAII for temporary
changes.

I think that we need to find a tighter way to express the idiom of "this
boolean variable is only true while executing this code". TemporaryChange is
not quite that, as it restores old value whatever it was. So, it's not the
right tool for the job.

> Source/WebCore/dom/ScriptExecutionContext.cpp:337
> -    m_inDispatchErrorEvent = true;
> -    RefPtr<ErrorEvent> errorEvent = ErrorEvent::create(message, sourceName,
line);
> -    target->dispatchEvent(errorEvent);
> -    m_inDispatchErrorEvent = false;
> +    RefPtr<ErrorEvent> errorEvent;
> +    {
> +	   TemporaryChange<bool> inDispatchErrorEvent(m_inDispatchErrorEvent,
true);
> +	   errorEvent = ErrorEvent::create(message, sourceName, line);
> +	   target->dispatchEvent(errorEvent);
> +    }

I love the change in most of these locations, but not in ones like this. The
code doesn't really get any safer, and it definitely gets more difficult to
read.

It's not great that the ASSERT(!m_inDispatchErrorEvent) is now so far from
where m_inDispatchErrorEvent is changed, and even at a different indentation
level.

Ideally, we would have a different helper that only temporarily changes from
false to true, and incorporates the assertion.

> Source/WebCore/html/HTMLPlugInElement.cpp:-149
>      // ASSERT(!m_inBeforeLoadEventHandler);
> -    m_inBeforeLoadEventHandler = true;
> +    TemporaryChange<bool>
inBeforeLoadEventHandler(m_inBeforeLoadEventHandler, true);
>      // static_cast is used to avoid a compile error since
dispatchBeforeLoadEvent
>      // is intentionally undefined on this class.
> -    bool beforeLoadAllowedLoad =
static_cast<HTMLFrameOwnerElement*>(this)->dispatchBeforeLoadEvent(sourceURL);
> -    m_inBeforeLoadEventHandler = false;

This is a change in behavior. The code used to change
m_inBeforeLoadEventHandler to false in the end, and it no longer does.

The change might be for the better, not sure. It's difficult to reason about it
as part of a large patch.

In this case, we had an assert, and found that it fails. Most of the changed
locations never asserted, so we don't know if their behavior changed, too.