Use HashMap<OwnPtr> for RenderBoxRegionInfo map in RenderRegion

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Caio Marcelo de Oliveira Filho]

Use HashMap<OwnPtr> for RenderBoxRegionInfo map in RenderRegion

[Caio Marcelo de Oliveira Filho]

Created attachment 120737 [details]
Patch

[Darin Adler]

Comment on attachment 120737 [details]
Patch

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

This is OK. But a better fix for this code is to change m_renderBoxRegionInfo to be a HashMap<const RenderBox*, RenderBoxRegionInfo>. There is no reason for all that heap allocation.

> Source/WebCore/rendering/RenderRegion.cpp:221
>  RenderBoxRegionInfo* RenderRegion::takeRenderBoxRegionInfo(const RenderBox* box)
>  {
> -    return m_renderBoxRegionInfo.take(box);
> +    return m_renderBoxRegionInfo.take(box).leakPtr();
>  }

If we are going to keep this using pointers, then this function should be changed so that it returns a PassOwnPtr instead of a raw pointer.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #2)
> This is OK. But a better fix for this code is to change m_renderBoxRegionInfo to be a HashMap<const RenderBox*, RenderBoxRegionInfo>. There is no reason for all that heap allocation.

I'll give it a shot instead of landing this now.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #3)
> (In reply to comment #2)
> > This is OK. But a better fix for this code is to change m_renderBoxRegionInfo to be a HashMap<const RenderBox*, RenderBoxRegionInfo>. There is no reason for all that heap allocation.
> 
> I'll give it a shot instead of landing this now.

Darin, I spent a bit of time looking the code would have to be modified, but I'm still a bit unsure -- maybe because I'm not very familiar with Render code. Some things I've noticed:

- Clients of RenderRegion manipulate RenderBoxRegionInfo* and not the value type.  It is convenient to have the null pointer as a way to indicate there's no RBRI. This avoids a second hash lookup. 

- According to comment in RenderRegion.h, RBRI will also contain more style information in the future, this could make it less fit to be used as a value type.

    // This map holds unique information about a block that is split across regions.
    // A RenderBoxRegionInfo* tells us about any layout information for a RenderBox that
    // is unique to the region. For now it just holds logical width information for RenderBlocks, but eventually
    // it will also hold a custom style for any box (for region styling).
    HashMap<const RenderBox*, RenderBoxRegionInfo> m_renderBoxRegionInfo;

I have considered the case of trying to keep RenderRegion interface, but for this I would need a "stable" storage for RBRI, which could be done by a Vector<RBRI> + HashMap<K, RBRI*>. I'm unsure this would give us practical benefits... maybe if the vector size is somehow predictable.

What do you think? Am I missing something here?

[Darin Adler]

I think it’s OK to leave this as a pointer for now. I think it’s possible to change it to pass things by value and not use pointers at all but it would indeed take a bit of refactoring and I would *not* suggest the Vector/HashMap approach.

[Caio Marcelo de Oliveira Filho]

Created attachment 121781 [details]
Patch

[Darin Adler]

Comment on attachment 121781 [details]
Patch

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

> Source/WebCore/rendering/RenderFlowThread.cpp:669
> -        delete oldInfo;
> +        oldInfo.clear();

I’m not sure it’s necessary to do this explicitly. The delete was needed before, but an OwnPtr takes care of this even if you don’t clear explicitly. I’d be tempted to just leave it out.

> Source/WebCore/rendering/RenderRegion.cpp:228
>  void RenderRegion::deleteAllRenderBoxRegionInfo()

The call to this function in the destructor is no longer needed. In fact, the destructor need not be declared or defined at all any more, since the default one will do the right thing.

[Caio Marcelo de Oliveira Filho]

Created attachment 122043 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 122043 [details]
Patch for landing

Clearing flags on attachment: 122043

Committed r104721: <http://trac.webkit.org/changeset/104721>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 122043 [details]
Patch for landing

Oops, sorry, should not have pushed the “EWS” button on a patch that’s already landed. Sorry.