Bug 81016 – [mac/chromium] Change platform code to use pixelSnappedRect methods

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Emil A Eklund]

Change platform code to use Node::getPixelSnappedRect, pixelSnappedBoundingBoxRect and pixelSnappedAbsoluteClippedOverflowRect to avoid exposing subpixel types to the platform layer.

[Emil A Eklund]

Created attachment 131688 [details]
Patch

[Eric Seidel (no email)]

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.

[Emil A Eklund]

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.

[Early Warning System Bot]

Comment on attachment 131688 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 131688 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 131688 [details]
Patch

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

[Emil A Eklund]

Created attachment 131723 [details]
Patch

[WebKit Review Bot]

Comment on attachment 131723 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 131723 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 131723 [details]
Patch

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

[Emil A Eklund]

Created attachment 131968 [details]
Patch

[Emil A Eklund]

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

[Eric Seidel (no email)]

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. :(

[Emil A Eklund]

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.

[WebKit Review Bot]

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

[Emil A Eklund]

Created attachment 132671 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 132671 [details]
Patch for landing

Clearing flags on attachment: 132671

Committed r111268: <http://trac.webkit.org/changeset/111268>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.