Bug 164627 - [DOMJIT] Document#body should have DOMJIT patchpoint
Summary: [DOMJIT] Document#body should have DOMJIT patchpoint
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks: 162544
  Show dependency treegraph
 
Reported: 2016-11-10 17:31 PST by Yusuke Suzuki
Modified: 2016-11-10 22:11 PST (History)
12 users (show)

See Also:


Attachments
Patch (14.79 KB, patch)
2016-11-10 18:15 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (19.18 KB, patch)
2016-11-10 19:54 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (22.23 KB, patch)
2016-11-10 21:23 PST, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff
Patch for landing (24.41 KB, patch)
2016-11-10 21:49 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2016-11-10 17:31:38 PST
It should be easy!
It requires some traversal things in ASM. But, it is what we did in CSSJIT!
Comment 1 Yusuke Suzuki 2016-11-10 17:52:14 PST
Ugh, autocorrection makes patchpoint => watchpoint.
Comment 2 Yusuke Suzuki 2016-11-10 18:15:30 PST
Created attachment 294455 [details]
Patch

WIP
Comment 3 WebKit Commit Bot 2016-11-10 18:19:11 PST
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.
Comment 4 Yusuke Suzuki 2016-11-10 19:54:29 PST
Created attachment 294463 [details]
Patch

WIP
Comment 5 Yusuke Suzuki 2016-11-10 21:23:43 PST
Created attachment 294471 [details]
Patch
Comment 6 Darin Adler 2016-11-10 21:31:11 PST
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?
Comment 7 Yusuke Suzuki 2016-11-10 21:40:26 PST
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.
Comment 8 Yusuke Suzuki 2016-11-10 21:49:38 PST
Created attachment 294472 [details]
Patch for landing
Comment 9 Yusuke Suzuki 2016-11-10 22:11:28 PST
Committed r208579: <http://trac.webkit.org/changeset/208579>