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

[Dawit Alemayehu]

> On Feb. 20, 2012, 7:16 p.m., David Faure wrote:
> > kparts/htmlextension.h, line 245
> > <http://git.reviewboard.kde.org/r/103973/diff/4/?file=50594#file50594line245>
> >
> >     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).

The extension is named HtmlExtension so there is no reason not to name the interface HtmlSettingsInterface following the qt naming style as well.


> On Feb. 20, 2012, 7:16 p.m., David Faure wrote:
> > kparts/htmlextension.h, line 251
> > <http://git.reviewboard.kde.org/r/103973/diff/4/?file=50594#file50594line251>
> >
> >     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).
> >

Ok. I renamed the enums "HtmlSettingsType", the getter/setter functions to "htmlSettingsProperty" and "setHtmlSettingsProperty". Is that acceptable ?


> On Feb. 20, 2012, 7:16 p.m., David Faure wrote:
> > kparts/htmlextension.h, line 274
> > <http://git.reviewboard.kde.org/r/103973/diff/4/?file=50594#file50594line274>
> >
> >     Should this method return a bool maybe, so that the caller can find out that the part didn't support a specific setting?

Is it a good idea to return a boolean from "setter" function call ? You can always call the get function to see if the value was properly set, no ? Or did you want to provide a shortcut ? I always avoid a return in set functions on purpose. Alternatively, we can add "bool testHtmlSettingsProperty(...)" const;.


- Dawit


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


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/63961a32/attachment.htm>