Let CRIWVWebView inherit UIView instead of NSObject.

Let CRIWVWebView inherit UIView instead of NSObject.

With this change, I merged protocol CRIWVWebView and class
CRIWVWebViewImpl into a class CRIWVWebView. But the class still has two
headers (public/criwv_web_view.h and
internal/criwv_web_view_internal.h) to define public and internal
methods.

BUG=687827
Review-Url: https://codereview.chromium.org/2675633002
Cr-Commit-Position: refs/heads/master@{#447957}
Committed: https://chromium.googlesource.com/chromium/src/+/cfcff2790fed123279ba60ddcd265a3a26e38a60

[Hiroshi Ichikawa, 2017-02-02 07:39:59]

Description was changed from

==========
Let CRIWVWebView inherit UIView instead of NSObject.

BUG=
==========

to

==========
Let CRIWVWebView inherit UIView instead of NSObject.

With this change, I merged protocol CRIWVWebView and class CRIWVWebViewImpl into
a class CRIWVWebView. But the class still has two headers
(public/criwv_web_view.h and internal/criwv_web_view_internal.h) to define
public and internal methods.
==========

[Hiroshi Ichikawa, 2017-02-02 07:40:00]

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

[Hiroshi Ichikawa, 2017-02-02 07:41:14]

Description was changed from

==========
Let CRIWVWebView inherit UIView instead of NSObject.

With this change, I merged protocol CRIWVWebView and class CRIWVWebViewImpl into
a class CRIWVWebView. But the class still has two headers
(public/criwv_web_view.h and internal/criwv_web_view_internal.h) to define
public and internal methods.
==========

to

==========
Let CRIWVWebView inherit UIView instead of NSObject.

https://bugs.chromium.org/p/chromium/issues/detail?id=687827

With this change, I merged protocol CRIWVWebView and class CRIWVWebViewImpl into
a class CRIWVWebView. But the class still has two headers
(public/criwv_web_view.h and internal/criwv_web_view_internal.h) to define
public and internal methods.
==========

[Hiroshi Ichikawa, 2017-02-02 08:15:47]

Description was changed from

==========
Let CRIWVWebView inherit UIView instead of NSObject.

https://bugs.chromium.org/p/chromium/issues/detail?id=687827

With this change, I merged protocol CRIWVWebView and class CRIWVWebViewImpl into
a class CRIWVWebView. But the class still has two headers
(public/criwv_web_view.h and internal/criwv_web_view_internal.h) to define
public and internal methods.
==========

to

==========
Let CRIWVWebView inherit UIView instead of NSObject.

With this change, I merged protocol CRIWVWebView and class CRIWVWebViewImpl into
a class CRIWVWebView. But the class still has two headers
(public/criwv_web_view.h and internal/criwv_web_view_internal.h) to define
public and internal methods.

BUG=687827
==========

[michaeldo, 2017-02-02 19:39:18]

LGTM

Looks great, thank you! Please fix one comment before landing.

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

https://codereview.chromium.org/2675633002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/criwv_web_view.mm:43: NSAssert(NO, @"Should not be
called");
Please use NOTREACHED() instead of NSAssert for both of these initializers.

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[michaeldo, 2017-02-02 19:40:08]

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

[michaeldo, 2017-02-02 19:40:08]

+ eugenebut for committers approval

[Eugene But (OOO till 7-30), 2017-02-02 20:54:32]

Could you please reformat your CL description to fit 72 symbols. lgtm

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

https://codereview.chromium.org/2675633002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/criwv_web_view.mm:43: NSAssert(NO, @"Should not be
called");
On 2017/02/02 19:39:18, michaeldo wrote:
> Please use NOTREACHED() instead of NSAssert for both of these initializers.
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
Do we even need this implementation? No one can call this method because it is
NS_UNAVAILABLE

Same for initWithCoder:

https://codereview.chromium.org/2675633002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/criwv_web_view.mm:76: subview.frame = self.frame;
Do you need to check if |subview| is already a subview of self?

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

https://codereview.chromium.org/2675633002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/criwv_web_view_internal.h:16: -
(instancetype)initWithFrame:(CGRect)frame
Could you please document that |browserState| can not be null in the comments.

https://codereview.chromium.org/2675633002/diff/20001/ios/web_view/public/cri...
File ios/web_view/public/criwv_web_view.h (right):

https://codereview.chromium.org/2675633002/diff/20001/ios/web_view/public/cri...
ios/web_view/public/criwv_web_view.h:11: // Primary objective-c interface for
web/.  Just like a WKWebView, but better.
Do you want to update this comment? This is not an interface for web/ anymore.
Also "better" part is very subjective :). I think the comments should explain
that CRIWVWebView is responsible for rendering web content and in addition to
WKWebView allows Translate, Find In Page and Customizable Context Menus, <maybe
something else?>

[Hiroshi Ichikawa, 2017-02-03 01:37:44]

Description was changed from

==========
Let CRIWVWebView inherit UIView instead of NSObject.

With this change, I merged protocol CRIWVWebView and class CRIWVWebViewImpl into
a class CRIWVWebView. But the class still has two headers
(public/criwv_web_view.h and internal/criwv_web_view_internal.h) to define
public and internal methods.

BUG=687827
==========

to

==========
Let CRIWVWebView inherit UIView instead of NSObject.

With this change, I merged protocol CRIWVWebView and class
CRIWVWebViewImpl into a class CRIWVWebView. But the class still has two
headers (public/criwv_web_view.h and
internal/criwv_web_view_internal.h) to define public and internal
methods.

BUG=687827
==========

[Hiroshi Ichikawa, 2017-02-03 02:14:58]

> Could you please reformat your CL description to fit 72 symbols. lgtm

Do you mean to fit each line in 72 characters? Done.

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

https://codereview.chromium.org/2675633002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/criwv_web_view.mm:43: NSAssert(NO, @"Should not be
called");
On 2017/02/02 20:54:32, Eugene But wrote:
> On 2017/02/02 19:39:18, michaeldo wrote:
> > Please use NOTREACHED() instead of NSAssert for both of these initializers.
> > 
> >
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> Do we even need this implementation? No one can call this method because it is
> NS_UNAVAILABLE
> 
> Same for initWithCoder:

Good point, I found that I don't need this implementation. Removed.

https://codereview.chromium.org/2675633002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/criwv_web_view.mm:76: subview.frame = self.frame;
On 2017/02/02 20:54:32, Eugene But wrote:
> Do you need to check if |subview| is already a subview of self?

Done.

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

https://codereview.chromium.org/2675633002/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/criwv_web_view_internal.h:16: -
(instancetype)initWithFrame:(CGRect)frame
On 2017/02/02 20:54:32, Eugene But wrote:
> Could you please document that |browserState| can not be null in the comments.

Done.

https://codereview.chromium.org/2675633002/diff/20001/ios/web_view/public/cri...
File ios/web_view/public/criwv_web_view.h (right):

https://codereview.chromium.org/2675633002/diff/20001/ios/web_view/public/cri...
ios/web_view/public/criwv_web_view.h:11: // Primary objective-c interface for
web/.  Just like a WKWebView, but better.
On 2017/02/02 20:54:32, Eugene But wrote:
> Do you want to update this comment? This is not an interface for web/ anymore.
> Also "better" part is very subjective :). I think the comments should explain
> that CRIWVWebView is responsible for rendering web content and in addition to
> WKWebView allows Translate, Find In Page and Customizable Context Menus,
<maybe
> something else?>

Done.

[Eugene But (OOO till 7-30), 2017-02-03 02:21:22]

>> Do you mean to fit each line in 72 characters? Done.
Yes :) Thank you!

[Hiroshi Ichikawa, 2017-02-03 02:51:02]

The CQ bit was checked by ichikawa@chromium.org

[Hiroshi Ichikawa, 2017-02-03 02:51:02]

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

[commit-bot: I haz the power, 2017-02-03 02:51:59]

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-02-03 04:12:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 04:12:42]

Failed to apply patch for ios/web_view/public/criwv.h:
While running git apply --index -p1;
  error: patch failed: ios/web_view/public/criwv.h:5
  error: ios/web_view/public/criwv.h: patch does not apply

Patch:       ios/web_view/public/criwv.h
Index: ios/web_view/public/criwv.h
diff --git a/ios/web_view/public/criwv.h b/ios/web_view/public/criwv.h
index
a858af24f72cdf585475360a30be072cd3af2c2d..722f4d9a459621b1611c5265ea92620195301ad5
100644
--- a/ios/web_view/public/criwv.h
+++ b/ios/web_view/public/criwv.h
@@ -5,10 +5,11 @@
 #ifndef IOS_WEB_VIEW_PUBLIC_CRIWV_H_
 #define IOS_WEB_VIEW_PUBLIC_CRIWV_H_
 
+#import <CoreGraphics/CoreGraphics.h>
 #import <Foundation/Foundation.h>
 
 @protocol CRIWVDelegate;
-@protocol CRIWVWebView;
+@class CRIWVWebView;
 
 // Main interface for the CRIWV library.
 @interface CRIWV : NSObject
@@ -22,7 +23,7 @@
 + (void)shutDown;
 
 // Creates and returns a web view.
-+ (id<CRIWVWebView>)webView;
++ (CRIWVWebView*)webViewWithFrame:(CGRect)frame;
 
 @end

[Hiroshi Ichikawa, 2017-02-03 06:25:21]

The CQ bit was checked by ichikawa@chromium.org

[Hiroshi Ichikawa, 2017-02-03 06:25:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeldo@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2675633002/#ps80001
(title: "Fix format.")

[commit-bot: I haz the power, 2017-02-03 06:25:37]

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-02-03 07:34:38]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486103121075710,
"parent_rev": "1e271e8741b49d2096d15ea07dac63ec7e685cea", "commit_rev":
"cfcff2790fed123279ba60ddcd265a3a26e38a60"}

[commit-bot: I haz the power, 2017-02-03 07:35:31]

Description was changed from

==========
Let CRIWVWebView inherit UIView instead of NSObject.

With this change, I merged protocol CRIWVWebView and class
CRIWVWebViewImpl into a class CRIWVWebView. But the class still has two
headers (public/criwv_web_view.h and
internal/criwv_web_view_internal.h) to define public and internal
methods.

BUG=687827
==========

to

==========
Let CRIWVWebView inherit UIView instead of NSObject.

With this change, I merged protocol CRIWVWebView and class
CRIWVWebViewImpl into a class CRIWVWebView. But the class still has two
headers (public/criwv_web_view.h and
internal/criwv_web_view_internal.h) to define public and internal
methods.

BUG=687827

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

[commit-bot: I haz the power, 2017-02-03 07:35:32]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/cfcff2790fed123279ba60ddcd26...