Bug 25689 – [Qt] Implement SearchPopupMenuQt

• Component: Forms
• Status: RESOLVED INVALID

[Holger Freyther]

Implement the SearchPopupMenu for Qt using QSettings.

[Holger Freyther]

Created attachment 30185 [details]
Implementation using QSettings

Feedback welcome, specially on the ::enabled implementation

[Ariya Hidayat]

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.

[Holger Freyther]

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.

[Holger Freyther]

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,

[George Staikos]

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?

[George Staikos]

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.

[Holger Freyther]

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.

[Holger Freyther]

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?

[Kenneth Rohde Christiansen]

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.

[Adam Treat]

 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?

[Adam Treat]

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

[Jesus Sanchez-Palencia]

Is this still valid? Long time since the last update here...