<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>
<p>On July 25th, 2011, 10:31 p.m., <b>Pierre Rossi</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 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>
</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;">The sanity checks on the url are not doubled as they should. What does it happen when you load an invalid url in a new window? Are you going to create the window before the check?
About multithreading, it's just the note than we are going to support its structure in each window, instead of supporting it once.
In our code there are tons of calls to Application::loadUrl. This is because it is there: to let it reachable from everywhere. In fact this change does NOT remove it, just moves its implementation.
So, I'm asking: in which rekonq parts is it safe to use the new API? Probably nowhere but in this new code.</pre>
<br />
<p>- Andrea</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>