Bug 127276

Summary: Move TreeScope, IdTargetObserverRegistry to std::unique_ptr
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, bfulgham, commit-queue, esprehn+autocc, kangil.han, kling, koivisto
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 128007    
Attachments:
Description Flags
Patch
none
Patch kling: review+

Description Zan Dobersek 2014-01-20 02:28:56 PST 
Move TreeScope, IdTargetObserverRegistry to std::unique_ptr
Comment 1 Zan Dobersek 2014-01-20 02:31:22 PST 
Created attachment 221639 [details]
Patch
Comment 2 Zan Dobersek 2014-01-20 22:34:55 PST 
Created attachment 221721 [details]
Patch
Comment 3 Brent Fulgham 2014-01-21 12:08:08 PST 
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)".
Comment 4 Anders Carlsson 2014-01-21 12:38:46 PST 
(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.
Comment 5 Andreas Kling 2014-02-06 12:57:31 PST 
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;"
Comment 6 Zan Dobersek 2014-02-08 11:58:30 PST 
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.
Comment 7 Zan Dobersek 2014-02-08 12:05:19 PST 
Committed r163719: <http://trac.webkit.org/changeset/163719>