Move internal testing classes from ExceptionCode to Exception
Created attachment 291846 [details] Patch
Comment on attachment 291846 [details] Patch Attachment 291846 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2305974 New failing tests: fast/shrink-wrap/rect-shrink-wrap.html
Created attachment 291858 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 291846 [details] Patch Attachment 291846 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2306008 New failing tests: fast/shrink-wrap/rect-shrink-wrap.html
Created attachment 291859 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 291846 [details] Patch Attachment 291846 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2305956 New failing tests: fast/shrink-wrap/rect-shrink-wrap.html
Created attachment 291860 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 291846 [details] Patch Attachment 291846 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2306181 Number of test failures exceeded the failure limit.
Created attachment 291862 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 292026 [details] Patch
Comment on attachment 292026 [details] Patch Attachment 292026 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2320655 Number of test failures exceeded the failure limit.
Created attachment 292031 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 292026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292026&action=review > Source/WebCore/dom/Element.cpp:1803 > -RefPtr<ShadowRoot> Element::createShadowRoot(ExceptionCode& ec) > +ShadowRoot* Element::createShadowRoot(ExceptionCode& ec) This seems rather unrelated. But why don't we change it to ExceptionOr<ShadowRoot*> while we're at it? > Source/WebCore/testing/InternalSettings.cpp:375 > - InternalSettingsGuardForSettings(); > + INTERNAL_SETTINGS_GUARD It's not obvious at all that this macro is returning an exception when m_page is null. Can we rename it to something more descriptive like RETURN_EXCEPTION_IF_PAGE_IS_NULL? One more idea (not that we should implement in this patch). We could add some sort of extended IDL attribute e.g. ValidateAccess, and then call a helper function that throws an exception automatically in the binding code. e.g. In IDL: [ValidateAccess] setCanStartMedia(boolean enabled); then in C++, ExceptionOr<void> validateAccess() { return m_page ? { } : INVALID_ACCESS_ERR; } void setCanStartMedia(bool enabled); I'm not sure if such an extended attribute will be useful in other IDL files though... Or maybe we can just get rid of the marco and just have if (!m_page) return INVALID_ACCESS_ERR; Honestly, I think two lines of extra code is okay given that it'll make the code much easier to understand. > Source/WebCore/testing/InternalSettings.h:50 > + void hostDestroyed() { m_page = nullptr; } Do we really need to inline this?
Comment on attachment 292026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292026&action=review >> Source/WebCore/dom/Element.cpp:1803 >> +ShadowRoot* Element::createShadowRoot(ExceptionCode& ec) > > This seems rather unrelated. But why don't we change it to ExceptionOr<ShadowRoot*> while we're at it? Internals::ensureShadowRoot was the reason I touched this. I promise to come back here and take care of converting to ExceptionOr. I don’t want to do just that one function in Element.cpp at this time. >> Source/WebCore/testing/InternalSettings.cpp:375 >> + INTERNAL_SETTINGS_GUARD > > It's not obvious at all that this macro is returning an exception when m_page is null. > Can we rename it to something more descriptive like RETURN_EXCEPTION_IF_PAGE_IS_NULL? > > One more idea (not that we should implement in this patch). > > We could add some sort of extended IDL attribute e.g. ValidateAccess, > and then call a helper function that throws an exception automatically in the binding code. > > e.g. In IDL: > [ValidateAccess] setCanStartMedia(boolean enabled); > then in C++, > ExceptionOr<void> validateAccess() { return m_page ? { } : INVALID_ACCESS_ERR; } > void setCanStartMedia(bool enabled); > > I'm not sure if such an extended attribute will be useful in other IDL files though... > > Or maybe we can just get rid of the marco and just have if (!m_page) return INVALID_ACCESS_ERR; > Honestly, I think two lines of extra code is okay given that it'll make the code much easier to understand. I agree about the two lines of code. I will get rid of the macro. >> Source/WebCore/testing/InternalSettings.h:50 >> + void hostDestroyed() { m_page = nullptr; } > > Do we really need to inline this? No.
Committed r207521: <http://trac.webkit.org/changeset/207521>