Implement CWVWebView.scrollView.

Implement CWVWebView.scrollView.

BUG=714543
Review-Url: https://codereview.chromium.org/2842953002
Cr-Commit-Position: refs/heads/master@{#469925}
Committed: https://chromium.googlesource.com/chromium/src/+/ec1c2d94aaa5bc86e24dac95d990286a706df439

[Hiroshi Ichikawa, 2017-04-26 06:29:50]

ichikawa@chromium.org changed reviewers:
+ michaeldo@chromium.org

[Eugene But (OOO till 7-30), 2017-04-26 19:11:30]

eugenebut@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-04-26 19:11:31]

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_scroll_view.mm (right):

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv_scroll_view.mm:20: if (_proxy) {
There is no need for this check, calling a method on nil is fine

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv_scroll_view.mm:25: [_proxy addObserver:self];
ditto

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv_scroll_view.mm:78: if ([_delegate
respondsToSelector:@selector
Do you want to create a local variable for selector to improve the formatting?

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv_scroll_view.mm:97:
respondsToSelector:@selector(webViewScrollViewDidEndDecelerating:)]) {
ditto

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_scroll_view_internal.h (right):

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv_scroll_view_internal.h:17: @property(nonatomic, weak,
readwrite) CRWWebViewScrollViewProxy* proxy;
Please add a comment

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_scroll_view.h (right):

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/public/cwv...
ios/web_view/public/cwv_scroll_view.h:17: // we don't want to expose the raw
scroll view.
Please avoid using "we" in the comments (go/avoidwe)

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/public/cwv...
ios/web_view/public/cwv_scroll_view.h:37: //
https://chromereviews.googleplex.com/323287014/
Please link rdar://23584409 instead of internal google CL

[Hiroshi Ichikawa, 2017-04-27 02:30:11]

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_scroll_view.mm (right):

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv_scroll_view.mm:20: if (_proxy) {
On 2017/04/26 19:11:31, Eugene But wrote:
> There is no need for this check, calling a method on nil is fine

Done.

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv_scroll_view.mm:25: [_proxy addObserver:self];
On 2017/04/26 19:11:31, Eugene But wrote:
> ditto

Done.

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv_scroll_view.mm:78: if ([_delegate
respondsToSelector:@selector
On 2017/04/26 19:11:31, Eugene But wrote:
> Do you want to create a local variable for selector to improve the formatting?

Done.

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv_scroll_view.mm:97:
respondsToSelector:@selector(webViewScrollViewDidEndDecelerating:)]) {
On 2017/04/26 19:11:31, Eugene But wrote:
> ditto

Done.

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/internal/c...
File ios/web_view/internal/cwv_scroll_view_internal.h (right):

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv_scroll_view_internal.h:17: @property(nonatomic, weak,
readwrite) CRWWebViewScrollViewProxy* proxy;
On 2017/04/26 19:11:31, Eugene But wrote:
> Please add a comment

Done.

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_scroll_view.h (right):

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/public/cwv...
ios/web_view/public/cwv_scroll_view.h:17: // we don't want to expose the raw
scroll view.
On 2017/04/26 19:11:31, Eugene But wrote:
> Please avoid using "we" in the comments (go/avoidwe)

Done.

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/public/cwv...
ios/web_view/public/cwv_scroll_view.h:37: //
https://chromereviews.googleplex.com/323287014/
On 2017/04/26 19:11:31, Eugene But wrote:
> Please link rdar://23584409 instead of internal google CL

Looks like the issue is not visible at Open Radar?
https://openradar.appspot.com/23584409
In that case, I believe the code review URL is more useful because people cannot
get more information from the rdar URL. What do you think? I put both of the
code review URL and the rdar URL for now.

[Hiroshi Ichikawa, 2017-04-27 07:24:15]

FYI I made a change to make contentSize property KVO compliant.

[Eugene But (OOO till 7-30), 2017-04-27 12:25:24]

lgtm, but please do not use link to internal Google services (we trying to avoid
using those in Chromium)

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_scroll_view.h (right):

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/public/cwv...
ios/web_view/public/cwv_scroll_view.h:37: //
https://chromereviews.googleplex.com/323287014/
On 2017/04/27 02:30:11, Hiroshi Ichikawa wrote:
> On 2017/04/26 19:11:31, Eugene But wrote:
> > Please link rdar://23584409 instead of internal google CL
> 
> Looks like the issue is not visible at Open Radar?
> https://openradar.appspot.com/23584409
> In that case, I believe the code review URL is more useful because people
cannot
> get more information from the rdar URL. What do you think? I put both of the
> code review URL and the rdar URL for now.
In Chromium we do not use links to internal google services. rdar://23584409 is
also internal, and if you want to use something public, then we can use a WebKit
bug for this (I think one already exist, but I could not find it). Link to
crbug.com/, WebKit bugs, or radars are common and you can use any of those.

[Hiroshi Ichikawa, 2017-04-28 01:30:47]

michaeldo@ Can you also review this?

Or maybe should I submit this without michaeldo@'s review because I got
eugenebut@'s LGTM?

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_scroll_view.h (right):

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/public/cwv...
ios/web_view/public/cwv_scroll_view.h:37: //
https://chromereviews.googleplex.com/323287014/
On 2017/04/27 12:25:23, Eugene But wrote:
> On 2017/04/27 02:30:11, Hiroshi Ichikawa wrote:
> > On 2017/04/26 19:11:31, Eugene But wrote:
> > > Please link rdar://23584409 instead of internal google CL
> > 
> > Looks like the issue is not visible at Open Radar?
> > https://openradar.appspot.com/23584409
> > In that case, I believe the code review URL is more useful because people
> cannot
> > get more information from the rdar URL. What do you think? I put both of the
> > code review URL and the rdar URL for now.
> In Chromium we do not use links to internal google services. rdar://23584409
is
> also internal, and if you want to use something public, then we can use a
WebKit
> bug for this (I think one already exist, but I could not find it). Link to
> crbug.com/, WebKit bugs, or radars are common and you can use any of those.

Oh I assumed that all Chromium code review URLs are public, and didn't notice
that the URL is not public. Sorry for that.

This is probably a WebKit bug for this issue, right?:
https://bugs.webkit.org/show_bug.cgi?id=134230

But its description is too short. I put both of the WebKit bug URL and Rdar URL,
but also added more words to the comment so that it is hopefully
self-explanatory.

Just curious, am I correct that content of Rdar URLs are visible to only the
reporter and Apple employees, unless the reporter copies its content to Open
Radar?

[Eugene But (OOO till 7-30), 2017-04-28 02:44:20]

Thanks Hiroshi!

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_scroll_view.h (right):

https://codereview.chromium.org/2842953002/diff/20001/ios/web_view/public/cwv...
ios/web_view/public/cwv_scroll_view.h:37: //
https://chromereviews.googleplex.com/323287014/
On 2017/04/28 01:30:47, Hiroshi Ichikawa wrote:
> On 2017/04/27 12:25:23, Eugene But wrote:
> > On 2017/04/27 02:30:11, Hiroshi Ichikawa wrote:
> > > On 2017/04/26 19:11:31, Eugene But wrote:
> > > > Please link rdar://23584409 instead of internal google CL
> > > 
> > > Looks like the issue is not visible at Open Radar?
> > > https://openradar.appspot.com/23584409
> > > In that case, I believe the code review URL is more useful because people
> > cannot
> > > get more information from the rdar URL. What do you think? I put both of
the
> > > code review URL and the rdar URL for now.
> > In Chromium we do not use links to internal google services. rdar://23584409
> is
> > also internal, and if you want to use something public, then we can use a
> WebKit
> > bug for this (I think one already exist, but I could not find it). Link to
> > crbug.com/, WebKit bugs, or radars are common and you can use any of those.
> 
> Oh I assumed that all Chromium code review URLs are public, and didn't notice
> that the URL is not public. Sorry for that.
> 
> This is probably a WebKit bug for this issue, right?:
> https://bugs.webkit.org/show_bug.cgi?id=134230
Yes
 
> Just curious, am I correct that content of Rdar URLs are visible to only the
> reporter and Apple employees, unless the reporter copies its content to Open
> Radar?
Kind of, but not exactly :) see go/applebugs

[michaeldo, 2017-05-01 18:49:53]

Sorry for the delay. Please take a look at a few comments.

Thank you for this implementation! LGTM

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/internal/...
File ios/web_view/internal/cwv_scroll_view.mm (right):

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/internal/...
ios/web_view/internal/cwv_scroll_view.mm:28: @synthesize contentSize =
_contentSize;
nit: Please sort alphabetically.

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/internal/...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/internal/...
ios/web_view/internal/cwv_web_view.mm:121: _scrollView = [[CWVScrollView alloc]
init];
This readonly property should be redefined above as writeable which I couldn't
find unless I'm missing it somewhere.

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/public/cw...
File ios/web_view/public/cwv_scroll_view.h (right):

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/public/cw...
ios/web_view/public/cwv_scroll_view.h:20: // See the document of UIScrollView
for the document of each
Please re-wrap comments.

Please replace "See the document of UIScrollView for the document of each
method." with

"These methods are forwarded to the internal UIScrollView. Please see the
<UIKit/UIScrollView.h> documentation for details about the following methods."

With this explanation, you can probably remove "Has subset of methods of
UIScrollView." from earlier in the comment.

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/public/cw...
ios/web_view/public/cwv_scroll_view.h:23: // TODO: Add nullability annotations.
Please add a bug number and update TODO formatting.

// TODO(crbug.com/######): Something that needs doing.

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/public/cw...
ios/web_view/public/cwv_scroll_view.h:27: @property(nonatomic, readonly) CGRect
frame;
Can you confirm that |frame| needs to be exposed?

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/public/cw...
ios/web_view/public/cwv_scroll_view.h:32: -
(void)addGestureRecognizer:(UIGestureRecognizer*)gestureRecognizer;
Please move method after all properties.

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/public/cw...
File ios/web_view/public/cwv_scroll_view_delegate.h (right):

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/public/cw...
ios/web_view/public/cwv_scroll_view_delegate.h:13: // UIScrollViewDelegate. See
the document of
To match above suggestion, I recommend:

These methods are forwarded from the internal UIScrollViewDelegate. Please see
the <UIKit/UIScrollViewDelegate.h> documentation for details about the following
methods.

[Hiroshi Ichikawa, 2017-05-08 06:52:41]

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/internal/...
File ios/web_view/internal/cwv_scroll_view.mm (right):

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/internal/...
ios/web_view/internal/cwv_scroll_view.mm:28: @synthesize contentSize =
_contentSize;
On 2017/05/01 18:49:53, michaeldo wrote:
> nit: Please sort alphabetically.

Done.

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/internal/...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/internal/...
ios/web_view/internal/cwv_web_view.mm:121: _scrollView = [[CWVScrollView alloc]
init];
On 2017/05/01 18:49:53, michaeldo wrote:
> This readonly property should be redefined above as writeable which I couldn't
> find unless I'm missing it somewhere.

Why? Here I directly assign to the underlying instance variable |_scrollView|,
not |self.scrollView|, so it doesn't require the property to be readwrite. And
it's assigned only in the initializer here.

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/public/cw...
File ios/web_view/public/cwv_scroll_view.h (right):

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/public/cw...
ios/web_view/public/cwv_scroll_view.h:20: // See the document of UIScrollView
for the document of each
On 2017/05/01 18:49:53, michaeldo wrote:
> Please re-wrap comments.
> 
> Please replace "See the document of UIScrollView for the document of each
> method." with
> 
> "These methods are forwarded to the internal UIScrollView. Please see the
> <UIKit/UIScrollView.h> documentation for details about the following methods."
> 
> With this explanation, you can probably remove "Has subset of methods of
> UIScrollView." from earlier in the comment.

Done.

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/public/cw...
ios/web_view/public/cwv_scroll_view.h:23: // TODO: Add nullability annotations.
On 2017/05/01 18:49:53, michaeldo wrote:
> Please add a bug number and update TODO formatting.
> 
> // TODO(crbug.com/######): Something that needs doing.

Done.

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/public/cw...
ios/web_view/public/cwv_scroll_view.h:27: @property(nonatomic, readonly) CGRect
frame;
On 2017/05/01 18:49:53, michaeldo wrote:
> Can you confirm that |frame| needs to be exposed?

Hm it may not be necessary. Let me remove it for now. I may add back later if I
find it is needed.

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/public/cw...
ios/web_view/public/cwv_scroll_view.h:32: -
(void)addGestureRecognizer:(UIGestureRecognizer*)gestureRecognizer;
On 2017/05/01 18:49:53, michaeldo wrote:
> Please move method after all properties.

Done.

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/public/cw...
File ios/web_view/public/cwv_scroll_view_delegate.h (right):

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/public/cw...
ios/web_view/public/cwv_scroll_view_delegate.h:13: // UIScrollViewDelegate. See
the document of
On 2017/05/01 18:49:53, michaeldo wrote:
> To match above suggestion, I recommend:
> 
> These methods are forwarded from the internal UIScrollViewDelegate. Please see
> the <UIKit/UIScrollViewDelegate.h> documentation for details about the
following
> methods.

Done.

[Hiroshi Ichikawa, 2017-05-08 06:52:54]

The CQ bit was checked by ichikawa@chromium.org

[Hiroshi Ichikawa, 2017-05-08 06:52:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, michaeldo@chromium.org
Link to the patchset: https://codereview.chromium.org/2842953002/#ps140001
(title: "Apply review comments.")

[commit-bot: I haz the power, 2017-05-08 06:53:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 08:42:01]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494226374180040,
"parent_rev": "32ad6568b0726bf88a1cc0a2cd0e9eaff84d8292", "commit_rev":
"ec1c2d94aaa5bc86e24dac95d990286a706df439"}

[commit-bot: I haz the power, 2017-05-08 08:42:12]

Description was changed from

==========
Implement CWVWebView.scrollView.

BUG=714543
==========

to

==========
Implement CWVWebView.scrollView.

BUG=714543

Review-Url: https://codereview.chromium.org/2842953002
Cr-Commit-Position: refs/heads/master@{#469925}
Committed:
https://chromium.googlesource.com/chromium/src/+/ec1c2d94aaa5bc86e24dac95d990...
==========

[commit-bot: I haz the power, 2017-05-08 08:42:13]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/ec1c2d94aaa5bc86e24dac95d990...

[michaeldo, 2017-05-08 15:18:18]

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/internal/...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2842953002/diff/120001/ios/web_view/internal/...
ios/web_view/internal/cwv_web_view.mm:121: _scrollView = [[CWVScrollView alloc]
init];
On 2017/05/08 06:52:41, Hiroshi Ichikawa wrote:
> On 2017/05/01 18:49:53, michaeldo wrote:
> > This readonly property should be redefined above as writeable which I
couldn't
> > find unless I'm missing it somewhere.
> 
> Why? Here I directly assign to the underlying instance variable |_scrollView|,
> not |self.scrollView|, so it doesn't require the property to be readwrite. And
> it's assigned only in the initializer here.

This is a good point. You're right that we don't need to redefine it. Sorry, I
was under the impression we always do so, but I don't see a reason to change it
here.