Bug 52911 – [v8] Refactoring: extract IntrusiveDOMWrapperMap into a seprate class and files.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[anton muhin]

[v8] Refactoring: extract IntrusiveDOMWrapperMap into a seprate class and files.

[anton muhin]

Created attachment 79773 [details]
Patch

[anton muhin]

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

[WebKit Review Bot]

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.

[Nate Chapin]

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.

[anton muhin]

(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.

[anton muhin]

Created attachment 79935 [details]
Patch

[anton muhin]

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.

[anton muhin]

Comment on attachment 79935 [details]
Patch

Thanks a lot, Nate

[WebKit Commit Bot]

Comment on attachment 79935 [details]
Patch

Clearing flags on attachment: 79935

Committed r76541: <http://trac.webkit.org/changeset/76541>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.