[Qt] Remove superfluous function calls
Created attachment 58217 [details] Patch
Comment on attachment 58217 [details] Patch Why are the superfluous? Specifically the second one? Please explain in your ChangeLog.
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?
Created attachment 58661 [details] Patch
New patch with better description of why the calls are superfluous is uploadd.
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.
Nice change :)
@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.
(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));
Created attachment 58672 [details] Patch
(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+ :)
Created attachment 58676 [details] Patch
(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 on attachment 58676 [details] Patch r=me Anders, please remember to set cq? if you don't want to land the change yourself.
Comment on attachment 58676 [details] Patch Clearing flags on attachment: 58676 Committed r62903: <http://trac.webkit.org/changeset/62903>
All reviewed patches have been landed. Closing bug.