Bug 30216 – [Qt] better handle possible edge cases on qwebframe::requestedUrl use

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

[Antonio Gomes]

requestedUrl was failing is it was called from within qwebpage::extension reimplementation...

[Antonio Gomes]

Created attachment 40880 [details]
patch 0.1 - make use or error.failingURL as much as possible

[Eric Seidel (no email)]

Comment on attachment 40880 [details]
patch 0.1 - make use or error.failingURL as much as possible

The ChagneLog doesn't say what this does.

What edge cases?  What is the modification to that if and why?

Why do we need to clear the previous error.  Comments should ideally be sentences, beginning with a capital and ending with a period.

[Antonio Gomes]

Created attachment 40925 [details]
patch 0.2 - make use or error.failingURL as much as possible

* improved changelog description
* code/changes commented.
* fixed issue w/ comment

[Simon Hausmann]

Is there any way to write a unit test for these edge cases? I'm worried about this function becoming untouchable code.

[Antonio Gomes]

simon, thx for you comments.

(In reply to comment #4)
> Is there any way to write a unit test for these edge cases?

The use cases described are already implemented as tests in tst_qwebframe. I am actually making the current implementation "better".

> I'm worried about this function becoming untouchable code.

you mean no one explicitly using it ?

imho (and in defense of requestedUrl :) is can be a important method to offer to the client since many of internal WebCore action's use the "original submitted url" , and not the resolved/redirected url.

e.g. icondatabase, global and session history.

[Antonio Gomes]

Created attachment 41017 [details]
(committed in r49497) patch 0.3 - make use or error.failingURL as much as possible

one more shot ...

* same patch as 0.2, but more legible/cleaner code ...

[Simon Hausmann]

(In reply to comment #5)
> simon, thx for you comments.
> 
> (In reply to comment #4)
> > Is there any way to write a unit test for these edge cases?
> 
> The use cases described are already implemented as tests in tst_qwebframe. I am
> actually making the current implementation "better".

Ahh, I overlooked that. Thanks!

> > I'm worried about this function becoming untouchable code.
> 
> you mean no one explicitly using it ?
> 
> imho (and in defense of requestedUrl :) is can be a important method to offer
> to the client since many of internal WebCore action's use the "original
> submitted url" , and not the resolved/redirected url.
> 
> e.g. icondatabase, global and session history.

No, I was worried that the code because untouchable because it tests many edge cases
without us verifiying them in unit tests. But I'm wrong, you're doing a great effort in testing
this stuff. Sorry :)

[Simon Hausmann]

Comment on attachment 41017 [details]
(committed in r49497) patch 0.3 - make use or error.failingURL as much as possible

r=me

> +        This patch makes requestUrl calls to fallsback to FrameLoaderClient

Small typo here. Would be good to fix when landing :)

[Antonio Gomes]

(In reply to comment #8)
> (From update of attachment 41017 [details])
> r=me
> 
> > +        This patch makes requestUrl calls to fallsback to FrameLoaderClient
> 
> Small typo here. Would be good to fix when landing :)

thx simon

landed in r49497

[Antonio Gomes]

Comment on attachment 41017 [details]
(committed in r49497) patch 0.3 - make use or error.failingURL as much as possible

clearing r+ flag since it has landed.