Bug 75348 - Use HashMap<OwnPtr> for RenderBoxRegionInfo map in RenderRegion
Summary: Use HashMap<OwnPtr> for RenderBoxRegionInfo map in RenderRegion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-29 05:51 PST by Caio Marcelo de Oliveira Filho
Modified: 2012-01-11 10:19 PST (History)
3 users (show)

See Also:


Attachments
Patch (3.65 KB, patch)
2011-12-29 05:53 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (5.21 KB, patch)
2012-01-09 18:52 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch for landing (5.77 KB, patch)
2012-01-11 09:56 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Marcelo de Oliveira Filho 2011-12-29 05:51:29 PST
Use HashMap<OwnPtr> for RenderBoxRegionInfo map in RenderRegion
Comment 1 Caio Marcelo de Oliveira Filho 2011-12-29 05:53:19 PST
Created attachment 120737 [details]
Patch
Comment 2 Darin Adler 2012-01-08 22:06:19 PST
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.
Comment 3 Caio Marcelo de Oliveira Filho 2012-01-09 05:51:02 PST
(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.
Comment 4 Caio Marcelo de Oliveira Filho 2012-01-09 09:02:39 PST
(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?
Comment 5 Darin Adler 2012-01-09 18:12:15 PST
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.
Comment 6 Caio Marcelo de Oliveira Filho 2012-01-09 18:52:32 PST
Created attachment 121781 [details]
Patch
Comment 7 Darin Adler 2012-01-10 09:12:32 PST
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.
Comment 8 Caio Marcelo de Oliveira Filho 2012-01-11 09:56:20 PST
Created attachment 122043 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-01-11 10:18:15 PST
Comment on attachment 122043 [details]
Patch for landing

Clearing flags on attachment: 122043

Committed r104721: <http://trac.webkit.org/changeset/104721>
Comment 10 WebKit Review Bot 2012-01-11 10:18:21 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Darin Adler 2012-01-11 10:19:06 PST
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.