Bug 139971 – Convert OwnPtr to std::unique_ptr in WebCore/graphics/

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 243780 [details]
Patch

[Gyuyoung Kim]

Created attachment 243781 [details]
Patch

[Darin Adler]

Comment on attachment 243781 [details]
Patch

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

> Source/WebCore/platform/graphics/GlyphMetricsMap.h:114
>                  return page;
>          } else
> -            m_pages = adoptPtr(new HashMap<int, OwnPtr<GlyphMetricsPage>>);
> +            m_pages = std::make_unique<HashMap<int, std::unique_ptr<GlyphMetricsPage>>>();
>          page = new GlyphMetricsPage;
> -        m_pages->set(pageNumber, adoptPtr(page));
> +        m_pages->set(pageNumber, std::unique_ptr<GlyphMetricsPage>(page));

We are really trying to avoid this idiom where we adopt by invoking the std::unique_ptr constructor. This function also does double hash table lookup and it need not do that. I suggest this code instead:

    if (!m_pages)
        m_pages = std::make_unique<HashMap<int, std::unique_ptr<GlyphMetricsPage>>>();
    auto& pageInMap = m_pages->add(pageNumber, nullptr).iterator->value;
    if (!pageInMap)
        pageInMap = std::make_unique<GlyphMetricsPage>();
    page = pageInMap;

This will set up the page in the map with only a single hash table lookup.

> Source/WebCore/platform/graphics/GlyphPageTreeNode.cpp:345
> +        m_children.set(fontData, std::unique_ptr<GlyphPageTreeNode>(child));

Again, this is not the idiom we want. Instead we should put the GlyphPageTreeNode into a local variable that is already a unique_ptr.

One way to do this would be to have both a unique_ptr and a raw pointer pointing at the same thing.

But even better, it’s possible to rearrange this function so it won’t have a raw pointer and also eliminate the double hashing it does. We would figure out where the child goes right at the start, replacing the "foundChild" code.

    auto& child = fontData ? m_children.add(fontData, nullptr).iterator->value : m_systemFallbackChild;
    if (child)
        return child.get();

    child = std::make_unique<GlyphPageTreeNode>();

    ...

    return child.get();

The other code in the function would stay the same.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.h:40
> +    explicit Extensions3DOpenGL(GraphicsContext3D*);

This argument should really be a reference rather than a pointer. But no need to change it right now.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.h:61
>      // This class only needs to be instantiated by GraphicsContext3D implementations.

I don’t think this comment makes sense any more.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.h:62
>      friend class GraphicsContext3D;

I don’t think this line of code is helpful any more.

[Darin Adler]

Comment on attachment 243781 [details]
Patch

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

>> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.h:61
>>      // This class only needs to be instantiated by GraphicsContext3D implementations.
> 
> I don’t think this comment makes sense any more.

The comment *might* have some value, but it belongs up at the constructor, I think.

[Gyuyoung Kim]

Created attachment 243788 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 243781 [details]
Patch

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

>> Source/WebCore/platform/graphics/GlyphMetricsMap.h:114
>> +        m_pages->set(pageNumber, std::unique_ptr<GlyphMetricsPage>(page));
> 
> We are really trying to avoid this idiom where we adopt by invoking the std::unique_ptr constructor. This function also does double hash table lookup and it need not do that. I suggest this code instead:
> 
>     if (!m_pages)
>         m_pages = std::make_unique<HashMap<int, std::unique_ptr<GlyphMetricsPage>>>();
>     auto& pageInMap = m_pages->add(pageNumber, nullptr).iterator->value;
>     if (!pageInMap)
>         pageInMap = std::make_unique<GlyphMetricsPage>();
>     page = pageInMap;
> 
> This will set up the page in the map with only a single hash table lookup.

Thank you for your suggestion. New patch uses this code.

>> Source/WebCore/platform/graphics/GlyphPageTreeNode.cpp:345
>> +        m_children.set(fontData, std::unique_ptr<GlyphPageTreeNode>(child));
> 
> Again, this is not the idiom we want. Instead we should put the GlyphPageTreeNode into a local variable that is already a unique_ptr.
> 
> One way to do this would be to have both a unique_ptr and a raw pointer pointing at the same thing.
> 
> But even better, it’s possible to rearrange this function so it won’t have a raw pointer and also eliminate the double hashing it does. We would figure out where the child goes right at the start, replacing the "foundChild" code.
> 
>     auto& child = fontData ? m_children.add(fontData, nullptr).iterator->value : m_systemFallbackChild;
>     if (child)
>         return child.get();
> 
>     child = std::make_unique<GlyphPageTreeNode>();
> 
>     ...
> 
>     return child.get();
> 
> The other code in the function would stay the same.

I try to apply your second suggestion to new patch. Thanks.

[WebKit Commit Bot]

Comment on attachment 243788 [details]
Patch

Clearing flags on attachment: 243788

Committed r177786: <http://trac.webkit.org/changeset/177786>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Gyuyoung Kim]

This commit made a build break on win port. I landed two fixes for win port though, second fix is not tested on win bot yet.

https://trac.webkit.org/changeset/177787
https://trac.webkit.org/changeset/177788

[Chris Dumez]

Comment on attachment 243788 [details]
Patch

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

> Source/WebCore/platform/graphics/GlyphMetricsMap.h:-110
> -                return page;

Previously, there was an early return here, which meant that we would only do the for loop below (the one initializing the page with unknown metrics) if the page is new. After your change, we always fill the page with unknown metrics, even if it is an existing page :/
I will fix.