Bug 169991

Summary: [Cocoa] Add an option to exclude overflow when snapshotting a WKWebProcessPlugInNodeHandle
Product: WebKit Reporter: Andy Estes <aestes>
Component: New BugsAssignee: Andy Estes <aestes>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, buildbot, commit-queue, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Andy Estes 2017-03-22 21:02:01 PDT 
[Cocoa] Add an option to exclude overflow when snapshotting a WKWebProcessPlugInNodeHandle
Comment 1 Andy Estes 2017-03-22 21:02:50 PDT 
rdar://problem/30141083
Comment 2 Andy Estes 2017-03-22 21:17:13 PDT 
Created attachment 305160 [details]
Patch
Comment 3 Build Bot 2017-03-22 21:18:28 PDT 
Attachment 305160 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandle.mm:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Tim Horton 2017-03-22 21:24:50 PDT 
Comment on attachment 305160 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305160&action=review

> Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandle.mm:71
> -    RefPtr<WebImage> image = _nodeHandle->renderedImage(options);
> +    RefPtr<WebImage> image = _nodeHandle->renderedImage(toSnapshotOptions(options), options & kWKSnapshotOptionsExcludeOverflow);

Maybe it's obvious and I'm missing it, but why not add a SnapshotOptions bit for this?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/rendered-image-excluding-overflow.html:20
> +    <div class=container>
> +        <div class=inner>

Missing quotes, eww :)
Comment 5 Andy Estes 2017-03-22 22:17:50 PDT 
(In reply to Tim Horton from comment #4)
> Comment on attachment 305160 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305160&action=review
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandle.mm:71
> > -    RefPtr<WebImage> image = _nodeHandle->renderedImage(options);
> > +    RefPtr<WebImage> image = _nodeHandle->renderedImage(toSnapshotOptions(options), options & kWKSnapshotOptionsExcludeOverflow);
> 
> Maybe it's obvious and I'm missing it, but why not add a SnapshotOptions bit
> for this?

I didn't do this because SnapshotOptions is used in a bunch of other places (for drag images, text indicators, etc), but kWKSnapshotOptionsExcludeOverflow is only meaningful in this context.

Thanks for the review!
Comment 6 WebKit Commit Bot 2017-03-22 23:21:29 PDT 
Comment on attachment 305160 [details]
Patch

Clearing flags on attachment: 305160

Committed r214297: <http://trac.webkit.org/changeset/214297>
Comment 7 WebKit Commit Bot 2017-03-22 23:21:33 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 8 Simon Fraser (smfr) 2017-03-23 12:11:10 PDT 
Comment on attachment 305160 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305160&action=review

> Source/WebKit2/WebProcess/InjectedBundle/DOM/InjectedBundleNodeHandle.cpp:186
> +        paintingRect = renderer->absoluteBoundingBoxRectIgnoringTransforms();

Did you test this on a transformed box?
Comment 9 Tim Horton 2017-03-23 12:15:54 PDT 
Comment on attachment 305160 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305160&action=review

>> Source/WebKit2/WebProcess/InjectedBundle/DOM/InjectedBundleNodeHandle.cpp:186
>> +        paintingRect = renderer->absoluteBoundingBoxRectIgnoringTransforms();
> 
> Did you test this on a transformed box?

It surely doesn't work, but it's not a regression because paintingRootRect does the same thing.