Bug 143198

Summary: Convert arguments to ScrollingCoordinator functions to references
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, cmarcelo, commit-queue, esprehn+autocc, glenn, jamesr, japhet, kangil.han, kondapallykalyan, luiz, simon.fraser, thorton, tonikitoo, zalan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch darin: review+

Simon Fraser (smfr)
Reported 2015-03-29 13:11:38 PDT
Convert arguments to ScrollingCoordinator functions to references
Attachments
Patch (67.28 KB, patch)
2015-03-29 13:14 PDT, Simon Fraser (smfr)
no flags
Patch (68.63 KB, patch)
2015-03-29 13:29 PDT, Simon Fraser (smfr)
no flags
Patch (72.10 KB, patch)
2015-03-29 13:35 PDT, Simon Fraser (smfr)
no flags
Patch (72.07 KB, patch)
2015-03-29 13:41 PDT, Simon Fraser (smfr)
no flags
Patch (73.54 KB, patch)
2015-03-29 14:08 PDT, Simon Fraser (smfr)
darin: review+
Simon Fraser (smfr)
Comment 1 2015-03-29 13:14:25 PDT
Simon Fraser (smfr)
Comment 2 2015-03-29 13:29:58 PDT
Simon Fraser (smfr)
Comment 3 2015-03-29 13:35:08 PDT
Simon Fraser (smfr)
Comment 4 2015-03-29 13:41:40 PDT
Simon Fraser (smfr)
Comment 5 2015-03-29 14:08:02 PDT
Darin Adler
Comment 6 2015-03-29 16:32:01 PDT
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)
Comment 7 2015-03-29 21:16:02 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.