[webkit-reviews] review granted: [Bug 86969] Match Firefox restrictions to window.blur and window.focus : [Attachment 144224] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted jochen at chromium.org's request for
review:
Bug 86969: Match Firefox restrictions to window.blur and window.focus
https://bugs.webkit.org/show_bug.cgi?id=86969

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144224&action=review


> Source/WebCore/GNUmakefile.list.am:3081
> +	   Source/WebCore/page/WindowFocusAllowedIndicator.cpp \
> +	   Source/WebCore/page/WindowFocusAllowedIndicator.h \

Bad indent.

> Source/WebCore/notifications/Notification.cpp:226
> +    // During clicks on notifications, the page is allowed to focus itself.

This comment is redundant with the code below.

> Source/WebCore/page/DOMWindow.cpp:913
> +    Settings* settings = m_frame->settings();
> +    bool allowFocus = WindowFocusAllowedIndicator::windowFocusAllowed() ||
(settings && !settings->windowFocusRestricted());

Note: settings will always be non-null here because page is non-null.

> Source/WebCore/page/DOMWindow.cpp:917
> +	   // Only allow our opener to focus us.

This comment is redundant with the code below.

> Source/WebCore/page/WindowFocusAllowedIndicator.h:35
> +// Lifts restrictions on window.focus() and window.blur() as imposed by
WebCore
> +// during its lifetime.

window.blur doesn't seem to interact with WindowFocusAllowedIndicator.	I'd
probably just remove this comment because it's likely to get out of date.  The
real purpose of this class is easy to determine by grepping the codebase.

> Source/WebKit/chromium/src/WebNotification.cpp:144
>      // Make sure clicks on notifications are treated as user gestures.
>      UserGestureIndicator gestureIndicator(DefinitelyProcessingUserGesture);
> +    // During clicks on notifications, the page is allowed to focus itself.
> +    WindowFocusAllowedIndicator windowFocusAllowed;
>      dispatchEvent(eventNames().clickEvent);

I would remove both of these comments.	They're just noise.