[Textfield] Consider select all status for select all context menu

[Textfield] Consider select all status for select all context menu

Disable select all context menu when text is all selected, too.

BUG=670855
R=pkasting@chromium.org
TEST=views_unittests --gtest_filter=TextfieldTest.ContextMenuDisplayTest

Review-Url: https://codereview.chromium.org/2829823002
Cr-Commit-Position: refs/heads/master@{#466132}
Committed: https://chromium.googlesource.com/chromium/src/+/51afc830d2cb4e3c15efb393ab8c8bb689159d20

[simonhong, 2017-04-19 20:40:04]

PTAL.

[simonhong, 2017-04-19 20:44:04]

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

[commit-bot: I haz the power, 2017-04-19 20:44:48]

Dry run: 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-04-19 21:09:03]

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

[commit-bot: I haz the power, 2017-04-19 21:09:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[simonhong, 2017-04-19 21:25:56]

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

[commit-bot: I haz the power, 2017-04-19 21:26:27]

Dry run: 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-04-19 22:20:29]

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

[commit-bot: I haz the power, 2017-04-19 22:20:29]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-04-20 00:08:31]

https://codereview.chromium.org/2829823002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2829823002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:357: return !text().empty() &&
GetSelectedText().length() == text().length();
It seems like it would be more efficient to see if GetSelectedRange() covers the
whole text range.  That wouldn't result in copying the text string just to check
its length.

https://codereview.chromium.org/2829823002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/2829823002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:108: // Return false when text is empty.
Nit: "Returns whether all the text is currently selected.  Returns false if
there is no text."

https://codereview.chromium.org/2829823002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:109: bool IsAllSelected() const;
Nit: Does this need to be public?  If we're only using it internally, why not
make it private?  If it's only being called in one place, why not just inline
the implementation there?  Or are you planning on using this in other places?

https://codereview.chromium.org/2829823002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/2829823002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:962:
textfield_->SetText(base::string16());
Maybe after this we should SelectAll(), before checking IsAllSelected().

[simonhong, 2017-04-20 13:11:51]

PTAL again.

https://codereview.chromium.org/2829823002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2829823002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:357: return !text().empty() &&
GetSelectedText().length() == text().length();
On 2017/04/20 00:08:30, Peter Kasting (OOO until 4-24) wrote:
> It seems like it would be more efficient to see if GetSelectedRange() covers
the
> whole text range.  That wouldn't result in copying the text string just to
check
> its length.

Done.

https://codereview.chromium.org/2829823002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/2829823002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:108: // Return false when text is empty.
On 2017/04/20 00:08:30, Peter Kasting (OOO until 4-24) wrote:
> Nit: "Returns whether all the text is currently selected.  Returns false if
> there is no text."

Removed.

https://codereview.chromium.org/2829823002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.h:109: bool IsAllSelected() const;
On 2017/04/20 00:08:30, Peter Kasting (OOO until 4-24) wrote:
> Nit: Does this need to be public?  If we're only using it internally, why not
> make it private?  If it's only being called in one place, why not just inline
> the implementation there?  Or are you planning on using this in other places?

Making it public is mistake.
At first, I added as a method for test purpose.
However, I convinced that we can test without this method.
Inlined.

https://codereview.chromium.org/2829823002/diff/20001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/2829823002/diff/20001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:962:
textfield_->SetText(base::string16());
On 2017/04/20 00:08:30, Peter Kasting (OOO until 4-24) wrote:
> Maybe after this we should SelectAll(), before checking IsAllSelected().

We don't need to that because when different text is set, previous selection is
cleared.
Anyway, this test case was removed because Textfield::IsAllSelected() doesn't
exist anymore.

[simonhong, 2017-04-20 13:21:22]

Description was changed from

==========
[Textfield] Consider select all status for select all context menu

Disable select all context menu when text is all selected.
To do this, Textfield::IsAllSelected() is added.

BUG=670855
R=pkasting@chromium.org
TEST=views_unittests --gtest_filter=TextfieldTest.IsAllSelected
==========

to

==========
[Textfield] Consider select all status for select all context menu

Disable select all context menu when text is all selected, too.

BUG=670855
R=pkasting@chromium.org
TEST=views_unittests --gtest_filter=TextfieldTest.ContextMenuDisplayTest
==========

[Peter Kasting, 2017-04-20 17:11:36]

LGTM

[simonhong, 2017-04-20 19:50:32]

The CQ bit was checked by simonhong@chromium.org

[commit-bot: I haz the power, 2017-04-20 19:51:16]

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-04-20 21:10:58]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492717832562600,
"parent_rev": "dbf935c161d8a9e8da1a1f4bfa9c407967dd7db3", "commit_rev":
"51afc830d2cb4e3c15efb393ab8c8bb689159d20"}

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

Description was changed from

==========
[Textfield] Consider select all status for select all context menu

Disable select all context menu when text is all selected, too.

BUG=670855
R=pkasting@chromium.org
TEST=views_unittests --gtest_filter=TextfieldTest.ContextMenuDisplayTest
==========

to

==========
[Textfield] Consider select all status for select all context menu

Disable select all context menu when text is all selected, too.

BUG=670855
R=pkasting@chromium.org
TEST=views_unittests --gtest_filter=TextfieldTest.ContextMenuDisplayTest

Review-Url: https://codereview.chromium.org/2829823002
Cr-Commit-Position: refs/heads/master@{#466132}
Committed:
https://chromium.googlesource.com/chromium/src/+/51afc830d2cb4e3c15efb393ab8c...
==========

[commit-bot: I haz the power, 2017-04-20 21:11:06]

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