Bug 73964 - Use HashMap<Node*, OwnPtr<...>> in ChildListMutationScope
Summary: Use HashMap<Node*, OwnPtr<...>> in ChildListMutationScope
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on: 74032
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-06 16:57 PST by Adam Klein
Modified: 2011-12-08 17:57 PST (History)
4 users (show)

See Also:


Attachments
Patch (5.58 KB, patch)
2011-12-06 16:58 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (5.62 KB, patch)
2011-12-08 11:01 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (5.61 KB, patch)
2011-12-08 15:04 PST, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2011-12-06 16:57:01 PST
Use HashMap<Node*, OwnPtr<...>> in ChildListMutationScope
Comment 1 Adam Klein 2011-12-06 16:58:38 PST
Created attachment 118144 [details]
Patch
Comment 2 WebKit Review Bot 2011-12-07 13:02:24 PST
Comment on attachment 118144 [details]
Patch

Clearing flags on attachment: 118144

Committed r102267: <http://trac.webkit.org/changeset/102267>
Comment 3 WebKit Review Bot 2011-12-07 13:02:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 Adam Klein 2011-12-07 16:06:29 PST
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.
Comment 5 Adam Klein 2011-12-08 11:01:12 PST
Created attachment 118423 [details]
Patch
Comment 6 Darin Adler 2011-12-08 14:52:01 PST
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 7 Adam Klein 2011-12-08 15:03:27 PST
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.
Comment 8 Adam Klein 2011-12-08 15:04:38 PST
Created attachment 118476 [details]
Patch for landing
Comment 9 Adam Klein 2011-12-08 15:08:02 PST
(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.
Comment 10 Darin Adler 2011-12-08 16:29:00 PST
(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 11 WebKit Review Bot 2011-12-08 17:57:16 PST
Comment on attachment 118476 [details]
Patch for landing

Clearing flags on attachment: 118476

Committed r102410: <http://trac.webkit.org/changeset/102410>
Comment 12 WebKit Review Bot 2011-12-08 17:57:21 PST
All reviewed patches have been landed.  Closing bug.