Bug 29899 – [Qt] Add a way to stop delayed redirect requests

• Status: CLOSED
• Resolution: FIXED
• Component: WebKit Qt

[Dawit A.]

Currently there is no way one can stop delayed redirect requests such as 

<META HTTP-EQUIV="Refresh" CONTENT="10;URL=http://qt.nokia.org">

tag. Neither QWebView::stop() nor QWebPage::triggerAction(...) prevent this action from being cancelled. Perhaps a new action is needed for this QWebPage::StopRedirect ?

[Tor Arne Vestbø]

Thanks for your bug report.

In the future, please follow the QtWebKit bug reporting guidlines:

http://trac.webkit.org/wiki/QtWebKitContrib#ReportingBugs

In particular:

    - All bugs related to the Qt port of WebKit should have the keyword 'Qt'

[Simon Hausmann]

This seems similar to http://bugreports.qt.nokia.com/browse/QTWEBKIT-4

[Simon Hausmann]

Dawit, do you want to dynamically decide whether to stop a redirection or not or would you prefer just to have an attribute in QWebSettings to turn off support for http-equiv=refresh alltogether?

[Dawit A.]

(In reply to comment #3)
> Dawit, do you want to dynamically decide whether to stop a redirection or not
> or would you prefer just to have an attribute in QWebSettings to turn off
> support for http-equiv=refresh alltogether?

Hi Simon, 

I need the the ability to stop the redirection dynamically.  In my case I want to be able to defer the ability to cancel the redirection to the user application (Konqueror) from kwebkitpart which I am not able to do right now. Thanks.

[Dawit A.]

Created attachment 52217 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect request I...

Here is one possible patch to solve this problem.

[Early Warning System Bot]

Attachment 52217 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1590129

[Dawit A.]

Created attachment 52219 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests II...

Fixed compile problems...

[Early Warning System Bot]

Attachment 52219 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1658047

[Dawit A.]

Created attachment 52241 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests III...

I guess the third time is the charm yet again... *sigh* still getting hang of ?!?! git.

[Dawit A.]

Created attachment 52258 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests IV...

Moved the new StopScheduledPageRefresh to the end of enum WebAction to avoid crashes due to BIC (duh!).
Still not sure about the name I chose however! Perhaps "StopMetaRefresh" would have been better ? Not sure...

[Simon Hausmann]

(In reply to comment #4)
> (In reply to comment #3)
> > Dawit, do you want to dynamically decide whether to stop a redirection or not
> > or would you prefer just to have an attribute in QWebSettings to turn off
> > support for http-equiv=refresh alltogether?
> 
> Hi Simon, 
> 
> I need the the ability to stop the redirection dynamically.  In my case I want
> to be able to defer the ability to cancel the redirection to the user
> application (Konqueror) from kwebkitpart which I am not able to do right now.

I see. What is the use-case for this?

I understand that you'd like to give the user the ability to prevent a redirect, but at the same time for pages that atuomatically refresh themselves the user gets the impression that the page never finished loading. That doesn't seem correct either.

[Simon Hausmann]

Let me rephrase: Deferring this to the end user seems confusing.

I'm not wildly opposed to adding new API, but at the same time I'd like to avoid that the API becomes too confusing for developers, too :). The action itself is not useful unless you also have the extra code to determine the existance of the refresh.

[Simon Hausmann]

Comment on attachment 52258 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests IV...

r- because of the coding style issues (* placement, space after it declaration and {} for one-line body).

However I would be interested in hearing the opinion of a UI designer on this feature in general. Kenneth, any chance you could prod one of your guys and see if there's a use-case?

[Dawit A.]

(In reply to comment #11)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Dawit, do you want to dynamically decide whether to stop a redirection or not
> > > or would you prefer just to have an attribute in QWebSettings to turn off
> > > support for http-equiv=refresh alltogether?
> > 
> > Hi Simon, 
> > 
> > I need the the ability to stop the redirection dynamically.  In my case I want
> > to be able to defer the ability to cancel the redirection to the user
> > application (Konqueror) from kwebkitpart which I am not able to do right now.
> 
> I see. What is the use-case for this?
> 
> I understand that you'd like to give the user the ability to prevent a
> redirect, but at the same time for pages that atuomatically refresh themselves
> the user gets the impression that the page never finished loading. That doesn't
> seem correct either.

Well actually that depends on your implementation does it not ? In Konqueror for example the spinning wheel will stop, but the Stop button will not be disabled if there are redirections. It has been that way since ages KDE 2.x days. And I remember this because... I was the one that implemented the last half of this spec as a result of a bug report:)  Anyhow, to further avoid confusing the user, one can simply add a tooltip to the Stop button to indicate that there is a pending page refresh or redirect ; so that should not be a problem.

However, even if such feature is not a requirement in all or even most clients. It is simply not right for me to not have the ability to stop page loading completely. That is right now I can stop the current page load, but not the scheduled refresh ; so IMHO this functionality by itself is essential to prevent this inconsistency.

[Dawit A.]

(In reply to comment #12)
> Let me rephrase: Deferring this to the end user seems confusing.

This has existed for ages in Konqueror without a complaint for a long time now. :) But even without the need for deferring stuff to the user, there must be a mechanism to stop the refresh if the developer so chooses. Just like there is one for stopping the loading of the page, no ? I mean, right now I can cancel the page being loaded, but even if I do that and it contains a refresh meta tag, the next page load will happen anyways. To put it simply, deferring to the user is but only one example of where such an action is of use for me.
 
> I'm not wildly opposed to adding new API, but at the same time I'd like to
> avoid that the API becomes too confusing for developers, too :). The action
> itself is not useful unless you also have the extra code to determine the
> existance of the refresh.

Determining the existence of a refresh tag is very very simple thanks to QWebElement and is mostly a one liner thing to do:

frame->findAllElements(QLatin1String("head > meta[http-equiv=refresh]")); OR
frame->findAllElements(QLatin1String("meta[http-equiv=refresh]"));

Then you can use isEmpty() or the QWebElement collection to findout anything you want about this ; so does that really need an additional API ? Unlike finding out there is a refresh element or not, there is no mechanism to stop the redirection that I can think of... Even attempting to remove the element does not seem to solve the problem here...

[Dawit A.]

(In reply to comment #13)
> (From update of attachment 52258 [details])
> r- because of the coding style issues (* placement, space after it declaration
> and {} for one-line body).

hmm... confused about that. It passed the style check both locally and through the  bot. Is the style for the Qt specific poritions of the code different than what is required for webkit itself ?

[Simon Hausmann]

(In reply to comment #16)
> (In reply to comment #13)
> > (From update of attachment 52258 [details] [details])
> > r- because of the coding style issues (* placement, space after it declaration
> > and {} for one-line body).
> 
> hmm... confused about that. It passed the style check both locally and through
> the  bot. Is the style for the Qt specific poritions of the code different than
> what is required for webkit itself ?

The style checker isn't perfect :). Although... it's likely that it's skipping these files currently.

[Dawit A.]

Created attachment 53857 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests V...

Updated patch to fix the style issues that caused the rejection...

[Kenneth Rohde Christiansen]

I'm kind of OK with the change and I don't find the action so confusing.

"but at the same time for pages that atuomatically refresh themselves
the user gets the impression that the page never finished loading.", why? Maybe I didn't understand something there.

We are having Tamara (UX designer) have a look at the use-cases.

[Jesus Sanchez-Palencia]

(In reply to comment #13)
> However I would be interested in hearing the opinion of a UI designer on this
> feature in general. Kenneth, any chance you could prod one of your guys and see
> if there's a use-case?

I talked to Tamara Baia, from INdT's Design Team, and she said that the only
use-case she could think of was the one stated here: to prevent a web page of
keep refreshing when you are there.
She even gave a good example where this can be helpful: a local news website
which also has info about movies, concerts, etc. Sometimes you are watching a
movie trailer on it and it keeps refreshing after each minute because of the
news, so you can't ever finish seeing the movie trailer which is 2 minutes
long.

Antonio raised something interesting: we are not aware of any mobile browser
that does auto refresh by default. But this would need to be confirmed.

[Dawit A.]

Perhaps I should have given more information, use cases. The whole idea behind <mea> refresh is completely DEPRECATED by w3c. See the links provided below[1][2]. 
Amongst other things you can easily disorient the user if you refresh/redirect the page while the user is viewing the website. I think that should be very clear.

The problem here remains the same. There is no way you can stop this redirection in QtWebKit without resorting to hacks of removing that <meta> element from the page manually yourself. Not even sure that will work either...

[1] http://www.w3.org/TR/WCAG10-HTML-TECHS/#meta-element
[2] http://en.wikipedia.org/wiki/Meta_refresh

[Dawit A.]

(In reply to comment #20)
> (In reply to comment #13)
> > However I would be interested in hearing the opinion of a UI designer on this
> > feature in general. Kenneth, any chance you could prod one of your guys and see
> > if there's a use-case?
> 
> I talked to Tamara Baia, from INdT's Design Team, and she said that the only
> use-case she could think of was the one stated here: to prevent a web page of
> keep refreshing when you are there.
> She even gave a good example where this can be helpful: a local news website
> which also has info about movies, concerts, etc. Sometimes you are watching a
> movie trailer on it and it keeps refreshing after each minute because of the
> news, so you can't ever finish seeing the movie trailer which is 2 minutes
> long.
> 
> Antonio raised something interesting: we are not aware of any mobile browser
> that does auto refresh by default. But this would need to be confirmed.

Hmm... if any of these mobile browsers are based on webkit, then they must have a code somewhere in their platform specific section that disables this feature. Otherwise, I do not see how they can possibly avoid it because it is part of webkit itself.

[Kenneth Rohde Christiansen]

> Hmm... if any of these mobile browsers are based on webkit, then they must have a code somewhere in their platform specific section that disables this feature. Otherwise, I do not see how they can possibly avoid it because it is part of webkit itself.

Jesus, can you confirm this? if not, please ping me tomorrow and I will test it on the iphone.

[Kenneth Rohde Christiansen]

Comment on attachment 53857 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests V...

WebKit/qt/Api/qwebpage.cpp:2371
 +              QWebFrame *topFrame = mainFrame();
Coding style issue, * is placed wrong.

WebKit/qt/Api/qwebpage.cpp:2373
 +              QListIterator<QWebFrame *> it (topFrame->childFrames());
Should be <QWebFrame*> it(topFrame...

WebKit/qt/Api/qwebpage.cpp:1680
 +      \value StopScheduledPageRefresh Stop all pending page refesh/redirect requests.
Spelling issue, 'refesh'

r- due to these issues.

[Dawit A.]

Created attachment 56891 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests VI...

Fixed coding style issues...

[Kenneth Rohde Christiansen]

Comment on attachment 56891 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests VI...

LGTM, r=me

[Kenneth Rohde Christiansen]

Could you make a autotest for this?

[Dawit A.]

(In reply to comment #27)
> Could you make a autotest for this?

Well... I can definitely add an autotest to check the state of the webaction, i.e. the StopScheduledPageRefresh. However, to check whether or not the redirection has actually been prevented once the webaction was activated would require the use of something like a QEventLoop to wait for a duration greater than that of the refresh timer and verify that the url has not changed afterwards. Is QEventLoop something that can used in the autotest classes ? Also I presume the autotest should be added to "WebKit/qt/tests/qwebpage/tst_qwebpage.cpp", right ?

[Dawit A.]

Created attachment 56934 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests VII...

Corrected the patch and added autotest to exercise the new feature as requested...

[Kenneth Rohde Christiansen]

I dont like so much that you depend on an external page. Can't you create a fake QNetworkAccessManager or so? I believe that other tests are doing that. Also, if you do so, please minimize the timeout.

[Dawit A.]

Created attachment 56966 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests VIII...

Modified the autotest code as follows:
   ** Used a fake networkaccessmanager to avoid the external link dependency.
   ** Reduced the timeout values from a total of 8 sec. down to 2 sec.

[Simon Hausmann]

I'm removing this from the list of bugs that block the 2.0 release. This is an enhancement, not a bug for a crash, data corruption or a regression.

However I see the importance of WebKit in KDE, so if the patch makes it in time before the release then let's include it. Therefore it's now blocking 39313.

[Simon Hausmann]

Comment on attachment 56966 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests VIII...

    }
> +        case StopScheduledPageRefresh: {
> +            QWebFrame* topFrame = mainFrame();
> +            topFrame->d->frame->redirectScheduler()->cancel();
> +            QListIterator<QWebFrame*> it(topFrame->childFrames());
> +            while (it.hasNext())
> +                it.next()->d->frame->redirectScheduler()->cancel();

Shouldn't this be done recursively?

As far as I can see now the cancellation is done only for the main frame and its direct children. What about children of the child frames of the main frame? :)


The rest looks good to me. Dawit, do you want us to land the patch manually or go through the commit-queue?

[Dawit A.]

(In reply to comment #33)
> (From update of attachment 56966 [details])
>     }
> > +        case StopScheduledPageRefresh: {
> > +            QWebFrame* topFrame = mainFrame();
> > +            topFrame->d->frame->redirectScheduler()->cancel();
> > +            QListIterator<QWebFrame*> it(topFrame->childFrames());
> > +            while (it.hasNext())
> > +                it.next()->d->frame->redirectScheduler()->cancel();
> 
> Shouldn't this be done recursively?

Hmm. Good question...

> As far as I can see now the cancellation is done only for the main frame and its 
> direct children. What about children of the child frames of the main frame? :)

At what level should we stop the recursion ? Until there are no more child frames ? Perhaps using something like the function below...

static QList<QWebFrame*> findAllChildFrames(QWebFrame* frame)
{
    QList<QWebFrame*> frames = frame->childFrames();
    QListIterator<QWebFrame*> it (frame->childFrames());
    while (it.hasNext()) {
        frames << findAllChildFrames(it.next());
    }    
    return frames;
}

> The rest looks good to me. Dawit, do you want us to land the patch manually or 
> go through the commit-queue?

Whichever way is the easiest for you is fine by me...

[Simon Hausmann]

(In reply to comment #34)
> (In reply to comment #33)
> > (From update of attachment 56966 [details] [details])
> >     }
> > > +        case StopScheduledPageRefresh: {
> > > +            QWebFrame* topFrame = mainFrame();
> > > +            topFrame->d->frame->redirectScheduler()->cancel();
> > > +            QListIterator<QWebFrame*> it(topFrame->childFrames());
> > > +            while (it.hasNext())
> > > +                it.next()->d->frame->redirectScheduler()->cancel();
> > 
> > Shouldn't this be done recursively?
> 
> Hmm. Good question...
> 
> > As far as I can see now the cancellation is done only for the main frame and its 
> > direct children. What about children of the child frames of the main frame? :)
> 
> At what level should we stop the recursion ? Until there are no more child frames ? Perhaps using something like the function below...
> 
> static QList<QWebFrame*> findAllChildFrames(QWebFrame* frame)
> {
>     QList<QWebFrame*> frames = frame->childFrames();
>     QListIterator<QWebFrame*> it (frame->childFrames());
>     while (it.hasNext()) {
>         frames << findAllChildFrames(it.next());
>     }    
>     return frames;
> }

Yeah, something like that. To avoid excessive copying it'd be nice to always append to the same list, something like:

static void collectChildFrames(QWebFrame* frame, QList<QWebFrame*>& list);

> > The rest looks good to me. Dawit, do you want us to land the patch manually or 
> > go through the commit-queue?
> 
> Whichever way is the easiest for you is fine by me...

Commit-queue is the easiest, so feel free to set commit-queue? for your patches :)

[Simon Hausmann]

Comment on attachment 56966 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests VIII...

r- because of the missing recursion. The rest looks good :)

[Dawit A.]

Created attachment 57412 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests IX...

Updated patch as suggested in comment #35 by Simon...

[Kenneth Rohde Christiansen]

Comment on attachment 57412 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests IX...

2122     while (it.hasNext()) {
 2123         collectChildFrames(it.next(), list);
 2124     }

It should be without braces :-) Please read the coding style guide

This is not correct coding style

[WebKit Commit Bot]

Comment on attachment 57412 [details]
New WebAction, StopScheduledPageRefresh, to stop all page refresh/redirect requests IX...

Clearing flags on attachment: 57412

Committed r60410: <http://trac.webkit.org/changeset/60410>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r60410 cherry-picked into qtwebkit-2.0 with commit 59ecfcc719190fea3b1340967bce210ae6457425