Bug 64989 – [Qt] Fix tst_QDeclarativeWebView::historyNav() autotests

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKit Qt

[Rafael Brandao]

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)]

[Rafael Brandao]

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.

[Alexis Menard (darktears)]

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.

[Benjamin Poulain]

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.

[Adenilson Cavalcanti Silva]

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.

[Adenilson Cavalcanti Silva]

Created attachment 103783 [details]
Make possible action 'reload' be active at an earlier stage

[Adenilson Cavalcanti Silva]

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/

[Adenilson Cavalcanti Silva]

Created attachment 103819 [details]
patch

proposed patch

[Antonio Gomes]

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.

[Adenilson Cavalcanti Silva]

(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).

[Benjamin Poulain]

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.

[Csaba Osztrogonác]

Are we still interested in fixing Qt 4.8 related bugs in WebKit 
trunk for QtWebKit 2.3 branch?

[Jocelyn Turcotte]

=== 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.