[ios showcase] Integrated TextBadgeView with Showcase.

[ios showcase] Integrated TextBadgeView with Showcase.

This CL adds TextBadgeView to iOS Showcase. This involves adding a
text_badge_view folder to showcase, creating a glue view controller,
and modifying the Showcase configuration file and BUILD.gn files as
necessary.

Since TextBadgeView is currently a stub implementation, Showcase
displays a blank white screen.

BUG=740133
.

Review-Url: https://codereview.chromium.org/2967113003
Cr-Commit-Position: refs/heads/master@{#485302}
Committed: https://chromium.googlesource.com/chromium/src/+/ac8a93564a2560a5ec54673246c551e04e11ed02

[helenlyang, 2017-07-05 21:45:49]

Description was changed from

==========
[ios showcase] Integrated TextBadgeView with Showcase.

This CL adds TextBadgeView to iOS Showcase. This involves adding a
text_badge_view folder to showcase, creating a glue view controller,
and modifying the Showcase configuration file and BUILD.gn files as
necessary.

Since TextBadgeView is currently a stub implementation, Showcase
displays a blank white screen.

BUG=none.
==========

to

==========
[ios showcase] Integrated TextBadgeView with Showcase.

This CL adds TextBadgeView to iOS Showcase. This involves adding a
text_badge_view folder to showcase, creating a glue view controller,
and modifying the Showcase configuration file and BUILD.gn files as
necessary.

Since TextBadgeView is currently a stub implementation, Showcase
displays a blank white screen.

BUG=none.
==========

[helenlyang, 2017-07-05 21:45:49]

helenlyang@google.com changed reviewers:
+ edchin@chromium.org, gchatz@chromium.org

[helenlyang, 2017-07-05 21:45:49]

helenlyang@google.com changed required reviewers:
+ edchin@chromium.org, gchatz@chromium.org

[edchin, 2017-07-06 04:38:17]

On 2017/07/05 21:45:49, helenlyang wrote:
> mailto:helenlyang@google.com changed required reviewers:
> + mailto:edchin@chromium.org, mailto:gchatz@chromium.org

Great start!
The setup looks correct to me. 
You'll need to actually use the text_badge_view in the view controller. 
Also, I'd like for you to add a single egtest that checks for the 
existence of an accessibility label. See this example:
https://cs.chromium.org/chromium/src/ios/showcase/toolbar/sc_toolbar_egtest.m...

[helenlyang, 2017-07-06 21:42:41]

Patchset #2 (id:20001) has been deleted

[helenlyang, 2017-07-06 21:43:44]

On 2017/07/06 04:38:17, edchin wrote:
> On 2017/07/05 21:45:49, helenlyang wrote:
> > mailto:helenlyang@google.com changed required reviewers:
> > + mailto:edchin@chromium.org, mailto:gchatz@chromium.org
> 
> Great start!
> The setup looks correct to me. 
> You'll need to actually use the text_badge_view in the view controller. 
> Also, I'd like for you to add a single egtest that checks for the 
> existence of an accessibility label. See this example:
>
https://cs.chromium.org/chromium/src/ios/showcase/toolbar/sc_toolbar_egtest.m...

Added the text_badge_view to view controller and created an egtest. PTAL.

Thank you!

[edchin, 2017-07-07 04:38:04]

lgtm. Please address the comments below.

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
File ios/showcase/text_badge_view/BUILD.gn (right):

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
ios/showcase/text_badge_view/BUILD.gn:3: "text_badge_view_view_controller.h",
I see why you reiterated the word 'view'. 
Please remove the duplicate. It looks better if you do so.

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
File ios/showcase/text_badge_view/text_badge_view_egtest.mm (right):

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
ios/showcase/text_badge_view/text_badge_view_egtest.mm:26:
Open(@"TextBadgeView");
Please just put the open and close directly 
into the test (since it's just a single test).

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
File ios/showcase/text_badge_view/text_badge_view_view_controller.mm (right):

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
ios/showcase/text_badge_view/text_badge_view_view_controller.mm:17:
self.view.backgroundColor = [UIColor whiteColor];
I suggest that you set this view to another color, such as grey. 
The reason is that the badge will initially not have any color 
or background.

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
ios/showcase/text_badge_view/text_badge_view_view_controller.mm:19: [self.view
addSubview:textBadge];
You'll need to set the frame of the badge
otherwise, it will default to CGRectZero.

[edchin, 2017-07-07 04:38:34]

Please also create a bug to track this work.

[gchatz, 2017-07-07 04:40:39]

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
File ios/showcase/text_badge_view/BUILD.gn (right):

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
ios/showcase/text_badge_view/BUILD.gn:3: "text_badge_view_view_controller.h",
New files should follow showcase convention where "sc_" is prepended.

[edchin, 2017-07-07 05:00:49]

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
File ios/showcase/text_badge_view/BUILD.gn (right):

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
ios/showcase/text_badge_view/BUILD.gn:3: "text_badge_view_view_controller.h",
On 2017/07/07 04:40:38, gchatz wrote:
> New files should follow showcase convention where "sc_" is prepended.

I agree with gchatz. But, I can see why you didn't follow the convention.
The only view controller in the example list does not use the prepend, while 
all coordinators do. I think it makes sense to use the convention.

[helenlyang, 2017-07-07 15:46:38]

Description was changed from

==========
[ios showcase] Integrated TextBadgeView with Showcase.

This CL adds TextBadgeView to iOS Showcase. This involves adding a
text_badge_view folder to showcase, creating a glue view controller,
and modifying the Showcase configuration file and BUILD.gn files as
necessary.

Since TextBadgeView is currently a stub implementation, Showcase
displays a blank white screen.

BUG=none.
==========

to

==========
[ios showcase] Integrated TextBadgeView with Showcase.

This CL adds TextBadgeView to iOS Showcase. This involves adding a
text_badge_view folder to showcase, creating a glue view controller,
and modifying the Showcase configuration file and BUILD.gn files as
necessary.

Since TextBadgeView is currently a stub implementation, Showcase
displays a blank white screen.

BUG=740133.
==========

[helenlyang, 2017-07-07 16:31:13]

Added a tracking bug and fixed comments. PTAL.

Thanks!

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
File ios/showcase/text_badge_view/BUILD.gn (right):

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
ios/showcase/text_badge_view/BUILD.gn:3: "text_badge_view_view_controller.h",
On 2017/07/07 04:38:03, edchin wrote:
> I see why you reiterated the word 'view'. 
> Please remove the duplicate. It looks better if you do so.

Done.

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
File ios/showcase/text_badge_view/text_badge_view_egtest.mm (right):

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
ios/showcase/text_badge_view/text_badge_view_egtest.mm:26:
Open(@"TextBadgeView");
On 2017/07/07 04:38:04, edchin wrote:
> Please just put the open and close directly 
> into the test (since it's just a single test).

Done.

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
File ios/showcase/text_badge_view/text_badge_view_view_controller.mm (right):

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
ios/showcase/text_badge_view/text_badge_view_view_controller.mm:17:
self.view.backgroundColor = [UIColor whiteColor];
On 2017/07/07 04:38:04, edchin wrote:
> I suggest that you set this view to another color, such as grey. 
> The reason is that the badge will initially not have any color 
> or background.

Done.

https://codereview.chromium.org/2967113003/diff/40001/ios/showcase/text_badge...
ios/showcase/text_badge_view/text_badge_view_view_controller.mm:19: [self.view
addSubview:textBadge];
On 2017/07/07 04:38:04, edchin wrote:
> You'll need to set the frame of the badge
> otherwise, it will default to CGRectZero.

Done.

[edchin, 2017-07-07 21:00:10]

https://codereview.chromium.org/2967113003/diff/60001/ios/showcase/core/showc...
File ios/showcase/core/showcase_model.mm (right):

https://codereview.chromium.org/2967113003/diff/60001/ios/showcase/core/showc...
ios/showcase/core/showcase_model.mm:96: showcase::kClassForInstantiationKey :
@"TextBadgeViewController",
Update this as well.

https://codereview.chromium.org/2967113003/diff/60001/ios/showcase/text_badge...
File ios/showcase/text_badge_view/sc_text_badge_view_controller.h (right):

https://codereview.chromium.org/2967113003/diff/60001/ios/showcase/text_badge...
ios/showcase/text_badge_view/sc_text_badge_view_controller.h:10: @interface
TextBadgeViewController : UIViewController
This should also have the prepend "SC".

https://codereview.chromium.org/2967113003/diff/60001/ios/showcase/text_badge...
File ios/showcase/text_badge_view/sc_text_badge_view_controller.mm (right):

https://codereview.chromium.org/2967113003/diff/60001/ios/showcase/text_badge...
ios/showcase/text_badge_view/sc_text_badge_view_controller.mm:20:
textBadge.frame = CGRectMake(0.0f, 0.0f, 100.0f, 100.0f);
It's a good idea to offset from the edges a little bit so it's clear where the
boundaries are.

[helenlyang, 2017-07-07 23:53:00]

Addressed comments. PTAL; thanks!

https://codereview.chromium.org/2967113003/diff/60001/ios/showcase/core/showc...
File ios/showcase/core/showcase_model.mm (right):

https://codereview.chromium.org/2967113003/diff/60001/ios/showcase/core/showc...
ios/showcase/core/showcase_model.mm:96: showcase::kClassForInstantiationKey :
@"TextBadgeViewController",
On 2017/07/07 21:00:09, edchin wrote:
> Update this as well.

Done.

https://codereview.chromium.org/2967113003/diff/60001/ios/showcase/text_badge...
File ios/showcase/text_badge_view/sc_text_badge_view_controller.h (right):

https://codereview.chromium.org/2967113003/diff/60001/ios/showcase/text_badge...
ios/showcase/text_badge_view/sc_text_badge_view_controller.h:10: @interface
TextBadgeViewController : UIViewController
On 2017/07/07 21:00:09, edchin wrote:
> This should also have the prepend "SC".

Done.

https://codereview.chromium.org/2967113003/diff/60001/ios/showcase/text_badge...
File ios/showcase/text_badge_view/sc_text_badge_view_controller.mm (right):

https://codereview.chromium.org/2967113003/diff/60001/ios/showcase/text_badge...
ios/showcase/text_badge_view/sc_text_badge_view_controller.mm:20:
textBadge.frame = CGRectMake(0.0f, 0.0f, 100.0f, 100.0f);
On 2017/07/07 21:00:09, edchin wrote:
> It's a good idea to offset from the edges a little bit so it's clear where the
> boundaries are.

I've modified this so textBadge is centered on its superview.

[gchatz, 2017-07-08 00:23:30]

lgtm thanks!

[helenlyang, 2017-07-10 15:52:58]

The CQ bit was checked by helenlyang@google.com

[helenlyang, 2017-07-10 15:52:58]

The patchset sent to the CQ was uploaded after l-g-t-m from edchin@chromium.org
Link to the patchset: https://codereview.chromium.org/2967113003/#ps80001
(title: "Added SC prefix, centered text badge")

[commit-bot: I haz the power, 2017-07-10 15:53:14]

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-07-10 17:13:33]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499701978147960,
"parent_rev": "d7d9e946cef9ec23de8ac9496c80f3d831142e81", "commit_rev":
"ac8a93564a2560a5ec54673246c551e04e11ed02"}

[commit-bot: I haz the power, 2017-07-10 17:13:48]

Description was changed from

==========
[ios showcase] Integrated TextBadgeView with Showcase.

This CL adds TextBadgeView to iOS Showcase. This involves adding a
text_badge_view folder to showcase, creating a glue view controller,
and modifying the Showcase configuration file and BUILD.gn files as
necessary.

Since TextBadgeView is currently a stub implementation, Showcase
displays a blank white screen.

BUG=740133.
==========

to

==========
[ios showcase] Integrated TextBadgeView with Showcase.

This CL adds TextBadgeView to iOS Showcase. This involves adding a
text_badge_view folder to showcase, creating a glue view controller,
and modifying the Showcase configuration file and BUILD.gn files as
necessary.

Since TextBadgeView is currently a stub implementation, Showcase
displays a blank white screen.

BUG=740133.

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

[commit-bot: I haz the power, 2017-07-10 17:13:50]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ac8a93564a2560a5ec54673246c5...

[michaeldo, 2017-07-10 20:16:28]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/2978533002/ by michaeldo@chromium.org.

The reason for reverting is: This CL breaks ios_showcase_egtests suite. PTAL at
the failure here:
https://build.chromium.org/p/chromium.fyi/builders/EarlGreyiOS/builds/29530

and fix:
- SCToolbarTestCase/testRotationSizeClassChange
-
 SCToolbarTestCase/testVerifyToolbarButtonsLabelAndAction

then re-land..