Bug 129058

Summary: Move to using std::unique_ptr for Element, Node and related classes
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cmarcelo, commit-queue, darin, esprehn+autocc, kangil.han
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 129321    
Bug Blocks: 128007    
Attachments:
Description Flags
Patch
none
Patch none

Description Zan Dobersek 2014-02-19 13:50:21 PST
Move to using std::unique_ptr for Element, Node and related classes
Comment 1 Zan Dobersek 2014-02-19 13:56:04 PST
Created attachment 224671 [details]
Patch
Comment 2 WebKit Commit Bot 2014-02-19 13:58:43 PST
Attachment 224671 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/ElementIteratorAssertions.h:57:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Zan Dobersek 2014-02-19 14:00:32 PST
Created attachment 224672 [details]
Patch
Comment 4 WebKit Commit Bot 2014-02-19 14:03:22 PST
Attachment 224672 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/ElementIteratorAssertions.h:57:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Anders Carlsson 2014-02-24 08:31:49 PST
Comment on attachment 224672 [details]
Patch

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

> Source/WebCore/dom/Node.cpp:344
> -        data = ElementRareData::create(toRenderElement(m_data.m_renderer)).leakPtr();
> +        data = new ElementRareData(toRenderElement(m_data.m_renderer));
>      else
> -        data = NodeRareData::create(m_data.m_renderer).leakPtr();
> +        data = new NodeRareData(m_data.m_renderer);

I think you should use std::make_unique with release() here to avoid the ugly new expressions.
Comment 6 Zan Dobersek 2014-02-24 11:48:24 PST
Comment on attachment 224672 [details]
Patch

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

>> Source/WebCore/dom/Node.cpp:344
>> +        data = new NodeRareData(m_data.m_renderer);
> 
> I think you should use std::make_unique with release() here to avoid the ugly new expressions.

Should this be a general rule or just a one-time exception? Should we rigorously try to remove any use of new?
Comment 7 Zan Dobersek 2014-02-25 00:42:54 PST
Comment on attachment 224672 [details]
Patch

Clearing flags on attachment: 224672

Committed r164638: <http://trac.webkit.org/changeset/164638>
Comment 8 Zan Dobersek 2014-02-25 00:43:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Brent Fulgham 2014-02-25 11:57:03 PST
This broke the Windows build:

    1>c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\dom\DocumentOrderedMap.cpp(227): error C2280: 'std::unique_ptr<WebCore::NoEventDispatchAssertion,std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : attempting to reference a deleted function
                 with
                 [
                     _Ty=WebCore::NoEventDispatchAssertion
                 ] (..\dom\DOMAllInOne.cpp)
                 C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\memory(1486) : see declaration of 'std::unique_ptr<WebCore::NoEventDispatchAssertion,std::default_delete<_Ty>>::unique_ptr'
                 with
                 [
                     _Ty=WebCore::NoEventDispatchAssertion
                 ]
                 This diagnostic occurred in the compiler generated function 'WebCore::ElementIteratorAssertions::ElementIteratorAssertions(const WebCore::ElementIteratorAssertions &)'
Comment 10 Brent Fulgham 2014-02-25 12:25:14 PST
(In reply to comment #9)
> This broke the Windows build:
> 
>     1>c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\dom\DocumentOrderedMap.cpp(227): error C2280: 'std::unique_ptr<WebCore::NoEventDispatchAssertion,std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : attempting to reference a deleted function
>                  with
>                  [
>                      _Ty=WebCore::NoEventDispatchAssertion
>                  ] (..\dom\DOMAllInOne.cpp)
>                  C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\memory(1486) : see declaration of 'std::unique_ptr<WebCore::NoEventDispatchAssertion,std::default_delete<_Ty>>::unique_ptr'
>                  with
>                  [
>                      _Ty=WebCore::NoEventDispatchAssertion
>                  ]
>                  This diagnostic occurred in the compiler generated function 'WebCore::ElementIteratorAssertions::ElementIteratorAssertions(const WebCore::ElementIteratorAssertions &)'

It's trying to copy the unique_ptr in this routine.
Comment 11 WebKit Commit Bot 2014-02-25 12:33:08 PST
Re-opened since this is blocked by bug 129321
Comment 12 Brent Fulgham 2014-02-25 12:51:12 PST
Windows is complaining because it doesn't like this line:

        auto it = entry.element ? elementDescandents.beginAt(*entry.element) : elementDescandents.begin();

You can establish the value of 'it' using either elementDescandents.begin(), or elementDescandents.beginAt(*entry.element), and everything is fine. But if you attempt to use the ternary operator you get the build error.

If you attempt to do it without the ternary operator:

auto it = elementDescandents.begin();
if (entry.element)
    it = elementDescandents.beginAT(*entryElement)

You also get the error.
Comment 13 Darin Adler 2015-04-20 10:05:52 PDT
This was all done eventually; the code in question is gone.