[rekonq] Re: Review Request: Add loadUrl functionality to MainWindow, and use it from Application

[Andrea Diamantini]

> On July 25, 2011, 5:20 p.m., Andrea Diamantini wrote:
> > 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.
> 
> Pierre Rossi wrote:
>     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. :)

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.


- Andrea


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102065/#review5096
-----------------------------------------------------------


On July 24, 2011, 7:50 a.m., Tirtha Chatterjee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102065/
> -----------------------------------------------------------
> 
> (Updated July 24, 2011, 7:50 a.m.)
> 
> 
> Review request for rekonq.
> 
> 
> Summary
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   src/application.h cc9bc43 
>   src/application.cpp 8bca4f6 
>   src/mainwindow.h e7a5207 
>   src/mainwindow.cpp e4bddd1 
> 
> Diff: http://git.reviewboard.kde.org/r/102065/diff
> 
> 
> Testing
> -------
> 
> Tested. Works correctly.
> 
> 
> Thanks,
> 
> Tirtha
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/rekonq/attachments/20110726/f19846a7/attachment.htm