Bug 25689 - [Qt] Implement SearchPopupMenuQt
Summary: [Qt] Implement SearchPopupMenuQt
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2009-05-11 05:33 PDT by Holger Freyther
Modified: 2014-01-13 22:03 PST (History)
5 users (show)

See Also:


Attachments
Implementation using QSettings (3.49 KB, patch)
2009-05-11 05:33 PDT, Holger Freyther
no flags Details | Formatted Diff | Diff
Add SearchPopupMenuDelegate to bridge to the Kit (19.04 KB, patch)
2009-05-14 07:10 PDT, Holger Freyther
staikos: review-
Details | Formatted Diff | Diff
Implementation for Qt using QSettings and QWebSettings (3.76 KB, patch)
2009-05-14 07:11 PDT, Holger Freyther
no flags Details | Formatted Diff | Diff
Add SearchPopupMenuDelegate to bridge to the Kit now with ChangeLog (24.33 KB, patch)
2009-05-22 22:50 PDT, Holger Freyther
manyoso: review-
Details | Formatted Diff | Diff
Implementation for Qt using QSettings and QWebSettings with ChangeLog (5.12 KB, patch)
2009-05-22 22:54 PDT, Holger Freyther
manyoso: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Freyther 2009-05-11 05:33:14 PDT
Implement the SearchPopupMenu for Qt using QSettings.
Comment 1 Holger Freyther 2009-05-11 05:33:52 PDT
Created attachment 30185 [details]
Implementation using QSettings

Feedback welcome, specially on the ::enabled implementation
Comment 2 Ariya Hidayat 2009-05-12 04:01:30 PDT
My only concern is that the code will save something to QSettings behind the user's back. This can trip something w.r.t to the privacy issues.

An alternative will be to use QWebSettings, this needs adding a pointer to the Frame somewhere in the code path.
Comment 3 Holger Freyther 2009-05-14 07:10:02 PDT
Created attachment 30334 [details]
Add SearchPopupMenuDelegate to bridge to the Kit

Add Document* to the signature (like we did for Cookies), and pick *Delegate as the name as we have a PopupMenuClient and a SearchPopupMenuClient would be different. The Delegate approach was used for the Clipboard implementation on Gtk+ as well.

Also start using the new #if USE() instead of having PLATFORM(GTK) && PLATFORM(QT) and share the generic part.
Comment 4 Holger Freyther 2009-05-14 07:11:21 PDT
Created attachment 30335 [details]
Implementation for Qt using QSettings and QWebSettings

First attempt on the API layer, simply add an attribute to QWebSettings and rely on QSettings... document the usage of QSettings() and the need for proper app and org name,
Comment 5 George Staikos 2009-05-19 20:04:43 PDT
Comment on attachment 30334 [details]
Add SearchPopupMenuDelegate to bridge to the Kit

Looks okay but needs a changelog. Also the GTK change doesn't look to be related?
Comment 6 George Staikos 2009-05-19 20:07:46 PDT
Comment on attachment 30335 [details]
Implementation for Qt using QSettings and QWebSettings


> +    foreach(QString string, urls)
> +        searchItems.append(string);

  That should be const QString, right?

  Beyond that, I agree with Ariya that maybe this should not be saved in QSettings for privacy reasons.
Comment 7 Holger Freyther 2009-05-22 22:50:07 PDT
Created attachment 30605 [details]
Add SearchPopupMenuDelegate to bridge to the Kit now with ChangeLog

Same patch with a ChangeLog entry, explaining the motive, the reason to name it Delegate and the plan to make use of it in Gtk+ and Qt.
Comment 8 Holger Freyther 2009-05-22 22:54:38 PDT
Created attachment 30606 [details]
Implementation for Qt using QSettings and QWebSettings with ChangeLog

So far only changed to const QString as requested by George. My take on privacy is the following:
   - It is off by default
   - The "cached" queries are never revealed to HTML/JS
   - It is using QSettings(appName, orgName) so outside of the app it is not used. For hosts like dashboard/plasma if you open the google search in two plasmoids this google field will have a shared history... I think that is feature and not a security issue?
Comment 9 Kenneth Rohde Christiansen 2009-06-04 06:09:01 PDT
I agree with Zecke and don't see any larger issues with privacy, as it is disabled by default.

Still it feels a big strange storing this are settings, but maybe that is just me.
Comment 10 Adam Treat 2009-06-18 10:05:40 PDT
 118 static void initializeWebCoreIfNeeded()
 119 {
 120     static bool called = false;
 121     if (!called)
 122         return;
 123 
 124     called = true;

I believe that should be 'if (called) no?

Also, I think you've addressed the privacy issues, but might I suggest adding API so clients can very easily delete the stored settings much like they would do when clearing cookies, history, etc?
Comment 11 Adam Treat 2009-06-18 10:09:35 PDT
Comment on attachment 30606 [details]
Implementation for Qt using QSettings and QWebSettings with ChangeLog

> +        past search queries should be used for search fields. The search queries will be
> +        store with QSettings, for this to work make sure to call

Minor nit, but that should be 'stored' I believe.

>  void SearchPopupMenuDelegate::saveRecentSearches(const AtomicString& name, const Vector<String>& searchItems, Document*)
>  {
> +    if (name.isEmpty())
> +        return;
> +
> +    // put into the QStringList
> +    QStringList urls;
> +    for (int i = 0; i < searchItems.size(); ++i)
> +        urls.append(searchItems[i]);

Shouldn't we have a maximum number of entries here?

Adam
Comment 12 Jesus Sanchez-Palencia 2010-05-13 14:01:08 PDT
Is this still valid? Long time since the last update here...