Bug 56856 – RenderListBox needs to be added to Page::scrollableAreaSet()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

http://trac.webkit.org/changeset/81209 added a HashSet of ScrollableAreas to Page. The patch I attached to https://bugs.webkit.org/show_bug.cgi?id=56067 correctly added code to RenderListBox to add and remove them from the set, but somehow this code did not get committed. Here is a patch to add RenderListBoxes to the set.

[Beth Dakin]

Created attachment 86493 [details]
Pacth

[Simon Fraser (smfr)]

Comment on attachment 86493 [details]
Pacth

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

> Source/WebCore/rendering/RenderListBox.cpp:88
> +    if (Page* page = frame()->page()) {
> +        m_page = page;
> +        m_page->addScrollableArea(this);
> +    }

It's unfortunate to have to store the Page here. Is there a way to do the removeScrollableArea() earlier, while the renderer can still get to the page?

[Beth Dakin]

An alternative design that might work would be to have a call in Document::detach() that empties the Page's HashSet completely, and then to leave code in the RenderBlah destructors that removes the object from the set only if the frame still has a page, thereby implying that the document has not been detached. 

However, I would really rather file a different bug to do this differently if that's what we want to do RenderLayer and RenderDataGrid already got local Page pointers in https://bugs.webkit.org/show_bug.cgi?id=56067 , and it was just a mistake that this code was not committed then. I would rather not spend time re-designing this right now to fix this bad oversight, but I am definitely open to re-designing it at a later point in time.

[Beth Dakin]

Thanks!

Fixed with revision 81704.

[Leandro Graciá Gil]

Created attachment 88813 [details]
Patch sent to wrong bug. Please delete.

Adding the new tests to the skip lists of all non-chromium platforms as the Media Stream API is not enabled in them.

[Leandro Graciá Gil]

(In reply to comment #5)
> Created an attachment (id=88813) [details]
> Patch
> 
> Adding the new tests to the skip lists of all non-chromium platforms as the Media Stream API is not enabled in them.

Sorry, wrong bug number when submitting it (56586). Please delete if possible.