Convert arguments to ScrollingCoordinator functions to references
Created attachment 249694 [details] Patch
Created attachment 249695 [details] Patch
Created attachment 249696 [details] Patch
Created attachment 249697 [details] Patch
Created attachment 249699 [details] Patch
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.
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.