Bug 142892 - Web Inspector: Support attaching to another view other than the WKView
Summary: Web Inspector: Support attaching to another view other than the WKView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-03-19 19:22 PDT by Timothy Hatcher
Modified: 2015-03-23 19:42 PDT (History)
10 users (show)

See Also:


Attachments
Patch (14.24 KB, patch)
2015-03-19 19:27 PDT, Timothy Hatcher
thorton: review+
thorton: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2015-03-19 19:22:52 PDT
To be more flexible with the host application, we should support attaching to a view other than the WKView.
Comment 1 Radar WebKit Bug Importer 2015-03-19 19:23:05 PDT
<rdar://problem/20234342>
Comment 2 Timothy Hatcher 2015-03-19 19:27:26 PDT
Created attachment 249078 [details]
Patch
Comment 3 Joseph Pecoraro 2015-03-20 15:48:57 PDT
Comment on attachment 249078 [details]
Patch

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

Looks good to me. Needs a WK2 Owner's approval.

> Source/WebKit2/ChangeLog:19
> +        Use platformCanAttach as a final check incase there is a different

Typo: "incase" => "in case"

> Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:102
> +@property (retain, nonatomic, setter=_setInspectorAttachmentView:) NSView *_inspectorAttachmentView;

Interesting that one of the above new properties has WK_AVAILABLE. That said, I don't really think those are needed, especially in *Private.h, so what you have looks fine to me.

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:597
> +    unsigned maximumAttachedHeight = NSHeight(inspectedViewFrame) * maximumAttachedHeightRatio;

Should this be "float" instead of unsigned? You're dealing with two floats on the right.
Comment 4 Timothy Hatcher 2015-03-22 19:28:49 PDT
Sam, Anders, or Tim, please review.
Comment 5 Timothy Hatcher 2015-03-22 19:29:47 PDT
Comment on attachment 249078 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:102
>> +@property (retain, nonatomic, setter=_setInspectorAttachmentView:) NSView *_inspectorAttachmentView;
> 
> Interesting that one of the above new properties has WK_AVAILABLE. That said, I don't really think those are needed, especially in *Private.h, so what you have looks fine to me.

I don't see the point for private API. I can add it if needed.

>> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:597
>> +    unsigned maximumAttachedHeight = NSHeight(inspectedViewFrame) * maximumAttachedHeightRatio;
> 
> Should this be "float" instead of unsigned? You're dealing with two floats on the right.

Yeah, I'll fix that up.
Comment 6 Tim Horton 2015-03-23 16:14:55 PDT
Comment on attachment 249078 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:41
>  #import "WKViewInternal.h"
> +#import "WKViewPrivate.h"

I thought WKViewInternal imported WKViewPrivate? (it does)
Comment 7 Anders Carlsson 2015-03-23 16:37:25 PDT
Comment on attachment 249078 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:102
>>> +@property (retain, nonatomic, setter=_setInspectorAttachmentView:) NSView *_inspectorAttachmentView;
>> 
>> Interesting that one of the above new properties has WK_AVAILABLE. That said, I don't really think those are needed, especially in *Private.h, so what you have looks fine to me.
> 
> I don't see the point for private API. I can add it if needed.

Please add them, and use strong instead of retain.
Comment 8 Timothy Hatcher 2015-03-23 16:43:57 PDT
https://trac.webkit.org/r181881
Comment 9 Joseph Pecoraro 2015-03-23 19:28:18 PDT
32-bit build fixes:
https://trac.webkit.org/changeset/181884
https://trac.webkit.org/changeset/181885
Comment 10 Timothy Hatcher 2015-03-23 19:42:14 PDT
Thanks Joe. Sorry about that.