Bug 130273

Summary: Implement contentInset for Mac WebKit2
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: WebKit APIAssignee: Beth Dakin <bdakin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bdakin, benjamin, commit-queue, esprehn+autocc, glenn, jonlee, kondapallykalyan, sam, simon.fraser, thorton
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
simon.fraser: review-
Patch
none
Patch
none
Patch simon.fraser: review+

Description Beth Dakin 2014-03-14 16:46:25 PDT 
Implement contentInset for Mac WebKit2

<rdar://problem/16175195>
Comment 1 Beth Dakin 2014-03-20 12:31:41 PDT 
Created attachment 227319 [details]
Patch
Comment 2 WebKit Commit Bot 2014-03-20 12:33:08 PDT 
Attachment 227319 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/Internals.h:257:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ScrollView.cpp:275:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ScrollView.cpp:689:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ScrollView.cpp:691:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ScrollView.cpp:1093:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ScrollView.cpp:1095:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Simon Fraser (smfr) 2014-03-20 13:35:13 PDT 
Comment on attachment 227319 [details]
Patch

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

> Source/WebCore/page/Page.h:282
> +    void setTopContentInset(float contentInset) { m_topContentInset = contentInset; }

Doesn't this need to cause layout or other rejiggery?

> Source/WebCore/platform/ScrollView.cpp:280
> +        return visibleCOntentSize;

visibleCOntentSize

> Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:84
> +@property (readwrite) CGFloat topContentInset;

Should be on WKWebView.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:3303
> +- (void)setTopContentInset:(CGFloat)contentInset
> +{
> +    _data->_page->setTopContentInset(contentInset);
> +}
> +
> +- (CGFloat)topContentInset
> +{
> +    return _data->_page->topContentInset();
> +}

We're not using WKView any more
Comment 4 Beth Dakin 2014-03-20 14:46:32 PDT 
(In reply to comment #3)
> (From update of attachment 227319 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227319&action=review
> 
> > Source/WebCore/page/Page.h:282
> > +    void setTopContentInset(float contentInset) { m_topContentInset = contentInset; }
> 
> Doesn't this need to cause layout or other rejiggery?
> 

Yes, fixing.

> > Source/WebCore/platform/ScrollView.cpp:280
> > +        return visibleCOntentSize;
> 
> visibleCOntentSize
> 

Oops!

> > Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:84
> > +@property (readwrite) CGFloat topContentInset;
> 
> Should be on WKWebView.
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:3303
> > +- (void)setTopContentInset:(CGFloat)contentInset
> > +{
> > +    _data->_page->setTopContentInset(contentInset);
> > +}
> > +
> > +- (CGFloat)topContentInset
> > +{
> > +    return _data->_page->topContentInset();
> > +}
> 
> We're not using WKView any more

My new patch adds it to both.
Comment 5 Beth Dakin 2014-03-20 14:46:53 PDT 
Created attachment 227337 [details]
Patch
Comment 6 WebKit Commit Bot 2014-03-20 14:49:13 PDT 
Attachment 227337 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Page.h:282:  The parameter name "contentInset" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:97:  Missing spaces around =  [whitespace/operators] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:84:  Missing spaces around =  [whitespace/operators] [4]
ERROR: Source/WebCore/platform/ScrollView.cpp:689:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ScrollView.cpp:691:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ScrollView.cpp:1093:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ScrollView.cpp:1095:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Beth Dakin 2014-03-20 15:02:11 PDT 
Created attachment 227340 [details]
Patch

Stylebot is just wrong about the WK2 setter things, but here is a patch that will hopefully placate the other complaints.
Comment 8 WebKit Commit Bot 2014-03-20 15:04:10 PDT 
Attachment 227340 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:97:  Missing spaces around =  [whitespace/operators] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:84:  Missing spaces around =  [whitespace/operators] [4]
ERROR: Source/WebCore/platform/ScrollView.cpp:689:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/ScrollView.cpp:1093:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 4 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Beth Dakin 2014-03-20 15:27:26 PDT 
Created attachment 227346 [details]
Patch

Once again for stylebot.
Comment 10 WebKit Commit Bot 2014-03-20 15:29:26 PDT 
Attachment 227346 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:97:  Missing spaces around =  [whitespace/operators] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:84:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 2 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Beth Dakin 2014-03-20 16:20:01 PDT 
Thanks, Simon! 

http://trac.webkit.org/changeset/166017
Comment 12 Alexey Proskuryakov 2014-03-20 17:38:45 PDT 
Comment on attachment 227346 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:1831
> +void Internals::setTopContentInset(float contentInset)

Is there something resetting the inset between tests?
Comment 13 Simon Fraser (smfr) 2014-03-20 17:51:55 PDT 
This broke tests because
ScrollView::visibleContentRectInternal(...IncludeScollbars) calls unobscuredContentRect() which excludes them.
Comment 14 Simon Fraser (smfr) 2014-03-20 18:17:26 PDT 
Fixed in http://trac.webkit.org/changeset/166032