[v8] Refactoring: extract IntrusiveDOMWrapperMap into a seprate class and files.
Created attachment 79773 [details] Patch
It violates style rule about indentation in namespaces. I'd ask for waiver as it's a common style under bindings/v8. But I can easily adjust
Attachment 79773 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/v8/IntrusiveDOMWrapperMap.h:39: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 79773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79773&action=review I'd be inclined to just go ahead and fix the namespace indent. Start this file off confirming to style guide and all that :) > Source/WebCore/bindings/v8/IntrusiveDOMWrapperMap.cpp:36 > +bool IntrusiveDOMWrapperMap::removeIfPresent(Node* obj, v8::Persistent<v8::Object> value) Is there something special about this function that it's the only one implemented in the cpp? Just seems a weird split between the .h and the .cpp to me.
(In reply to comment #4) > (From update of attachment 79773 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79773&action=review > > I'd be inclined to just go ahead and fix the namespace indent. Start this file off confirming to style guide and all that :) Ok, np. > > > Source/WebCore/bindings/v8/IntrusiveDOMWrapperMap.cpp:36 > > +bool IntrusiveDOMWrapperMap::removeIfPresent(Node* obj, v8::Persistent<v8::Object> value) > > Is there something special about this function that it's the only one implemented in the cpp? Just seems a weird split between the .h and the .cpp to me. I don't remember---something tells me it was missing class definitions, but I am not sure. I'll give a try next week. It's not performance critical though, so keeping it in .cpp might be a good thing code-size-wise. Thanks a lot for feedback, Nate! And have a nice weekend.
Created attachment 79935 [details] Patch
Nate, I've uploaded new version which fixes style violation and brings removeIfPresent method into the header. May you have another look? (In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 79773 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=79773&action=review > > > > I'd be inclined to just go ahead and fix the namespace indent. Start this file off confirming to style guide and all that :) > > Ok, np. > > > > > > Source/WebCore/bindings/v8/IntrusiveDOMWrapperMap.cpp:36 > > > +bool IntrusiveDOMWrapperMap::removeIfPresent(Node* obj, v8::Persistent<v8::Object> value) > > > > Is there something special about this function that it's the only one implemented in the cpp? Just seems a weird split between the .h and the .cpp to me. > > I don't remember---something tells me it was missing class definitions, but I am not sure. I'll give a try next week. It's not performance critical though, so keeping it in .cpp might be a good thing code-size-wise. > > Thanks a lot for feedback, Nate! And have a nice weekend.
Comment on attachment 79935 [details] Patch Thanks a lot, Nate
Comment on attachment 79935 [details] Patch Clearing flags on attachment: 79935 Committed r76541: <http://trac.webkit.org/changeset/76541>
All reviewed patches have been landed. Closing bug.