Bug 63458 – WebKitTestRunner needs layoutTestController.setPopupBlockingEnabled

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Balazs Kelemen]

Sure he needs it.

[Balazs Kelemen]

Same story as in https://bugs.webkit.org/show_bug.cgi?id=46714
(on Mac skipped by other reason but all wk2 platforms are affected).

[Balazs Kelemen]

Created attachment 98733 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 98733 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98733&action=review

> LayoutTests/ChangeLog:9
> +        Move the group to the section where the items have bug riport.

report

[Balazs Kelemen]

Committed r89829: <http://trac.webkit.org/changeset/89829>

[Nandor Huszka]

Created attachment 123092 [details]
Implementation of setPopupBlockingEnabled method.

There are tests which fail the missing method so far, but now they fail because of an another problem. So I have rearranged the skiplist. These two tests:
- plugins/plugin-initiate-popup-window.html: now it fails on WK1, too
- fast/events/popup-blocked-from-fake-user-gesture.html: times out because of the lines in clickHandler1 function

    win = window.open("about:blank", "blank");
    shouldBeNonNull("win");

  win is null after opening.

[Nandor Huszka]

WebKit must be builded with the Tools/Scripts/build-webkit DEFINES+=ENABLE_CLIENT_BASED_GEOLOCATION=1 command, if we want to test this special function.

[Balazs Kelemen]

Looks good to me.

[Adam Roben (:aroben)]

Comment on attachment 123092 [details]
Implementation of setPopupBlockingEnabled method.

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.

[Nandor Huszka]

(In reply to comment #8)

> 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.

You are right, we do not need the getter method, because it is not used in tests, but in my opinion the setter is necessary. I had a look at DRT and I found win and mac implementations. 
From LayoutTestControllerWin.cpp:
void LayoutTestController::setPopupBlockingEnabled(bool enabled) {
...
    preferences->setJavaScriptCanOpenWindowsAutomatically(!enabled);
}

From LayoutTestControllerMac.mm:
void LayoutTestController::setPopupBlockingEnabled(bool popupBlockingEnabled)
{
    [[[mainFrame webView] preferences] setJavaScriptCanOpenWindowsAutomatically:!popupBlockingEnabled];
}

This is the first reason why I implemented it in this way. The other reason is that tests use the layoutTestController.setPopupBlockingEnabled method. I think we rather do not need the setJavaScriptCanOpenWindowsAutomatically and javaScriptCanOpenWindowsAutomatically methods, these are unused in tests, however these are used in cpp files... It seems we need both accessors, or rewrite the tests to use the layoutTestController.setJavaScriptCanOpenWindowsAutomatically method.

> 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.

I followed the implementation of similar methods in InjectedBundle.cpp. For example:
void InjectedBundle::setFrameFlatteningEnabled(WebPageGroupProxy* pageGroup, bool enabled)
{
    const HashSet<Page*>& pages = PageGroup::pageGroup(pageGroup->identifier())->pages();
    for (HashSet<Page*>::iterator iter = pages.begin(); iter != pages.end(); ++iter)
        (*iter)->settings()->setFrameFlatteningEnabled(enabled);
}

I thought it would be correct if I write a similar method. However that is true .end() callings are unnecessary in every iteration, I will fix it, thank you for the comments.

[Nandor Huszka]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > 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.

I have misinterpreted your comment. I understand already why we do not need the setPopupBlockingEnabled method in class Settings, I will fix it too.

[Balazs Kelemen]

> 
> > 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.
> 
> I followed the implementation of similar methods in InjectedBundle.cpp. For example:
> void InjectedBundle::setFrameFlatteningEnabled(WebPageGroupProxy* pageGroup, bool enabled)
> {
>     const HashSet<Page*>& pages = PageGroup::pageGroup(pageGroup->identifier())->pages();
>     for (HashSet<Page*>::iterator iter = pages.begin(); iter != pages.end(); ++iter)
>         (*iter)->settings()->setFrameFlatteningEnabled(enabled);
> }
> 
> I thought it would be correct if I write a similar method. However that is true .end() callings are unnecessary in every iteration, I will fix it, thank you for the comments.

I guess the drt code is using non-const iterators because constness of hashmap/hashset iterators was broken for a long time, i.e. you could not write myConstIt != myHashSet.end() because it did not compile. But it has been fixed not so far ago, so now you can use const iterators which is a better practice (if you don't modify the data of course).

[Nandor Huszka]

Created attachment 123900 [details]
Implementation of setPopuBlockingEnabled method.

Balazs, I see, thank you for the information.

[Adam Roben (:aroben)]

Comment on attachment 123900 [details]
Implementation of setPopuBlockingEnabled method.

Nice!

[WebKit Review Bot]

Comment on attachment 123900 [details]
Implementation of setPopuBlockingEnabled method.

Clearing flags on attachment: 123900

Committed r105884: <http://trac.webkit.org/changeset/105884>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.