Remove "paste as plain text" from selection menu

Remove "paste as plain text" from selection menu

"paste as plain text" introduced an empty option in selection menu due
to the runtime string resource setting. Since we need to update
|canEditRichly| flag for the option in selection menu, there is no
simple way to do so right now, so simply remove this option.
Need to add it back later.

BUG=718330
Review-Url: https://codereview.chromium.org/2858333002
Cr-Commit-Position: refs/heads/master@{#469483}
Committed: https://chromium.googlesource.com/chromium/src/+/a038875de60252fb6bf6fb429e5feab5b1d0c3de

[Shimi Zhang, 2017-05-04 18:20:54]

The CQ bit was checked by ctzsm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 18:21:35]

Dry run: CQ is trying da patch.

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

[Shimi Zhang, 2017-05-04 18:23:38]

ctzsm@chromium.org changed reviewers:
+ aelias@chromium.org, amaralp@chromium.org

[Shimi Zhang, 2017-05-04 18:23:39]

PTAL, taking care of SelectionPopup menu for paste as plain text, thanks!

[aelias_OOO_until_Jul13, 2017-05-04 18:35:45]

lgtm

[amaralp, 2017-05-04 18:48:18]

https://codereview.chromium.org/2858333002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
(right):

https://codereview.chromium.org/2858333002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:499:
if (!canPasteAsPlainText()) {
Doesn't this rely on having a fresh value for |mCanEditRichlyForPastePopup|?
Since |mCanEditRichlyForPastePopup| is only set for the paste popup I think you
will be using a stale value. I have a CL (crrev.com/2785853002) that should fix
this problem. I'd recommend waiting for that CL to land.

[commit-bot: I haz the power, 2017-05-04 19:04:32]

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

[commit-bot: I haz the power, 2017-05-04 19:04:33]

Dry run: This issue passed the CQ dry run.

[Shimi Zhang, 2017-05-04 19:10:55]

On 2017/05/04 18:48:18, amaralp wrote:
>
https://codereview.chromium.org/2858333002/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
> (right):
> 
>
https://codereview.chromium.org/2858333002/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:499:
> if (!canPasteAsPlainText()) {
> Doesn't this rely on having a fresh value for |mCanEditRichlyForPastePopup|?
> Since |mCanEditRichlyForPastePopup| is only set for the paste popup I think
you
> will be using a stale value. I have a CL (crrev.com/2785853002) that should
fix
> this problem. I'd recommend waiting for that CL to land.

|mCanEditRichlyForPastePopup| is an attribute of text area, it won't stale. But
you are right, I didn't get this attribute for selection popup, so when we
select some text before any paste popups, this option won't show in the menu.

This CL is actually fixing the bug that an empty option is showing in selection
popup for blew O, and the feature itself only exist in O and above, so I think
it is OK to land this one first, then wait for your CL so I can fix the
|mCanEditRichlyForPastePopup| issue. WDYT?

[Tima Vaisburd, 2017-05-04 19:16:41]

On 2017/05/04 19:10:55, Shimi Zhang wrote:
> On 2017/05/04 18:48:18, amaralp wrote:
> >
>
https://codereview.chromium.org/2858333002/diff/1/content/public/android/java...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2858333002/diff/1/content/public/android/java...
> >
>
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:499:
> > if (!canPasteAsPlainText()) {
> > Doesn't this rely on having a fresh value for |mCanEditRichlyForPastePopup|?
> > Since |mCanEditRichlyForPastePopup| is only set for the paste popup I think
> you
> > will be using a stale value. I have a CL (crrev.com/2785853002) that should
> fix
> > this problem. I'd recommend waiting for that CL to land.
> 
> |mCanEditRichlyForPastePopup| is an attribute of text area, it won't stale.
But
> you are right, I didn't get this attribute for selection popup, so when we
> select some text before any paste popups, this option won't show in the menu.
> 
> This CL is actually fixing the bug that an empty option is showing in
selection
> popup for blew O, and the feature itself only exist in O and above, so I think
> it is OK to land this one first, then wait for your CL so I can fix the
> |mCanEditRichlyForPastePopup| issue. WDYT?

Please expand the CL description.

[amaralp, 2017-05-04 19:18:39]

On 2017/05/04 at 19:10:55, ctzsm wrote:
> On 2017/05/04 18:48:18, amaralp wrote:
> >
https://codereview.chromium.org/2858333002/diff/1/content/public/android/java...
> > File
> >
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
> > (right):
> > 
> >
https://codereview.chromium.org/2858333002/diff/1/content/public/android/java...
> >
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:499:
> > if (!canPasteAsPlainText()) {
> > Doesn't this rely on having a fresh value for |mCanEditRichlyForPastePopup|?
> > Since |mCanEditRichlyForPastePopup| is only set for the paste popup I think
you
> > will be using a stale value. I have a CL (crrev.com/2785853002) that should
fix
> > this problem. I'd recommend waiting for that CL to land.
> 
> |mCanEditRichlyForPastePopup| is an attribute of text area, it won't stale.
But you are right, I didn't get this attribute for selection popup, so when we
select some text before any paste popups, this option won't show in the menu.
> 
What if there are multiple text areas? For example if you long-press in a text
area that allows for rich editing and then select some text in a different text
area that doesn't allow rich editing. Couldn't that lead to showing the option
when you shouldn't?

> This CL is actually fixing the bug that an empty option is showing in
selection popup for blew O, and the feature itself only exist in O and above, so
I think it is OK to land this one first, then wait for your CL so I can fix the
|mCanEditRichlyForPastePopup| issue. WDYT?
Why not always remove the option for the selection menu?

[Shimi Zhang, 2017-05-04 19:21:59]

Description was changed from

==========
Add "paste as plain text" to SelectionPopup menu

BUG=718330
==========

to

==========
Add "paste as plain text" to SelectionPopup menu

a) Remove blank option in selection popup for below O,
b) Set MenuItem title for "paste as plain text" for O and above.

BUG=718330
==========

[Shimi Zhang, 2017-05-04 19:29:03]

On 2017/05/04 19:18:39, amaralp wrote:
> On 2017/05/04 at 19:10:55, ctzsm wrote:
> > On 2017/05/04 18:48:18, amaralp wrote:
> > >
>
https://codereview.chromium.org/2858333002/diff/1/content/public/android/java...
> > > File
> > >
>
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
> > > (right):
> > > 
> > >
>
https://codereview.chromium.org/2858333002/diff/1/content/public/android/java...
> > >
>
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:499:
> > > if (!canPasteAsPlainText()) {
> > > Doesn't this rely on having a fresh value for
|mCanEditRichlyForPastePopup|?
> > > Since |mCanEditRichlyForPastePopup| is only set for the paste popup I
think
> you
> > > will be using a stale value. I have a CL (crrev.com/2785853002) that
should
> fix
> > > this problem. I'd recommend waiting for that CL to land.
> > 
> > |mCanEditRichlyForPastePopup| is an attribute of text area, it won't stale.
> But you are right, I didn't get this attribute for selection popup, so when we
> select some text before any paste popups, this option won't show in the menu.
> > 
> What if there are multiple text areas? For example if you long-press in a text
> area that allows for rich editing and then select some text in a different
text
> area that doesn't allow rich editing. Couldn't that lead to showing the option
> when you shouldn't?
> 
> > This CL is actually fixing the bug that an empty option is showing in
> selection popup for blew O, and the feature itself only exist in O and above,
so
> I think it is OK to land this one first, then wait for your CL so I can fix
the
> |mCanEditRichlyForPastePopup| issue. WDYT?
> Why not always remove the option for the selection menu?

That could also work. Either way I need to take care of selection menu after
your CL landed, but I intend to keep this one simple.

[amaralp, 2017-05-04 20:11:18]

On 2017/05/04 at 19:29:03, ctzsm wrote:
> On 2017/05/04 19:18:39, amaralp wrote:
> > On 2017/05/04 at 19:10:55, ctzsm wrote:
> > > On 2017/05/04 18:48:18, amaralp wrote:
> > > >
> >
https://codereview.chromium.org/2858333002/diff/1/content/public/android/java...
> > > > File
> > > >
> >
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
> > > > (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/2858333002/diff/1/content/public/android/java...
> > > >
> >
content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:499:
> > > > if (!canPasteAsPlainText()) {
> > > > Doesn't this rely on having a fresh value for
|mCanEditRichlyForPastePopup|?
> > > > Since |mCanEditRichlyForPastePopup| is only set for the paste popup I
think
> > you
> > > > will be using a stale value. I have a CL (crrev.com/2785853002) that
should
> > fix
> > > > this problem. I'd recommend waiting for that CL to land.
> > > 
> > > |mCanEditRichlyForPastePopup| is an attribute of text area, it won't
stale.
> > But you are right, I didn't get this attribute for selection popup, so when
we
> > select some text before any paste popups, this option won't show in the
menu.
> > > 
> > What if there are multiple text areas? For example if you long-press in a
text
> > area that allows for rich editing and then select some text in a different
text
> > area that doesn't allow rich editing. Couldn't that lead to showing the
option
> > when you shouldn't?
> > 
> > > This CL is actually fixing the bug that an empty option is showing in
> > selection popup for blew O, and the feature itself only exist in O and
above, so
> > I think it is OK to land this one first, then wait for your CL so I can fix
the
> > |mCanEditRichlyForPastePopup| issue. WDYT?
> > Why not always remove the option for the selection menu?
> 
> That could also work. Either way I need to take care of selection menu after
your CL landed, but I intend to keep this one simple.

Let's remove the option for now and add it back after my CL lands.

[Shimi Zhang, 2017-05-04 20:49:52]

> Let's remove the option for now and add it back after my CL lands.

Updated, PTAL, thanks! :)

[Shimi Zhang, 2017-05-04 20:49:57]

The CQ bit was checked by ctzsm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 20:50:20]

Dry run: CQ is trying da patch.

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

[amaralp, 2017-05-04 20:52:13]

On 2017/05/04 at 20:49:52, ctzsm wrote:
> > Let's remove the option for now and add it back after my CL lands.
> 
> Updated, PTAL, thanks! :)

thanks! lgtm

[Shimi Zhang, 2017-05-04 21:00:05]

Description was changed from

==========
Add "paste as plain text" to SelectionPopup menu

a) Remove blank option in selection popup for below O,
b) Set MenuItem title for "paste as plain text" for O and above.

BUG=718330
==========

to

==========
Remove "paste as plain text" from selection menu

"paste as plain text" introduced an empty option in selection menu due to the
runtime string resource setting. Since we need to update |canEditRichly| flag
for the option in selection menu, there is no simple way to do so right now, so
simply remove this option. Need to add it back later.

BUG=718330
==========

[Shimi Zhang, 2017-05-04 21:00:46]

Description was changed from

==========
Remove "paste as plain text" from selection menu

"paste as plain text" introduced an empty option in selection menu due to the
runtime string resource setting. Since we need to update |canEditRichly| flag
for the option in selection menu, there is no simple way to do so right now, so
simply remove this option. Need to add it back later.

BUG=718330
==========

to

==========
Remove "paste as plain text" from selection menu

"paste as plain text" introduced an empty option in selection menu due
to the runtime string resource setting. Since we need to update |canEditRichly|
flag for the option in selection menu, there is no
simple way to do so right now, so simply remove this option.
Need to add it back later.

BUG=718330
==========

[Shimi Zhang, 2017-05-04 21:01:18]

Description was changed from

==========
Remove "paste as plain text" from selection menu

"paste as plain text" introduced an empty option in selection menu due
to the runtime string resource setting. Since we need to update |canEditRichly|
flag for the option in selection menu, there is no
simple way to do so right now, so simply remove this option.
Need to add it back later.

BUG=718330
==========

to

==========
Remove "paste as plain text" from selection menu

"paste as plain text" introduced an empty option in selection menu due
to the runtime string resource setting. Since we need to update
|canEditRichly| flag for the option in selection menu, there is no
simple way to do so right now, so simply remove this option.
Need to add it back later.

BUG=718330
==========

[commit-bot: I haz the power, 2017-05-04 21:40:13]

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

[commit-bot: I haz the power, 2017-05-04 21:40:14]

Dry run: This issue passed the CQ dry run.

[Shimi Zhang, 2017-05-04 21:40:43]

The CQ bit was checked by ctzsm@chromium.org

[Shimi Zhang, 2017-05-04 21:40:44]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2858333002/#ps20001
(title: "remove the option for now")

[commit-bot: I haz the power, 2017-05-04 21:41:22]

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-04 21:49:38]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493934043734640,
"parent_rev": "3ca7863c41560e94efdfdb21f0b1b4231e4a589e", "commit_rev":
"a038875de60252fb6bf6fb429e5feab5b1d0c3de"}

[commit-bot: I haz the power, 2017-05-04 21:49:51]

Description was changed from

==========
Remove "paste as plain text" from selection menu

"paste as plain text" introduced an empty option in selection menu due
to the runtime string resource setting. Since we need to update
|canEditRichly| flag for the option in selection menu, there is no
simple way to do so right now, so simply remove this option.
Need to add it back later.

BUG=718330
==========

to

==========
Remove "paste as plain text" from selection menu

"paste as plain text" introduced an empty option in selection menu due
to the runtime string resource setting. Since we need to update
|canEditRichly| flag for the option in selection menu, there is no
simple way to do so right now, so simply remove this option.
Need to add it back later.

BUG=718330

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

[commit-bot: I haz the power, 2017-05-04 21:49:52]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a038875de60252fb6bf6fb429e5f...