Bug 160797

Summary:

customElements.define should retrieve lifecycle callbacks

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

DOM

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, cdumez, cmarcelo, commit-queue, esprehn+autocc, kangil.han, kling, koivisto, mmaxfield, rniwa, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

160801, 160932

Bug Blocks:

154907

Attachments:

Description	Flags
Fixes the behavior	none
Fixes the behavior	none
Archive of layout-test-results from ews102 for mac-yosemite	none
Archive of layout-test-results from ews104 for mac-yosemite-wk2	none
Archive of layout-test-results from ews114 for mac-yosemite	none
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2	none
Patch	none
Added one more test case	none
Patch for landing	none
Patch for landing	none

Ryosuke Niwa

Reported 2016-08-11 20:27:16 PDT

The latest spec mandates that `customElements.define` invoke `Get(constructor, "prototype")` as well as `Get(prototype, callbackName)` for each lifecycle callback. Do that, and also add the support for observedAttributes.

Attachments
Fixes the behavior (127.21 KB, patch) 2016-08-11 20:40 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Fixes the behavior (39.88 KB, patch) 2016-08-11 20:42 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews102 for mac-yosemite (1.11 MB, application/zip) 2016-08-11 21:21 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.02 MB, application/zip) 2016-08-11 21:32 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews114 for mac-yosemite (1.30 MB, application/zip) 2016-08-11 21:41 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 (604.98 KB, application/zip) 2016-08-11 21:56 PDT, Build Bot	no flags	Details
Patch (44.62 KB, patch) 2016-08-16 00:25 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Added one more test case (45.88 KB, patch) 2016-08-16 00:51 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch for landing (49.92 KB, patch) 2016-08-16 20:31 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch for landing (49.93 KB, patch) 2016-08-16 20:39 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2016-08-11 20:40:29 PDT

Created attachment 285886 [details] Fixes the behavior

Ryosuke Niwa

Comment 2 2016-08-11 20:42:10 PDT

Created attachment 285887 [details] Fixes the behavior

Build Bot

Comment 3 2016-08-11 21:21:48 PDT

Comment on attachment 285887 [details] Fixes the behavior Attachment 285887 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1855682 Number of test failures exceeded the failure limit.

Build Bot

Comment 4 2016-08-11 21:21:51 PDT

Created attachment 285889 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5

Build Bot

Comment 5 2016-08-11 21:32:22 PDT

Comment on attachment 285887 [details] Fixes the behavior Attachment 285887 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1855720 Number of test failures exceeded the failure limit.

Build Bot

Comment 6 2016-08-11 21:32:25 PDT

Created attachment 285890 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Build Bot

Comment 7 2016-08-11 21:41:55 PDT

Comment on attachment 285887 [details] Fixes the behavior Attachment 285887 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1855724 Number of test failures exceeded the failure limit.

Build Bot

Comment 8 2016-08-11 21:41:58 PDT

Created attachment 285891 [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 9 2016-08-11 21:56:53 PDT

Comment on attachment 285887 [details] Fixes the behavior Attachment 285887 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1855750 Number of test failures exceeded the failure limit.

Build Bot

Comment 10 2016-08-11 21:56:56 PDT

Created attachment 285892 [details] Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5

Ryosuke Niwa

Comment 11 2016-08-11 23:30:29 PDT

Comment on attachment 285887 [details] Fixes the behavior I need to fix the sequence<T> support for iterators first.

Ryosuke Niwa

Comment 12 2016-08-16 00:25:08 PDT

Created attachment 286160 [details] Patch

Ryosuke Niwa

Comment 13 2016-08-16 00:51:45 PDT

Created attachment 286163 [details] Added one more test case

Chris Dumez

Comment 14 2016-08-16 15:26:39 PDT

Comment on attachment 286163 [details] Added one more test case View in context: https://bugs.webkit.org/attachment.cgi?id=286163&action=review r=me with comments > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:42 > +static JSObject* getLifecycleCallback(ExecState& state, JSValue prototype, const Identifier& id) At the call sites, you already check that prototype is a JSObject, therefore, could we take a JSObject& here instead of a JSValue? > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:47 > + if (callback.isUndefined()) Seems redundant given the isFunction() check below? > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:85 > + // FIXME: Check re-entracy here. typo > Source/WebCore/bindings/js/JSCustomElementsRegistryCustom.cpp:127 > + if (attributeChangedCallback) { Can this really be null if there was no exception thrown? Could this simply be an assertion? > Source/WebCore/dom/LifecycleCallbackQueue.cpp:113 > + RELEASE_ASSERT(elementInterface); Is this useful? I mean, it will crash on the next line anyway. > Source/WebCore/dom/LifecycleCallbackQueue.h:30 > +#include "Element.h" Hmm, why do we need to include Element.h here now ? :( We should try and avoid this for the sake of my build time.

Ryosuke Niwa

Comment 15 2016-08-16 16:57:25 PDT

Committed r204540: <http://trac.webkit.org/changeset/204540>

Myles C. Maxfield

Comment 16 2016-08-16 20:12:54 PDT

I think this broke the Windows build. https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/96729/steps/compile-webkit/logs/stdio/text C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\bindings\js\JSCustomElementsRegistryCustom.cpp(123): error C2236: unexpected token 'struct'. Did you forget a ';'? [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\bindings\js\JSCustomElementsRegistryCustom.cpp(123): error C2332: 'struct': missing tag name [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\bindings\js\JSCustomElementsRegistryCustom.cpp(123): error C2513: '': no variable declared before '=' [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\bindings\js\JSCustomElementsRegistryCustom.cpp(133): error C2332: 'struct': missing tag name [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\bindings\js\JSCustomElementsRegistryCustom.cpp(133): warning C4094: untagged 'struct' declared no symbols [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\bindings\js\JSCustomElementsRegistryCustom.cpp(133): error C2143: syntax error: missing ';' before '->' [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\bindings\js\JSCustomElementsRegistryCustom.cpp(133): error C2059: syntax error: '->' [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\bindings\js\JSCustomElementsRegistryCustom.cpp(139): error C2332: 'struct': missing tag name [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\bindings\js\JSCustomElementsRegistryCustom.cpp(139): error C2062: type '' unexpected [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj]

WebKit Commit Bot

Comment 17 2016-08-16 20:15:41 PDT

Re-opened since this is blocked by bug 160932

Myles C. Maxfield

Comment 18 2016-08-16 20:17:21 PDT

(In reply to comment #16) > I think this broke the Windows build. Oh, it looks like you're already working on this at https://trac.webkit.org/changeset/204547

Ryosuke Niwa

Comment 19 2016-08-16 20:17:39 PDT

I've landed two speculative fixes but didn't work. The build failure error makes zero sense to me. There is no keyword "struct" at line 123.

Ryosuke Niwa

Comment 20 2016-08-16 20:31:59 PDT

Created attachment 286261 [details] Patch for landing

Ryosuke Niwa

Comment 21 2016-08-16 20:39:02 PDT

Created attachment 286262 [details] Patch for landing

Ryosuke Niwa

Comment 22 2016-08-16 21:44:29 PDT

Committed r204553: <http://trac.webkit.org/changeset/204553>

Ryosuke Niwa

Comment 23 2016-09-01 16:16:47 PDT

<rdar://problem/28090374

Note You need to log in before you can comment on or make changes to this bug.