[rekonq] Re: Review Request: Restore the tab's history when restoring a tab/session

[Pierre Rossi]

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


Thumbs up to restoring the history along with the tab ! :)


src/history/historymanager.h
<http://git.reviewboard.kde.org/r/100604/#comment2149>

    It feels "url" would work well for this field's name, and a bunch of lines above would get to stay the same. Otherwise title should be called currentTitle for consistency, but since it's quite obvious I think you can drop the "current" altogether.



src/mainview.cpp
<http://git.reviewboard.kde.org/r/100604/#comment2153>

    do we really need this all foreach loop ? It doesn't seem very likely to me that two closed tabs will have the same history...



src/mainview.cpp
<http://git.reviewboard.kde.org/r/100604/#comment2151>

    Is this really necessary ? (considering the takeFirst() above)



src/sessionmanager.cpp
<http://git.reviewboard.kde.org/r/100604/#comment2150>

    is base64 really necessary if it's already in a CDATA section within a xml document ?



src/sessionmanager.cpp
<http://git.reviewboard.kde.org/r/100604/#comment2148>

    I'm a bit confused here... Did you mean to call the slot (without emit) or emit the currentChanged signal ?



src/sessionmanager.cpp
<http://git.reviewboard.kde.org/r/100604/#comment2146>

    Nice catch on this refactoring, but I feel it could go in a separate micro-commit, since it is not specially tied to this change.
    (r=me for that commit if you feel like doing so)



src/sessionmanager.cpp
<http://git.reviewboard.kde.org/r/100604/#comment2147>

    how about using something like this instead:
    foreach (const QDomElement element, document.elementsByTagName("tab"))


- Pierre


On April 7, 2011, 6:48 p.m., Anton Kreuzkamp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100604/
> -----------------------------------------------------------
> 
> (Updated April 7, 2011, 6:48 p.m.)
> 
> 
> Review request for rekonq.
> 
> 
> Summary
> -------
> 
> Restore the history of the tab when restoring a tab from the list of closed tabs, by open last closed tab or by restoring the session.
> Therefore I ported the session-file to xml, as it allows more complex session-files (which is also needed for multiple sessions support).
> (Doesn't require a qtwebkit-patch anymore)
> 
> 
> Diffs
> -----
> 
>   src/application.cpp 1fe936f 
>   src/history/historymanager.h eb8d78d 
>   src/mainview.h acc2d8c 
>   src/mainview.cpp b34acc3 
>   src/newtabpage.cpp a598066 
>   src/sessionmanager.h 2297290 
>   src/sessionmanager.cpp 68bc635 
>   src/tabbar.cpp 1ab357f 
> 
> Diff: http://git.reviewboard.kde.org/r/100604/diff
> 
> 
> Testing
> -------
> 
> tested and everything seems to work fine.
> 
> 
> Thanks,
> 
> Anton
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/rekonq/attachments/20110407/2a8d8ac6/attachment-0001.htm