Review Request: Using KWallet to store Cookies

David Faure faure at kde.org
Tue Nov 16 11:21:25 GMT 2010 

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/4927/#review8753
-----------------------------------------------------------



/trunk/KDE/kdebase/apps/konqueror/settings/kio/kcookiespolicies.cpp
<http://svn.reviewboard.kde.org/r/4927/#comment9342>

    Typo in the key name, better fix it before we have to support config files written with the typo...
      (Avaliable -> Available).
    Same typo in the variable name (less of a concern there, of course).



/trunk/KDE/kdebase/apps/konqueror/settings/kio/kcookiespolicies.cpp
<http://svn.reviewboard.kde.org/r/4927/#comment9343>

    The existing code is weird, the bool state is quite a useless intermediary :-)



/trunk/KDE/kdebase/apps/konqueror/settings/kio/kcookiespoliciesdlg.ui
<http://svn.reviewboard.kde.org/r/4927/#comment9344>

    Same typo everywhere :)



/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookiejar.cpp
<http://svn.reviewboard.kde.org/r/4927/#comment9345>

    There's no need to insert into the map if an entry isn't found. So you should use cookieInfo.value(name+"_value") rather than cookieInfo[name+"_value"];



/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.cpp
<http://svn.reviewboard.kde.org/r/4927/#comment9346>

    Why kapp? Usually not a good idea.



/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.cpp
<http://svn.reviewboard.kde.org/r/4927/#comment9347>

    Typo :)



/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.cpp
<http://svn.reviewboard.kde.org/r/4927/#comment9348>

    No if(foo) needed before delete foo.
    (delete NULL is a no-op)



/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.cpp
<http://svn.reviewboard.kde.org/r/4927/#comment9350>

    Eek, a nested event loop.
    This event loop will process incoming dbus calls for instance; which will re-enter functions such as this one, and lead to unexpected trouble.
    
    I'm a bit confused: this seems to use nice async API including a slotWalletHasBeenLoaded... why the nested event loop, then?



/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.cpp
<http://svn.reviewboard.kde.org/r/4927/#comment9351>

    QFile::remove takes a QString, no need for the encodeName().
    
    Compile this code with QT_NO_CAST_FROM_ASCII to detect such mistakes :)
     [but in a different commit]



/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.cpp
<http://svn.reviewboard.kde.org/r/4927/#comment9352>

    Same here. Why use Asynchronous if you make it sync anyway?
    
    I see you handle re-entrancy in this particular method, but I'm still worried about re-entrancy at a higher level.
    
    Please consider either
    1) reworking this to be fully async, or
    2) using the Sync API from KWallet, even though this will time out if the user takes too much time to enter his wallet password.
    
    To make it all async, you can solve the problem of synchronous DBus calls by using dbus transactions -- although one tends to hit the stupidly-low DBus timeout quite often this way, again if the user takes too much time to react, so this doesn't gain much.
    
    The "real" solution is probably to add an async API to kcookieserver, although even javascript requires synchronous access to cookies IIRC... so this might not work.
    
    You're hitting one of the most complex issues with Qt (and DBus) - once something low-level is async, everything on top of it needs to be async too, which can be pretty difficult.
    
    Anyway. I'd much rather see dbus calls time out (due to using the kwallet sync API), then the user can just hit reload after entering his wallet password, than seeing complex-to-debug crashes due to unexpected re-entrancy due to the use of QEventLoop.



/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.cpp
<http://svn.reviewboard.kde.org/r/4927/#comment9353>

    I'm curious, why does "load from wallet" have to be called everywhere, while "load from file" didn't have to be?



/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.cpp
<http://svn.reviewboard.kde.org/r/4927/#comment9354>

    The <bool> is not necessary, the default value is a boolean anyway.


- David


On 2010-10-26 01:24:01, José Millán Soto wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/4927/
> -----------------------------------------------------------
> 
> (Updated 2010-10-26 01:24:01)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> Currently cookies are stored in a plain text file. This patch allows KCookieJar to store the cookies securely using KWallet.
> 
> The main problem I had writing this patch was that when a web page is requested, KIO ask for the cookies to kded using dbus. In the first implementations that I wrote, if the user took too long to open the wallet, KIO received a dbus timeout.
> 
> To prevent this, if it takes more than 10 seconds to open the wallet, the web page will be requested without sending the cookies (or sending the available cookies if there's still the plain text cookie file). If the wallet is opened after that, the cookies stored in the wallet will be available since then.
> 
> Because of this, the feature is disabled by default.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/apps/konqueror/settings/kio/kcookiespolicies.cpp 1189829 
>   /trunk/KDE/kdebase/apps/konqueror/settings/kio/kcookiespoliciesdlg.ui 1189829 
>   /trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookiejar.h 1189787 
>   /trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookiejar.cpp 1189787 
>   /trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.h 1189787 
>   /trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.cpp 1189787 
> 
> Diff: http://svn.reviewboard.kde.org/r/4927/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> José
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20101116/ad99ea5f/attachment.htm>

More information about the kde-core-devel mailing list