Bug 105983 – Add some constness to FrameView::ScrollableAreaSet and friends

• Component: Layout and Rendering
• Status: RESOLVED INVALID

[Antonio Gomes]

It was a request from James Robinson on IRC today. Should be quick ...

[Antonio Gomes]

Created attachment 181134 [details]
patch

[Alexey Proskuryakov]

It's not obvious to me if ScrollableArea needs any constness. There are certain types of classes where we avoid using const, because the cost of maintenance and confusion is much higher than benefits.

Semantically, what does it mean for a ScrollableArea to be constant? As a specific example, why is invalidateScrollbarRect() not marked const? This method doesn't change the area, it only affects its content. All scrolling methods are arguably in the same category.

[Antonio Gomes]

Some context:

(jbarth = james robinson)

[21:28]  <jbarth> tonikitoo: can you make FrameView::containsScrollableArea() take a const pointer instead of const_cast<>ing?
[21:30]  <jbarth> does it change anything other than this callsite?
[21:30]  <jbarth> looks like it's just used to do a map lookup, which should be fine to do with a const ptr
[21:30]  * dethbakin has quit (Quit: dethbakin)
[21:31]  <tonikitoo> ok, do you want it in a separated/preparatory step?
[21:31]  <jbarth> no same one is fine

[James Robinson]

Comment on attachment 181134 [details]
patch

I think having the FrameView::ScrollableAreaSet be a set of const ScrollableArea*s is reasonable since Antonio wants to add a caller that is adding things to the set from a const ScrollableArea*.

[Antonio Gomes]

(In reply to comment #4)
> (From update of attachment 181134 [details])
> I think having the FrameView::ScrollableAreaSet be a set of const ScrollableArea*s is reasonable since Antonio wants to add a caller that is adding things to the set from a const ScrollableArea*.

In fact, after rebasing against ToT, the code in bug 95494 is now adding "this" to the cache/set from a non const method, which requires no const_cast (previously it did need to be cast'ed). So then the first reason for this patch is now INVALID.
That said, I will close it as INVALID for now. Other Sets in FrameView also use a non-const form, so lets keep them in sync.

[Eric Seidel (no email)]

Comment on attachment 181134 [details]
patch

Cleared review? from attachment 181134 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).