[rekonq] Re: Review Request: User Agent Switcher.

[Benjamin Poulain]

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



src/mainwindow.cpp
<http://git.reviewboard.kde.org/r/100373/#comment642>

    Why do you convert the data as String? You could use integers with QVariant and avoid all the conversions back and forth.
    
    I am also not a fan of this special case at -1. I see that becoming a problem when someone will forget and use that index in a data structure.



src/mainwindow.cpp
<http://git.reviewboard.kde.org/r/100373/#comment647>

    Coding style: missing a space between the if and (. And there is one superfluous space between the two parenthesis at the end :)



src/mainwindow.cpp
<http://git.reviewboard.kde.org/r/100373/#comment649>

    Since you are using ::exec(), the dialog cannot exist behond this function. So you could just allocated it on the stack.



src/mainwindow.cpp
<http://git.reviewboard.kde.org/r/100373/#comment648>

    Does this really need to be modal?



src/useragent/useragentinfo.h
<http://git.reviewboard.kde.org/r/100373/#comment644>

    I think this API is more complicated than it should be.
    
    I would have UserAgentInfo with the same method but without the integer as argument, and a provider returning a QList of UserAgentInfo.



src/useragent/useragentwidget.cpp
<http://git.reviewboard.kde.org/r/100373/#comment646>

    You could just not have a destructor.


- Benjamin


On Jan. 12, 2011, 10:56 p.m., Andrea Diamantini wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100373/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2011, 10:56 p.m.)
> 
> 
> Review request for rekonq.
> 
> 
> Summary
> -------
> 
> User Agent Switcher. 
> 
> This patch should implement the UA switcher ability for rekonq. This is a first implementation, but it seems working quite well. It is based on KDE UA management and should be fully compatible with konqueror's one i.e. it should be possible using alternatively rekonq and konqueror sharing the same setting.
> 
> Anyway, this is a different implementation from the konqueror's one. Simpler and based on the idea
> of a future moving to a plugin.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt f0310b4d91229a6d8322c163069165b940c41efe 
>   src/application.cpp 95aa9cf0b0a40006c5c0e1663acce506e4c4e123 
>   src/mainwindow.h 33fd20212b9bb69d9450298e4e43324885f8bdf0 
>   src/mainwindow.cpp f662d7aa77d8045372c43514c6a04ea497a9afbc 
>   src/useragent/useragentinfo.h PRE-CREATION 
>   src/useragent/useragentinfo.cpp PRE-CREATION 
>   src/useragent/useragentsettings.ui PRE-CREATION 
>   src/useragent/useragentwidget.h PRE-CREATION 
>   src/useragent/useragentwidget.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/100373/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrea
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/rekonq/attachments/20110112/9c744da1/attachment.html