Bug 48440

Summary: [Qt] QtTestBrowser: User input lost when toggling use of QGraphicsView
Product: WebKit Reporter: Dinu Jacob <dinu.jacob>
Component: Tools / TestsAssignee: Dinu Jacob <dinu.jacob>
Status: RESOLVED FIXED    
Severity: Minor CC: commit-queue, cshu, kenneth, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Patch
kling: review-
Patch with review comment changes
none
Style fixed
kenneth: review-
Patch with review comment changes
none
With style fix
none
Preserve user input
none
With style fix
none
Updated with review comment none

Description Dinu Jacob 2010-10-27 10:42:07 PDT
1. Open a launcher window
2. Type in google.com
3. Select Develop->QGraphicsView-> Toggle use of QGraphicsView

Result: View changed but 'google.com' is removed from the url text box.

Related use case: After loading a page, and the user enters a new url and then changes to graphicsview, old page is reloaded and not the new url entered by the user.
Comment 1 Dinu Jacob 2010-10-28 06:45:23 PDT
Created attachment 72182 [details]
Patch
Comment 2 Andreas Kling 2010-10-28 07:00:09 PDT
Comment on attachment 72182 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72182&action=review

First off, this behavior sounds a bit strange behavior to me, shouldn't we load what's currently loaded, and simply preserve the text in the location edit?

> WebKitTools/QtTestBrowser/launcherwindow.cpp:97
> +    QUrl url =  getAddressUrl();

Extra space after =

> WebKitTools/QtTestBrowser/mainwindow.h:56
> +    QString getAddressUrl() const;

Name should be "addressUrl"
Comment 3 Dinu Jacob 2010-10-28 07:08:50 PDT
(In reply to comment #2)
> (From update of attachment 72182 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72182&action=review
> 
> First off, this behavior sounds a bit strange behavior to me, shouldn't we load what's currently loaded, and simply preserve the text in the location edit?
> 
> > WebKitTools/QtTestBrowser/launcherwindow.cpp:97
> > +    QUrl url =  getAddressUrl();
> 
> Extra space after =
> 
> > WebKitTools/QtTestBrowser/mainwindow.h:56
> > +    QString getAddressUrl() const;
> 
> Name should be "addressUrl"

As the page was being loaded when the view is toggled, I used the user input rather than the page to avoid having to request the page to be loaded again. If the current page is loaded and the user input is preserved, it would be confusing as there is indication of a page being loaded but that doesn't match the user input.
Comment 4 Dinu Jacob 2010-10-28 09:33:38 PDT
Created attachment 72196 [details]
Patch with review comment changes
Comment 5 WebKit Review Bot 2010-10-28 09:36:22 PDT
Attachment 72196 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/QtTestBrowser/launcherwindow.cpp:100:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Dinu Jacob 2010-10-28 09:39:08 PDT
Created attachment 72198 [details]
Style fixed
Comment 7 Kenneth Rohde Christiansen 2010-10-28 09:41:38 PDT
Comment on attachment 72198 [details]
Style fixed

View in context: https://bugs.webkit.org/attachment.cgi?id=72198&action=review

> WebKitTools/QtTestBrowser/launcherwindow.cpp:97
> +    QUrl url = addressUrl();

In your code below (impl of addressUrl) you return a string, which you convert into an QUrl here.

> WebKitTools/QtTestBrowser/launcherwindow.cpp:143
> +        load(url.toString());

Here you convert it into a string again!

> WebKitTools/QtTestBrowser/mainwindow.cpp:120
> +QString MainWindow::addressUrl() const
> +{
> +    return urlEdit->text();
> +}

This is what I mean!
Comment 8 Dinu Jacob 2010-10-28 09:52:18 PDT
(In reply to comment #7)
> (From update of attachment 72198 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72198&action=review
> 
> > WebKitTools/QtTestBrowser/launcherwindow.cpp:97
> > +    QUrl url = addressUrl();
> 
> In your code below (impl of addressUrl) you return a string, which you convert into an QUrl here.
> 
> > WebKitTools/QtTestBrowser/launcherwindow.cpp:143
> > +        load(url.toString());
> 
> Here you convert it into a string again!
> 
> > WebKitTools/QtTestBrowser/mainwindow.cpp:120
> > +QString MainWindow::addressUrl() const
> > +{
> > +    return urlEdit->text();
> > +}
> 
> This is what I mean!

load is an overloaded function. I want the one that takes in a string so that the appropriate scheme is added which is done by the version that takes in a string. I convert it into QUrl to check whether it is valid before actually loading it. What can I do differently?
Comment 9 Kenneth Rohde Christiansen 2010-10-28 10:22:03 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 72198 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=72198&action=review
> > 
> > > WebKitTools/QtTestBrowser/launcherwindow.cpp:97
> > > +    QUrl url = addressUrl();
> > 
> > In your code below (impl of addressUrl) you return a string, which you convert into an QUrl here.
> > 
> > > WebKitTools/QtTestBrowser/launcherwindow.cpp:143
> > > +        load(url.toString());
> > 
> > Here you convert it into a string again!
> > 
> > > WebKitTools/QtTestBrowser/mainwindow.cpp:120
> > > +QString MainWindow::addressUrl() const
> > > +{
> > > +    return urlEdit->text();
> > > +}
> > 
> > This is what I mean!
> 
> load is an overloaded function. I want the one that takes in a string so that the appropriate scheme is added which is done by the version that takes in a string. I convert it into QUrl to check whether it is valid before actually loading it. What can I do differently?

We have a method that handles this QUrl::fromUserInput(). I'm pretty sure we are already using this in the test browser. At the end, WebKit expects a QUrl.
Comment 10 Antonio Gomes 2010-10-28 10:42:00 PDT
(In reply to comment #0)
> 1. Open a launcher window
> 2. Type in google.com
> 3. Select Develop->QGraphicsView-> Toggle use of QGraphicsView
> 
> Result: View changed but 'google.com' is removed from the url text box.
> 
> Related use case: After loading a page, and the user enters a new url and then changes to graphicsview, old page is reloaded and not the new url entered by the user.

I can not reproduce it on trunk. Are you running 2.1 branch?
Comment 11 Dinu Jacob 2010-10-28 10:57:09 PDT
(In reply to comment #10)
> (In reply to comment #0)
> > 1. Open a launcher window
> > 2. Type in google.com
> > 3. Select Develop->QGraphicsView-> Toggle use of QGraphicsView
> > 
> > Result: View changed but 'google.com' is removed from the url text box.
> > 
> > Related use case: After loading a page, and the user enters a new url and then changes to graphicsview, old page is reloaded and not the new url entered by the user.
> 
> I can not reproduce it on trunk. Are you running 2.1 branch?

No, I am on trunk.
Comment 12 Dinu Jacob 2010-10-28 11:03:21 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (From update of attachment 72198 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=72198&action=review
> > > 
> > > > WebKitTools/QtTestBrowser/launcherwindow.cpp:97
> > > > +    QUrl url = addressUrl();
> > > 
> > > In your code below (impl of addressUrl) you return a string, which you convert into an QUrl here.
> > > 
> > > > WebKitTools/QtTestBrowser/launcherwindow.cpp:143
> > > > +        load(url.toString());
> > > 
> > > Here you convert it into a string again!
> > > 
> > > > WebKitTools/QtTestBrowser/mainwindow.cpp:120
> > > > +QString MainWindow::addressUrl() const
> > > > +{
> > > > +    return urlEdit->text();
> > > > +}
> > > 
> > > This is what I mean!
> > 
> > load is an overloaded function. I want the one that takes in a string so that the appropriate scheme is added which is done by the version that takes in a string. I convert it into QUrl to check whether it is valid before actually loading it. What can I do differently?
> 
> We have a method that handles this QUrl::fromUserInput(). I'm pretty sure we are already using this in the test browser. At the end, WebKit expects a QUrl.

Yes, that is what MainWindow::load(const QString& url) is using too but in addition to that it it also converts relative file path to absolute file path and I am just continuing to use that.
Comment 13 Dinu Jacob 2010-10-28 13:23:19 PDT
(In reply to comment #12)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > (From update of attachment 72198 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=72198&action=review
> > > > 
> > > > > WebKitTools/QtTestBrowser/launcherwindow.cpp:97
> > > > > +    QUrl url = addressUrl();
> > > > 
> > > > In your code below (impl of addressUrl) you return a string, which you convert into an QUrl here.
> > > > 
> > > > > WebKitTools/QtTestBrowser/launcherwindow.cpp:143
> > > > > +        load(url.toString());
> > > > 
> > > > Here you convert it into a string again!
> > > > 
> > > > > WebKitTools/QtTestBrowser/mainwindow.cpp:120
> > > > > +QString MainWindow::addressUrl() const
> > > > > +{
> > > > > +    return urlEdit->text();
> > > > > +}
> > > > 
> > > > This is what I mean!
> > > 
> > > load is an overloaded function. I want the one that takes in a string so that the appropriate scheme is added which is done by the version that takes in a string. I convert it into QUrl to check whether it is valid before actually loading it. What can I do differently?
> > 
> > We have a method that handles this QUrl::fromUserInput(). I'm pretty sure we are already using this in the test browser. At the end, WebKit expects a QUrl.
> 
> Yes, that is what MainWindow::load(const QString& url) is using too but in addition to that it it also converts relative file path to absolute file path and I am just continuing to use that.

So to use the overloaded load method that takes in a QString and to check whether the url is valid before calling load, I need the conversion from Url to String again.
Comment 14 Dinu Jacob 2010-10-29 11:22:13 PDT
Created attachment 72352 [details]
Patch with review comment changes

Changed logic to avoid converting to QString->QUrl->QString. Also, caling MainWindow::load instead of page()->mainFrame()->load to set the input field value that would otherwise be removed when toggling view (and set only later)
Comment 15 WebKit Review Bot 2010-10-29 11:25:53 PDT
Attachment 72352 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/QtTestBrowser/launcherwindow.cpp:98:  Extra space before ) in if  [whitespace/parens] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Dinu Jacob 2010-10-29 11:58:57 PDT
Created attachment 72359 [details]
With style fix
Comment 17 Antonio Gomes 2010-10-29 12:02:22 PDT
I am on trunk link, running qt 4.7 and am not able to run into this bug.

Could you please help me to reproduce it then I can help reviewing and testing.
Comment 18 Antonio Gomes 2010-10-29 12:03:09 PDT
(In reply to comment #17)
> I am on trunk link,

link = linux
Comment 19 Dinu Jacob 2010-11-01 08:55:23 PDT
(In reply to comment #17)
> I am on trunk link, running qt 4.7 and am not able to run into this bug.
> 
> Could you please help me to reproduce it then I can help reviewing and testing.

Provided clarification to Antonio on IRC
Comment 20 Antonio Gomes 2010-11-01 22:53:08 PDT
Comment on attachment 72359 [details]
With style fix

Before reviewing, with this patch we would end up with the following behavior:

1) load qttestbrowser
2) type 'google.com' in the urlbar
(do NOT press ENTER or any other action that triggers loading)
3) switch to graphicsview mode.
result: google.com is loaded.

do we want this, or we want to keep the loaded current URL (about:blank in this case) and keep the current entered content in the urlbar ('google.com' in this case)?
Comment 21 Dinu Jacob 2010-11-02 07:47:19 PDT
(In reply to comment #20)
> (From update of attachment 72359 [details])
> Before reviewing, with this patch we would end up with the following behavior:
> 
> 1) load qttestbrowser
> 2) type 'google.com' in the urlbar
> (do NOT press ENTER or any other action that triggers loading)
> 3) switch to graphicsview mode.
> result: google.com is loaded.
> 
> do we want this, or we want to keep the loaded current URL (about:blank in this case) and keep the current entered content in the urlbar ('google.com' in this case)?

I used the entered url to avoid doing another load (if a page has already been loaded, it will re-load the page and then the user has to press ENTER key to load the new page again). Also, the url displayed will not be consistent with the page loaded.
Comment 22 Antonio Gomes 2010-11-03 10:50:55 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 72359 [details] [details])
> > Before reviewing, with this patch we would end up with the following behavior:
> > 
> > 1) load qttestbrowser
> > 2) type 'google.com' in the urlbar
> > (do NOT press ENTER or any other action that triggers loading)
> > 3) switch to graphicsview mode.
> > result: google.com is loaded.
> > 
> > do we want this, or we want to keep the loaded current URL (about:blank in this case) and keep the current entered content in the urlbar ('google.com' in this case)?
> 
> I used the entered url to avoid doing another load (if a page has already been loaded, it will re-load the page and then the user has to press ENTER key to load the new page again). Also, the url displayed will not be consistent with the page loaded.

Kenneth, KLing?
Comment 23 Dinu Jacob 2011-01-17 10:14:19 PST
Created attachment 79184 [details]
Preserve user input
Comment 24 WebKit Review Bot 2011-01-17 10:16:00 PST
Attachment 79184 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QtTestBrowser/la..." exit_code: 1

Tools/QtTestBrowser/launcherwindow.cpp:143:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/QtTestBrowser/launcherwindow.cpp:144:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Tools/QtTestBrowser/launcherwindow.cpp:536:  Extra space before ) in if  [whitespace/parens] [5]
Tools/QtTestBrowser/launcherwindow.cpp:538:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/QtTestBrowser/launcherwindow.cpp:539:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Dinu Jacob 2011-01-17 10:17:27 PST
New changes made where the current page is loaded but preserve the user input, based on comments received.
Comment 26 Dinu Jacob 2011-01-17 10:23:26 PST
Created attachment 79186 [details]
With style fix
Comment 27 Antonio Gomes 2011-01-17 10:27:02 PST
Comment on attachment 79186 [details]
With style fix

View in context: https://bugs.webkit.org/attachment.cgi?id=79186&action=review

> Tools/QtTestBrowser/launcherwindow.h:210
> +    QString inputUrl;

m_inputUrl as the other?
Comment 28 Dinu Jacob 2011-01-17 10:32:47 PST
Created attachment 79187 [details]
Updated with review comment
Comment 29 Andreas Kling 2011-01-26 16:20:44 PST
Comment on attachment 79187 [details]
Updated with review comment

r=me
Comment 30 WebKit Commit Bot 2011-01-27 16:22:03 PST
Comment on attachment 79187 [details]
Updated with review comment

Clearing flags on attachment: 79187

Committed r76860: <http://trac.webkit.org/changeset/76860>
Comment 31 WebKit Commit Bot 2011-01-27 16:22:08 PST
All reviewed patches have been landed.  Closing bug.