[webkit-reviews] review granted: [Bug 18595] REGRESSION (r20766): Setting display:none on an iframe causes the ownerDocument to freeze : [Attachment 57043] proposed fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 18595: REGRESSION (r20766): Setting display:none on an iframe causes the
ownerDocument to freeze
https://bugs.webkit.org/show_bug.cgi?id=18595

Attachment 57043: proposed fix
https://bugs.webkit.org/attachment.cgi?id=57043&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Fix now covers <object> and <embed>, but the test case doesn't cover those.

> +	   RefPtr and is initizlized automatically.

This has a typo: "initizlized".

>      , m_resizeLayer(0)
> -    , m_capturingMouseEventsNode(0)
>      , m_clickCount(0)

m_shouldResetCapturingMouseEventsNode ought to be initialized to make memory
debugging tools happy if nothing else.

> -    void setCapturingMouseEventsNode(PassRefPtr<Node>);
> +    void setCapturingMouseEventsNode(PassRefPtr<Node>); // A caller is
respponsible for resetting capturing node to 0.

This has a typo: "respponsible".

I also find this comment a bit confusing. In the context of this patch I know
what it means.

I think we should have a comment in the one place where we set
m_shouldResetCapturingMouseEventsNode saying when the capturing will
automatically end. In fact, I think the name of the boolean data member should
communicate when the capturing will automatically get reset, not just the fact
that it will be reset.

review+ but please fix at least the uninitialized boolean