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

[James Simonsen]

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

[James Simonsen]

Created attachment 174026 [details]
Patch

[Early Warning System Bot]

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 on attachment 174026 [details]
Patch

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

[Build Bot]

Comment on attachment 174026 [details]
Patch

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

[James Simonsen]

Created attachment 174037 [details]
Patch

[Darin Adler]

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]

(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 on attachment 174037 [details]
Patch

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

[Adam Barth]

> m_frame->loader()->load(FrameLoadRequest(m_frame, resourceRequest));

That might improve the call sites.

[EFL EWS Bot]

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 on attachment 174037 [details]
Patch

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

[James Simonsen]

Created attachment 174269 [details]
Patch

[James Simonsen]

(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 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 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 on attachment 174269 [details]
Patch

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

[Build Bot]

Comment on attachment 174269 [details]
Patch

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

[James Simonsen]

Created attachment 174282 [details]
Patch

[Early Warning System Bot]

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 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 on attachment 174282 [details]
Patch

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

[Build Bot]

Comment on attachment 174282 [details]
Patch

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

[James Simonsen]

Created attachment 174552 [details]
Patch

[Adam Barth]

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 on attachment 174552 [details]
Patch

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

[James Simonsen]

Created attachment 175119 [details]
Patch

[James Simonsen]

(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 on attachment 175119 [details]
Patch

Clearing flags on attachment: 175119

Committed r135295: <http://trac.webkit.org/changeset/135295>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

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]

(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]

Thanks!

[WebKit Review Bot]

Re-opened since this is blocked by bug 102834

[James Simonsen]

Created attachment 175329 [details]
Patch

[James Simonsen]

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 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 on attachment 175329 [details]
Patch

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

[Build Bot]

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 on attachment 175329 [details]
Patch

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

[Csaba Osztrogonác]

(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]

Created attachment 175539 [details]
Patch

[Adam Barth]

> 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]

(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]

Created attachment 176094 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 176094 [details]
Patch for landing

Clearing flags on attachment: 176094

Committed r135786: <http://trac.webkit.org/changeset/135786>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roger Fong]

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]

(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]

(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.

Same tests started to timeout on Qt.

[Csaba Osztrogonác]

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

[James Simonsen]

Created attachment 176358 [details]
Patch

[James Simonsen]

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 on attachment 176358 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 176358 [details]
Patch

Clearing flags on attachment: 176358

Committed r135952: <http://trac.webkit.org/changeset/135952>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.