Bug 81016

Created attachment 131688 [details] Patch

Comment on attachment 131688 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131688&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:810 > - IntRect rect = node->Node::getRect(); > + IntRect rect = node->Node::getPixelSnappedRect(); Why does this method have "get" in the name? That's generlaly disroucaged in WebKIt? Is it DOM API or somethign? > Source/WebKit/mac/WebView/WebFrame.mm:1297 > + float printWidth = root->style()->isHorizontalWritingMode() ? static_cast<float>(documentRect.width()) / printScaleFactor : pageSize.width; Isn't there a .toFloat() method instead of a static_cast? I guess that's only for hte rect as a whole since LayoutUnit isn't a class yet... > Source/WebKit/mac/WebView/WebView.mm:2039 > + WebDashboardRegion *webRegion = [[WebDashboardRegion alloc] initWithRect:pixelSnappedIntRect(region.bounds) clip:pixelSnappedIntRect(region.clip) type:type]; Are bounds/clip LayoutRects here? > Source/WebKit/mac/WebView/WebView.mm:2500 > + [rectsArray addObject:[NSValue valueWithRect:pixelSnappedIntRect(repaintRects[i])]]; Here too? I'm surprised you need these functions much at this layer. Apple Graphics APIs all function in floats.

Comment on attachment 131688 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131688&action=review >> Source/WebKit/chromium/src/WebViewImpl.cpp:810 >> + IntRect rect = node->Node::getPixelSnappedRect(); > > Why does this method have "get" in the name? That's generlaly disroucaged in WebKIt? Is it DOM API or somethign? To be consistent with the current API. I'd gladly rename both to rect and pixelSnappedRect respectively. >> Source/WebKit/mac/WebView/WebFrame.mm:1297 >> + float printWidth = root->style()->isHorizontalWritingMode() ? static_cast<float>(documentRect.width()) / printScaleFactor : pageSize.width; > > Isn't there a .toFloat() method instead of a static_cast? I guess that's only for hte rect as a whole since LayoutUnit isn't a class yet... LayoutUnit has a toFloat method, int doesn't though. This way it'll work both with LayoutRect defined as an IntRect and as a FractionalLayoutRect. >> Source/WebKit/mac/WebView/WebView.mm:2039 >> + WebDashboardRegion *webRegion = [[WebDashboardRegion alloc] initWithRect:pixelSnappedIntRect(region.bounds) clip:pixelSnappedIntRect(region.clip) type:type]; > > Are bounds/clip LayoutRects here? Yeah. >> Source/WebKit/mac/WebView/WebView.mm:2500 >> + [rectsArray addObject:[NSValue valueWithRect:pixelSnappedIntRect(repaintRects[i])]]; > > Here too? I'm surprised you need these functions much at this layer. Apple Graphics APIs all function in floats. The APIs are all float but we do most painting snapped to device pixels.

Comment on attachment 131688 [details] Patch Attachment 131688 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11949585

Comment on attachment 131688 [details] Patch Attachment 131688 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11941746

Comment on attachment 131688 [details] Patch Attachment 131688 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11949618

Created attachment 131723 [details] Patch

Comment on attachment 131723 [details] Patch Attachment 131723 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11949730

Comment on attachment 131723 [details] Patch Attachment 131723 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11942968

Comment on attachment 131723 [details] Patch Attachment 131723 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11951140

Created attachment 131968 [details] Patch

Eric, would you mind taking another look at this when you get a chance?

Comment on attachment 131968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131968&action=review Thank you for the reminder. bug mail gets lost in the torrent... > Source/WebCore/dom/Node.h:378 > virtual LayoutRect getRect() const; > + IntRect getPixelSnappedRect() const { return pixelSnappedIntRect(getRect()); } I really don't like the "get" here. "Rect" is also not very useful, as you don't know what "rect" it means. :(

Thanks Eric! > > Source/WebCore/dom/Node.h:378 > > virtual LayoutRect getRect() const; > > + IntRect getPixelSnappedRect() const { return pixelSnappedIntRect(getRect()); } > > I really don't like the "get" here. "Rect" is also not very useful, as you don't know what "rect" it means. :( If we can come up with a better name I'll gladly do the work to rename it. I filed bug 81413 to track this.

Comment on attachment 131968 [details] Patch Rejecting attachment 131968 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: webkit-commit-queue/Source/WebKit/chromium/webkit --revision 126866 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 48>At revision 126866. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/11963520

Created attachment 132671 [details] Patch for landing

Comment on attachment 132671 [details] Patch for landing Clearing flags on attachment: 132671 Committed r111268: <http://trac.webkit.org/changeset/111268>

All reviewed patches have been landed. Closing bug.