Use HashMap<Node*, OwnPtr<...>> in ChildListMutationScope
Created attachment 118144 [details] Patch
Comment on attachment 118144 [details] Patch Clearing flags on attachment: 118144 Committed r102267: <http://trac.webkit.org/changeset/102267>
All reviewed patches have been landed. Closing bug.
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.
Created attachment 118423 [details] Patch
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.
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.
Created attachment 118476 [details] Patch for landing
(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.
(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.
Comment on attachment 118476 [details] Patch for landing Clearing flags on attachment: 118476 Committed r102410: <http://trac.webkit.org/changeset/102410>