Bug 30216 - [Qt] better handle possible edge cases on qwebframe::requestedUrl use
Summary: [Qt] better handle possible edge cases on qwebframe::requestedUrl use
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords: Qt
Depends on: 30213
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-08 09:33 PDT by Antonio Gomes
Modified: 2009-10-15 10:20 PDT (History)
1 user (show)

See Also:


Attachments
patch 0.1 - make use or error.failingURL as much as possible (3.00 KB, patch)
2009-10-08 09:36 PDT, Antonio Gomes
eric: review-
Details | Formatted Diff | Diff
patch 0.2 - make use or error.failingURL as much as possible (4.93 KB, patch)
2009-10-08 20:06 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
(committed in r49497) patch 0.3 - make use or error.failingURL as much as possible (5.47 KB, patch)
2009-10-11 21:14 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2009-10-08 09:33:58 PDT
requestedUrl was failing is it was called from within qwebpage::extension reimplementation...
Comment 1 Antonio Gomes 2009-10-08 09:36:38 PDT
Created attachment 40880 [details]
patch 0.1 - make use or error.failingURL as much as possible
Comment 2 Eric Seidel (no email) 2009-10-08 11:09:37 PDT
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.
Comment 3 Antonio Gomes 2009-10-08 20:06:51 PDT
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
Comment 4 Simon Hausmann 2009-10-09 07:38:32 PDT
Is there any way to write a unit test for these edge cases? I'm worried about this function becoming untouchable code.
Comment 5 Antonio Gomes 2009-10-09 10:53:41 PDT
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.
Comment 6 Antonio Gomes 2009-10-11 21:14:24 PDT
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 ...
Comment 7 Simon Hausmann 2009-10-12 22:26:52 PDT
(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 :)
Comment 8 Simon Hausmann 2009-10-12 22:28:29 PDT
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 :)
Comment 9 Antonio Gomes 2009-10-13 03:58:32 PDT
(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
Comment 10 Antonio Gomes 2009-10-15 10:20:14 PDT
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.