Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10824054:
add oauth2 zippies to views extension install dialog (Closed)
|
Created:
8 years, 5 months ago by Evan Stade Modified:
8 years, 4 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionadd oauth2 zippies to views extension install dialog
BUG=130206
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148879
Patch Set 1 #
Total comments: 13
Patch Set 2 : review #
Total comments: 10
Patch Set 3 : 89deg hack #
Total comments: 2
Patch Set 4 : s/layout_/layout #
Messages
Total messages: 12 (0 generated)
screenshots on bug.
http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:412: layout->AddPaddingRow(0, views::kRelatedControlSmallVerticalSpacing); This results in two "small vertical spacing" padding rows in a row; seems incorrect http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:528: SetLayoutManager(layout_); Nit: By convention we'd normally do most of this in ViewHierarchyChanged() when this gets added to a parent view. This makes sure that all the views we create get correct memory management (since they won't be deleted if there's no parent to own them). Technically it's not terribly important in your case since you create this view and then immediately add it to a hierarchy, so only an exception could interrupt the process (and in that case Chrome would terminate), but for future-proofing and consistency, it might be nice to do that here as well. http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:575: for (size_t i = 0; i < issue_advice.details.size(); ++i) { Just making sure -- it looks like as the details view expands, the contents inside will appear to be pinned at the top, and we'll reveal more and more at the bottom until all the text is visible, without the text actually shifting. Is this right? That's how things should work, I just want to make sure the text doesn't shift vertically while the slide animation is running. http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:590: if (slide_animation_.IsShowing()) Nit: I'm not sure this is quite right -- seems like if the animation is fully open, we'll click once to begin hiding, and then if we click again while the animation is running it will continue to hide. Seems like instead each click should reverse the direction of any running animation.
http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:412: layout->AddPaddingRow(0, views::kRelatedControlSmallVerticalSpacing); On 2012/07/27 00:45:09, Peter Kasting wrote: > This results in two "small vertical spacing" padding rows in a row; seems > incorrect I was doing this intentionally for reasons I won't attempt to explain, but I found a problem with my approach and I've fixed this as well. http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:528: SetLayoutManager(layout_); On 2012/07/27 00:45:09, Peter Kasting wrote: > Nit: By convention we'd normally do most of this in ViewHierarchyChanged() when > this gets added to a parent view. This makes sure that all the views we create > get correct memory management (since they won't be deleted if there's no parent > to own them). well as long as |this| owns them, and |this| gets deleted, we're ok, right? Meaning the child views would only leak if |this| leaks. I've gone ahead and made the change though. > > Technically it's not terribly important in your case since you create this view > and then immediately add it to a hierarchy, so only an exception could interrupt > the process (and in that case Chrome would terminate), but for future-proofing > and consistency, it might be nice to do that here as well. http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:575: for (size_t i = 0; i < issue_advice.details.size(); ++i) { On 2012/07/27 00:45:09, Peter Kasting wrote: > Just making sure -- it looks like as the details view expands, the contents > inside will appear to be pinned at the top, and we'll reveal more and more at > the bottom until all the text is visible, without the text actually shifting. > Is this right? That's how things should work, I just want to make sure the text > doesn't shift vertically while the slide animation is running. yes, that happens. I can create a video of it if you want. http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:590: if (slide_animation_.IsShowing()) On 2012/07/27 00:45:09, Peter Kasting wrote: > Nit: I'm not sure this is quite right -- seems like if the animation is fully > open, we'll click once to begin hiding, and then if we click again while the > animation is running it will continue to hide. Seems like instead each click > should reverse the direction of any running animation. IsShowing() refers to the goal state actually, so this is correct (and no matter how madly I click, it always ends up in the right state). http://codereview.chromium.org/10824054/diff/5001/chrome/browser/ui/views/ext... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): http://codereview.chromium.org/10824054/diff/5001/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:506: SetLayoutManager(layout_); does this also need to be in ViewHierarchyChanged? I put it here since it doesn't create any views.
LGTM http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:528: SetLayoutManager(layout_); On 2012/07/27 03:06:54, Evan Stade wrote: > On 2012/07/27 00:45:09, Peter Kasting wrote: > > Nit: By convention we'd normally do most of this in ViewHierarchyChanged() > when > > this gets added to a parent view. This makes sure that all the views we > create > > get correct memory management (since they won't be deleted if there's no > parent > > to own them). > > well as long as |this| owns them, and |this| gets deleted, we're ok, right? > Meaning the child views would only leak if |this| leaks. Oh, sorry, I didn't realize that with a layout manager, AddView() basically means |this| now owns the view. My fault for being murky on layout managers. You're right that this was safe as it was, and it was probably simpler. You may want to get an authoritative opinion from sky on what the best practice here is, but I'm now kind of leaning toward doing things as you had them here. (Sorry...) http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:575: for (size_t i = 0; i < issue_advice.details.size(); ++i) { On 2012/07/27 03:06:54, Evan Stade wrote: > On 2012/07/27 00:45:09, Peter Kasting wrote: > > Just making sure -- it looks like as the details view expands, the contents > > inside will appear to be pinned at the top, and we'll reveal more and more at > > the bottom until all the text is visible, without the text actually shifting. > > Is this right? That's how things should work, I just want to make sure the > text > > doesn't shift vertically while the slide animation is running. > > yes, that happens. I can create a video of it if you want. Don't worry, I believe you :) http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:590: if (slide_animation_.IsShowing()) On 2012/07/27 03:06:54, Evan Stade wrote: > On 2012/07/27 00:45:09, Peter Kasting wrote: > > Nit: I'm not sure this is quite right -- seems like if the animation is fully > > open, we'll click once to begin hiding, and then if we click again while the > > animation is running it will continue to hide. Seems like instead each click > > should reverse the direction of any running animation. > > IsShowing() refers to the goal state actually, so this is correct (and no matter > how madly I click, it always ends up in the right state). Awesome! http://codereview.chromium.org/10824054/diff/5001/chrome/browser/ui/views/ext... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): http://codereview.chromium.org/10824054/diff/5001/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:140: OVERRIDE; Nit: I think our normal practice here is to treat OVERRIDE like "const" and say it should be on the same line as the last arg -- so we normally would put the three args on separate lines. http://codereview.chromium.org/10824054/diff/5001/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:506: SetLayoutManager(layout_); On 2012/07/27 03:06:54, Evan Stade wrote: > does this also need to be in ViewHierarchyChanged? I put it here since it > doesn't create any views. It doesn't (and I'm leaning toward nuking that anyway, see previous comment). http://codereview.chromium.org/10824054/diff/5001/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:518: 0, views::kRelatedControlSmallVerticalSpacing); Tiny nit: Slightly unusual wrapping... normally we'd probably wrap after the third 0. http://codereview.chromium.org/10824054/diff/5001/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:543: issue_advice_(issue_advice), Nit: If ViewHierarchyChanged() gets nuked, do you actually need this member? http://codereview.chromium.org/10824054/diff/5001/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:544: horizontal_space_(horizontal_space), Nit: If ViewHierarchyChanged() gets nuked and you move the code from DetailsView() into this class, I think you can eliminate this member as well.
http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:528: SetLayoutManager(layout_); On 2012/07/27 03:38:03, Peter Kasting wrote: > On 2012/07/27 03:06:54, Evan Stade wrote: > > On 2012/07/27 00:45:09, Peter Kasting wrote: > > > Nit: By convention we'd normally do most of this in ViewHierarchyChanged() > > when > > > this gets added to a parent view. This makes sure that all the views we > > create > > > get correct memory management (since they won't be deleted if there's no > > parent > > > to own them). > > > > well as long as |this| owns them, and |this| gets deleted, we're ok, right? > > Meaning the child views would only leak if |this| leaks. > > Oh, sorry, I didn't realize that with a layout manager, AddView() basically > means |this| now owns the view. My fault for being murky on layout managers. > > You're right that this was safe as it was, and it was probably simpler. You may > want to get an authoritative opinion from sky on what the best practice here is, > but I'm now kind of leaning toward doing things as you had them here. > (Sorry...) View owns the LayoutManager, when you do SetLayoutManager(layout_manager); |this| will own |layout_manager|. That is why we don't need to store any pointer in the class, and just have it as a local variable here.
http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): http://codereview.chromium.org/10824054/diff/1/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:528: SetLayoutManager(layout_); On 2012/07/27 03:50:34, tfarina wrote: > On 2012/07/27 03:38:03, Peter Kasting wrote: > > On 2012/07/27 03:06:54, Evan Stade wrote: > > > On 2012/07/27 00:45:09, Peter Kasting wrote: > > > > Nit: By convention we'd normally do most of this in ViewHierarchyChanged() > > > when > > > > this gets added to a parent view. This makes sure that all the views we > > > create > > > > get correct memory management (since they won't be deleted if there's no > > > parent > > > > to own them). > > > > > > well as long as |this| owns them, and |this| gets deleted, we're ok, right? > > > Meaning the child views would only leak if |this| leaks. > > > > Oh, sorry, I didn't realize that with a layout manager, AddView() basically > > means |this| now owns the view. My fault for being murky on layout managers. > > > > You're right that this was safe as it was, and it was probably simpler. You > may > > want to get an authoritative opinion from sky on what the best practice here > is, > > but I'm now kind of leaning toward doing things as you had them here. > > (Sorry...) > > View owns the LayoutManager, when you do SetLayoutManager(layout_manager); > |this| will own |layout_manager|. That is why we don't need to store any pointer > in the class, and just have it as a local variable here. The issue was the ownership of the views inside the layout manager (which it turns out are owned by |this| once added to the layout manager), not the ownership of the layout manager itself. But yes, we don't need to keep the layout manager as a member if we don't need to access it outside the constructor.
http://codereview.chromium.org/10824054/diff/5001/chrome/browser/ui/views/ext... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): http://codereview.chromium.org/10824054/diff/5001/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:140: OVERRIDE; function removed. http://codereview.chromium.org/10824054/diff/5001/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:518: 0, views::kRelatedControlSmallVerticalSpacing); On 2012/07/27 03:38:03, Peter Kasting wrote: > Tiny nit: Slightly unusual wrapping... normally we'd probably wrap after the > third 0. the reason I grouped like this is because the first 2 args are related to the row, and the second 2 are related to the padding. http://codereview.chromium.org/10824054/diff/5001/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:543: issue_advice_(issue_advice), On 2012/07/27 03:38:03, Peter Kasting wrote: > Nit: If ViewHierarchyChanged() gets nuked, do you actually need this member? no http://codereview.chromium.org/10824054/diff/5001/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:544: horizontal_space_(horizontal_space), On 2012/07/27 03:38:03, Peter Kasting wrote: > Nit: If ViewHierarchyChanged() gets nuked and you move the code from > DetailsView() into this class, I think you can eliminate this member as well. I like having the DetailsView code in the DetailsView class, but yea I can remove this member regardless.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/10824054/10002
LGTM (just in case) https://chromiumcodereview.appspot.com/10824054/diff/10002/chrome/browser/ui/... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://chromiumcodereview.appspot.com/10824054/diff/10002/chrome/browser/ui/... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:537: layout_(new views::GridLayout(this)), Nit: If you want, you can make this member into a local
https://chromiumcodereview.appspot.com/10824054/diff/10002/chrome/browser/ui/... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://chromiumcodereview.appspot.com/10824054/diff/10002/chrome/browser/ui/... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:537: layout_(new views::GridLayout(this)), On 2012/07/28 00:45:55, Peter Kasting wrote: > Nit: If you want, you can make this member into a local sure
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/10824054/9005
Change committed as 148879 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
