Bug 102151

Summary:

Consolidate FrameLoader::load() into one function taking a FrameLoadRequest

Product:

WebKit

Reporter:

James Simonsen <simonjam>

Component:

New Bugs

Assignee:

James Simonsen <simonjam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, ap, beidson, gyuyoung.kim, japhet, mifenton, ossy, philn, rakuco, roger_fong, rwlbuis, tonikitoo, webkit-ews, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

102834, 103371, 103379

Bug Blocks:

84883

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none
Patch	none

James Simonsen

Reported 2012-11-13 16:35:08 PST

Consolidate FrameLoader::load() into one function taking a FrameLoadRequest

Attachments
Patch (41.93 KB, patch) 2012-11-13 16:40 PST, James Simonsen	no flags	Details Formatted Diff Diff
Patch (43.01 KB, patch) 2012-11-13 17:50 PST, James Simonsen	no flags	Details Formatted Diff Diff
Patch (42.15 KB, patch) 2012-11-14 15:22 PST, James Simonsen	no flags	Details Formatted Diff Diff
Patch (42.16 KB, patch) 2012-11-14 16:22 PST, James Simonsen	no flags	Details Formatted Diff Diff
Patch (42.13 KB, patch) 2012-11-15 16:29 PST, James Simonsen	no flags	Details Formatted Diff Diff
Patch (56.99 KB, patch) 2012-11-19 19:52 PST, James Simonsen	no flags	Details Formatted Diff Diff
Patch (56.32 KB, patch) 2012-11-20 20:27 PST, James Simonsen	no flags	Details Formatted Diff Diff
Patch (56.46 KB, patch) 2012-11-21 15:45 PST, James Simonsen	no flags	Details Formatted Diff Diff
Patch for landing (50.49 KB, patch) 2012-11-26 15:33 PST, James Simonsen	no flags	Details Formatted Diff Diff
Patch (50.35 KB, patch) 2012-11-27 16:13 PST, James Simonsen	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

James Simonsen

Comment 1 2012-11-13 16:40:38 PST

Created attachment 174026 [details] Patch

Early Warning System Bot

Comment 2 2012-11-13 16:51:24 PST

Comment on attachment 174026 [details] Patch Attachment 174026 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14833048

EFL EWS Bot

Comment 3 2012-11-13 17:14:19 PST

Comment on attachment 174026 [details] Patch Attachment 174026 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14817853

Build Bot

Comment 4 2012-11-13 17:16:22 PST

Comment on attachment 174026 [details] Patch Attachment 174026 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14821760

James Simonsen

Comment 5 2012-11-13 17:50:09 PST

Created attachment 174037 [details] Patch

Darin Adler

Comment 6 2012-11-13 17:56:23 PST

Comment on attachment 174037 [details] Patch It’s neat having only one function to call. But almost every single call site is getting significantly more complex and uglier here. Especially the long expression to fetch the security origin. Is there any way to do this without uglifying so many different call sites?

James Simonsen

Comment 7 2012-11-13 18:04:40 PST

(In reply to comment #6) > (From update of attachment 174037 [details]) > It’s neat having only one function to call. But almost every single call site is getting significantly more complex and uglier here. Especially the long expression to fetch the security origin. Is there any way to do this without uglifying so many different call sites? That particular issue could be fixed by deriving the SecurityOrigin from the Frame. That just needs a new constructor for FrameLoadRequest. I did not find a single instance where lockHistory was true. Did I miss it? If not, that could be removed. With those changes, a lot of the call sites could become one-liners ala: m_frame->loader()->load(FrameLoadRequest(m_frame, resourceRequest));

Build Bot

Comment 8 2012-11-13 18:11:40 PST

Comment on attachment 174037 [details] Patch Attachment 174037 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14823621

Adam Barth

Comment 9 2012-11-13 18:29:04 PST

> m_frame->loader()->load(FrameLoadRequest(m_frame, resourceRequest)); That might improve the call sites.

EFL EWS Bot

Comment 10 2012-11-13 19:07:38 PST

Comment on attachment 174037 [details] Patch Attachment 174037 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14665656

EFL EWS Bot

Comment 11 2012-11-13 19:25:44 PST

Comment on attachment 174037 [details] Patch Attachment 174037 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14838062

James Simonsen

Comment 12 2012-11-14 15:22:18 PST

Created attachment 174269 [details] Patch

James Simonsen

Comment 13 2012-11-14 15:25:45 PST

(In reply to comment #9) > > m_frame->loader()->load(FrameLoadRequest(m_frame, resourceRequest)); > > That might improve the call sites. Done. It cleaned up the most common case. We could add similar constructors for the other two cases: plugins (which pass a frame name) and substitute data. Let me know.

Early Warning System Bot

Comment 14 2012-11-14 15:28:52 PST

Comment on attachment 174269 [details] Patch Attachment 174269 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14837493

Early Warning System Bot

Comment 15 2012-11-14 15:37:49 PST

Comment on attachment 174269 [details] Patch Attachment 174269 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14834497

EFL EWS Bot

Comment 16 2012-11-14 16:01:49 PST

Comment on attachment 174269 [details] Patch Attachment 174269 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14848100

Build Bot

Comment 17 2012-11-14 16:19:13 PST

Comment on attachment 174269 [details] Patch Attachment 174269 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14843305

James Simonsen

Comment 18 2012-11-14 16:22:25 PST

Created attachment 174282 [details] Patch

Early Warning System Bot

Comment 19 2012-11-14 16:31:18 PST

Comment on attachment 174282 [details] Patch Attachment 174282 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14848115

Early Warning System Bot

Comment 20 2012-11-14 16:35:49 PST

Comment on attachment 174282 [details] Patch Attachment 174282 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14834516

Build Bot

Comment 21 2012-11-14 21:36:40 PST

Comment on attachment 174282 [details] Patch Attachment 174282 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14832660

Build Bot

Comment 22 2012-11-15 05:21:48 PST

Comment on attachment 174282 [details] Patch Attachment 174282 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14832819

James Simonsen

Comment 23 2012-11-15 16:29:55 PST

Created attachment 174552 [details] Patch

Adam Barth

Comment 24 2012-11-15 17:47:52 PST

Comment on attachment 174552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174552&action=review > Source/WebCore/loader/FrameLoadRequest.h:30 > +#include "Document.h" > +#include "Frame.h" These are some large headers. Do we really need to include them, or can we forward declare the classes? > Source/WebCore/loader/FrameLoadRequest.h:69 > + FrameLoadRequest(Frame* frame, const ResourceRequest& resourceRequest) > + : m_requester(frame->document()->securityOrigin()) > + , m_resourceRequest(resourceRequest) > + , m_lockHistory(false) > + , m_shouldCheckNewWindowPolicy(false) > { > } Maybe we should move this constructor out of line? > Source/WebKit/blackberry/Api/WebPage.cpp:753 > + frameRequest.setSubstituteData(substituteData); I wonder if it would be worth adding an optional SubstituteData parameter to the above given that you've got this pattern in a bunch of places. > Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp:137 > + request.setSubstituteData(SubstituteData(buf, String("text/html"), String("UTF-8"), KURL())); There's no need to call the String constructor explicitly. The compiler should let you call it implicitly. Also, I would probably rename buf to buffer and len to length. This is really a lot of work to load the empty string! > Source/WebKit/mac/Plugins/WebPluginController.mm:407 > + frameRequest.setShouldCheckNewWindowPolicy(true); I like that we don't have these mysterious "false" parameters anymore.

Build Bot

Comment 25 2012-11-15 19:46:24 PST

Comment on attachment 174552 [details] Patch Attachment 174552 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14857465

James Simonsen

Comment 26 2012-11-19 19:52:13 PST

Created attachment 175119 [details] Patch

James Simonsen

Comment 27 2012-11-19 19:53:38 PST

(In reply to comment #24) > (From update of attachment 174552 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174552&action=review > > > Source/WebCore/loader/FrameLoadRequest.h:30 > > +#include "Document.h" > > +#include "Frame.h" > > These are some large headers. Do we really need to include them, or can we forward declare the classes? Done. > > > Source/WebCore/loader/FrameLoadRequest.h:69 > > + FrameLoadRequest(Frame* frame, const ResourceRequest& resourceRequest) > > + : m_requester(frame->document()->securityOrigin()) > > + , m_resourceRequest(resourceRequest) > > + , m_lockHistory(false) > > + , m_shouldCheckNewWindowPolicy(false) > > { > > } > > Maybe we should move this constructor out of line? Done. > > > Source/WebKit/blackberry/Api/WebPage.cpp:753 > > + frameRequest.setSubstituteData(substituteData); > > I wonder if it would be worth adding an optional SubstituteData parameter to the above given that you've got this pattern in a bunch of places. Done. > > > Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp:137 > > + request.setSubstituteData(SubstituteData(buf, String("text/html"), String("UTF-8"), KURL())); > > There's no need to call the String constructor explicitly. The compiler should let you call it implicitly. Done. > > Also, I would probably rename buf to buffer and len to length. This is really a lot of work to load the empty string! Done and agreed. > > > Source/WebKit/mac/Plugins/WebPluginController.mm:407 > > + frameRequest.setShouldCheckNewWindowPolicy(true); > > I like that we don't have these mysterious "false" parameters anymore. Agreed. I'll land this after all the EWS bots go green. It changes a bunch of port-specific code.

WebKit Review Bot

Comment 28 2012-11-20 10:31:16 PST

Comment on attachment 175119 [details] Patch Clearing flags on attachment: 175119 Committed r135295: <http://trac.webkit.org/changeset/135295>

WebKit Review Bot

Comment 29 2012-11-20 10:31:23 PST

All reviewed patches have been landed. Closing bug.

Adam Barth

Comment 30 2012-11-20 11:23:50 PST

Comment on attachment 175119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175119&action=review > Source/WebKit/blackberry/Api/WebPage.cpp:753 > + FrameLoadRequest frameRequest(m_mainFrame, request, substituteData); > + m_mainFrame->loader()->load(frameRequest); These lines can be combined > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:755 > + FrameLoadRequest frameRequest(m_frame, originalRequest, errorData); > + m_frame->loader()->load(frameRequest); ditto > Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp:137 > + FrameLoadRequest request(webFrame->frame(), ResourceRequest(url), SubstituteData(buffer, "text/html", "UTF-8", KURL())); > + webFrame->frame()->loader()->load(request); ditto > Source/WebKit/efl/ewk/ewk_frame.cpp:421 > + WebCore::FrameLoadRequest frameRequest(smartData->frame, request, substituteData); > + smartData->frame->loader()->load(frameRequest); ditto > Source/WebKit/gtk/webkit/webkitwebframe.cpp:717 > + FrameLoadRequest frameRequest(coreFrame, request, substituteData); > + coreFrame->loader()->load(frameRequest); ditto > Source/WebKit/mac/WebView/WebFrame.mm:1405 > + FrameLoadRequest frameRequest(_private->coreFrame, request, substituteData); > + _private->coreFrame->loader()->load(frameRequest); ditto > Source/WebKit/qt/Api/qwebframe.cpp:891 > + FrameLoadRequest frameRequest(d->frame, request, substituteData); > + d->frame->loader()->load(frameRequest); ditto > Source/WebKit/qt/Api/qwebframe.cpp:922 > + FrameLoadRequest frameRequest(d->frame, request, substituteData); > + d->frame->loader()->load(frameRequest); ditto > Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:980 > + FrameLoadRequest frameRequest(coreFrame, request, substituteData); > + coreFrame->loader()->load(frameRequest); ditto > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1169 > + WebCore::FrameLoadRequest frameRequest(m_frame, request, substituteData); > + m_frame->loader()->load(frameRequest); ditto > Source/WebKit/win/WebFrame.cpp:584 > + FrameLoadRequest frameRequest(coreFrame, request, substituteData); > + coreFrame->loader()->load(frameRequest); ditto > Source/WebKit/wx/WebFrame.cpp:328 > + WebCore::FrameLoadRequest frameRequest(m_impl->frame, WebCore::ResourceRequest(url), substituteData); > + m_impl->frame->loader()->load(frameRequest); ditto > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:837 > + FrameLoadRequest frameRequest(m_mainFrame->coreFrame(), request, substituteData); > + m_mainFrame->coreFrame()->loader()->load(frameRequest); ditto

James Simonsen

Comment 31 2012-11-20 11:27:47 PST

(In reply to comment #30) > (From update of attachment 175119 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175119&action=review > > > Source/WebKit/blackberry/Api/WebPage.cpp:753 > > + FrameLoadRequest frameRequest(m_mainFrame, request, substituteData); > > + m_mainFrame->loader()->load(frameRequest); > > These lines can be combined I'll get these in a followup patch. There are still a few other load* functions (loadURLIntoChildFrame, loadFrameRequest) that should be combined. I'll do it then.

Adam Barth

Comment 32 2012-11-20 11:29:37 PST

Thanks!

WebKit Review Bot

Comment 33 2012-11-20 13:57:23 PST

Re-opened since this is blocked by bug 102834

James Simonsen

Comment 34 2012-11-20 20:27:28 PST

Created attachment 175329 [details] Patch

James Simonsen

Comment 35 2012-11-20 20:30:42 PST

I removed the ASSERT that caused this to be reverted. The ASSERT was new in this patch and I only added it because I thought it was a good, paranoid thing to do. But, at least with Chrome's DRT, the baseURL passed in is empty, so it was causing the ASSERT to fail. Since I'm doing another rev on this patch anyway, I also worked in the change to make most of callsites one-liners.

Early Warning System Bot

Comment 36 2012-11-20 20:36:09 PST

Comment on attachment 175329 [details] Patch Attachment 175329 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14907845

EFL EWS Bot

Comment 37 2012-11-20 20:44:27 PST

Comment on attachment 175329 [details] Patch Attachment 175329 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14905839

Build Bot

Comment 38 2012-11-20 20:56:53 PST

Comment on attachment 175329 [details] Patch Attachment 175329 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14916851

Build Bot

Comment 39 2012-11-20 21:04:17 PST

Comment on attachment 175329 [details] Patch Attachment 175329 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14901807

Csaba Osztrogonác

Comment 40 2012-11-20 21:36:59 PST

(In reply to comment #33) > Re-opened since this is blocked by bug 102834 And it made plugin related tests crash on Qt: http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/45265 I think it is unrelated to the assertion mentioned, because it is a relase tester bot.

James Simonsen

Comment 41 2012-11-21 15:45:59 PST

Created attachment 175539 [details] Patch

Adam Barth

Comment 42 2012-11-21 22:48:11 PST

> And it made plugin related tests crash on Qt: > http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/45265 ^^^ James, did you see this failure with the previous patch?

James Simonsen

Comment 43 2012-11-26 13:13:02 PST

(In reply to comment #42) > > And it made plugin related tests crash on Qt: > > http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/45265 > > ^^^ James, did you see this failure with the previous patch? Yeah, I fixed that too. Sorry for not updating the bug with the status before going on vacation. I also made the callsites one-liners as you'd requested earlier. I will merge and land this again later today.

James Simonsen

Comment 44 2012-11-26 15:33:10 PST

Created attachment 176094 [details] Patch for landing

WebKit Review Bot

Comment 45 2012-11-26 16:18:36 PST

Comment on attachment 176094 [details] Patch for landing Clearing flags on attachment: 176094 Committed r135786: <http://trac.webkit.org/changeset/135786>

WebKit Review Bot

Comment 46 2012-11-26 16:18:45 PST

All reviewed patches have been landed. Closing bug.

Roger Fong

Comment 47 2012-11-26 18:34:39 PST

Caused some tests to fail on Windows port: http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r135786%20(30279)/results.html plugins/get-file-url.html plugins/geturl-replace-query.html http/tests/plugins/post-url-file.html They all seem to time out now.

Brady Eidson

Comment 48 2012-11-26 20:58:24 PST

(In reply to comment #47) > Caused some tests to fail on Windows port: > http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r135786%20(30279)/results.html > > plugins/get-file-url.html > plugins/geturl-replace-query.html > http/tests/plugins/post-url-file.html > > They all seem to time out now. Then we need to roll this out then, right?

Csaba Osztrogonác

Comment 49 2012-11-26 22:42:13 PST

Csaba Osztrogonác

Comment 50 2012-11-27 01:42:43 PST

Rolled out by http://trac.webkit.org/changeset/135837

James Simonsen

Comment 51 2012-11-27 16:13:20 PST

Created attachment 176358 [details] Patch

James Simonsen

Comment 52 2012-11-27 16:14:54 PST

Okay, I built the Qt port and debugged this. I think we're good to go now. I've tested it on Chrome, Mac, and Qt. No wonder nobody had done this earlier.

Adam Barth

Comment 53 2012-11-27 16:19:46 PST

Comment on attachment 176358 [details] Patch ok

WebKit Review Bot

Comment 54 2012-11-27 17:08:26 PST

Comment on attachment 176358 [details] Patch Clearing flags on attachment: 176358 Committed r135952: <http://trac.webkit.org/changeset/135952>

WebKit Review Bot

Comment 55 2012-11-27 17:08:36 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.