Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2964233002:
[ios] BubbleViewController and BubbleView 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] BubbleViewController and BubbleView stubs.
Added bubble_view_controller interface and implementation stubs,
bubble_view interface and implementation stubs, and unit test
stubs.
These files live in the bubble_promo directory. For context, they are
being used for the User Education project.
BUG=740145
.
Review-Url: https://codereview.chromium.org/2964233002
Cr-Commit-Position: refs/heads/master@{#485735}
Committed: https://chromium.googlesource.com/chromium/src/+/5d0b9fa8555eb3a5b4ebada265794d04ac33c49b
Patch Set 1 #
Total comments: 24
Patch Set 2 : Add BubbleView, remove BubbleConfiguration #
Total comments: 3
Patch Set 3 : Add BubbleView property to BubbleVC #
Total comments: 36
Patch Set 4 : Rename directory, add unit test dep, other comments #
Total comments: 10
Patch Set 5 : Fix enum style, comments, and label attributes #
Total comments: 3
Patch Set 6 : Move view initialization to loadView, rebase #
Total comments: 1
Patch Set 7 : Add initial values for enums #Messages
Total messages: 58 (30 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Added bubble_view_controller header and implementation stubs, bubble_configuration header and implementation stubs, and unit test stubs. These files live in the bubble_promo directory (for context, they are being used for the User Education project). BUG=none. ========== to ========== Added bubble_view_controller header and implementation stubs, bubble_configuration header and implementation stubs, and unit test stubs. These files live in the bubble_promo directory. For context, they are being used for the User Education project. BUG=none. ==========
Description was changed from ========== Added bubble_view_controller header and implementation stubs, bubble_configuration header and implementation stubs, and unit test stubs. These files live in the bubble_promo directory. For context, they are being used for the User Education project. BUG=none. ========== to ========== [ios] BubbleViewController and BubbleConfiguration stubs. Added bubble_view_controller interface and implementation stubs, bubble_configuration interface and implementation stubs, and unit test stubs. These files live in the bubble_promo directory. For context, they are being used for the User Education project. BUG=none. ==========
helenlyang@google.com changed reviewers: + edchin@chromium.org, gchatz@chromium.org
helenlyang@google.com changed required reviewers: + edchin@chromium.org, gchatz@chromium.org
The more I think about this, I think we need to make 2 changes: 1) We should remove the BubbleConfiguration. It was originally a mechanism to extract the tabSwitcherButton from the toolbar. That's a detail for the BVC to handle, not for the Bubble. 2) We should create a BubbleView, which does all the drawing and sizing. The reason is that -sizeToFit: and -intrinsicContentSize: are methods on UIView, not UIViewController. Therefore, we need to subclass UIView to override these methods. I'm happy to chat more about this and give more concrete directions. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_configuration.h (right): https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:11: enum class BubbleArrowDirection { How about calling it "PointingDirection"? This should be in the BubbleVC header. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:19: enum class BubbleAlignment { LEADING, CENTER, TRAILING }; This should be in the BubbleVC header. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:23: @interface BubbleConfiguration : NSObject The original purpose of this class was to extract the target element from a branch of the parent view hierarchy. I think most of the properties here really belong in the bubbleVC itself. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:25: @property(nonatomic) NSString* displayText; Drawing inspiration from UILabel and UIButton, the property "text" should be descriptive enough. There should be a memory management attribute for this property. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:26: @property(nonatomic) BubbleAlignment* alignment; Enums should not be pointers. Pointers are only used for objects. You can think of an enum like a primitive type (such as a number). https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h (right): https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:12: enum class BubbleArrowDirection; Both of these should be defined in this header file. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:16: // message in white text and points to the UI element of interest. White text is irrelevant. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:19: // The preferred origin of |self.view|. Note that this does not set "Preferred origin" assumes that the BubbleVC knows about the coordinate system of the parent. It does not. We can instead implement -sizeThatFits:(CGSize)size. Please remove this property. https://developer.apple.com/documentation/uikit/uiview/1622625-sizethatfits?l... https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:26: - (instancetype)initWithConfiguration:(BubbleConfiguration*)configuration I suggest that we initialize with the necessary information directly. Please initWith text, pointingDirection, and alignment. Please do not include the UI element in this initializer. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:32: - (instancetype)initWithCoder:(NSCoder*)aDecoder NS_UNAVAILABLE; The important one to set as unavailable is -init; The initWithNib and initWithCoder are only used with storyboard or Nibs/Xibs. We don't use that in chromium. Feel free to leave them for completeness. Just add -init;
On 2017/07/01 15:29:28, edchin wrote: > The more I think about this, I think we need to make 2 changes: > > 1) We should remove the BubbleConfiguration. It was originally a mechanism to > extract the tabSwitcherButton from the toolbar. That's a detail for the BVC to > handle, not for the Bubble. > > 2) We should create a BubbleView, which does all the drawing and sizing. The > reason is that -sizeToFit: and -intrinsicContentSize: are methods on UIView, not > UIViewController. Therefore, we need to subclass UIView to override these > methods. > > I'm happy to chat more about this and give more concrete directions. > > https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... > File ios/chrome/browser/ui/bubble_promo/bubble_configuration.h (right): > > https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... > ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:11: enum class > BubbleArrowDirection { > How about calling it "PointingDirection"? This should be in the BubbleVC header. > > https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... > ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:19: enum class > BubbleAlignment { LEADING, CENTER, TRAILING }; > This should be in the BubbleVC header. > > https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... > ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:23: @interface > BubbleConfiguration : NSObject > The original purpose of this class was to extract the target element > from a branch of the parent view hierarchy. I think most of the properties > here really belong in the bubbleVC itself. > > https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... > ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:25: > @property(nonatomic) NSString* displayText; > Drawing inspiration from UILabel and UIButton, the property "text" should be > descriptive enough. > There should be a memory management attribute for this property. > > https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... > ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:26: > @property(nonatomic) BubbleAlignment* alignment; > Enums should not be pointers. Pointers are only used for objects. > You can think of an enum like a primitive type (such as a number). > > https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... > File ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h (right): > > https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... > ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:12: enum class > BubbleArrowDirection; > Both of these should be defined in this header file. > > https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... > ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:16: // message in > white text and points to the UI element of interest. > White text is irrelevant. > > https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... > ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:19: // The preferred > origin of |self.view|. Note that this does not set > "Preferred origin" assumes that the BubbleVC knows about the coordinate system > of the parent. > It does not. We can instead implement -sizeThatFits:(CGSize)size. Please remove > this property. > > https://developer.apple.com/documentation/uikit/uiview/1622625-sizethatfits?l... > > https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... > ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:26: - > (instancetype)initWithConfiguration:(BubbleConfiguration*)configuration > I suggest that we initialize with the necessary information directly. > Please initWith text, pointingDirection, and alignment. > Please do not include the UI element in this initializer. > > https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... > ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:32: - > (instancetype)initWithCoder:(NSCoder*)aDecoder NS_UNAVAILABLE; > The important one to set as unavailable is -init; The initWithNib and > initWithCoder are only used with storyboard or Nibs/Xibs. > We don't use that in chromium. Feel free to leave them for completeness. Just > add -init; I just wanted to be clear that we need both a BubbleView and a BubbleViewController. The BubbleView does the drawing and sizing. The BubbleVC does the rest.
Thanks for the comments. I agree that the configuration object seems unnecessary now, especially since Mark suggested a way to get a reference to the target view without passing around a configuration object. I added BubbleView, removed BubbleConfiguration, and addressed Ed's comments in the most recent patch set. PTAL! https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_configuration.h (right): https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:11: enum class BubbleArrowDirection { On 2017/07/01 15:29:27, edchin wrote: > How about calling it "PointingDirection"? This should be in the BubbleVC header. I felt that "BubbleArrowDirection" was more consistent with the UIPopoverArrowDirection enum used by UIPopoverPresentationController (see https://developer.apple.com/documentation/uikit/uipopoverpresentationcontroll...). However, happy to change to "PointingDirection" if you think it would be more clear. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:19: enum class BubbleAlignment { LEADING, CENTER, TRAILING }; On 2017/07/01 15:29:27, edchin wrote: > This should be in the BubbleVC header. Done. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:23: @interface BubbleConfiguration : NSObject On 2017/07/01 15:29:27, edchin wrote: > The original purpose of this class was to extract the target element > from a branch of the parent view hierarchy. I think most of the properties > here really belong in the bubbleVC itself. Agreed; removed this class as you suggested. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:25: @property(nonatomic) NSString* displayText; On 2017/07/01 15:29:27, edchin wrote: > Drawing inspiration from UILabel and UIButton, the property "text" should be > descriptive enough. > There should be a memory management attribute for this property. I've added a label property to BubbleView. Since its text can be set during initialization, I don't think we need a text property. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:26: @property(nonatomic) BubbleAlignment* alignment; On 2017/07/01 15:29:27, edchin wrote: > Enums should not be pointers. Pointers are only used for objects. > You can think of an enum like a primitive type (such as a number). Done. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h (right): https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:12: enum class BubbleArrowDirection; On 2017/07/01 15:29:28, edchin wrote: > Both of these should be defined in this header file. Done. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:16: // message in white text and points to the UI element of interest. On 2017/07/01 15:29:28, edchin wrote: > White text is irrelevant. Removed. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:19: // The preferred origin of |self.view|. Note that this does not set On 2017/07/01 15:29:28, edchin wrote: > "Preferred origin" assumes that the BubbleVC knows about the coordinate system > of the parent. > It does not. We can instead implement -sizeThatFits:(CGSize)size. Please remove > this property. > > https://developer.apple.com/documentation/uikit/uiview/1622625-sizethatfits?l... Removed for now, but may change based on our discussion with Mark. The issue here is that BVC needs to know where the arrow is positioned in the bubble in order to properly attach to the target element. This would require some calculation that accounts for intrinsic content size, alignment, and arrow direction, which may be best to keep in BubbleVC. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:26: - (instancetype)initWithConfiguration:(BubbleConfiguration*)configuration On 2017/07/01 15:29:28, edchin wrote: > I suggest that we initialize with the necessary information directly. > Please initWith text, pointingDirection, and alignment. > Please do not include the UI element in this initializer. Done. I stuck with the "arrowDirection" parameter name for this reason: (https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b...) but will change it to pointingDirection if we change the enum name. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:32: - (instancetype)initWithCoder:(NSCoder*)aDecoder NS_UNAVAILABLE; On 2017/07/01 15:29:27, edchin wrote: > The important one to set as unavailable is -init; The initWithNib and > initWithCoder are only used with storyboard or Nibs/Xibs. > We don't use that in chromium. Feel free to leave them for completeness. Just > add -init; Done.
Description was changed from ========== [ios] BubbleViewController and BubbleConfiguration stubs. Added bubble_view_controller interface and implementation stubs, bubble_configuration interface and implementation stubs, and unit test stubs. These files live in the bubble_promo directory. For context, they are being used for the User Education project. BUG=none. ========== to ========== [ios] BubbleViewController and BubbleView stubs. Added bubble_view_controller interface and implementation stubs, bubble_view interface and implementation stubs, and unit test stubs. These files live in the bubble_promo directory. For context, they are being used for the User Education project. BUG=none. ==========
It's looking good. We'll finalize the interface tomorrow with marq@. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_configuration.h (right): https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:11: enum class BubbleArrowDirection { On 2017/07/05 20:35:36, helenlyang wrote: > On 2017/07/01 15:29:27, edchin wrote: > > How about calling it "PointingDirection"? This should be in the BubbleVC > header. > > I felt that "BubbleArrowDirection" was more consistent with the > UIPopoverArrowDirection enum used by UIPopoverPresentationController (see > https://developer.apple.com/documentation/uikit/uipopoverpresentationcontroll...). > > However, happy to change to "PointingDirection" if you think it would be > more clear. Good point. And good application of consistency with existing code. You've changed my mind! https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_configuration.h:25: @property(nonatomic) NSString* displayText; On 2017/07/05 20:35:36, helenlyang wrote: > On 2017/07/01 15:29:27, edchin wrote: > > Drawing inspiration from UILabel and UIButton, the property "text" should be > > descriptive enough. > > There should be a memory management attribute for this property. > > I've added a label property to BubbleView. Since its text can be set > during initialization, I don't think we need a text property. Acknowledged. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h (right): https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:26: - (instancetype)initWithConfiguration:(BubbleConfiguration*)configuration On 2017/07/05 20:35:37, helenlyang wrote: > On 2017/07/01 15:29:28, edchin wrote: > > I suggest that we initialize with the necessary information directly. > > Please initWith text, pointingDirection, and alignment. > > Please do not include the UI element in this initializer. > > Done. I stuck with the "arrowDirection" parameter name for this reason: > (https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b...) > but will change it to pointingDirection if we change the enum name. Acknowledged. https://codereview.chromium.org/2964233002/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:32: - (instancetype)initWithCoder:(NSCoder*)aDecoder NS_UNAVAILABLE; On 2017/07/05 20:35:37, helenlyang wrote: > On 2017/07/01 15:29:27, edchin wrote: > > The important one to set as unavailable is -init; The initWithNib and > > initWithCoder are only used with storyboard or Nibs/Xibs. > > We don't use that in chromium. Feel free to leave them for completeness. Just > > add -init; > > Done. As I was doing some other work, I discovered that initWithNib actually is the designated initializer for UIViewController. My comment still applies, but just wanted to note this discovery. https://codereview.chromium.org/2964233002/diff/60001/ios/chrome/browser/ui/B... File ios/chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2964233002/diff/60001/ios/chrome/browser/ui/B... ios/chrome/browser/ui/BUILD.gn:275: "//ios/chrome/browser/ui/bubble_promo", I'm curious as to the naming convention here. Why did you choose to append "_promo"?
I added a private BubbleView property to BubbleVC and removed intrinsicContentSize based on our discussion this morning (we would use sizeThatFits instead of intrinsicContentSize). PTAL. Thank you! https://codereview.chromium.org/2964233002/diff/60001/ios/chrome/browser/ui/B... File ios/chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2964233002/diff/60001/ios/chrome/browser/ui/B... ios/chrome/browser/ui/BUILD.gn:275: "//ios/chrome/browser/ui/bubble_promo", On 2017/07/06 04:56:19, edchin wrote: > I'm curious as to the naming convention here. Why did you choose to append > "_promo"? I felt that "bubble_promo" was more specific than just "bubble", which seems ambiguous (not all bubbles are speech bubbles). Thinking about it more, though, "bubble_promo" seems to exclude some of the bubble's potential use cases. Maybe "bubble_tooltip" or "speech_bubble"?
On 2017/07/06 22:21:57, helenlyang wrote: > I added a private BubbleView property to BubbleVC and removed > intrinsicContentSize based on our discussion this morning (we would use > sizeThatFits instead of intrinsicContentSize). PTAL. > Good idea! lgtm. Please address the comments below. After gchatz@ approves, please send to marq@ and rohitrao@. https://codereview.chromium.org/2964233002/diff/60001/ios/chrome/browser/ui/B... File ios/chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2964233002/diff/60001/ios/chrome/browser/ui/B... ios/chrome/browser/ui/BUILD.gn:275: "//ios/chrome/browser/ui/bubble_promo", On 2017/07/06 22:21:57, helenlyang wrote: > On 2017/07/06 04:56:19, edchin wrote: > > I'm curious as to the naming convention here. Why did you choose to append > > "_promo"? > > I felt that "bubble_promo" was more specific than just "bubble", which > seems ambiguous (not all bubbles are speech bubbles). Thinking > about it more, though, "bubble_promo" seems to exclude some of the > bubble's potential use cases. > > Maybe "bubble_tooltip" or "speech_bubble"? I think we should just go with bubble. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/B... File ios/chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/B... ios/chrome/browser/ui/BUILD.gn:275: "//ios/chrome/browser/ui/bubble_promo", I don't think the sources above depend on bubble. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/B... ios/chrome/browser/ui/BUILD.gn:397: "//ios/chrome/browser/ui/bubble_promo", Let's put this in later when code is actually put into the BVC. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view.h (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view.h:10: enum class BubbleArrowDirection; I think these enums need to be declared in this header file as opposed to the BubbleViewController. The reason is that this implementation will need it, and the VC depends on the view, not the other way around. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view.h:16: @property(nonatomic, readonly) BubbleArrowDirection arrowDirection; I don't think these two lines are going to be needed. You can remove them for now. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view.mm (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view.mm:12: @property(nonatomic, readonly, weak) UILabel* label; Remove readonly. You'll want to be able to set it. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view.mm:27: Add this line to document this section: #pragma mark - UIView overrides https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view_controller_unittest.mm (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller_unittest.mm:6: #import "ios/chrome/browser/ui/bubble_promo/bubble_configuration.h" Remove this. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller_unittest.mm:8: #import <Foundation/Foundation.h> Remove this.
https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/BUILD.gn (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/BUILD.gn:19: source_set("unit_tests") { This target is never depended on. So need to add it to a unit test target in ios/chrome/browser/ui/BUILD.gn. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view.h (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view.h:16: @property(nonatomic, readonly) BubbleArrowDirection arrowDirection; Need comments for properties and designated initializer. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view.mm (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view.mm:12: @property(nonatomic, readonly, weak) UILabel* label; Need comment here too. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view.mm:28: - (CGSize)sizeThatFits:(CGSize)size { Should have comment explaining why override is happening. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:11: enum class BubbleArrowDirection { I think for the enums, we should use the NS_ENUM macro since we are in Objective-C code. Also, see the style guide for guidance on how to name the enums members. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:21: // View controller that manages a BubbleView, which points to the UI element of Replace "the UI element" with "a UI element" https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view_controller.mm (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.mm:13: @property BubbleView* bubbleView; Property should have options specified in (), like (nonatomic, strong). I am not sure you will need a property for the BubbleView. UIViewController has a view property. Typical pattern is to create a UIView and assign it to view property in UIViewController's loadView method. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.mm:28: } Could be good to call NOTIMPLEMENTED() in these empty methods. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view_controller_unittest.mm (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller_unittest.mm:6: #import "ios/chrome/browser/ui/bubble_promo/bubble_configuration.h" bubble_configuration.h was removed. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller_unittest.mm:16: These tests are likely going to do similar set up, so a test fixture would be appropriate. This doc explains them: https://github.com/google/googletest/blob/master/googletest/docs/Primer.md Look at our other unittests to see examples. We already have a variety of test fixture subclasses that can be used. (if you are planning on saving this for a later part of the implementation, then feel free to ignore this comment)
https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/B... File ios/chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/B... ios/chrome/browser/ui/BUILD.gn:397: "//ios/chrome/browser/ui/bubble_promo", On 2017/07/07 04:24:10, edchin wrote: > Let's put this in later when code is actually put into the BVC. If it is not added now, then the target does not compile.
https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/B... File ios/chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/B... ios/chrome/browser/ui/BUILD.gn:397: "//ios/chrome/browser/ui/bubble_promo", On 2017/07/07 04:30:39, gchatz wrote: > On 2017/07/07 04:24:10, edchin wrote: > > Let's put this in later when code is actually put into the BVC. > > If it is not added now, then the target does not compile. Acknowledged.
Description was changed from ========== [ios] BubbleViewController and BubbleView stubs. Added bubble_view_controller interface and implementation stubs, bubble_view interface and implementation stubs, and unit test stubs. These files live in the bubble_promo directory. For context, they are being used for the User Education project. BUG=none. ========== to ========== [ios] BubbleViewController and BubbleView stubs. Added bubble_view_controller interface and implementation stubs, bubble_view interface and implementation stubs, and unit test stubs. These files live in the bubble_promo directory. For context, they are being used for the User Education project. BUG=740145. ==========
Responded to all comments--PTAL. Thank you! https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/B... File ios/chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/B... ios/chrome/browser/ui/BUILD.gn:275: "//ios/chrome/browser/ui/bubble_promo", On 2017/07/07 04:24:10, edchin wrote: > I don't think the sources above depend on bubble. Done. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/BUILD.gn (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/BUILD.gn:19: source_set("unit_tests") { On 2017/07/07 04:29:10, gchatz wrote: > This target is never depended on. So need to add it to a unit test target in > ios/chrome/browser/ui/BUILD.gn. Done. I added it to ios_chrome_unittests in ios/chrome/test/BUILD.gn. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view.h (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view.h:10: enum class BubbleArrowDirection; On 2017/07/07 04:24:10, edchin wrote: > I think these enums need to be declared in this header file > as opposed to the BubbleViewController. The reason > is that this implementation will need it, and > the VC depends on the view, not the other way around. Done. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view.h:16: @property(nonatomic, readonly) BubbleArrowDirection arrowDirection; On 2017/07/07 04:24:10, edchin wrote: > I don't think these two lines are going to be needed. You can remove them for > now. Done. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view.mm (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view.mm:12: @property(nonatomic, readonly, weak) UILabel* label; On 2017/07/07 04:24:10, edchin wrote: > Remove readonly. You'll want to be able to set it. My reasoning was that we want to modify the text, not the label itself. I think we can still set label.text when label is readonly. See NumberBadgeView: https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/reading_list/numbe... https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view.mm:27: On 2017/07/07 04:24:10, edchin wrote: > Add this line to document this section: > #pragma mark - UIView overrides Done. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view.mm:28: - (CGSize)sizeThatFits:(CGSize)size { On 2017/07/07 04:29:10, gchatz wrote: > Should have comment explaining why override is happening. Done. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:11: enum class BubbleArrowDirection { On 2017/07/07 04:29:10, gchatz wrote: > I think for the enums, we should use the NS_ENUM macro since we are in > Objective-C code. Also, see the style guide for guidance on how to name the > enums members. Done. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.h:21: // View controller that manages a BubbleView, which points to the UI element of On 2017/07/07 04:29:10, gchatz wrote: > Replace "the UI element" with "a UI element" Done. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view_controller.mm (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.mm:13: @property BubbleView* bubbleView; On 2017/07/07 04:29:11, gchatz wrote: > Property should have options specified in (), like (nonatomic, strong). > > I am not sure you will need a property for the BubbleView. UIViewController has > a view property. Typical pattern is to create a UIView and assign it to view > property in UIViewController's loadView method. You're right. I've removed the property and assigned it to self.view instead. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller.mm:28: } On 2017/07/07 04:29:11, gchatz wrote: > Could be good to call NOTIMPLEMENTED() in these empty methods. Done. I've also added calls to NOTIMPLEMENTED() in bubble_view.mm. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view_controller_unittest.mm (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller_unittest.mm:6: #import "ios/chrome/browser/ui/bubble_promo/bubble_configuration.h" On 2017/07/07 04:29:11, gchatz wrote: > bubble_configuration.h was removed. Done. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller_unittest.mm:8: #import <Foundation/Foundation.h> On 2017/07/07 04:24:10, edchin wrote: > Remove this. Done. https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller_unittest.mm:16: On 2017/07/07 04:29:11, gchatz wrote: > These tests are likely going to do similar set up, so a test fixture would be > appropriate. This doc explains them: > https://github.com/google/googletest/blob/master/googletest/docs/Primer.md > > Look at our other unittests to see examples. We already have a variety of test > fixture subclasses that can be used. > > (if you are planning on saving this for a later part of the implementation, then > feel free to ignore this comment) Added a stub implementation for now. Is PlatformTest the appropriate fixture subclass?
https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view.mm (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view.mm:12: @property(nonatomic, readonly, weak) UILabel* label; On 2017/07/07 23:29:16, helenlyang wrote: > On 2017/07/07 04:24:10, edchin wrote: > > Remove readonly. You'll want to be able to set it. > > My reasoning was that we want to modify the text, not the label itself. > I think we can still set label.text when label is readonly. > See NumberBadgeView: > https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/reading_list/numbe... Overall with the label in NumberBadgeView, here is what is happening with its object lifetime: NumberBadgeView creates the label, stores it in a weak property, and adds it as a subview of itself. The reason it is retained is because UIViews all have strong references to its subviews. If the label is somehow removed from the view hierarchy, then it will be deallocated and the |label| property becomes nil. So, this set up is fine as long as there is no plan to remove the label from the hierarchy and then use that instance again later. My guess is that they did not make the label property strong because the same object (the UIView) would have a two strong references to the same object (in the UIView superclass and in the subclass). https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bubble_promo/bubble_view_controller_unittest.mm (right): https://codereview.chromium.org/2964233002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bubble_promo/bubble_view_controller_unittest.mm:16: On 2017/07/07 23:29:16, helenlyang wrote: > On 2017/07/07 04:29:11, gchatz wrote: > > These tests are likely going to do similar set up, so a test fixture would be > > appropriate. This doc explains them: > > https://github.com/google/googletest/blob/master/googletest/docs/Primer.md > > > > Look at our other unittests to see examples. We already have a variety of test > > fixture subclasses that can be used. > > > > (if you are planning on saving this for a later part of the implementation, > then > > feel free to ignore this comment) > > Added a stub implementation for now. Is PlatformTest the appropriate > fixture subclass? Yeah that should be fine.
lgtm with comments. Thanks! https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bubble/bubble_view.mm (right): https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bubble/bubble_view.mm:31: // Overriding sizeThatFits allows BubbleViewController and I think the standard practice is to use an imperative, like: Override sizeThatFits so that ... https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bubble/bubble_view_controller.h (right): https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bubble/bubble_view_controller.h:12: // enum class BubbleArrowDirection; These probably should be gone. They are commented out.
Please address these comments as well before landing this CL. https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bubble/bubble_view.h (right): https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bubble/bubble_view.h:15: BubbleArrowDirectionDown Our convention is to put a comma after even the last line. The reason is that it is easy to forget to add a comma when you decide to add additional lines later. You can look up other ENUM examples on code search. https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bubble/bubble_view.mm (right): https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bubble/bubble_view.mm:15: @property(nonatomic, readonly, weak) UILabel* label; Please remove the "readonly" attribute here. I previously commented on this same line in this CL but it seems to have reappeared in this latest patch. Here's a link that explains more about the property attributes: https://useyourloaf.com/blog/default-property-attributes-with-arc/ https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bubble/bubble_view.mm:32: // BubbleViewController's parent to appropriately set the BubbleView's size. This class should not know about BubbleViewController. Please refrain from commenting about BubbleVC.
https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bubble/bubble_view.h (right): https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bubble/bubble_view.h:15: BubbleArrowDirectionDown On 2017/07/08 07:58:26, edchin wrote: > Our convention is to put a comma after even the last line. > The reason is that it is easy to forget to add a comma > when you decide to add additional lines later. > You can look up other ENUM examples on code search. Done. https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bubble/bubble_view.mm (right): https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bubble/bubble_view.mm:15: @property(nonatomic, readonly, weak) UILabel* label; On 2017/07/08 07:58:26, edchin wrote: > Please remove the "readonly" attribute here. > I previously commented on this same line in this CL > but it seems to have reappeared in this latest patch. > Here's a link that explains more about the property attributes: > https://useyourloaf.com/blog/default-property-attributes-with-arc/ Done. https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bubble/bubble_view.mm:31: // Overriding sizeThatFits allows BubbleViewController and On 2017/07/08 00:46:22, gchatz wrote: > I think the standard practice is to use an imperative, like: > Override sizeThatFits so that ... Done. https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bubble/bubble_view.mm:32: // BubbleViewController's parent to appropriately set the BubbleView's size. On 2017/07/08 07:58:26, edchin wrote: > This class should not know about BubbleViewController. > Please refrain from commenting about BubbleVC. Done. https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bubble/bubble_view_controller.h (right): https://codereview.chromium.org/2964233002/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bubble/bubble_view_controller.h:12: // enum class BubbleArrowDirection; On 2017/07/08 00:46:22, gchatz wrote: > These probably should be gone. They are commented out. Ack sorry! Removed.
helenlyang@google.com changed reviewers: + marq@chromium.org, rohitrao@chromium.org
helenlyang@google.com changed required reviewers: + marq@chromium.org
The CQ bit was checked by helenlyang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
helenlyang@google.com changed required reviewers: - marq@chromium.org
https://codereview.chromium.org/2964233002/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bubble/bubble_view_controller.mm (right): https://codereview.chromium.org/2964233002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bubble/bubble_view_controller.mm:19: self.view = [[BubbleView alloc] initWithText:text Should this assignment be in loadView instead?
https://codereview.chromium.org/2964233002/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bubble/bubble_view_controller.mm (right): https://codereview.chromium.org/2964233002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bubble/bubble_view_controller.mm:19: self.view = [[BubbleView alloc] initWithText:text On 2017/07/10 17:44:28, rohitrao (ping after 24h) wrote: > Should this assignment be in loadView instead? Yes, you're right. We probably would have caught this in the true implementation. Helen, here's more reading on loadView. Notice the difference between loadView and viewDidLoad. https://developer.apple.com/documentation/uikit/uiviewcontroller/1621454-load...
https://codereview.chromium.org/2964233002/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bubble/bubble_view_controller.mm (right): https://codereview.chromium.org/2964233002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bubble/bubble_view_controller.mm:19: self.view = [[BubbleView alloc] initWithText:text On 2017/07/10 17:51:59, edchin wrote: > On 2017/07/10 17:44:28, rohitrao (ping after 24h) wrote: > > Should this assignment be in loadView instead? > > Yes, you're right. We probably would have caught this in the true > implementation. > > Helen, here's more reading on loadView. Notice the difference between loadView > and viewDidLoad. > https://developer.apple.com/documentation/uikit/uiviewcontroller/1621454-load... Fixed. In order to initialize BubbleView in loadView, I stored text, arrowDirection, and alignment as properties of BubbleVC.
The CQ bit was checked by helenlyang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks!
LGTM with nit. https://codereview.chromium.org/2964233002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bubble/bubble_view.h (right): https://codereview.chromium.org/2964233002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bubble/bubble_view.h:13: BubbleArrowDirectionUp, nit: Provide an initial value for enums: BubbleArrowDirectionUp = 0,
The CQ bit was checked by helenlyang@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from edchin@chromium.org, gchatz@chromium.org, marq@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2964233002/#ps160001 (title: "Add initial values for enums")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gchatz@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gchatz@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/07/11 19:56:19, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) This might need a rebase in order for this bot to pass.
On 2017/07/11 20:18:50, gchatz wrote: > On 2017/07/11 19:56:19, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > This might need a rebase in order for this bot to pass. NVM, CQ should do this for you.
The CQ bit was checked by gchatz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by gchatz@chromium.org
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": 160001, "attempt_start_ts": 1499812622777010,
"parent_rev": "8b79bb8538ba6be9a898f414e5c768c442773a7f", "commit_rev":
"5d0b9fa8555eb3a5b4ebada265794d04ac33c49b"}
Message was sent while issue was closed.
Description was changed from ========== [ios] BubbleViewController and BubbleView stubs. Added bubble_view_controller interface and implementation stubs, bubble_view interface and implementation stubs, and unit test stubs. These files live in the bubble_promo directory. For context, they are being used for the User Education project. BUG=740145. ========== to ========== [ios] BubbleViewController and BubbleView stubs. Added bubble_view_controller interface and implementation stubs, bubble_view interface and implementation stubs, and unit test stubs. These files live in the bubble_promo directory. For context, they are being used for the User Education project. BUG=740145. Review-Url: https://codereview.chromium.org/2964233002 Cr-Commit-Position: refs/heads/master@{#485735} Committed: https://chromium.googlesource.com/chromium/src/+/5d0b9fa8555eb3a5b4ebada26579... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as https://chromium.googlesource.com/chromium/src/+/5d0b9fa8555eb3a5b4ebada26579... |
