Bug 40353

Summary: [Qt] Remove superfluous function calls
Product: WebKit Reporter: Anders Bakken <agbakken>
Component: New BugsAssignee: Anders Bakken <agbakken>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, hausmann, jedrzej.nowacki, kenneth, ossy
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Other   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Anders Bakken 2010-06-09 00:11:59 PDT 
[Qt] Remove superfluous function calls
Comment 1 Anders Bakken 2010-06-09 00:13:20 PDT 
Created attachment 58217 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-06-12 18:56:55 PDT 
Comment on attachment 58217 [details]
Patch

Why are the superfluous?  Specifically the second one?  Please explain in your ChangeLog.
Comment 3 Csaba Osztrogonác 2010-06-14 07:54:01 PDT 
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?
Comment 4 Anders Bakken 2010-06-14 09:28:12 PDT 
Created attachment 58661 [details]
Patch
Comment 5 Anders Bakken 2010-06-14 09:28:49 PDT 
New patch with better description of why the calls are superfluous is uploadd.
Comment 6 Kenneth Rohde Christiansen 2010-06-14 09:48:25 PDT 
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.
Comment 7 Simon Hausmann 2010-06-14 10:34:15 PDT 
Nice change :)
Comment 8 Anders Bakken 2010-06-14 10:38:40 PDT 
@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.
Comment 9 Kenneth Rohde Christiansen 2010-06-14 11:03:47 PDT 
(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));
Comment 10 Anders Bakken 2010-06-14 11:07:24 PDT 
Created attachment 58672 [details]
Patch
Comment 11 Csaba Osztrogonác 2010-06-14 11:17:03 PDT 
(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+ :)
Comment 12 Anders Bakken 2010-06-14 11:43:10 PDT 
Created attachment 58676 [details]
Patch
Comment 13 Csaba Osztrogonác 2010-06-14 11:47:17 PDT 
(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.
Comment 14 Simon Hausmann 2010-07-08 23:48:35 PDT 
Comment on attachment 58676 [details]
Patch

r=me

Anders, please remember to set cq? if you don't want to land the change yourself.
Comment 15 WebKit Commit Bot 2010-07-09 00:11:29 PDT 
Comment on attachment 58676 [details]
Patch

Clearing flags on attachment: 58676

Committed r62903: <http://trac.webkit.org/changeset/62903>
Comment 16 WebKit Commit Bot 2010-07-09 00:11:35 PDT 
All reviewed patches have been landed.  Closing bug.