MacViews: Use bullets for displaying obscured text.

MacViews: Use bullets for displaying obscured text.

BUG=657562
TEST=Enable chrome://flags/#mac-views-webui-dialogs. Go to an http auth dialog.
Verify bullets are used for displaying obscured text on Mac.

Committed: https://crrev.com/d7ca7598545ba71a7d7297675b889d7d96b54a0d
Cr-Commit-Position: refs/heads/master@{#427540}

[karandeepb, 2016-10-20 09:08:27]

Description was changed from

==========
MacViews: Use bullets for displaying obscuring text.
==========

to

==========
MacViews: Use bullets for displaying obscured text.

BUG=657562
TEST=Enable chrome://flags/#mac-views-webui-dialogs. Go to an http auth dialog. 
Verify bullets are used for displaying obscured text on Mac.
==========

[karandeepb, 2016-10-20 09:08:45]

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

[commit-bot: I haz the power, 2016-10-20 09:08:51]

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, 2016-10-20 10:00:34]

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

[commit-bot: I haz the power, 2016-10-20 10:00:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[karandeepb, 2016-10-21 05:52:13]

karandeepb@chromium.org changed reviewers:
+ sky@chromium.org

[karandeepb, 2016-10-21 05:52:13]

PTAL sky@.

[sky, 2016-10-21 15:06:41]

https://codereview.chromium.org/2439693002/diff/20001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2439693002/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:127: #if defined(OS_MACOSX)
Move kPasswordReplacementChar to header so you aren't duplicating the define.

https://codereview.chromium.org/2439693002/diff/20001/ui/views/controls/label...
File ui/views/controls/label_unittest.cc (right):

https://codereview.chromium.org/2439693002/diff/20001/ui/views/controls/label...
ui/views/controls/label_unittest.cc:31: const base::char16 kObscuredCharacter =
0x2022;
Use the define in rendertext.

[karandeepb, 2016-10-24 06:39:28]

Patchset #3 (id:40001) has been deleted

[karandeepb, 2016-10-24 06:41:59]

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

[commit-bot: I haz the power, 2016-10-24 06:42:13]

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, 2016-10-24 07:45:35]

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

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

Dry run: This issue passed the CQ dry run.

[karandeepb, 2016-10-24 12:09:28]

Patchset #3 (id:60001) has been deleted

[karandeepb, 2016-10-24 12:09:54]

PTAL sky@.

https://codereview.chromium.org/2439693002/diff/20001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2439693002/diff/20001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:127: #if defined(OS_MACOSX)
On 2016/10/21 15:06:41, sky wrote:
> Move kPasswordReplacementChar to header so you aren't duplicating the define.

Done.

https://codereview.chromium.org/2439693002/diff/20001/ui/views/controls/label...
File ui/views/controls/label_unittest.cc (right):

https://codereview.chromium.org/2439693002/diff/20001/ui/views/controls/label...
ui/views/controls/label_unittest.cc:31: const base::char16 kObscuredCharacter =
0x2022;
On 2016/10/21 15:06:41, sky wrote:
> Use the define in rendertext.

Done.

[sky, 2016-10-24 16:13:25]

LGTM

https://codereview.chromium.org/2439693002/diff/80001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/2439693002/diff/80001/ui/gfx/render_text.h#ne...
ui/gfx/render_text.h:215: static const base::char16 kPasswordReplacementChar;
Style const says constants before destructor. Also, use constexpr.

https://codereview.chromium.org/2439693002/diff/80001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2439693002/diff/80001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:289: int reveal_index,
If length is going to be size_t, than make real_index size_t as well.

[karandeepb, 2016-10-25 02:08:27]

Patchset #5 (id:120001) has been deleted

[karandeepb, 2016-10-25 02:14:01]

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

[commit-bot: I haz the power, 2016-10-25 02:14:31]

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

[karandeepb, 2016-10-25 03:31:03]

https://codereview.chromium.org/2439693002/diff/80001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/2439693002/diff/80001/ui/gfx/render_text.h#ne...
ui/gfx/render_text.h:215: static const base::char16 kPasswordReplacementChar;
On 2016/10/24 16:13:25, sky wrote:
> Style const says constants before destructor. Also, use constexpr.

Done. I initially preferred using a static const since static constexpr requires
initializing the variable along with the declaration.

https://codereview.chromium.org/2439693002/diff/80001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2439693002/diff/80001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:289: int reveal_index,
On 2016/10/24 16:13:25, sky wrote:
> If length is going to be size_t, than make real_index size_t as well.

Done.

[karandeepb, 2016-10-25 03:55:18]

PTAL sky@ in case you have any more comments related to the constexpr changes.

[commit-bot: I haz the power, 2016-10-25 03:56:17]

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

[commit-bot: I haz the power, 2016-10-25 03:56:18]

Dry run: This issue passed the CQ dry run.

[sky, 2016-10-25 16:40:16]

SLGTM - but I'm surprised you still need the definition.

https://codereview.chromium.org/2439693002/diff/140001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/2439693002/diff/140001/ui/gfx/render_text.cc#...
ui/gfx/render_text.cc:425: constexpr base::char16
RenderText::kPasswordReplacementChar;
Are you sure this is still needed?

[tapted, 2016-10-25 23:09:38]

tapted@chromium.org changed reviewers:
+ tapted@chromium.org

[tapted, 2016-10-25 23:09:39]

https://codereview.chromium.org/2439693002/diff/140001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/2439693002/diff/140001/ui/gfx/render_text.cc#...
ui/gfx/render_text.cc:425: constexpr base::char16
RenderText::kPasswordReplacementChar;
On 2016/10/25 16:40:16, sky wrote:
> Are you sure this is still needed?

drive-by: It's needed if the address is ever taken, and this includes being
passed as a const-ref argument at any point. For example the line

std::vector<base::char16> arr(length, RenderText::kPasswordReplacementChar);

will pass kPasswordReplacementChar into a std::vector constructor that takes a
`const value_type& value` argument.

[karandeepb, 2016-10-26 00:02:45]

https://codereview.chromium.org/2439693002/diff/140001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/2439693002/diff/140001/ui/gfx/render_text.cc#...
ui/gfx/render_text.cc:425: constexpr base::char16
RenderText::kPasswordReplacementChar;
On 2016/10/25 23:09:39, tapted wrote:
> On 2016/10/25 16:40:16, sky wrote:
> > Are you sure this is still needed?
> 
> drive-by: It's needed if the address is ever taken, and this includes being
> passed as a const-ref argument at any point. For example the line
> 
> std::vector<base::char16> arr(length, RenderText::kPasswordReplacementChar);
> 
> will pass kPasswordReplacementChar into a std::vector constructor that takes a
> `const value_type& value` argument.

Yeah the standard calls these usages "odr-used".

[karandeepb, 2016-10-26 00:03:05]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2016-10-26 00:03:05]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2439693002/#ps140001
(title: "Make kPasswordReplacementChar constexpr.")

[commit-bot: I haz the power, 2016-10-26 00:03:34]

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, 2016-10-26 00:10:00]

Description was changed from

==========
MacViews: Use bullets for displaying obscured text.

BUG=657562
TEST=Enable chrome://flags/#mac-views-webui-dialogs. Go to an http auth dialog. 
Verify bullets are used for displaying obscured text on Mac.
==========

to

==========
MacViews: Use bullets for displaying obscured text.

BUG=657562
TEST=Enable chrome://flags/#mac-views-webui-dialogs. Go to an http auth dialog. 
Verify bullets are used for displaying obscured text on Mac.
==========

[commit-bot: I haz the power, 2016-10-26 00:10:01]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-10-26 00:12:26]

Description was changed from

==========
MacViews: Use bullets for displaying obscured text.

BUG=657562
TEST=Enable chrome://flags/#mac-views-webui-dialogs. Go to an http auth dialog. 
Verify bullets are used for displaying obscured text on Mac.
==========

to

==========
MacViews: Use bullets for displaying obscured text.

BUG=657562
TEST=Enable chrome://flags/#mac-views-webui-dialogs. Go to an http auth dialog.
Verify bullets are used for displaying obscured text on Mac.

Committed: https://crrev.com/d7ca7598545ba71a7d7297675b889d7d96b54a0d
Cr-Commit-Position: refs/heads/master@{#427540}
==========

[commit-bot: I haz the power, 2016-10-26 00:12:29]

Patchset 5 (id:??) landed as
https://crrev.com/d7ca7598545ba71a7d7297675b889d7d96b54a0d
Cr-Commit-Position: refs/heads/master@{#427540}