Bug 164627 – [DOMJIT] Document#body should have DOMJIT patchpoint

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

It should be easy!
It requires some traversal things in ASM. But, it is what we did in CSSJIT!

[Yusuke Suzuki]

Ugh, autocorrection makes patchpoint => watchpoint.

[Yusuke Suzuki]

Created attachment 294455 [details]
Patch

WIP

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 6 files


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

[Yusuke Suzuki]

Created attachment 294463 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 294471 [details]
Patch

[Darin Adler]

Comment on attachment 294471 [details]
Patch

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

> Source/WebCore/dom/Document.idl:104
> +    [DOMJIT, CEReactions, ImplementedAs=bodyOrFrameset, SetterMayThrowException] attribute HTMLElement? body;

I have been trying to keep these extended attributes in alphabetical order. This means I would have put DOMJIT after CEReactions. Not sure if you have been doing this when adding DOMJIT to other attributes.

It’s a bit annoying that DOMJIT implicitly means the getter, not the setter.

> Source/WebCore/domjit/DOMJITHelpers.h:31
> +#include "Node.h"
>  #include "ScriptWrappable.h"

Now that we are including Node.h we don’t need to include ScriptWrappable.h any more.

> Source/WebCore/domjit/JSDocumentDOMJIT.cpp:114
> +        RELEASE_ASSERT(!CAST_OFFSET(Node*, ContainerNode*));
> +        RELEASE_ASSERT(!CAST_OFFSET(Node*, Element*));
> +        RELEASE_ASSERT(!CAST_OFFSET(Node*, HTMLElement*));

I know I asked this on a previous patch: These can’t be compile-time assertions instead of runtime assertions?

[Yusuke Suzuki]

Comment on attachment 294471 [details]
Patch

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

>> Source/WebCore/dom/Document.idl:104
>> +    [DOMJIT, CEReactions, ImplementedAs=bodyOrFrameset, SetterMayThrowException] attribute HTMLElement? body;
> 
> I have been trying to keep these extended attributes in alphabetical order. This means I would have put DOMJIT after CEReactions. Not sure if you have been doing this when adding DOMJIT to other attributes.
> 
> It’s a bit annoying that DOMJIT implicitly means the getter, not the setter.

"It’s a bit annoying that DOMJIT implicitly means the getter, not the setter." Yeah, right.
We should rename it to something like, `DOMJIT=Getter` thing. I'll upload the patch for that.

>> Source/WebCore/domjit/DOMJITHelpers.h:31
>>  #include "ScriptWrappable.h"
> 
> Now that we are including Node.h we don’t need to include ScriptWrappable.h any more.

Dropped.

>> Source/WebCore/domjit/JSDocumentDOMJIT.cpp:114
>> +        RELEASE_ASSERT(!CAST_OFFSET(Node*, HTMLElement*));
> 
> I know I asked this on a previous patch: These can’t be compile-time assertions instead of runtime assertions?

Yeah, unfortunately. Since CAST_OFFSET uses reinterpret_cast, it cannot be evaluated at compile time.
(And OBJECT_OFFSETOF is the same. That's why we cannot make XXX::offsetOfXXX() constexpr...) That's sad.

[Yusuke Suzuki]

Created attachment 294472 [details]
Patch for landing

[Yusuke Suzuki]

Committed r208579: <http://trac.webkit.org/changeset/208579>