Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1757103002:
Add explanation text and legal message footer to upload bubble. (Closed)
|
Created:
4 years, 9 months ago by Justin Donnelly Modified:
4 years, 9 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews, rouslan+autofill_chromium.org, bondd+autofillwatch_chromium.org, jdonnelly+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd explanation text and legal message footer to upload bubble.
BUG=566271
Committed: https://crrev.com/319c897905a59fbfc6a0bcc4dd91470df9ab5bb0
Cr-Commit-Position: refs/heads/master@{#379655}
Patch Set 1 #
Total comments: 40
Patch Set 2 : Respond to comments #Patch Set 3 : Small tweak to bubble size #
Messages
Total messages: 11 (3 generated)
jdonnelly@chromium.org changed reviewers: + groby@chromium.org
https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm (right): https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:250: makeLabel:SysUTF16ToNSString(base::UTF8ToUTF16("\xE2\x8B\xAF") + I know, it's not your code, but can this be kEllipsis please? https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:259: base::scoped_nsobject<NSTextView> explanationLabel( Why are you still alloc/initing here? https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:266: .release()); Please don't use dot notation for message invocation. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:277: CGFloat length = [learnMoreString length]; Might want to create the range here. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:316: for (auto lineIter = lines.end() - 1; lineIter != lines.begin() - 1; Why not build in the reverse direction and use (for const auto& line : lines)? If that doesn't work, rbegin() and rend() are your friend and save you weird arithmetic. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:324: for (auto linkIter = lineIter->links().begin(); C++11 loop? https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:347: [footerView setFrameOrigin:NSMakePoint(0, 0)]; NSZeroPoint https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:414: clickedOnLink:(id)link Instead of doing the whole iteration thing - you can give IDs to links. Why not use that? https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:421: for (auto lineIter = lines.begin(); lineIter != lines.end(); ++lineIter) { C++11 loop? https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:424: for (auto linkIter = lineIter->links().begin(); You know what I'm going to say... :) https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm (right): https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm:54: value->GetAsDictionary(&legal_message); ASSERT_TRUE? https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm:76: void OnCancelButton() override { on_cancel_button_was_called_ = true; } It's not the CL to change this, but this looks like it would've been better off as a mock. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm:185: EXPECT_FALSE(bubble_controller_->on_learn_more_was_called()); Do these EXPECT_FALSE statements matter?
https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm (right): https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:250: makeLabel:SysUTF16ToNSString(base::UTF8ToUTF16("\xE2\x8B\xAF") + On 2016/03/03 00:40:22, groby wrote: > I know, it's not your code, but can this be kEllipsis please? Do you mean kEllipsis from src/ui/gfx/text_elider.h? That's a regular ellipsis not the midline version we need. Or did you just mean add a kEllipsis in this file? Either way, just FYI, I have a TODO that I'm going to tackle as soon as I'm done with this bubble to replace all of these (here and in various other places) with a common constant. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:259: base::scoped_nsobject<NSTextView> explanationLabel( On 2016/03/03 00:40:22, groby wrote: > Why are you still alloc/initing here? Having a zero-size label in the case where there's no explanation makes the layout code a little simpler. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:266: .release()); On 2016/03/03 00:40:22, groby wrote: > Please don't use dot notation for message invocation. I couldn't figure out any other way to make this work. To be honest, despite looking at the code many times, I'm still confused about exactly how scoped_nsobject works. So I copied this approach from some other c/b/ui/cocoa code. Right or wrong, it seems to be a not uncommon pattern[1]. Is there a better way? [1] https://code.google.com/p/chromium/codesearch#search/&q=%22.release()%22%20fi... https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:277: CGFloat length = [learnMoreString length]; On 2016/03/03 00:40:22, groby wrote: > Might want to create the range here. Done. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:316: for (auto lineIter = lines.end() - 1; lineIter != lines.begin() - 1; On 2016/03/03 00:40:22, groby wrote: > Why not build in the reverse direction and use (for const auto& line : lines)? The lines are built in cross-platform code also used by Views and Clank. > If that doesn't work, rbegin() and rend() are your friend and save you weird > arithmetic. Sweet! Done. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:324: for (auto linkIter = lineIter->links().begin(); On 2016/03/03 00:40:22, groby wrote: > C++11 loop? Done. So much nicer. (I couldn't find any examples of range based loops in c/b/ui/cocoa so I wasn't sure they were allowed.) https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:347: [footerView setFrameOrigin:NSMakePoint(0, 0)]; On 2016/03/03 00:40:23, groby wrote: > NSZeroPoint Done. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:414: clickedOnLink:(id)link On 2016/03/03 00:40:23, groby wrote: > Instead of doing the whole iteration thing - you can give IDs to links. Why not > use that? I'm not quite sure what you mean. UIView's tag property? Or passing the URL in addLinkRange:withURL:linkColor:? I don't do that latter because the other bubbles don't (I think in order to make the links not draggable). Like you alluded to yesterday, we do seem to revel in fighting the platform. :) Or did you mean something else? https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:421: for (auto lineIter = lines.begin(); lineIter != lines.end(); ++lineIter) { On 2016/03/03 00:40:23, groby wrote: > C++11 loop? Done. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:424: for (auto linkIter = lineIter->links().begin(); On 2016/03/03 00:40:23, groby wrote: > You know what I'm going to say... :) Done. :) https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm (right): https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm:54: value->GetAsDictionary(&legal_message); On 2016/03/03 00:40:23, groby wrote: > ASSERT_TRUE? Done. (Required moving this code to a void function.) https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm:76: void OnCancelButton() override { on_cancel_button_was_called_ = true; } On 2016/03/03 00:40:23, groby wrote: > It's not the CL to change this, but this looks like it would've been better off > as a mock. I don't disagree, but some reviewers are enthusiastically anti-mock so I tend to use the manual approach when the conditions I care about are relatively simple. If this gets any more complicated, I'll definitely switch. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm:185: EXPECT_FALSE(bubble_controller_->on_learn_more_was_called()); On 2016/03/03 00:40:23, groby wrote: > Do these EXPECT_FALSE statements matter? I think so. Effective unit tests IMO check both that the expected outcome occurred and that unexpected conditions don't arise.
LGTM w/ a few tiny comments https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm (right): https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:250: makeLabel:SysUTF16ToNSString(base::UTF8ToUTF16("\xE2\x8B\xAF") + On 2016/03/03 18:45:10, Justin Donnelly wrote: > On 2016/03/03 00:40:22, groby wrote: > > I know, it's not your code, but can this be kEllipsis please? > > Do you mean kEllipsis from src/ui/gfx/text_elider.h? That's a regular ellipsis > not the midline version we need. Or did you just mean add a kEllipsis in this > file? > > Either way, just FYI, I have a TODO that I'm going to tackle as soon as I'm done > with this bubble to replace all of these (here and in various other places) with > a common constant. SG - (I didn't mean the elider one) https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:259: base::scoped_nsobject<NSTextView> explanationLabel( On 2016/03/03 18:45:09, Justin Donnelly wrote: > On 2016/03/03 00:40:22, groby wrote: > > Why are you still alloc/initing here? > > Having a zero-size label in the case where there's no explanation makes the > layout code a little simpler. You know you can send messages to nil, right? Return values are nil, 0, or a zeroed out structure, depending on return type. Simplifies code even more :) https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:266: .release()); On 2016/03/03 18:45:09, Justin Donnelly wrote: > On 2016/03/03 00:40:22, groby wrote: > > Please don't use dot notation for message invocation. > > I couldn't figure out any other way to make this work. To be honest, despite > looking at the code many times, I'm still confused about exactly how > scoped_nsobject works. So I copied this approach from some other c/b/ui/cocoa > code. Right or wrong, it seems to be a not uncommon pattern[1]. > > Is there a better way? > > [1] > https://code.google.com/p/chromium/codesearch#search/&q=%22.release()%22%20fi... Sigh. Misread the release as being a Cocoa message. But yes, there's a simpler way - return an auto-released object instead of a scoped one. As for the "better" way - I personally believe that scoped1.reset([scoped2 retain]) is the more cocoa-y way to do this, but YMMV. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:316: for (auto lineIter = lines.end() - 1; lineIter != lines.begin() - 1; On 2016/03/03 00:40:22, groby wrote: > Why not build in the reverse direction and use (for const auto& line : lines)? > If that doesn't work, rbegin() and rend() are your friend and save you weird > arithmetic. No, I mean building the UI layout in the reverse direction :) But rbegin/rend works. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:414: clickedOnLink:(id)link On 2016/03/03 18:45:10, Justin Donnelly wrote: > On 2016/03/03 00:40:23, groby wrote: > > Instead of doing the whole iteration thing - you can give IDs to links. Why > not > > use that? > > I'm not quite sure what you mean. UIView's tag property? Or passing the URL in > addLinkRange:withURL:linkColor:? I don't do that latter because the other > bubbles don't (I think in order to make the links not draggable). Like you > alluded to yesterday, we do seem to revel in fighting the platform. :) > > Or did you mean something else? I like draggable links - it's a more OSXy thing. But if all bubbles don't do it, fine. (Also, you can add custom attributes to any NSAttributed string, but that's probably overkill...) https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:423: base::SysNSStringToUTF16([[textView textStorage] string])) { The text comparison thing just bugs me - but since we manually line break the legal message, I suppose we have to. (Why do we manually break?) https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:426: if (linkIter->range.start() <= charIndex && FWIW: You could do NSLocationInRange(charIndex, NSMakeRange(...)) bit more text, possibly more readable. It's your choice, I have no strong preference. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm (right): https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm:76: void OnCancelButton() override { on_cancel_button_was_called_ = true; } On 2016/03/03 18:45:10, Justin Donnelly wrote: > On 2016/03/03 00:40:23, groby wrote: > > It's not the CL to change this, but this looks like it would've been better > off > > as a mock. > > I don't disagree, but some reviewers are enthusiastically anti-mock so I tend to > use the manual approach when the conditions I care about are relatively simple. > If this gets any more complicated, I'll definitely switch. Argl. If it's more complicated, the likelihood it should be a mock is smaller :) We should restrict gmock to fairly trivial objects. But that's probably a discussion for chromium-dev when we're bored ;) https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm:185: EXPECT_FALSE(bubble_controller_->on_learn_more_was_called()); On 2016/03/03 18:45:10, Justin Donnelly wrote: > On 2016/03/03 00:40:23, groby wrote: > > Do these EXPECT_FALSE statements matter? > > I think so. Effective unit tests IMO check both that the expected outcome > occurred and that unexpected conditions don't arise. Meh... There's an infinite number of unexpected conditions. Can't test for them all.
https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm (right): https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:259: base::scoped_nsobject<NSTextView> explanationLabel( On 2016/03/07 19:54:57, groby wrote: > On 2016/03/03 18:45:09, Justin Donnelly wrote: > > On 2016/03/03 00:40:22, groby wrote: > > > Why are you still alloc/initing here? > > > > Having a zero-size label in the case where there's no explanation makes the > > layout code a little simpler. > > You know you can send messages to nil, right? Return values are nil, 0, or a > zeroed out structure, depending on return type. Simplifies code even more :) I had forgotten that until you asked me why I was doing this. :) But then when I tried to have explanationLabel be nil, I learned that you can't add nil as a subview. I could make the setSubviews code be a bit more complicated and skip adding it if it's nil. I'd prefer to leave this as-is but don't feel strongly about it. I'm going to submit now but I'd be happy to change it later if you feel the alternative is better. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:266: .release()); On 2016/03/07 19:54:58, groby wrote: > On 2016/03/03 18:45:09, Justin Donnelly wrote: > > On 2016/03/03 00:40:22, groby wrote: > > > Please don't use dot notation for message invocation. > > > > I couldn't figure out any other way to make this work. To be honest, despite > > looking at the code many times, I'm still confused about exactly how > > scoped_nsobject works. So I copied this approach from some other c/b/ui/cocoa > > code. Right or wrong, it seems to be a not uncommon pattern[1]. > > > > Is there a better way? > > > > [1] > > > https://code.google.com/p/chromium/codesearch#search/&q=%22.release()%22%20fi... > > Sigh. Misread the release as being a Cocoa message. > > But yes, there's a simpler way - return an auto-released object instead of a > scoped one. As for the "better" way - I personally believe that > scoped1.reset([scoped2 retain]) is the more cocoa-y way to do this, but YMMV. As discussed out-of-band, leaving this as is. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_bridge.mm:423: base::SysNSStringToUTF16([[textView textStorage] string])) { On 2016/03/07 19:54:57, groby wrote: > The text comparison thing just bugs me - but since we manually line break the > legal message, I suppose we have to. (Why do we manually break?) Payments wanted it that way for reasons I can no longer recall. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm (right): https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm:76: void OnCancelButton() override { on_cancel_button_was_called_ = true; } On 2016/03/07 19:54:58, groby wrote: > On 2016/03/03 18:45:10, Justin Donnelly wrote: > > On 2016/03/03 00:40:23, groby wrote: > > > It's not the CL to change this, but this looks like it would've been better > > off > > > as a mock. > > > > I don't disagree, but some reviewers are enthusiastically anti-mock so I tend > to > > use the manual approach when the conditions I care about are relatively > simple. > > If this gets any more complicated, I'll definitely switch. > > Argl. If it's more complicated, the likelihood it should be a mock is smaller :) > We should restrict gmock to fairly trivial objects. > > But that's probably a discussion for chromium-dev when we're bored ;) I'm not sure there's anything in all of software engineering on which there's less consensus than mocks. :) I look forward to the discussion. https://codereview.chromium.org/1757103002/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm:185: EXPECT_FALSE(bubble_controller_->on_learn_more_was_called()); On 2016/03/07 19:54:58, groby wrote: > On 2016/03/03 18:45:10, Justin Donnelly wrote: > > On 2016/03/03 00:40:23, groby wrote: > > > Do these EXPECT_FALSE statements matter? > > > > I think so. Effective unit tests IMO check both that the expected outcome > > occurred and that unexpected conditions don't arise. > > Meh... There's an infinite number of unexpected conditions. Can't test for them > all. Granted, you can't check them all. But I don't think you should check for none, either, and I don't think I'm alone in that. Besides, this way, what to check becomes a subjective question, ideal for bikeshedding. ;)
The CQ bit was checked by jdonnelly@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1757103002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1757103002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add explanation text and legal message footer to upload bubble. BUG=566271 ========== to ========== Add explanation text and legal message footer to upload bubble. BUG=566271 Committed: https://crrev.com/319c897905a59fbfc6a0bcc4dd91470df9ab5bb0 Cr-Commit-Position: refs/heads/master@{#379655} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/319c897905a59fbfc6a0bcc4dd91470df9ab5bb0 Cr-Commit-Position: refs/heads/master@{#379655} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
