<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/102065/">http://git.reviewboard.kde.org/r/102065/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 25th, 2011, 5:20 p.m., <b>Andrea Diamantini</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;">I don't like this change a lot, because the loadUrl method was not thought to be in one mainwindow: the multithreaded part, "replied" in each window, or the url sanity checks to be done twice.
I probably prefer your first suggestion here: adding a MainWindow pointer to the original loadUrl slot, that becomes
void Application::loadUrl(const KUrl &url, const Rekonq::OpenType& type = Rekonq::CurrentTab, MainWindow *w = 0);
It seems easier and cleaner to me. Waiting for comments.</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;">I think the sanity checks are relatively different, and even if there was some redundancy, I don't think that's a lot of overhead. Could you expand on the multi-threaded part ? When I reviewed it earlier, it seemed to me that this change was sane, it's probably a good thing to have more pairs of eyes on all those patches. :)</pre>
<br />
<p>- Pierre</p>
<br />
<p>On July 24th, 2011, 7:50 a.m., Tirtha Chatterjee 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 rekonq.</div>
<div>By Tirtha Chatterjee.</div>
<p style="color: grey;"><i>Updated July 24, 2011, 7:50 a.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;">For my GSoC project of sessions management, I need to be able to load URLs in specific windows. The use case is basically this --
I drag a tab from an inactive session, and drop it into an active session.
So the URL of the inactive session should get loaded into specifically that window which corresponds to the active session. So I moved the loadUrl functionality to MainWindow. But I kept Application::loadUrl, it just passes on the task to the appropriate window's loadUrl method.</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;">Tested. Works correctly.</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>src/application.h <span style="color: grey">(cc9bc43)</span></li>
<li>src/application.cpp <span style="color: grey">(8bca4f6)</span></li>
<li>src/mainwindow.h <span style="color: grey">(e7a5207)</span></li>
<li>src/mainwindow.cpp <span style="color: grey">(e4bddd1)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102065/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>