Bug 70050

Created attachment 110899 [details] Patch

Comment on attachment 110899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110899&action=review > Source/WebKit/mac/WebView/WebView.mm:2703 > +- (CGFloat)_getBackingScaleFactor Why the word “get” in this method name? > Tools/DumpRenderTree/LayoutTestController.cpp:2204 > + // The second argument is a callbac that is called once the backing scale factor has been set. The word “callback” here is missing its final “k”. > Tools/DumpRenderTree/LayoutTestController.cpp:2488 > + { "setBackingScaleFactor", setBackingScaleFactorCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete }, How about using a property that you set instead of a function that you call? > Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm:198 > + if (deviceScaleFactor != 1) { > + [view displayRectIgnoringOpacity:[view bounds] inContext:nsContext]; > + if ([view isTrackingRepaints]) > + paintRepaintRectOverlay(view, context); This code could use a why comment. > Tools/WebKitTestRunner/TestController.cpp:451 > + // Re-set to the default backing scale factor by setting the custom scale factor to 0. > + WKPageSetCustomBackingScaleFactor(m_mainWebView->page(), 0); It’s typically more elegant to use a separate function rather than a magic value for this sort of thing. But I guess we decided that before. > Tools/WebKitTestRunner/cg/TestInvocationCG.cpp:164 > + // Don't use window snapshots for HiDPI tests. > + bool testIsHiDPI = WKPageGetBackingScaleFactor(webView->page()) != 1; This comment says “what” the code does, but the code already says that. What the comment needs to do is to say “why”. Also, the code would match the comment better if it set windowSnapshot to 0 instead of using a separate boolean that’s tested below. Otherwise, the comment belongs below where testIsHiDPI is used, not here where it says. > LayoutTests/ChangeLog:8 > + New HiDPI tests and results. These should be skipped on all non-Lion platforms. We’d want to run these on iOS too, so all non-Lion seems a little wrong. > LayoutTests/ChangeLog:9 > + * fast/scaling-zooming-hidpi: Added. Maybe this should just be named “hidpi”. I presume it would not be good to put scaling and zooming tests in this directory since they should not be skipped.

Comment on attachment 110899 [details] Patch Attachment 110899 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10060323 New failing tests: fast/scaling-zooming-hidpi/broken-image-icon-hidpi.html fast/scaling-zooming-hidpi/video-controls-in-hidpi.html fast/scaling-zooming-hidpi/clip-text-in-hidpi.html fast/scaling-zooming-hidpi/resize-corner-hidpi.html fast/scaling-zooming-hidpi/broken-image-with-size-hidpi.html

(In reply to comment #2) > (From update of attachment 110899 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110899&action=review > > > Source/WebKit/mac/WebView/WebView.mm:2703 > > +- (CGFloat)_getBackingScaleFactor > > Why the word “get” in this method name? > "get" is a part of the equivalent WK2 function name (WKPageGetBackingScaleFactor). But I agree it doesn't seem very WebKit 1-ish. I removed the "get." > > Tools/DumpRenderTree/LayoutTestController.cpp:2204 > > + // The second argument is a callbac that is called once the backing scale factor has been set. > > The word “callback” here is missing its final “k”. > Oops, thanks! Fixed. > > Tools/DumpRenderTree/LayoutTestController.cpp:2488 > > + { "setBackingScaleFactor", setBackingScaleFactorCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete }, > > How about using a property that you set instead of a function that you call? > I don't think I understand this suggestion. Could you elaborate? > > Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm:198 > > + if (deviceScaleFactor != 1) { > > + [view displayRectIgnoringOpacity:[view bounds] inContext:nsContext]; > > + if ([view isTrackingRepaints]) > > + paintRepaintRectOverlay(view, context); > > This code could use a why comment. > Added one. > > Tools/WebKitTestRunner/TestController.cpp:451 > > + // Re-set to the default backing scale factor by setting the custom scale factor to 0. > > + WKPageSetCustomBackingScaleFactor(m_mainWebView->page(), 0); > > It’s typically more elegant to use a separate function rather than a magic value for this sort of thing. But I guess we decided that before. > We could definitely revisit this. I remember discussing it before, but I don't remember why we decided that the 0 was okay. I thought we at least wanted a constant for 0? Maybe I just forgot to do that… > > Tools/WebKitTestRunner/cg/TestInvocationCG.cpp:164 > > + // Don't use window snapshots for HiDPI tests. > > + bool testIsHiDPI = WKPageGetBackingScaleFactor(webView->page()) != 1; > > This comment says “what” the code does, but the code already says that. What the comment needs to do is to say “why”. > > Also, the code would match the comment better if it set windowSnapshot to 0 instead of using a separate boolean that’s tested below. Otherwise, the comment belongs below where testIsHiDPI is used, not here where it says. > Adjusted. Thanks. > > LayoutTests/ChangeLog:8 > > + New HiDPI tests and results. These should be skipped on all non-Lion platforms. > > We’d want to run these on iOS too, so all non-Lion seems a little wrong. > Fixed wording. > > LayoutTests/ChangeLog:9 > > + * fast/scaling-zooming-hidpi: Added. > > Maybe this should just be named “hidpi”. I presume it would not be good to put scaling and zooming tests in this directory since they should not be skipped. True. Re-named. Committed changes with revision 97407.