Review Request: New KPart extension for manupilating a browser engine's settings

[David Faure]

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



khtml/khtml_ext.cpp
<http://git.reviewboard.kde.org/r/103973/#comment8747>

    no space after the method name



kparts/htmlextension.h
<http://git.reviewboard.kde.org/r/103973/#comment8742>

    instances -> instance



kparts/htmlextension.h
<http://git.reviewboard.kde.org/r/103973/#comment8743>

    I doubt this will convert to HTML correctly. I guess it needs escaping, such as qobject_cast<KParts::SettingsInterface>



kparts/htmlextension.h
<http://git.reviewboard.kde.org/r/103973/#comment8744>

    KParts::SettingsInterface is a bit generic for a name (it sounds like something that could apply to all parts), maybe this should be HTMLSettingsInterface instead (the qt naming style would be HtmlSettingsInterface, but IMHO we should remain consistent with HTMLExtension).



kparts/htmlextension.h
<http://git.reviewboard.kde.org/r/103973/#comment8746>

    Browser Attribute seems wrong, this isn't an attribute of the browser (app) but an attribute of the html part.
    
    HTMLAttributeType enum, setHTMLAttribute and err... htmlAttribute? The capitalization makes this tricky.
    
    Alternatively, SettingPropertyType, settingProperty(), and setSettingProperty(). I changed attribute to property because this looks very much like qobject properties (but attribute is fine with me if you prefer that).
    



kparts/htmlextension.h
<http://git.reviewboard.kde.org/r/103973/#comment8745>

    Should this method return a bool maybe, so that the caller can find out that the part didn't support a specific setting?


- David Faure


On Feb. 20, 2012, 5:11 p.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103973/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2012, 5:11 p.m.)
> 
> 
> Review request for kdelibs and David Faure.
> 
> 
> Description
> -------
> 
> This patch adds a new KPart extension, BrowserSettingsExtension, for setting as well as accessing a browser engine's properties in a generic fashion from KPart plugins. This is yet another step towards making Konqueror's plugins and settings module independent of the default browser engine in use. IOW, they do not have to directly link to a specific browser engine.
> 
> 
> Diffs
> -----
> 
>   khtml/khtml_ext.h ced53a3 
>   khtml/khtml_ext.cpp 6e8a846 
>   kparts/htmlextension.h 9833d9a 
> 
> Diff: http://git.reviewboard.kde.org/r/103973/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

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