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 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?
> 
> Dawit Alemayehu wrote:
>     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;.
> 
> David Faure wrote:
>     The idea comes from bool QObject::setProperty().
>     
>     I think it makes perfect sense. You're asking an object to store a specific setting, and you don't know if it supports it or not.
>     With a bool return value we can catch errors (unsupported setting, or possibly even invalid value). A "test" method (I guess you mean "isHtmlSettingSupported") would only test the first one, and would require more code than a simple bool return value.
>     Getting the value again with a getter sounds too tricky, e.g. your handling of data: url would not make this symmetric.

OK, I guess this does make some kind of sense since the property being set might not necessarily have an equivalent get function as it is the case in khtml for the user defined style sheet url. However, in the case for the user defined style sheet url, the bool return value from "setHtmlSettingsProperty" is literally going to be useless because we will have no way of knowing if "setting" the property actually succeeded or not.


- Dawit


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


On Feb. 20, 2012, 11:22 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, 11:22 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/20120221/1213bedf/attachment.htm>