Bug 127276 – Move TreeScope, IdTargetObserverRegistry to std::unique_ptr

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

Move TreeScope, IdTargetObserverRegistry to std::unique_ptr

[Zan Dobersek]

Created attachment 221639 [details]
Patch

[Zan Dobersek]

Created attachment 221721 [details]
Patch

[Brent Fulgham]

Comment on attachment 221721 [details]
Patch

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

This looks great!  I would prefer we not use the 'get()' method in the shouldCacheLabelsByForAttribute.  Can you confirm it must be implemented the way you have written that?

> Source/WebCore/dom/TreeScope.h:83
> +    bool shouldCacheLabelsByForAttribute() const { return m_labelsByForAttribute.get(); }

std::unique_ptr has an operator (bool).  Why is the 'get()' necessary here? At the worst, you should be doing something like "static_cast<bool>(m_labelsByForAttribute)".

[Anders Carlsson]

(In reply to comment #3)
> (From update of attachment 221721 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221721&action=review
> 
> This looks great!  I would prefer we not use the 'get()' method in the shouldCacheLabelsByForAttribute.  Can you confirm it must be implemented the way you have written that?
> 
> > Source/WebCore/dom/TreeScope.h:83
> > +    bool shouldCacheLabelsByForAttribute() const { return m_labelsByForAttribute.get(); }
> 
> std::unique_ptr has an operator (bool).  Why is the 'get()' necessary here? At the worst, you should be doing something like "static_cast<bool>(m_labelsByForAttribute)".

the bool operator is explicit. I think .get() is better than an explicit static cast.

[Andreas Kling]

Comment on attachment 221721 [details]
Patch

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

r=me

>>> Source/WebCore/dom/TreeScope.h:83
>>> +    bool shouldCacheLabelsByForAttribute() const { return m_labelsByForAttribute.get(); }
>> 
>> std::unique_ptr has an operator (bool).  Why is the 'get()' necessary here? At the worst, you should be doing something like "static_cast<bool>(m_labelsByForAttribute)".
> 
> the bool operator is explicit. I think .get() is better than an explicit static cast.

How about "return !!m_labelsByForAttribute;"

[Zan Dobersek]

Comment on attachment 221721 [details]
Patch

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

>>>> Source/WebCore/dom/TreeScope.h:83
>>>> +    bool shouldCacheLabelsByForAttribute() const { return m_labelsByForAttribute.get(); }
>>> 
>>> std::unique_ptr has an operator (bool).  Why is the 'get()' necessary here? At the worst, you should be doing something like "static_cast<bool>(m_labelsByForAttribute)".
>> 
>> the bool operator is explicit. I think .get() is better than an explicit static cast.
> 
> How about "return !!m_labelsByForAttribute;"

Sure.

[Zan Dobersek]

Committed r163719: <http://trac.webkit.org/changeset/163719>