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+

Zan Dobersek
Reported 2014-01-20 02:28:56 PST
Move TreeScope, IdTargetObserverRegistry to std::unique_ptr
Attachments
Patch (8.41 KB, patch)
2014-01-20 02:31 PST, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
Patch (8.37 KB, patch)
2014-01-20 22:34 PST, Zan Dobersek 		kling: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Zan Dobersek
Comment 1 2014-01-20 02:31:22 PST
Created attachment 221639 [details] Patch
Zan Dobersek
Comment 2 2014-01-20 22:34:55 PST
Created attachment 221721 [details] Patch
Brent Fulgham
Comment 3 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)".
Anders Carlsson
Comment 4 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.
Andreas Kling
Comment 5 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;"
Zan Dobersek
Comment 6 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.
Zan Dobersek
Comment 7 2014-02-08 12:05:19 PST
Committed r163719: <http://trac.webkit.org/changeset/163719>
Note You need to log in before you can comment on or make changes to this bug.