Add a inner text parameter to ContextMenuParams

Add a inner text parameter to ContextMenuParams

This CL adds a parameter to ContextMenuParams, the inner text of a link on
which a context menu is opened.

BUG=591365
Committed: https://crrev.com/698c843e73f50ed1448a7e3da456989eee9207b0
Cr-Commit-Position: refs/heads/master@{#423814}

[gambard, 2016-09-28 16:36:29]

gambard@chromium.org changed reviewers:
+ jyquinn@chromium.org, marq@chromium.org

[gambard, 2016-09-28 16:36:30]

PTAL.

[Jackie Quinn, 2016-09-28 18:24:42]

lgtm. I assume the plan is to display the link's text in addition to URL on
context menus?

[gambard, 2016-10-03 07:42:47]

On 2016/09/28 18:24:42, Jackie Quinn wrote:
> lgtm. I assume the plan is to display the link's text in addition to URL on
> context menus?

Thanks. For now it is for the "add to reading list" in the context menu, so we
can put something as title of the reading list entry.

[marq (ping after 24h), 2016-10-03 11:31:09]

https://codereview.chromium.org/2379653002/diff/1/ios/web/web_state/ui/crw_we...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2379653002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:4293: NSString* innerText =
element[@"innerText"];
If the JS innerText value is an empty string, what is the behavior here? Is
|innerText| null or an empty NSString? And what is the expected behavior of code
that uses the params struct?

(Ideally document in the params struct header whether the string can be empty or
not).

[gambard, 2016-10-03 12:05:52]

Thanks, PTAL.

https://codereview.chromium.org/2379653002/diff/1/ios/web/web_state/ui/crw_we...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2379653002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:4293: NSString* innerText =
element[@"innerText"];
On 2016/10/03 11:31:09, marq wrote:
> If the JS innerText value is an empty string, what is the behavior here? Is
> |innerText| null or an empty NSString? And what is the expected behavior of
code
> that uses the params struct?
> 
> (Ideally document in the params struct header whether the string can be empty
or
> not). 

The string can be empty or potentially nil (if the HTML element is not a <a/>).
Done.

[marq (ping after 24h), 2016-10-03 15:52:52]

On 2016/10/03 12:05:52, gambard wrote:
> Thanks, PTAL.
> 
>
https://codereview.chromium.org/2379653002/diff/1/ios/web/web_state/ui/crw_we...
> File ios/web/web_state/ui/crw_web_controller.mm (right):
> 
>
https://codereview.chromium.org/2379653002/diff/1/ios/web/web_state/ui/crw_we...
> ios/web/web_state/ui/crw_web_controller.mm:4293: NSString* innerText =
> element[@"innerText"];
> On 2016/10/03 11:31:09, marq wrote:
> > If the JS innerText value is an empty string, what is the behavior here? Is
> > |innerText| null or an empty NSString? And what is the expected behavior of
> code
> > that uses the params struct?
> > 
> > (Ideally document in the params struct header whether the string can be
empty
> or
> > not). 
> 
> The string can be empty or potentially nil (if the HTML element is not a
<a/>).
> Done.

OK, so if it's empty, calling code has to check two things before using the
string (nonnil and length), since you don't want an empty title for a reading
list item.

Why not establish in the API that the string is either nil or nonempty to
simplify usage for calling code? That means you're centralizing the check for
length > 0 into one place, and nobody will have to remember to do it again.

[gambard, 2016-10-05 07:35:59]

Thanks, PTAL.
I thought the check [string length] > 0 would have checked for nil + empty.

[gambard, 2016-10-06 13:25:05]

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

[gambard, 2016-10-06 13:25:05]

+eugenebut@ as marq@ is OOO.
PTAL

[gambard, 2016-10-06 14:55:12]

Description was changed from

==========
Add a inner text parameter to ContextMenuParams

This CL adds a parameter to ContextMenuParams, the inner text of a link on
which a context menu is opened.

BUG=none
==========

to

==========
Add a inner text parameter to ContextMenuParams

This CL adds a parameter to ContextMenuParams, the inner text of a link on
which a context menu is opened.

BUG=591365
==========

[marq (ping after 24h), 2016-10-06 17:02:10]

lgtm

[Eugene But (OOO till 7-30), 2016-10-06 22:49:45]

On 2016/10/06 17:02:10, marq wrote:
> lgtm
Please take a look at content::ContextMenuParams:
https://cs.chromium.org/chromium/src/content/public/common/context_menu_param...

How web's inner text is different from content's link_text? Can you use the name
from content::ContentMenuParams?

ios/web should be as closest to content as possible in terms of API.

[gambard, 2016-10-07 07:23:50]

Thanks, I have renamed the variable. We are different from the content API
because we return nil instead of an empty string.

[gambard, 2016-10-07 07:24:25]

The CQ bit was checked by gambard@chromium.org

[gambard, 2016-10-07 07:24:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyquinn@chromium.org, marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2379653002/#ps60001
(title: "Rename inner_text to link_text")

[commit-bot: I haz the power, 2016-10-07 07:24:40]

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

[Eugene But (OOO till 7-30), 2016-10-07 07:27:17]

Thank you! Returning nil makes total sense because the value is backed up by
NSString. 

lgtm

[commit-bot: I haz the power, 2016-10-07 07:40:28]

Description was changed from

==========
Add a inner text parameter to ContextMenuParams

This CL adds a parameter to ContextMenuParams, the inner text of a link on
which a context menu is opened.

BUG=591365
==========

to

==========
Add a inner text parameter to ContextMenuParams

This CL adds a parameter to ContextMenuParams, the inner text of a link on
which a context menu is opened.

BUG=591365
==========

[commit-bot: I haz the power, 2016-10-07 07:40:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-07 07:42:11]

Description was changed from

==========
Add a inner text parameter to ContextMenuParams

This CL adds a parameter to ContextMenuParams, the inner text of a link on
which a context menu is opened.

BUG=591365
==========

to

==========
Add a inner text parameter to ContextMenuParams

This CL adds a parameter to ContextMenuParams, the inner text of a link on
which a context menu is opened.

BUG=591365

Committed: https://crrev.com/698c843e73f50ed1448a7e3da456989eee9207b0
Cr-Commit-Position: refs/heads/master@{#423814}
==========

[commit-bot: I haz the power, 2016-10-07 07:42:12]

Patchset 4 (id:??) landed as
https://crrev.com/698c843e73f50ed1448a7e3da456989eee9207b0
Cr-Commit-Position: refs/heads/master@{#423814}