Bug 56856 - RenderListBox needs to be added to Page::scrollableAreaSet()
Summary: RenderListBox needs to be added to Page::scrollableAreaSet()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-22 12:33 PDT by Beth Dakin
Modified: 2011-04-08 06:53 PDT (History)
2 users (show)

See Also:


Attachments
Pacth (2.10 KB, patch)
2011-03-22 12:42 PDT, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff
Patch sent to wrong bug. Please delete. (8.46 KB, text/plain)
2011-04-08 06:51 PDT, Leandro Graciá Gil
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2011-03-22 12:33:14 PDT
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.
Comment 1 Beth Dakin 2011-03-22 12:42:50 PDT
Created attachment 86493 [details]
Pacth
Comment 2 Simon Fraser (smfr) 2011-03-22 13:06:14 PDT
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?
Comment 3 Beth Dakin 2011-03-22 13:18:02 PDT
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.
Comment 4 Beth Dakin 2011-03-22 14:33:49 PDT
Thanks!

Fixed with revision 81704.
Comment 5 Leandro Graciá Gil 2011-04-08 06:51:46 PDT
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.
Comment 6 Leandro Graciá Gil 2011-04-08 06:53:19 PDT
(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.