Review Request: Using KWallet to store Cookies

[Dawit Alemayehu]

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


In addition to my review below, I have one question for you... Is it really necessary to have the "StoreWhenWalletNotAvaliable" option ?


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

    For performance reasons, it is always a good idea to keep the call to "blah.constEnd()" outside of the loop statement.  Perhaps even a Q_FOREACH would have sufficed here ?



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

    Why not "const QString& name" ?



/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.h
<http://reviewboard.kde.org/r/4927/#comment7177>

    Why not const "const QString& domain = QString()" here ?



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

    I have an issue with how mCookiesLoadedFromWallet is being used here. Though I understand why you are doing what you are doing, the flag looses its meaning nonetheless. Why not simply use a single OR 'ed if statement instead ?
    
    if (!mUseWalletForCookies || mCookiesLoadedFromWallet || mTryingToObtainWallet || mTriedToObtainWallet)
        return;
    
    That way it is straight forward to whomever has to deal with it in the future what is being done here...



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

    Same here... Why would failing to open the wallet result in the mCookieLoadedFromWallet flag being set to true ? At least some comment would be necessary. Better yet, if the logic above is followed, this won't be necessary, no ?



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

    The If here will also be unnecessary, if the logic change in "loadCookiesFromWallet" is applied...



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

    This entire if statement block belongs under the previous if statement block. There is no reason to repeat the same check again!


- Dawit


On 2010-08-07 18:41:38, José Millán Soto wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4927/
> -----------------------------------------------------------
> 
> (Updated 2010-08-07 18:41:38)
> 
> 
> 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 1159393 
>   /trunk/KDE/kdebase/apps/konqueror/settings/kio/kcookiespoliciesdlg.ui 1159393 
>   /trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookiejar.h 1159393 
>   /trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookiejar.cpp 1159393 
>   /trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.h 1159393 
>   /trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.cpp 1159393 
> 
> Diff: http://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/20100818/f56cb205/attachment.htm>