Add CWVUIDelegate with method to customize context menu.

Add CWVUIDelegate with method to customize context menu.

BUG=696581
Review-Url: https://codereview.chromium.org/2723433004
Cr-Commit-Position: refs/heads/master@{#453387}
Committed: https://chromium.googlesource.com/chromium/src/+/b4ab2c32985eb0b6af0556c2376a8598e2b0c134

[michaeldo, 2017-02-27 15:51:31]

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

[Eugene But (OOO till 7-30), 2017-02-27 16:34:07]

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/internal/cwv_h...
File ios/web_view/internal/cwv_html_element.mm (right):

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/internal/cwv_h...
ios/web_view/internal/cwv_html_element.mm:23: _text = text;
[text copy]

https://google.github.io/styleguide/objcguide.xml#Setters_copy_NSStrings

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/internal/cwv_w...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view.mm:194: if ([_UIDelegate
respondsToSelector:@selector
Optional nit: do you want to reverse the condition and early return NO if
delegate does not respond to selector?

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view.mm:195:
(webView:runContextMenuWithTitle:forHTMLElement:inView
Optional nit: Do you want to create a local variable for selector to improve the
formatting?

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view.mm:199: CWVHTMLElement* htmlElement =
s/htmlElement/HTMLElement

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/public/cwv_htm...
File ios/web_view/public/cwv_html_element.h (right):

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/public/cwv_htm...
ios/web_view/public/cwv_html_element.h:16: - (nonnull
instancetype)initWithHyperlink:(nullable NSURL*)hyperlink
Should this be a part of public API? Do WebView need to create this object?

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/public/cwv_ui_...
File ios/web_view/public/cwv_ui_delegate.h (right):

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/public/cwv_ui_...
ios/web_view/public/cwv_ui_delegate.h:8: #import <CoreGraphics/CGGeometry.h>
<CoreGraphics/CoreGraphics.h>

https://google.github.io/styleguide/objcguide.xml?showone=Use_Root_Frameworks...

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/public/cwv_ui_...
ios/web_view/public/cwv_ui_delegate.h:12: @class NSString;
Optional nit: How about importing UIKit instead of predeclaring SDK classes?

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/public/cwv_web...
File ios/web_view/public/cwv_web_view.h (right):

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/public/cwv_web...
ios/web_view/public/cwv_web_view.h:29: @property(nonatomic, weak)
id<CWVUIDelegate> UIDelegate;
Feedback for a separate CL: we should probably use nullability annotations for
all WebView headers.

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/shell/shell_vi...
File ios/web_view/shell/shell_view_controller.h (right):

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/shell/shell_vi...
ios/web_view/shell/shell_view_controller.h:16: : UIViewController<CWVUIDelegate,
CWVWebViewDelegate, UITextFieldDelegate>
Do you want to move protocols conformance to the implementation file?

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/shell/shell_vi...
File ios/web_view/shell/shell_view_controller.m (right):

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/shell/shell_vi...
ios/web_view/shell/shell_view_controller.m:7: #import
<MobileCoreServices/UTCoreTypes.h>
<MobileCoreServices/MobileCoreServices.h>

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/shell/shell_vi...
ios/web_view/shell/shell_view_controller.m:198: void (^handler)(UIAlertAction*)
= ^(UIAlertAction* action) {
nit: s/handler/copyHandler ?

[michaeldo, 2017-02-27 19:02:06]

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/internal/cwv_h...
File ios/web_view/internal/cwv_html_element.mm (right):

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/internal/cwv_h...
ios/web_view/internal/cwv_html_element.mm:23: _text = text;
On 2017/02/27 16:34:06, Eugene But wrote:
> [text copy]
> 
> https://google.github.io/styleguide/objcguide.xml#Setters_copy_NSStrings

Done.

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/internal/cwv_w...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view.mm:194: if ([_UIDelegate
respondsToSelector:@selector
On 2017/02/27 16:34:06, Eugene But wrote:
> Optional nit: do you want to reverse the condition and early return NO if
> delegate does not respond to selector?

I agree, I like that better.

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view.mm:195:
(webView:runContextMenuWithTitle:forHTMLElement:inView
On 2017/02/27 16:34:06, Eugene But wrote:
> Optional nit: Do you want to create a local variable for selector to improve
the
> formatting?

Done.

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/internal/cwv_w...
ios/web_view/internal/cwv_web_view.mm:199: CWVHTMLElement* htmlElement =
On 2017/02/27 16:34:06, Eugene But wrote:
> s/htmlElement/HTMLElement

Done.

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/public/cwv_htm...
File ios/web_view/public/cwv_html_element.h (right):

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/public/cwv_htm...
ios/web_view/public/cwv_html_element.h:16: - (nonnull
instancetype)initWithHyperlink:(nullable NSURL*)hyperlink
On 2017/02/27 16:34:06, Eugene But wrote:
> Should this be a part of public API? Do WebView need to create this object?

No. I agree, this should be internal api. Done

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/public/cwv_ui_...
File ios/web_view/public/cwv_ui_delegate.h (right):

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/public/cwv_ui_...
ios/web_view/public/cwv_ui_delegate.h:8: #import <CoreGraphics/CGGeometry.h>
On 2017/02/27 16:34:06, Eugene But wrote:
> <CoreGraphics/CoreGraphics.h>
> 
>
https://google.github.io/styleguide/objcguide.xml?showone=Use_Root_Frameworks...

Done.

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/public/cwv_ui_...
ios/web_view/public/cwv_ui_delegate.h:12: @class NSString;
On 2017/02/27 16:34:06, Eugene But wrote:
> Optional nit: How about importing UIKit instead of predeclaring SDK classes?

Done.

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/public/cwv_web...
File ios/web_view/public/cwv_web_view.h (right):

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/public/cwv_web...
ios/web_view/public/cwv_web_view.h:29: @property(nonatomic, weak)
id<CWVUIDelegate> UIDelegate;
On 2017/02/27 16:34:07, Eugene But wrote:
> Feedback for a separate CL: we should probably use nullability annotations for
> all WebView headers.

Agreed, I filed crbug.com/696661

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/shell/shell_vi...
File ios/web_view/shell/shell_view_controller.h (right):

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/shell/shell_vi...
ios/web_view/shell/shell_view_controller.h:16: : UIViewController<CWVUIDelegate,
CWVWebViewDelegate, UITextFieldDelegate>
On 2017/02/27 16:34:07, Eugene But wrote:
> Do you want to move protocols conformance to the implementation file?

Done.

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/shell/shell_vi...
File ios/web_view/shell/shell_view_controller.m (right):

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/shell/shell_vi...
ios/web_view/shell/shell_view_controller.m:7: #import
<MobileCoreServices/UTCoreTypes.h>
On 2017/02/27 16:34:07, Eugene But wrote:
> <MobileCoreServices/MobileCoreServices.h>

Done.

https://codereview.chromium.org/2723433004/diff/1/ios/web_view/shell/shell_vi...
ios/web_view/shell/shell_view_controller.m:198: void (^handler)(UIAlertAction*)
= ^(UIAlertAction* action) {
On 2017/02/27 16:34:07, Eugene But wrote:
> nit: s/handler/copyHandler ?

Done.

[Eugene But (OOO till 7-30), 2017-02-27 21:10:46]

lgtm

https://codereview.chromium.org/2723433004/diff/20001/ios/web_view/shell/shel...
File ios/web_view/shell/shell_view_controller.m (right):

https://codereview.chromium.org/2723433004/diff/20001/ios/web_view/shell/shel...
ios/web_view/shell/shell_view_controller.m:188: if (element.hyperlink == nil) {
Sorry, missed during the previous round.
nit: |if (!element.hyperlink)|

[michaeldo, 2017-02-27 21:40:04]

https://codereview.chromium.org/2723433004/diff/20001/ios/web_view/shell/shel...
File ios/web_view/shell/shell_view_controller.m (right):

https://codereview.chromium.org/2723433004/diff/20001/ios/web_view/shell/shel...
ios/web_view/shell/shell_view_controller.m:188: if (element.hyperlink == nil) {
On 2017/02/27 21:10:46, Eugene But wrote:
> Sorry, missed during the previous round.
> nit: |if (!element.hyperlink)|

No problem. Thanks for catching it. Done.

[michaeldo, 2017-02-27 21:44:13]

The CQ bit was checked by michaeldo@chromium.org

[michaeldo, 2017-02-27 21:44:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2723433004/#ps40001
(title: "Respond to comment.")

[commit-bot: I haz the power, 2017-02-27 21:44:52]

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-27 23:26:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488231853822740,
"parent_rev": "da5e27fa70e22d8d54301b9dd93587730afac64f", "commit_rev":
"b4ab2c32985eb0b6af0556c2376a8598e2b0c134"}

[commit-bot: I haz the power, 2017-02-27 23:27:47]

Description was changed from

==========
Add CWVUIDelegate with method to customize context menu.

BUG=696581
==========

to

==========
Add CWVUIDelegate with method to customize context menu.

BUG=696581

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

[commit-bot: I haz the power, 2017-02-27 23:27:48]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b4ab2c32985eb0b6af0556c2376a...