Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2954853004:
[ios] TextBadgeView interface, implementation, and unit test stubs (Closed)
|
Created:
3 years, 5 months ago by helenlyang Modified:
3 years, 5 months ago CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ios] TextBadgeView interface, implementation, and unit test stubs.
Added header, implementation, and unit test files for TextBadgeView.
Updated the BUILD.gn file in the reading_list directory to include the
header and implementation files in the reading_list_ui source set, and
the unit test file in the unit_tests source set. The header,
implementation, and unit test files are stub implementations.
For context, the TextBadgeView is part of the User Education project.
BUG=none.
Review-Url: https://codereview.chromium.org/2954853004
Cr-Commit-Position: refs/heads/master@{#483205}
Committed: https://chromium.googlesource.com/chromium/src/+/e28c24b9848609678128c3644f752bcf8d5832d7
Patch Set 1 #
Total comments: 4
Patch Set 2 : Moved to reading_list directory and added unit test stub. #
Total comments: 3
Patch Set 3 : Added unit test stubs. #
Total comments: 18
Patch Set 4 : Fixed initializers and unit tests. #
Total comments: 3
Patch Set 5 : Fixed initializer and unit test comment. #
Messages
Total messages: 32 (13 generated)
Description was changed from ========== Added header and implementation files for TextBadgeView. Updated the BUILD.gn file in tools_menu directory to include those two new files in the source set. For context, the text badge view is part of the User Education project. Header and implementation files are still undergoing construction. BUG=none. ========== to ========== Added header and implementation files for TextBadgeView. Updated the BUILD.gn file in tools_menu directory to include those two new files in the source set. For context, the text badge view is part of the User Education project. Header and implementation files are still undergoing construction. BUG=none. ==========
helenlyang@google.com changed reviewers: + edchin@chromium.org, gchatz@chromium.org
helenlyang@google.com changed reviewers: + jif@chromium.org, rohitrao@chromium.org
helenlyang@google.com changed required reviewers: + jif@chromium.org, rohitrao@chromium.org
Nice first CL. Please create a placeholder unit test file as well. Also rename the CL to "[ios] TextBadgeView interface and implementation stubs." https://codereview.chromium.org/2954853004/diff/1/ios/chrome/browser/ui/tools... File ios/chrome/browser/ui/tools_menu/text_badge_view.mm (right): https://codereview.chromium.org/2954853004/diff/1/ios/chrome/browser/ui/tools... ios/chrome/browser/ui/tools_menu/text_badge_view.mm:9: // TODO(helenlyang): add private properties and methods. Let's not put TODOs like this in code. https://codereview.chromium.org/2954853004/diff/1/ios/chrome/browser/ui/tools... ios/chrome/browser/ui/tools_menu/text_badge_view.mm:13: // This implementation is incomplete. Remove this comment.
Looks good! Please consider adding some basic unittests in this CL. Even though the class doesn't do much yet, having the _unittest.mm file will make it easier to add tests in future CLs. https://codereview.chromium.org/2954853004/diff/1/ios/chrome/browser/ui/tools... File ios/chrome/browser/ui/tools_menu/text_badge_view.mm (right): https://codereview.chromium.org/2954853004/diff/1/ios/chrome/browser/ui/tools... ios/chrome/browser/ui/tools_menu/text_badge_view.mm:24: return; No need for a return statement at the end of methods that return void.
https://codereview.chromium.org/2954853004/diff/1/ios/chrome/browser/ui/tools... File ios/chrome/browser/ui/tools_menu/BUILD.gn (right): https://codereview.chromium.org/2954853004/diff/1/ios/chrome/browser/ui/tools... ios/chrome/browser/ui/tools_menu/BUILD.gn:10: "text_badge_view.h", Per our discussion, please move this to the reading list folder.
Description was changed from ========== Added header and implementation files for TextBadgeView. Updated the BUILD.gn file in tools_menu directory to include those two new files in the source set. For context, the text badge view is part of the User Education project. Header and implementation files are still undergoing construction. BUG=none. ========== to ========== Added header and implementation files for TextBadgeView, as well as a unit test file. Updated the BUILD.gn file in the reading_list directory to include the header and implementation files to the reading_list_ui source set, and the unit test file to the unit_tests source set. For context, the text badge view is part of the User Education project. Header, implementation, and unit test files are stub implementations. BUG=none. ==========
Patchset #2 (id:20001) has been deleted
On 2017/06/27 00:39:42, edchin wrote: > https://codereview.chromium.org/2954853004/diff/1/ios/chrome/browser/ui/tools... > File ios/chrome/browser/ui/tools_menu/BUILD.gn (right): > > https://codereview.chromium.org/2954853004/diff/1/ios/chrome/browser/ui/tools... > ios/chrome/browser/ui/tools_menu/BUILD.gn:10: "text_badge_view.h", > Per our discussion, please move this to the reading list folder. Thanks for the feedback. I've addressed all comments, except for adding basic unit tests. Will do so in a future patch set.
looks good, but can you tell me what is the reasoning behind the move to the reading list folder?
On 2017/06/27 15:11:47, jif wrote: > looks good, but can you tell me what is the reasoning behind the move to the > reading list folder? At this moment, we are only looking at using the text badge on the reading list cell. Since this is currently our only use case, adding the text badge to any other cell in the tools menu would require adding code to ToolsMenuViewCell that may not be used. We figured it would make the most sense to move the TextBadgeView to the tools_menu folder when we have a use for the text badge on another cell of the tools menu, which would also be when we make those additions/changes to ToolsMenuViewCell.
On 2017/06/27 17:38:09, helenlyang wrote: > On 2017/06/27 15:11:47, jif wrote: > > looks good, but can you tell me what is the reasoning behind the move to the > > reading list folder? > > At this moment, we are only looking at using the text badge on the reading list > cell. > Since this is currently our only use case, adding the text badge to any other > cell in > the tools menu would require adding code to ToolsMenuViewCell that may not be > used. > > We figured it would make the most sense to move the TextBadgeView to the > tools_menu > folder when we have a use for the text badge on another cell of the tools menu, > which > would also be when we make those additions/changes to ToolsMenuViewCell. Got it, thanks. lgtm
On 2017/06/27 17:48:46, jif-google wrote: > On 2017/06/27 17:38:09, helenlyang wrote: > > On 2017/06/27 15:11:47, jif wrote: > > > looks good, but can you tell me what is the reasoning behind the move to the > > > reading list folder? > > > > At this moment, we are only looking at using the text badge on the reading > list > > cell. > > Since this is currently our only use case, adding the text badge to any other > > cell in > > the tools menu would require adding code to ToolsMenuViewCell that may not be > > used. > > > > We figured it would make the most sense to move the TextBadgeView to the > > tools_menu > > folder when we have a use for the text badge on another cell of the tools > menu, > > which > > would also be when we make those additions/changes to ToolsMenuViewCell. > > Got it, thanks. > lgtm lgtm from here too, but you'll still need an owner.
The CL description should match the guidelines I sent out yesterday, primarily 72 char width. https://codereview.chromium.org/2954853004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/text_badge_view.mm (right): https://codereview.chromium.org/2954853004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view.mm:11: @interface TextBadgeView () Do we need the class extension, since there is nothing in it, as of now?
Description was changed from ========== Added header and implementation files for TextBadgeView, as well as a unit test file. Updated the BUILD.gn file in the reading_list directory to include the header and implementation files to the reading_list_ui source set, and the unit test file to the unit_tests source set. For context, the text badge view is part of the User Education project. Header, implementation, and unit test files are stub implementations. BUG=none. ========== to ========== [ios] TextBadgeView interface, implementation, and unit test stubs. Added header, implementation, and unit test files for TextBadgeView. Updated the BUILD.gn file in the reading_list directory to include the header and implementation files in the reading_list_ui source set, and the unit test file in the unit_tests source set. The header, implementation, and unit test files are stub implementations. For context, the TextBadgeView is part of the User Education project. BUG=none. ==========
lgtm once gchatz and edchin sign off, thanks!
Patchset #3 (id:60001) has been deleted
On 2017/06/27 19:25:10, rohitrao (ping after 24h) wrote: > lgtm once gchatz and edchin sign off, thanks! Addressed gchatz's comments and added some unit test stubs. PTAL. Thank you!
This is a good second iteration. I like the tests that you added. I've done a more thorough review this time so the comments are a bit more in-depth. https://codereview.chromium.org/2954853004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/text_badge_view.mm (right): https://codereview.chromium.org/2954853004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view.mm:11: @interface TextBadgeView () Remove this private category for now. https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/text_badge_view.h (right): https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view.h:10: // Pill-shaped view that displays white text. The view stretches to fit the We can emulate UILabel's intrinsic content size. Basically, a label says the size that it wants to be but doesn't actually set its own frame. This doesn't change the interface, but we can remove the comment that the view stretches to fit the content. Read this link for more information: https://developer.apple.com/library/content/documentation/UserExperience/Conc... https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view.h:11: // display text, and is hidden when the text is the empty string. We can treat the empty string as programmer error. Basically, we feel that the programmer is not properly using this API if they pass in an empty string. Inside the implementation, we can put this line: DCHECK(text); https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view.h:17: // Display the text badge view. If |animated| is true, the badge fades in. Let's remove this, since it violates the "dumb view" concept. https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/text_badge_view_unittest.mm (right): https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view_unittest.mm:6: #import <Foundation/Foundation.h> There should be a space after the first import and the frameworks imports. https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view_unittest.mm:9: #include "testing/gtest_mac.h" This has the macro EXPECT_NSEQ(). I'm sure you're going to need it when you implement this method. But this isn't quite needed yet. It's our policy to only introduce includes that are actually used in the CL. https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view_unittest.mm:16: TEST(TextBadgeViewTest, BadgeWithoutLabel) {} We can remove the first two tests since we think this is programmer error. https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view_unittest.mm:22: TEST(TextBadgeViewTest, BadgeWidthShortLabel) {} We can test the intrinsic content size. I like this test. Please update the comment to something like: "Tests that the badge's intrinsic content size given a short text." Please make a similar edit to the next test. See the doc on intrinsicContentSize here: https://developer.apple.com/documentation/uikit/uiview/1622600-intrinsicconte... https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view_unittest.mm:28: TEST(TextBadgeViewTest, RTL) {} This is a good idea. I'm not sure how this is tested, but we can figure it out when we implement this test. https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view_unittest.mm:30: // Tests accessibility features. This comment can be more specific. Consider something like this: "Tests that the accessibility label matches the display text".
https://codereview.chromium.org/2954853004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/text_badge_view.mm (right): https://codereview.chromium.org/2954853004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view.mm:11: @interface TextBadgeView () On 2017/06/28 00:38:23, edchin wrote: > Remove this private category for now. Sorry.. this was already removed, but somehow this comment lingered from a previous iteration.
https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/text_badge_view.h (right): https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view.h:15: - (instancetype)initWithFrame:(CGRect)frame displayText:(NSString*)text; We should designate this as the initializer. You can also mark other initializers as unavailable. Here are the ones to mark as unavailable: - initWithFrame:(CGRect)frame; - init; Here's an example: https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/ntp/new_tab_page_v...
Addressed edchin's comments. https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/text_badge_view.h (right): https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view.h:10: // Pill-shaped view that displays white text. The view stretches to fit the On 2017/06/28 00:38:23, edchin wrote: > We can emulate UILabel's intrinsic content size. > Basically, a label says the size that it wants to be > but doesn't actually set its own frame. This doesn't > change the interface, but we can remove the comment > that the view stretches to fit the content. > > Read this link for more information: > https://developer.apple.com/library/content/documentation/UserExperience/Conc... Done. https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view.h:11: // display text, and is hidden when the text is the empty string. On 2017/06/28 00:38:23, edchin wrote: > We can treat the empty string as programmer error. > Basically, we feel that the programmer is not properly > using this API if they pass in an empty string. > Inside the implementation, we can put this line: > DCHECK(text); Done (removed this part of the comment). https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view.h:15: - (instancetype)initWithFrame:(CGRect)frame displayText:(NSString*)text; On 2017/06/28 00:51:14, edchin wrote: > We should designate this as the initializer. You can also mark other > initializers as unavailable. > Here are the ones to mark as unavailable: > - initWithFrame:(CGRect)frame; > - init; > > Here's an example: > https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/ntp/new_tab_page_v... Done. I also marked initWithCoder: as unavailable. https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view.h:17: // Display the text badge view. If |animated| is true, the badge fades in. On 2017/06/28 00:38:23, edchin wrote: > Let's remove this, since it violates the "dumb view" concept. Done. https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/text_badge_view_unittest.mm (right): https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view_unittest.mm:6: #import <Foundation/Foundation.h> On 2017/06/28 00:38:24, edchin wrote: > There should be a space after the first import and the frameworks imports. Done. https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view_unittest.mm:16: TEST(TextBadgeViewTest, BadgeWithoutLabel) {} On 2017/06/28 00:38:23, edchin wrote: > We can remove the first two tests since we think this is programmer error. Done. https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view_unittest.mm:22: TEST(TextBadgeViewTest, BadgeWidthShortLabel) {} On 2017/06/28 00:38:23, edchin wrote: > We can test the intrinsic content size. I like this test. Please > update the comment to something like: > "Tests that the badge's intrinsic content size given a short text." > Please make a similar edit to the next test. > > See the doc on intrinsicContentSize here: > https://developer.apple.com/documentation/uikit/uiview/1622600-intrinsicconte... Done. https://codereview.chromium.org/2954853004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/text_badge_view_unittest.mm:30: // Tests accessibility features. On 2017/06/28 00:38:24, edchin wrote: > This comment can be more specific. Consider something like this: > "Tests that the accessibility label matches the display text". Done.
Patchset #4 (id:90001) has been deleted
lgtm -- once you make the two changes commented below. Thanks! https://codereview.chromium.org/2954853004/diff/110001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/text_badge_view.h (right): https://codereview.chromium.org/2954853004/diff/110001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/text_badge_view.h:14: - (instancetype)initWithFrame:(CGRect)frame Looking at this a bit more, I think we should remove the frame and simply have - (instancetype)initWithText:(NSString*)text; The reason is that the frame that is passed in will not be used at all. Eventually, the frame will be set again through layout constraints based on the intrinsic content size. https://codereview.chromium.org/2954853004/diff/110001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/text_badge_view_unittest.mm (right): https://codereview.chromium.org/2954853004/diff/110001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/text_badge_view_unittest.mm:19: // Tests the badge's intrinsic content size given short display text. s/short/long
lgtm with those changes as well. Thanks! https://codereview.chromium.org/2954853004/diff/110001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/text_badge_view.h (right): https://codereview.chromium.org/2954853004/diff/110001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/text_badge_view.h:14: - (instancetype)initWithFrame:(CGRect)frame On 2017/06/28 19:54:55, edchin wrote: > Looking at this a bit more, I think we should remove the frame and simply > have - (instancetype)initWithText:(NSString*)text; > The reason is that the frame that is passed in will not be used at all. > Eventually, the frame will be set again through layout constraints > based on the intrinsic content size. I assume then that you suggest passing CGRectZero into superclass constructor?
The CQ bit was checked by helenlyang@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org, jif@google.com, rohitrao@chromium.org, gchatz@chromium.org, edchin@chromium.org Link to the patchset: https://codereview.chromium.org/2954853004/#ps130001 (title: "Fixed initializer and unit test comment.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 130001, "attempt_start_ts": 1498682236185810,
"parent_rev": "788101ef734f894f076e941c40c2693bd64dd548", "commit_rev":
"e28c24b9848609678128c3644f752bcf8d5832d7"}
Message was sent while issue was closed.
Description was changed from ========== [ios] TextBadgeView interface, implementation, and unit test stubs. Added header, implementation, and unit test files for TextBadgeView. Updated the BUILD.gn file in the reading_list directory to include the header and implementation files in the reading_list_ui source set, and the unit test file in the unit_tests source set. The header, implementation, and unit test files are stub implementations. For context, the TextBadgeView is part of the User Education project. BUG=none. ========== to ========== [ios] TextBadgeView interface, implementation, and unit test stubs. Added header, implementation, and unit test files for TextBadgeView. Updated the BUILD.gn file in the reading_list directory to include the header and implementation files in the reading_list_ui source set, and the unit test file in the unit_tests source set. The header, implementation, and unit test files are stub implementations. For context, the TextBadgeView is part of the User Education project. BUG=none. Review-Url: https://codereview.chromium.org/2954853004 Cr-Commit-Position: refs/heads/master@{#483205} Committed: https://chromium.googlesource.com/chromium/src/+/e28c24b9848609678128c3644f75... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:130001) as https://chromium.googlesource.com/chromium/src/+/e28c24b9848609678128c3644f75... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
