Bug 173564 – Cleanup FrameLoadRequest

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

FrameLoadRequest has too many constructors. FrameLoadRequest should hold a Ref<SecurityOrigin> instead of a RefPtr<SecurityOrigin> as it is never passed a non-null SecurityOrigin object. We should also pass a Frame& instead of Frame* to the FrameLoadRequest constructor that take a frame as it expects the specified frame is non-null to be able to retrieve the SecurityOrigin of the document in the specified frame.

[Daniel Bates]

Created attachment 313336 [details]
Patch

[Build Bot]

Attachment 313336 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/FrameLoadRequest.cpp:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 313339 [details]
Patch

[Build Bot]

Attachment 313339 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/FrameLoadRequest.cpp:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 313339 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 313342 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 313339 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 313343 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 313339 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 313345 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 313339 [details]
Patch

Attachment 313339 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3963025

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 313347 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Brent Fulgham]

Comment on attachment 313339 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313339&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=173564

Please import to Radar so we can track this if any regressions are found.

> Source/WebCore/ChangeLog:15
> +        the target frame name as we do not have one. Use C++11 brace initialization syntax. Rename

Do we have a new style guideline to use the brace syntax? Does it impart some benefit over the traditional parenthetical version?

[Darin Adler]

Comment on attachment 313339 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313339&action=review

>> Source/WebCore/ChangeLog:15
>> +        the target frame name as we do not have one. Use C++11 brace initialization syntax. Rename
> 
> Do we have a new style guideline to use the brace syntax? Does it impart some benefit over the traditional parenthetical version?

We do not have a formal style guideline, but it’s definitely a trend in style, started by Anders Carlsson.

The benefit I am aware of: The braces syntax is stricter. Some type conversions that are done silently in the parentheses syntax are treated as errors if the braces syntax is used.

[Brent Fulgham]

Dan: This patch seems to be introducing regressions. Can you please review the errors to see why were are getting behavior changes?

[Brent Fulgham]

Comment on attachment 313339 [details]
Patch

While I like the direction of this patch, the test failures make me concerned that you have introduced changes in behavior, perhaps by unexpected changes in how these objects are being initialized. I don't feel comfortable approving the change with these failing tests.

[Daniel Bates]

Created attachment 313444 [details]
Patch

[Daniel Bates]

Created attachment 313446 [details]
Patch

[Daniel Bates]

Created attachment 313535 [details]
Patch

[Daniel Bates]

<rdar://problem/32903570>

[Brent Fulgham]

Comment on attachment 313535 [details]
Patch

Very nice! r=me.

[Darin Adler]

Comment on attachment 313535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313535&action=review

> Source/WebCore/loader/FrameLoadRequest.h:69
> +    FrameLoadRequest(const FrameLoadRequest& other)
> +        : m_requester { other.m_requester.copyRef() }
> +        , m_resourceRequest { other.m_resourceRequest }
> +        , m_frameName { other.m_frameName }
> +        , m_substituteData { other.m_substituteData }
> +        , m_shouldCheckNewWindowPolicy { other.m_shouldCheckNewWindowPolicy }
> +        , m_lockHistory { other.m_lockHistory }
> +        , m_lockBackForwardList { other.m_lockBackForwardList }
> +        , m_shouldSendReferrer { other.m_shouldSendReferrer }
> +        , m_allowNavigationToInvalidURL { other.m_allowNavigationToInvalidURL }
> +        , m_newFrameOpenerPolicy { other.m_newFrameOpenerPolicy }
> +        , m_shouldReplaceDocumentIfJavaScriptURL { other.m_shouldReplaceDocumentIfJavaScriptURL }
> +        , m_shouldOpenExternalURLsPolicy { other.m_shouldOpenExternalURLsPolicy }
> +        , m_downloadAttribute { other.m_downloadAttribute }
>      {
>      }

Irritating that we have to write this long function because Ref can’t be copied; it would be so easy to make the mistake of leaving one of these data members out and cause a bug that way. Also annoying that we ever have to copy a FrameLoadRequest. Maybe we can change at least some call sites so we move instead?

[Daniel Bates]

(In reply to Darin Adler from comment #22)
> Comment on attachment 313535 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313535&action=review
> 
> > Source/WebCore/loader/FrameLoadRequest.h:69
> > +    FrameLoadRequest(const FrameLoadRequest& other)
> > +        : m_requester { other.m_requester.copyRef() }
> > +        , m_resourceRequest { other.m_resourceRequest }
> > +        , m_frameName { other.m_frameName }
> > +        , m_substituteData { other.m_substituteData }
> > +        , m_shouldCheckNewWindowPolicy { other.m_shouldCheckNewWindowPolicy }
> > +        , m_lockHistory { other.m_lockHistory }
> > +        , m_lockBackForwardList { other.m_lockBackForwardList }
> > +        , m_shouldSendReferrer { other.m_shouldSendReferrer }
> > +        , m_allowNavigationToInvalidURL { other.m_allowNavigationToInvalidURL }
> > +        , m_newFrameOpenerPolicy { other.m_newFrameOpenerPolicy }
> > +        , m_shouldReplaceDocumentIfJavaScriptURL { other.m_shouldReplaceDocumentIfJavaScriptURL }
> > +        , m_shouldOpenExternalURLsPolicy { other.m_shouldOpenExternalURLsPolicy }
> > +        , m_downloadAttribute { other.m_downloadAttribute }
> >      {
> >      }
> 
> Irritating that we have to write this long function because Ref can’t be
> copied; it would be so easy to make the mistake of leaving one of these data
> members out and cause a bug that way. Also annoying that we ever have to
> copy a FrameLoadRequest. Maybe we can change at least some call sites so we
> move instead?

When I was writing this patch I thought about making FrameLoadRequest a move-only type. I thought to do this in a separate bug/patch as this patch was already sufficiently large.

[Daniel Bates]

Committed r218649: <http://trac.webkit.org/changeset/218649>