Bug 40353 – [Qt] Remove superfluous function calls

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anders Bakken]

[Qt] Remove superfluous function calls

[Anders Bakken]

Created attachment 58217 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 58217 [details]
Patch

Why are the superfluous?  Specifically the second one?  Please explain in your ChangeLog.

[Csaba Osztrogonác]

Modifying of QWebFrame::setUrl(...) LGTM.

But I'm not convinced if QWebFrame::load(...) is correct,
because load is a public API function, not only
the setUrl function can call it with absolute URL.

But the question is if we would like to apply the 
first API (binary) change before Qt-4.7.0 release.
Kenneth? Simon?

[Anders Bakken]

Created attachment 58661 [details]
Patch

[Anders Bakken]

New patch with better description of why the calls are superfluous is uploadd.

[Kenneth Rohde Christiansen]

Comment on attachment 58661 [details]
Patch

WebKit/qt/Api/qwebframe.cpp:716
 +      load(QNetworkRequest(url));
You should add a comment here to why the ensureAbso... is not needed here.

[Simon Hausmann]

Nice change :)

[Anders Bakken]

@Kenneth:

You mean an inline comment?

E.g.

void QWebFrame::load(const QUrl &url) 
{ 
    // No need to call ensureAbsoluteUrl() here since ::load(QNetworkRequest) 
    // will do it.
    load(QNetworkRequest(url)); 
} 

It seems a little un-qt-like to me I have to admit.

[Kenneth Rohde Christiansen]

(In reply to comment #8)
> @Kenneth:
> 
> You mean an inline comment?
> 
> E.g.
> 
> void QWebFrame::load(const QUrl &url) 
> { 
>     // No need to call ensureAbsoluteUrl() here since ::load(QNetworkRequest) 
>     // will do it.
>     load(QNetworkRequest(url)); 
> } 
> 
> It seems a little un-qt-like to me I have to admit.

Well, not very un-WebKitish anyway. And apparently someone added the ensureAb... before.

Something like:

// The load() overload ensures that the url is absolute.
load(QNetworkRequest(url));

[Anders Bakken]

Created attachment 58672 [details]
Patch

[Csaba Osztrogonác]

(In reply to comment #10)
> Created an attachment (id=58672) [details]
> Patch

LGTM, I see why both of them are superfluous.

Only a little nit-pick, please remove the unnecessary
newline from the end of ChangeLog before landing.
(after Kenneth give you an r+ :)

[Anders Bakken]

Created attachment 58676 [details]
Patch

[Csaba Osztrogonác]

(In reply to comment #12)
> Created an attachment (id=58676) [details]
> Patch

Kenneth r+ -ed your last patch, just commit it with 
this minor change. You don't have to wait for r+ again.

[Simon Hausmann]

Comment on attachment 58676 [details]
Patch

r=me

Anders, please remember to set cq? if you don't want to land the change yourself.

[WebKit Commit Bot]

Comment on attachment 58676 [details]
Patch

Clearing flags on attachment: 58676

Committed r62903: <http://trac.webkit.org/changeset/62903>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.