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.
Created attachment 313336 [details] Patch
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.
Created attachment 313339 [details] Patch
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.
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.
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
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.
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
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.
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
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.
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
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?
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.
Dan: This patch seems to be introducing regressions. Can you please review the errors to see why were are getting behavior changes?
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.
Created attachment 313444 [details] Patch
Created attachment 313446 [details] Patch
Created attachment 313535 [details] Patch
<rdar://problem/32903570>
Comment on attachment 313535 [details] Patch Very nice! r=me.
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?
(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.
Committed r218649: <http://trac.webkit.org/changeset/218649>