Bug 163553 – Move internal testing classes from ExceptionCode to Exception

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Darin Adler]

Move internal testing classes from ExceptionCode to Exception

[Darin Adler]

Created attachment 291846 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Darin Adler]

Created attachment 292026 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Ryosuke Niwa]

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?

[Darin Adler]

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.

[Darin Adler]

Committed r207521: <http://trac.webkit.org/changeset/207521>