Bug 143198 – Convert arguments to ScrollingCoordinator functions to references

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Convert arguments to ScrollingCoordinator functions to references

[Simon Fraser (smfr)]

Created attachment 249694 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 249695 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 249696 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 249697 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 249699 [details]
Patch

[Darin Adler]

Comment on attachment 249699 [details]
Patch

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

Not sure that there is a useful distinction between const FrameView& and FrameView&. While I understand that some functions might not change anything, generally FrameView is a pointer into a massive object graph, not a single object that can be mutated or left alone directly. If I get a FrameView’s document and the operate on it and it turns around and operates on the FrameView is that a const FrameView& thing or not?

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:193
> -        ForcedOnMainThread = 1 << 0,
> -        HasSlowRepaintObjects = 1 << 1,
> -        HasViewportConstrainedObjectsWithoutSupportingFixedLayers = 1 << 2,
> -        HasNonLayerViewportConstrainedObjects = 1 << 3,
> -        IsImageDocument = 1 << 4
> +        ForcedOnMainThread                                          = 1 << 0,
> +        HasSlowRepaintObjects                                       = 1 << 1,
> +        HasViewportConstrainedObjectsWithoutSupportingFixedLayers   = 1 << 2,
> +        HasNonLayerViewportConstrainedObjects                       = 1 << 3,
> +        IsImageDocument                                             = 1 << 4

We should discuss whether to do this or not, and put it in the coding style guidelines one way or another. If I had seen code like the “after” here, I would have changed it to look like the “before”. My rationale is that I don’t like having to re-line things up when renaming.

[Simon Fraser (smfr)]

https://trac.webkit.org/r182132

(In reply to comment #6)
> Comment on attachment 249699 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249699&action=review
> 
> Not sure that there is a useful distinction between const FrameView& and
> FrameView&. While I understand that some functions might not change
> anything, generally FrameView is a pointer into a massive object graph, not
> a single object that can be mutated or left alone directly. If I get a
> FrameView’s document and the operate on it and it turns around and operates
> on the FrameView is that a const FrameView& thing or not?

True. I added the const so that a later change which operates on a const ScrollableArea will work.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.h:193
> > -        ForcedOnMainThread = 1 << 0,
> > -        HasSlowRepaintObjects = 1 << 1,
> > -        HasViewportConstrainedObjectsWithoutSupportingFixedLayers = 1 << 2,
> > -        HasNonLayerViewportConstrainedObjects = 1 << 3,
> > -        IsImageDocument = 1 << 4
> > +        ForcedOnMainThread                                          = 1 << 0,
> > +        HasSlowRepaintObjects                                       = 1 << 1,
> > +        HasViewportConstrainedObjectsWithoutSupportingFixedLayers   = 1 << 2,
> > +        HasNonLayerViewportConstrainedObjects                       = 1 << 3,
> > +        IsImageDocument                                             = 1 << 4
> 
> We should discuss whether to do this or not, and put it in the coding style
> guidelines one way or another. If I had seen code like the “after” here, I
> would have changed it to look like the “before”. My rationale is that I
> don’t like having to re-line things up when renaming.

I'll start a thread about this on webkit-dev.