[webkit-reviews] review denied: [Bug 63458] WebKitTestRunner needs layoutTestController.setPopupBlockingEnabled : [Attachment 123092] Implementation of setPopupBlockingEnabled method.

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Nandor Huszka
<Huszka.Nandor at stud.u-szeged.hu>'s request for review:
Bug 63458: WebKitTestRunner needs layoutTestController.setPopupBlockingEnabled
https://bugs.webkit.org/show_bug.cgi?id=63458

Attachment 123092: Implementation of setPopupBlockingEnabled method.
https://bugs.webkit.org/attachment.cgi?id=123092&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=123092&action=review


Thanks for working on this!

> Source/WebCore/page/Settings.h:178
> +	   void setPopupBlockingEnabled(bool);
> +	   bool popupBlockingEnabled() const { return
!m_javaScriptCanOpenWindowsAutomatically; }

It doesn't seem good to introduce these new accessors for the
m_javaScriptCanOpenWindowsAutomatically member. We should just use the existing
accessors instead of adding new ones.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:165
> +    for (HashSet<Page*>::iterator iter = pages.begin(); iter != pages.end();
++iter)

I'm surprised you didn't have to specify const_iterator here.

We normally put the .end() iterator into a local rather than calling it on
every iteration of the loop.