Sure he needs it.
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).
Created attachment 98733 [details] Patch
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
Committed r89829: <http://trac.webkit.org/changeset/89829>
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.
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.
Looks good to me.
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.
(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.
(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.
> > > 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).
Created attachment 123900 [details] Implementation of setPopuBlockingEnabled method. Balazs, I see, thank you for the information.
Comment on attachment 123900 [details] Implementation of setPopuBlockingEnabled method. Nice!
Comment on attachment 123900 [details] Implementation of setPopuBlockingEnabled method. Clearing flags on attachment: 123900 Committed r105884: <http://trac.webkit.org/changeset/105884>
All reviewed patches have been landed. Closing bug.