<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/105749/">http://git.reviewboard.kde.org/r/105749/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 3rd, 2012, 9:09 a.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks for the additional testing, now the patch makes more sense overall.
It's more "special cases" scattered around the code, but not much choice there, with the existing design.
Please document what m_isPopupWindow means though. It's a bit confusing that in this code, a "m_isPopupWindow" means "window.open with no toolbars", while the method isPopupWindow() includes other cases (no menubar, absolute positioning, etc.), and the user settings for what to do about "popup windows" is for yet another case (the cases where isPopupWindow() returns false, i.e. new windows with all their toolbars and menubar and no positioning...). Confusing, heh?
=> In fact this bool should be renamed to m_isPopupWithoutToolbars or something. It's a special case of popup window.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">You are correct. m_isPopupWindow is indeed confusing. However, instead of changing it to "m_isPopupWithoutToolbars", I will rename it "m_isPopupWithProxyWindow" so that it correctly conveys its relation to the "m_popupProxyWindow" variable. I will also add some documentation above those two variables in the header file.</pre>
<br />
<p>- Dawit</p>
<br />
<p>On July 31st, 2012, 3:32 p.m., Dawit Alemayehu wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Dolphin, KDE Base Apps and David Faure.</div>
<div>By Dawit Alemayehu.</div>
<p style="color: grey;"><i>Updated July 31, 2012, 3:32 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The attached patch attempts to resolve a security concern in Konqueror when browsing the web. The concern results from a website, through the use of the javascript window.open API, requests the creation of a new window (pop up window) with all its toolbars disabled. When Konqueror gets such requests it simply disables all toolbars in the main window including the one that contains the address line edit widget. This is a problem because it makes it possible for sites to spoof the user into providing personal information by mimicking native input dialog such as the password dialog.
As such this patch attempts to solve the problem in the same manner it seems to be addressed in other major browsers such as Firefox and Chromium. Namely, Konqueror will no longer hide the toolbar containing the address line edit widget by default. The user must explicitly override the default settings by adding the following configuration option to konquerorrc:
[DisableWindowOpenFeatures]
LocationBar=false
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">TEST SCENARIOS:
1. Click on a link with its target property set to "_blank" in the popup window.
2. Middle click on a link.
3. Select text in the popup window and select "Search Google for...".
4. Right click on a link and select "Open in New Tab".
5. Press CTRL+T in the popup window.
6. Press CTRL+N in the popup window.
7. Press (ALT/CTRL)+Enter while the address widget has the focus.
EXPECTED RESULTS:
1. Depending on user's configuration, a brand new (non-popup) window or a new tab in the window from which
the popup originated showing the contents of the link that was clicked.
2. Same as #1.
3. Same as #1.
4. A new tab showing the contents of the link in the window from which the popup originated.
5. Nothing (ignored).
6. A blank non-popup window.
7. Treat it as if only Enter was pressed.
EXPECTED RESULTS (after closing the window from which the popup originated):
1. A new non-popup window showing the contents of the link that was clicked.
2. Same as #1.
3. Same as #1.
4. Same as #1.
5. Nothing (ignored).
6. A blank non-popup window.
7. Treat it as if only Enter was pressed.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>konqueror/src/konqmainwindow.h <span style="color: grey">(fd007e8)</span></li>
<li>konqueror/src/konqmainwindow.cpp <span style="color: grey">(081509e)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105749/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/105749/s/645/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/07/27/old_popup_window_400x100.png" style="border: 1px black solid;" alt="before the change" /></a>
<a href="http://git.reviewboard.kde.org/r/105749/s/646/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/07/27/new_popup_window_400x100.png" style="border: 1px black solid;" alt="after the change" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>