Bug 130273 - Implement contentInset for Mac WebKit2
Summary: Implement contentInset for Mac WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-03-14 16:46 PDT by Beth Dakin
Modified: 2014-03-20 18:17 PDT (History)
11 users (show)

See Also:


Attachments
Patch (33.51 KB, patch)
2014-03-20 12:31 PDT, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (36.14 KB, patch)
2014-03-20 14:46 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (36.24 KB, patch)
2014-03-20 15:02 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (37.20 KB, patch)
2014-03-20 15:27 PDT, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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