> On April 26, 2012, 5:09 p.m., Albert Astals Cid wrote:
> > If i understand you correctly you are suggesting to create a bug (option
> > that does nothing)?
> >
> > Doesn't make much sense.
>
> Dawit Alemayehu wrote:
> Huh ? I do not follow. By "option that does nothing" you mean this change
> by itself does nothing that is correct. But that is true of every option in
> that dialog as well. Moreover, it is a well known fact that you cannot post a
> patch for different components in reviewboard. Anyhow, the option will most
> definitely be used by kwebkitpart. Whether or not some body implements
> support for it in khtml is a question I cannot answer. That is what I meant.
>
> David Faure wrote:
> Do you have the kwebkitpart patch ready?
> I suppose it'll be easy to "apply" to khtml as well.
>
> Albert Astals Cid wrote:
> You are suggesting to add an option that does nothing in our default html
> rendering engine. That is adding a bug. The fact that you know it and don't
> care about it makes me sad.
>
> Dawit Alemayehu wrote:
> @David: Yes I have a patch for kwebkitpart, but unlike in khtml adding
> support for this in kwebkitpart required a very small change in one place in
> addition to adding code to read the config option itself in
> webkitettings.{h|cpp}. That does not seem to be the case in khtml. It is a
> little bit more invovled.
>
> @Albert: Really ?? So how exactly is another browser engine supposed to
> provide configuration option to the user if it is supposed to be embedded
> into Konqueror ? Why would a developer working on a separate browsing engine
> be forced to implement any functionality into khtml ? Would that requirement
> apply the other way around ? The last I checked, this is a konqueror setting.
> Whether a specific browser engine supports it or not is up to that browser
> engine.Just for the record I almost always go out of my way to implement
> things in khtml ; especially when I factor out features that are common to
> both engines. In this case, I neither have the time nor a complete grasp of
> how the KWallet integration works in khtml. As I stated above the change is
> not in a single isolated location like it is in kwebkitpart. Feel free to do
> a grep in khtml and see for yourself. I can always add the set/get functions
> to access the option in KHTMLSettings, but what use would that be if it is
> not implemented.
>
> Anyhow, I digress. If there are objections, I will simply move this
> feature into the webkit config module I will have to create down the road.
>
> Albert Astals Cid wrote:
> "So how exactly is another browser engine supposed to provide
> configuration option to the user if it is supposed to be embedded into
> Konqueror ?"
> Having it's own engine-only kcm for it's engine-specific options?
>
> "Why would a developer working on a separate browsing engine be forced to
> implement any functionality into khtml ?"
> When did i say that?
>
> "Would that requirement apply the other way around ?"
> If you want to use the "general" apply to all engines options page, of
> course.
>
> You probably don't have any bit of user mentallity left in your head,
> because it's pretty obvious that adding a configuration option to "web
> rendering configuration" that won't work with our default rendering engine
> it's bad usability.
>
> But hey, since David promised to implement this for khtml, go ahead,
> don't let me block progress.
>
I just briefly looked at how complex it would be to implement in KHTML, and it
seems to be enough with this three-line patch?
diff --git a/khtml/ui/passwordbar/storepassbar.cpp
b/khtml/ui/passwordbar/storepassbar.cpp
index 3f5c392..08d79a9 100644
--- a/khtml/ui/passwordbar/storepassbar.cpp
+++ b/khtml/ui/passwordbar/storepassbar.cpp
@@ -80,6 +80,10 @@ StorePass::~StorePass()
void StorePass::saveLoginInformation(const QString& host, const QString& key,
const QMap<QString, QString>& walletMap)
{
+ KConfigGroup config( KGlobal::config(), "HTML Settings" );
+ if (!config.readEntry<bool>("OfferToSaveWebsitePassword", true))
+ return;
+
m_host = host;
m_key = key;
m_walletMap = walletMap;
- Martin Tobias Holmedahl
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104631/#review12934
-----------------------------------------------------------
On April 26, 2012, 3:48 a.m., Dawit Alemayehu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104631/
> -----------------------------------------------------------
>
> (Updated April 26, 2012, 3:48 a.m.)
>
>
> Review request for KDE Base Apps, kdelibs and David Faure.
>
>
> Description
> -------
>
> A patch to make that provides an option to disable the "offer to save website
> passwords". Note that for this patch to take effect this option will have to
> be used at at the browser engine level. Those patches are separate to this
> one. This is just the GUI configuration.
>
>
> Diffs
> -----
>
> konqueror/settings/konqhtml/htmlopts.h 69f36d8
> konqueror/settings/konqhtml/htmlopts.cpp e5d6deb
>
> Diff: http://git.reviewboard.kde.org/r/104631/diff/
>
>
> Testing
> -------
>
>
> Screenshots
> -----------
>
> Option for disabling KWallet integration
> http://git.reviewboard.kde.org/r/104631/s/544/
>
>
> Thanks,
>
> Dawit Alemayehu
>
>
