Bug 173129 - Move TreeScope::adoptIfNeeded to Node and rename it to setTreeScopeRecursively
Summary: Move TreeScope::adoptIfNeeded to Node and rename it to setTreeScopeRecursively
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-08 22:13 PDT by Ryosuke Niwa
Modified: 2017-06-09 01:59 PDT (History)
10 users (show)

See Also:

Attachments
Cleanup (23.43 KB, patch)
2017-06-08 22:34 PDT, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
Fixed a typo (23.43 KB, patch)
2017-06-08 22:36 PDT, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
Fixed relase builds (23.48 KB, patch)
2017-06-08 23:01 PDT, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
Fixed release builds for real (23.54 KB, patch)
2017-06-08 23:47 PDT, Ryosuke Niwa 		koivisto: review+
Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2017-06-08 22:13:35 PDT 
We should move TreeScope::adoptIfNeeded to Nice and rename it to setTreeScopeRecursively.
This basically brings back the code prior to https://trac.webkit.org/changeset/104528.
Comment 1 Ryosuke Niwa 2017-06-08 22:34:45 PDT 
Created attachment 312383 [details]
Cleanup
Comment 2 Ryosuke Niwa 2017-06-08 22:36:46 PDT 
Created attachment 312384 [details]
Fixed a typo
Comment 3 Radar WebKit Bug Importer 2017-06-08 22:38:07 PDT 
<rdar://problem/32667282>
Comment 4 Ryosuke Niwa 2017-06-08 23:01:29 PDT 
Created attachment 312389 [details]
Fixed relase builds
Comment 5 Ryosuke Niwa 2017-06-08 23:47:27 PDT 
Created attachment 312394 [details]
Fixed release builds for real
Comment 6 Antti Koivisto 2017-06-09 00:40:58 PDT 
Comment on attachment 312394 [details]
Fixed release builds for real

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

> Source/WebCore/dom/Node.cpp:1926
> +#if !ASSERT_DISABLED || ENABLE(SECURITY_ASSERTIONS)
> +class DidMoveToNewDocumentAssertionScope {

Maybe we should add NodeAssertions.h or similar at some point for this sort of things? The main files are pretty bloated already.

> Source/WebCore/dom/Node.cpp:2019
> +        if (!is<Element>(*node))
> +            continue;

It might make sense to add 'element' local right after this check and use it instead of 'node'. The following code will read better and may even get less branchy.
Comment 7 Ryosuke Niwa 2017-06-09 01:48:28 PDT 
(In reply to Antti Koivisto from comment #6)
> Comment on attachment 312394 [details]
> Fixed release builds for real
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312394&action=review
> 
> > Source/WebCore/dom/Node.cpp:1926
> > +#if !ASSERT_DISABLED || ENABLE(SECURITY_ASSERTIONS)
> > +class DidMoveToNewDocumentAssertionScope {
> 
> Maybe we should add NodeAssertions.h or similar at some point for this sort
> of things? The main files are pretty bloated already.

That sounds like a good idea. There's already NoEventDispatchAssertion.h so maybe we could rename hat to NodeAssertions.h and move all these assertions there. Although this particular one is local to Node.cpp so we probably don't need to put into the header.

> > Source/WebCore/dom/Node.cpp:2019
> > +        if (!is<Element>(*node))
> > +            continue;
> 
> It might make sense to add 'element' local right after this check and use it
> instead of 'node'. The following code will read better and may even get less
> branchy.

Fixed that.
Comment 8 Ryosuke Niwa 2017-06-09 01:59:24 PDT 
Committed r217972: <http://trac.webkit.org/changeset/217972>