Bug 64989 - [Qt] Fix tst_QDeclarativeWebView::historyNav() autotests
Summary: [Qt] Fix tst_QDeclarativeWebView::historyNav() autotests
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Adenilson Cavalcanti Silva
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 38654 QtWebkit23
  Show dependency treegraph
 
Reported: 2011-07-21 16:00 PDT by Rafael Brandao
Modified: 2014-02-03 03:18 PST (History)
10 users (show)

See Also:


Attachments
Quick fix for the test failures. Force a stop action, so I can assure the QWebPage actions are updated. (3.88 KB, patch)
2011-07-21 16:07 PDT, Rafael Brandao
benjamin: review-
rafael.lobo: commit-queue?
Details | Formatted Diff | Diff
Make possible action 'reload' be active at an earlier stage (543 bytes, patch)
2011-08-12 11:06 PDT, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
patch (1.58 KB, patch)
2011-08-12 14:41 PDT, Adenilson Cavalcanti Silva
benjamin: review-
savagobr: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Brandao 2011-07-21 16:00:58 PDT
XPASS  : tst_QDeclarativeWebView::basicProperties() COMPARE()
   Loc: [/ramdisk/qt-linux-release/build/Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp(105)]
FAIL!  : tst_QDeclarativeWebView::historyNav() 'reloadAction->isEnabled()' returned FALSE. ()
   Loc: [/ramdisk/qt-linux-release/build/Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp(190)]
Comment 1 Rafael Brandao 2011-07-21 16:07:45 PDT
Created attachment 101658 [details]
Quick fix for the test failures. Force a stop action, so I can assure the QWebPage actions are updated.

Even though it was passing there (XPASS) the output is still wrong, but I'll fix it soon.
If anyone else wants to look deeper on this, feel welcome. The problem lies around the lack of updates on QWebPage actions, so at some point we check if it is enabled and it's not updated yet. I've realized when I do the stop action, it calls updateNavigationActions from QWebPagePrivate and it was enough to fix it. Maybe a better fix is to update the actions as soon as the loadFinished is triggered.
Comment 2 Alexis Menard (darktears) 2011-08-01 10:29:14 PDT
Comment on attachment 101658 [details]
Quick fix for the test failures. Force a stop action, so I can assure the QWebPage actions are updated.

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

Though I think it should be fixed in QWebPage, I think waiting the load to finish I expect the reloadAction to be enabled and the rest of them updated. Now not sure it is the priority.

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:-215
> -

Not needed.
Comment 3 Benjamin Poulain 2011-08-06 05:38:12 PDT
Comment on attachment 101658 [details]
Quick fix for the test failures. Force a stop action, so I can assure the QWebPage actions are updated.

That is fixing the test, not the bug.
You should check why updateNavigationActions() is not called correctly.
Comment 4 Adenilson Cavalcanti Silva 2011-08-11 11:14:11 PDT
I started to look in this bug and initially appended a call to updateNavigationActions() at QWebPage::triggerAction just for testing. The test continued to fail.

The state of action 'reload' depends in a FrameLoader object property (!isLoading()). I'm looking now at its usage within QWebPage trying to figure it out how to fix it.
Comment 5 Adenilson Cavalcanti Silva 2011-08-12 11:06:27 PDT
Created attachment 103783 [details]
Make possible action 'reload' be active at an earlier stage
Comment 6 Adenilson Cavalcanti Silva 2011-08-12 11:07:18 PDT
The patch will solve the issue of the testing failing at action reload.

But to be honest I don't think is a good approach, because it would change the behavior of QWebPage. If I understood correctly, WebCore/FrameLoader will only return isLoading() == false when it has loaded everything (the frame, the document, the resources, etc).

While FrameLoader::hasFrameLoaded() instead would return 'true' at an earlier stage (as soon the base frame has loaded). This is based on the article: http://www.webkit.org/blog/1188/how-webkit-loads-a-web-page/
Comment 7 Adenilson Cavalcanti Silva 2011-08-12 14:41:17 PDT
Created attachment 103819 [details]
patch

proposed patch
Comment 8 Antonio Gomes 2011-08-13 19:53:42 PDT
Comment on attachment 103819 [details]
patch

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

> Source/WebKit/qt/ChangeLog:7
> +
> +        Allowing the 'reload' action to be available at an earlier stage.
> +

You could add here why ::frameHasLoaded is different from isLoading, as you added to the bug comment. Also when/if it changed, so the test started failing.
Comment 9 Adenilson Cavalcanti Silva 2011-08-14 20:52:46 PDT
(In reply to comment #8)
> (From update of attachment 103819 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103819&action=review
> 
> > Source/WebKit/qt/ChangeLog:7
> > +
> > +        Allowing the 'reload' action to be available at an earlier stage.
> > +
> 
> You could add here why ::frameHasLoaded is different from isLoading, as you added to the bug comment. Also when/if it changed, so the test started failing.

I have being looking in the subject closer, and located that the second page in this test (loaded from a Qt resource file) used an image (forward.png).

If commenting out the link reference in the html page (i.e. forward.html), the test on action forward state will succeed.

The same happens if the url is a remote url (e.g. google.com), as long there is enough time for loading (by introducing a qWait() call).

So, my guess is that it is a change on FrameLoader behavior while loading a page from a Qt resource (assuming that this test succeeded in the past).
Comment 10 Benjamin Poulain 2011-08-15 04:04:10 PDT
Comment on attachment 103819 [details]
patch

r- because:
-Antonio comments. Such patch really needs a good explanation of why this is the correct way of solving the problem.
-The patch should also enable the failing auto tests.
Comment 11 Csaba Osztrogonác 2012-11-14 05:47:47 PST
Are we still interested in fixing Qt 4.8 related bugs in WebKit 
trunk for QtWebKit 2.3 branch?
Comment 12 Jocelyn Turcotte 2014-02-03 03:18:18 PST
=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.