Bug 73964 – Use HashMap<Node*, OwnPtr<...>> in ChildListMutationScope

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Klein]

Use HashMap<Node*, OwnPtr<...>> in ChildListMutationScope

[Adam Klein]

Created attachment 118144 [details]
Patch

[WebKit Review Bot]

Comment on attachment 118144 [details]
Patch

Clearing flags on attachment: 118144

Committed r102267: <http://trac.webkit.org/changeset/102267>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Klein]

Broke the mac dbg build:

    /b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../third_party/llvm-build/Release+Asserts/bin/clang -x c++ -arch i386 -fmessage-length=0 -pipe -Wno-trigraphs -fno-exceptions -fno-rtti -O0 -Werror -Wnewline-eof -DCHROMIUM_BUILD -DENABLE_REMOTING=1 -DENABLE_P2P_APIS=1 -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DENABLE_REGISTER_PROTOCOL_HANDLER=1 "-DWEBCORE_NAVIGATOR_VENDOR=\"Google Inc.\"" -DWEBCORE_NAVIGATOR_PLATFORM="MacIntel" -DWebCascadeList=ChromiumWebCoreObjCWebCascadeList -DScrollbarPrefsObserver=ChromiumWebCoreObjCScrollbarPrefsObserver -DWebCoreRenderThemeNotificationObserver=ChromiumWebCoreObjCWebCoreRenderThemeNotificationObserver -DWebFontCache=ChromiumWebCoreObjCWebFontCache -DScrollAnimationHelperDelegate=ChromiumWebCoreObjCScrollAnimationHelperDelegate -DScrollbarPainterControllerDelegate=ChromiumWebCoreObjCScrollbarPainterControllerDelegate -DScrollbarPainterDelegate=ChromiumWebCoreObjCScrollbarPainterDelegate -DScrollbarPartAnimation=ChromiumWebCoreObjCScrollbarPartAnimation -DENABLE_3D_PLUGIN=1 -DENABLE_BLOB=1 -DENABLE_BLOB_SLICE=1 -DENABLE_CHANNEL_MESSAGING=1 -DENABLE_CLIENT_BASED_GEOLOCATION=1 -DENABLE_DASHBOARD_SUPPORT=0 -DENABLE_DATA_TRANSFER_ITEMS=1 -DENABLE_DETAILS=1 -DENABLE_DEVICE_ORIENTATION=1 -DENABLE_DIRECTORY_UPLOAD=1 -DENABLE_DOWNLOAD_ATTRIBUTE=1 -DENABLE_FILE_SYSTEM=1 -DENABLE_FILTERS=1 -DENABLE_FULLSCREEN_API=1 -DENABLE_GAMEPAD=1 -DENABLE_GEOLOCATION=1 -DENABLE_GESTURE_EVENTS=1 -DENABLE_GESTURE_RECOGNIZER=1 -DENABLE_ICONDATABASE=0 -DENABLE_INDEXED_DATABASE=1 -DENABLE_INPUT_SPEECH=1 -DENABLE_INPUT_TYPE_COLOR=0 -DENABLE_INPUT_TYPE_DATE=0 -DENABLE_INPUT_TYPE_DATETIME=0 -DENABLE_INPUT_TYPE_DATETIMELOCAL=0 -DENABLE_INPUT_TYPE_MONTH=0 -DENABLE_INPUT_TYPE_TIME=0 -DENABLE_INPUT_TYPE_WEEK=0 -DENABLE_JAVASCRIPT_DEBUGGER=1 -DENABLE_JAVASCRIPT_I18N_API=1 -DENABLE_LINK_PREFETCH=1 -DENABLE_MEDIA_SOURCE=1 -DENABLE_MEDIA_STATISTICS=1 -DENABLE_MEDIA_STREAM=1 -DENABLE_METER_TAG=1 -DENABLE_MHTML=1 -DENABLE_MICRODATA=0 -DENABLE_MUTATION_OBSERVERS=1 -DENABLE_NOTIFICATIONS=1 -DENABLE_ORIENTATION_EVENTS=0 -DENABLE_PAGE_VISIBILITY_API=1 -DENABLE_POINTER_LOCK=1 -DENABLE_PROGRESS_TAG=1 -DENABLE_QUOTA=1 -DENABLE_REQUEST_ANIMATION_FRAME=1 -DENABLE_RUBY=1 -DENABLE_SANDBOX=1 -DENABLE_SHARED_WORKERS=1 -DENABLE_SMOOTH_SCROLLING=1 -DENABLE_SQL_DATABASE=1 -DENABLE_STYLE_SCOPED=0 -DENABLE_SVG=1 -DENABLE_SVG_FONTS=1 -DENABLE_TOUCH_EVENTS=1 -DENABLE_TOUCH_ICON_LOADING=0 -DENABLE_V8_SCRIPT_DEBUG_SERVER=1 -DENABLE_VIDEO=1 -DENABLE_VIDEO_TRACK=1 -DENABLE_WEBGL=1 -DENABLE_WEB_SOCKETS=1 -DENABLE_WEB_TIMING=1 -DENABLE_WORKERS=1 -DENABLE_XHR_RESPONSE_BLOB=1 -DENABLE_XSLT=1 -DWTF_USE_LEVELDB=1 -DWTF_USE_BUILTIN_UTF8_CODEC=1 -DWTF_USE_OPENTYPE_SANITIZER=1 -DWTF_USE_SKIA_TEXT=0 -DWTF_USE_WEBP=1 -DWTF_USE_WEBKIT_IMAGE_DECODERS=1 -DENABLE_WEB_AUDIO=1 -DWTF_USE_ACCELERATED_COMPOSITING=1 -DENABLE_3D_RENDERING=1 -DENABLE_RUBBER_BANDING=1 -DWTF_USE_SKIA_ON_MAC_CHROMIUM=0 -DBUILDING_CHROMIUM__=1 -DUSE_SYSTEM_MALLOC=1 -DWTF_USE_NEW_THEME=1 -DU_USING_ICU_NAMESPACE=0 -DU_STATIC_IMPLEMENTATION -DSK_BUILD_NO_IMAGE_ENCODE -DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h" -DGR_AGGRESSIVE_SHADER_OPTS=1 -DCHROME_PNG_WRITE_SUPPORT -DPNG_USER_CONFIG -DLIBXML_STATIC -DLIBXSLT_STATIC -D__STDC_FORMAT_MACROS -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -isysroot /Developer/SDKs/MacOSX10.5.sdk -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -mmacosx-version-min=10.5 -gdwarf-2 -Wall -Wendif-labels -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wheader-hygiene -Wno-char-subscripts -Wno-unused-function -Wno-unnamed-type-template-args -Wno-c++0x-compat -Wno-c++11-extensions -Wexit-time-destructors -F/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/Debug -F/Developer/SDKs/MacOSX10.5.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks -I/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/Debug/include -I../../../../icu/public/common -I../../../../icu/public/i18n -I../../../../../third_party/khronos -I../../../../.. -I/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/WebCore.build/DerivedSources/Debug -I../platform/graphics/cocoa -I../platform/graphics/cg -I.. -I../.. -I../Modules/gamepad -I../accessibility -I../accessibility/chromium -I../bindings -I../bindings/generic -I../bindings/v8 -I../bindings/v8/custom -I../bindings/v8/specialization -I../bridge -I../bridge/jni -I../bridge/jni/v8 -I../css -I../dom -I../dom/default -I../editing -I../fileapi -I../history -I../html -I../html/canvas -I../html/parser -I../html/shadow -I../html/track -I../inspector -I../loader -I../loader/appcache -I../loader/archive -I../loader/archive/cf -I../loader/archive/mhtml -I../loader/cache -I../loader/icon -I../mathml -I../mediastream -I../notifications -I../page -I../page/animation -I../page/chromium -I../platform -I../platform/animation -I../platform/audio -I../platform/audio/chromium -I../platform/chromium -I../platform/graphics -I../platform/graphics/chromium -I../platform/graphics/filters -I../platform/graphics/filters/arm -I../platform/graphics/gpu -I../platform/graphics/opentype -I../platform/graphics/skia -I../platform/graphics/transforms -I../platform/image-decoders -I../platform/image-decoders/bmp -I../platform/image-decoders/gif -I../platform/image-decoders/ico -I../platform/image-decoders/jpeg -I../platform/image-decoders/png -I../platform/image-decoders/skia -I../platform/image-decoders/xbm -I../platform/image-decoders/webp -I../platform/image-encoders/skia -I../platform/leveldb -I../platform/mediastream -I../platform/mock -I../platform/network -I../platform/network/chromium -I../platform/sql -I../platform/text -I../platform/text/transcoder -I../plugins -I../plugins/chromium -I../rendering -I../rendering/style -I../rendering/svg -I../storage -I../storage/chromium -I../svg -I../svg/animation -I../svg/graphics -I../svg/graphics/filters -I../svg/properties -I../../ThirdParty/glu -I../webaudio -I../websockets -I../workers -I../xml -I../xml/parser -I../platform/audio/mac -I../platform/cocoa -I../platform/graphics/mac -I../platform/mac -I../platform/text/mac -I../../../../../gpu -I../../../../../third_party/angle/include/GLSLANG -I/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/DerivedSources/Debug/webkit -I/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/DerivedSources/Debug/webkit/bindings -I../../../../../third_party/apple_webkit -I../../JavaScriptCore -I../../JavaScriptCore/wtf -I../../WTF -I../../../../../skia/config -I../../../../../third_party/skia/include/config -I../../../../../third_party/skia/include/core -I../../../../../third_party/skia/include/effects -I../../../../../third_party/skia/include/pdf -I../../../../../third_party/skia/include/gpu -I../../../../../third_party/skia/include/ports -I../../../../../skia/ext -I../../../../../third_party/skia/include/utils/mac -I../../../../iccjpeg -I../../../../libwebp -I../../../../libpng -I../../../../zlib -I../../../../libxml/mac/include -I../../../../libxml/src/include -I../../../../libxslt -I../../../../npapi -I../../../../npapi/bindings -I../../../../ots/include -I../../../../sqlite -I../../../../../v8/include -I../../../../libjpeg_turbo -I../../../../leveldatabase/src/include -I../../../../leveldatabase/src -I/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/WebCore.build/Debug/webcore_dom.build/DerivedSources/i386 -I/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/WebCore.build/Debug/webcore_dom.build/DerivedSources -fno-strict-aliasing -Xclang -load -Xclang /b/build/slave/Mac_Builder__dbg_/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib -Xclang -add-plugin -Xclang find-bad-constructs -fstack-protector-all -include /b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../WebCorePrefix.h -c /b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../dom/ChildListMutationScope.cpp -o /b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/WebCore.build/Debug/webcore_dom.build/Objects-normal/i386/ChildListMutationScope.o
In file included from /b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../dom/ChildListMutationScope.cpp:37:
In file included from ../dom/DocumentFragment.h:27:
In file included from ../dom/ContainerNode.h:28:
In file included from ../dom/Node.h:28:
In file included from ../dom/EventTarget.h:35:
In file included from ../dom/EventListenerMap.h:36:
In file included from ../dom/RegisteredEventListener.h:27:
In file included from ../dom/EventListener.h:24:
In file included from ../../JavaScriptCore/wtf/RefCounted.h:28:
../../JavaScriptCore/wtf/OwnPtr.h:52:9:error: function 'WTF::OwnPtr<WebCore::<anonymous>::ChildListMutationAccumulator>::OwnPtr' has internal linkage but is not defined [-Werror]
        OwnPtr(const OwnPtr<ValueType>&);
        ^
/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../dom/ChildListMutationScope.cpp:261:42: note: used here
    OwnPtr<ChildListMutationAccumulator> record = m_accumulations.take(target);
                                         ^
1 error generated.

Looks like clang doesn't optimize away the call to the OwnPtr copy constructor here.  We either need to fix OwnPtr or workaround this by avoiding the copy constructor.

[Adam Klein]

Created attachment 118423 [details]
Patch

[Darin Adler]

Comment on attachment 118423 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        * wtf/HashTraits.h: Add passOut(std::nullptr_t) to allow callers to use HashMap::take on an entry whose value is null.

This is an incorrect comment. The code is used when take is called and there is no entry, not when the entry’s value is null. But even if we never do that, the reason we have to add passOut is to make HashMake::take compile the code for that case.

> Source/WebCore/dom/ChildListMutationScope.cpp:245
> -        m_accumulations.set(target, 0);
> +        m_accumulations.set(target, nullptr);

Maybe we should do a remove here instead of a set.

> Source/WebCore/dom/ChildListMutationScope.cpp:261
> -    RefPtr<ChildListMutationAccumulator> record = m_accumulations.take(target);
> +    OwnPtr<ChildListMutationAccumulator> record(m_accumulations.take(target));

Why not use "="? I find the other style harder to read.

[Adam Klein]

Comment on attachment 118423 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        * wtf/HashTraits.h: Add passOut(std::nullptr_t) to allow callers to use HashMap::take on an entry whose value is null.
> 
> This is an incorrect comment. The code is used when take is called and there is no entry, not when the entry’s value is null. But even if we never do that, the reason we have to add passOut is to make HashMake::take compile the code for that case.

Fixed.

>> Source/WebCore/dom/ChildListMutationScope.cpp:245
>> +        m_accumulations.set(target, nullptr);
> 
> Maybe we should do a remove here instead of a set.

We actually use the nullptr here as a sentry value indicating that we've already checked for interested observers, so we can avoid doing that work each time we increase the scoping level.

>> Source/WebCore/dom/ChildListMutationScope.cpp:261
>> +    OwnPtr<ChildListMutationAccumulator> record(m_accumulations.take(target));
> 
> Why not use "="? I find the other style harder to read.

When building Chromium Mac, clang complains about the '=' form:

../../JavaScriptCore/wtf/OwnPtr.h:52:9:error: function 'WTF::OwnPtr<WebCore::<anonymous>::ChildListMutationAccumulator>::OwnPtr' has internal linkage but is not defined [-Werror]
        OwnPtr(const OwnPtr<ValueType>&);
        ^
/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../dom/ChildListMutationScope.cpp:261:42: note: used here
    OwnPtr<ChildListMutationAccumulator> record = m_accumulations.take(target);
                                         ^
1 error generated.

[Adam Klein]

Created attachment 118476 [details]
Patch for landing

[Adam Klein]

(In reply to comment #7)
> (From update of attachment 118423 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118423&action=review
>  
> >> Source/WebCore/dom/ChildListMutationScope.cpp:245
> >> +        m_accumulations.set(target, nullptr);
> > 
> > Maybe we should do a remove here instead of a set.
> 
> We actually use the nullptr here as a sentry value indicating that we've already checked for interested observers, so we can avoid doing that work each time we increase the scoping level.
>

Reading the code again I see that this is only used by ASSERTions, so we could get away with this. But I'd rather leave that for later if it's something Rafael wants to do.

[Darin Adler]

(In reply to comment #7)
> >> Source/WebCore/dom/ChildListMutationScope.cpp:261
> >> +    OwnPtr<ChildListMutationAccumulator> record(m_accumulations.take(target));
> > 
> > Why not use "="? I find the other style harder to read.
> 
> When building Chromium Mac, clang complains about the '=' form:

OK, makes sense.

[WebKit Review Bot]

Comment on attachment 118476 [details]
Patch for landing

Clearing flags on attachment: 118476

Committed r102410: <http://trac.webkit.org/changeset/102410>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.