Bug 36451

Summary: [Qt] User Agent Switcher on QtLauncher
Product: WebKit Reporter: Diego Gonzalez <diegohcg>
Component: WebKit QtAssignee: Diego Gonzalez <diegohcg>
Status: CLOSED FIXED    
Severity: Normal CC: hausmann, kenneth
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 35784    
Attachments:
Description Flags
user agent dialog
none
user agents predefined list
none
edit manually the user agent string
none
Proposed patch
hausmann: review-
Proposed patch
none
Proposed patch
hausmann: review+
Safari Develop->User Agent menu
none
Safari Develop->User Agent->Other none

Description Diego Gonzalez 2010-03-22 10:23:42 PDT
Make possible QtLauncher access pages that restrict access based
on the browser being used and/or simply test the browser with
different user agents.
Comment 1 Diego Gonzalez 2010-03-22 10:29:26 PDT
Created attachment 51302 [details]
user agent dialog
Comment 2 Diego Gonzalez 2010-03-22 10:32:08 PDT
Created attachment 51303 [details]
user agents predefined list
Comment 3 Diego Gonzalez 2010-03-22 10:35:02 PDT
Created attachment 51304 [details]
edit manually the user agent string

If the edited string is not in the user agents list it will be stored
Comment 4 Diego Gonzalez 2010-03-22 10:58:04 PDT
Created attachment 51309 [details]
Proposed patch
Comment 5 Simon Hausmann 2010-03-22 14:37:25 PDT
Comment on attachment 51309 [details]
Proposed patch


> +void LauncherWindow::showUserAgentDialog()
> +{
> +    QFile file("WebKitTools/QtLauncher/useragentlist.txt");

Instead of requiring that QtLauncher is started from the top-level directory by
using a relative path here, I think it would be better to compile the user agent list
into the application using a Qt resource.

> +    , m_userAgent(QString())

This initialization isn't necessary.

> +    void setUserAgent(QString ua) { m_userAgent = ua; }

The parameter should be a const QString&.

r- until the above issues are addressed. The rest looks good to me! It's great that you're adding this, it really helps for development/testing.
Comment 6 Diego Gonzalez 2010-03-22 16:28:45 PDT
(In reply to comment #5)
> (From update of attachment 51309 [details])
> 
> > +void LauncherWindow::showUserAgentDialog()
> > +{
> > +    QFile file("WebKitTools/QtLauncher/useragentlist.txt");
> 
> Instead of requiring that QtLauncher is started from the top-level directory by
> using a relative path here, I think it would be better to compile the user
> agent list
> into the application using a Qt resource.
> 

Thanks for the feedback Simon!

Using Qt resource is a good way if I want to only read the data from useragentlist.txt, because afaik we only can open it in ReadOnly mode, so is not possible to store a new user agent in useragentlist at runtime and is needed to recompile if we want the edit the list manually. Do you know if is possible do it usinf Qt Resource or do you have other suggestion? :)
Comment 7 Diego Gonzalez 2010-03-23 11:31:35 PDT
Created attachment 51440 [details]
Proposed patch

Changed according Simon's suggestions
Comment 8 Diego Gonzalez 2010-03-23 11:32:57 PDT
> Using Qt resource is a good way if I want to only read the data from
> useragentlist.txt, because afaik we only can open it in ReadOnly mode, so is
> not possible to store a new user agent in useragentlist at runtime and is
> needed to recompile if we want the edit the list manually. Do you know if is
> possible do it usinf Qt Resource or do you have other suggestion? :)

I am using now QSettings to store and keep the data persistence :)
Comment 9 Diego Gonzalez 2010-03-24 10:23:57 PDT
Created attachment 51518 [details]
Proposed patch

Simplifying the patch to only read and show the user agents from the 
predefined list for this moment.
Comment 10 Daniel Bates 2010-03-24 14:37:00 PDT
Created attachment 51541 [details]
Safari Develop->User Agent menu

Instead of a combined list and (custom user agent text field)?, I would suggest that we add the sub-menu User Agent to the Develop menu in QtLauncher and that it has at least the same options and behavior as the identical menu found in Safari. This will make QtLauncher consistent with Safari. Notice, the last option in the Safari User Agent menu is "Other..." which opens a dialog with an text field that is initially populated with the value of the currently active user agent string.
Comment 11 Daniel Bates 2010-03-24 14:39:06 PDT
Created attachment 51542 [details]
Safari Develop->User Agent->Other
Comment 12 Diego Gonzalez 2010-03-24 15:28:47 PDT
(In reply to comment #10)
> Created an attachment (id=51541) [details]
> Safari Develop->User Agent menu
> 
> Instead of a combined list and (custom user agent text field)?, I would suggest
> that we add the sub-menu User Agent to the Develop menu in QtLauncher and that
> it has at least the same options and behavior as the identical menu found in
> Safari. This will make QtLauncher consistent with Safari. Notice, the last
> option in the Safari User Agent menu is "Other..." which opens a dialog with an
> text field that is initially populated with the value of the currently active
> user agent string.

I thing it can be considered. Kenneth? Simon?
Comment 13 Kenneth Rohde Christiansen 2010-03-24 15:30:20 PDT
Sounds fine with me!
Comment 14 Simon Hausmann 2010-03-25 15:41:47 PDT
Absolutely! :)

Please create a new bugzilla entry for the custom UA entry
Comment 15 Simon Hausmann 2010-03-25 15:46:18 PDT
Comment on attachment 51518 [details]
Proposed patch


> +void LauncherWindow::showUserAgentDialog()
> +{
> +    QStringList items;
> +    QFile file(":/useragentlist.txt");
> +    if (file.open(QIODevice::ReadOnly)) {
> +         while (!file.atEnd())
> +            items << file.readLine().trimmed();
> +        file.close();
> +    }
> +
> +    QDialog* dialog = new QDialog(this);
> +    dialog->setWindowTitle("Change User Agent");
> +
> +    QVBoxLayout* layout = new QVBoxLayout(dialog);
> +    dialog->setLayout(layout);
> +
> +    QComboBox* combo = new QComboBox(dialog);
> +    combo->setMaximumWidth(size().width() * 0.7);
> +    combo->insertItems(0, items);
> +    layout->addWidget(combo);
> +
> +    QDialogButtonBox* buttonBox = new QDialogButtonBox(QDialogButtonBox::Ok
> +            | QDialogButtonBox::Cancel, Qt::Horizontal, dialog);
> +    connect(buttonBox, SIGNAL(accepted()), dialog, SLOT(accept()));
> +    connect(buttonBox, SIGNAL(rejected()), dialog, SLOT(reject()));
> +    layout->addWidget(buttonBox);
> +
> +    if (dialog->exec() && !combo->currentText().isEmpty())
> +        page()->setUserAgent(combo->currentText());
> +}
> +

Please delete the dialog object after use.

Otherwise r=me :)
Comment 16 Diego Gonzalez 2010-03-26 13:01:13 PDT
Landed by tonikitoo at r56636

> Please delete the dialog object after use.
> 
> Otherwise r=me :)
Comment 17 Simon Hausmann 2010-03-28 14:46:46 PDT
cherry-pick-for-backport: <r56636>
Comment 18 Simon Hausmann 2010-03-28 14:48:55 PDT
Revision r56636 cherry-picked into qtwebkit-2.0 with commit 0b6b1ebafb42ed11323faf42afb5eaa4b2ef1df1