Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11742003:
Implemented drop hander for the Home toolbar button that accepts (only) links (Closed)
|
Created:
7 years, 11 months ago by Tom Cassiotis Modified:
7 years, 10 months ago CC:
chromium-reviews, tfarina Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionDrag URL onto Home Button to set homepage
Implemented drop hander for the Home toolbar button that accepts links
and informs the user with a bubble with the option to undo.
Contributed by: tom.cassiotis@gmail.com
BUG=152210
TEST=Manually tested various links and other sources and followed
through all confirmation permutations (Undo, clicking on bubble, focus
loss/application switching, navigating off page). Also the special
case where the setting that the undo is resetting to is the NTP.
Tested RTL alignment to ensure that the Undo link is to the left
of the message in this situation.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179526
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 50
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 23
Patch Set 8 : #
Total comments: 19
Patch Set 9 : #
Total comments: 2
Patch Set 10 : #
Messages
Total messages: 30 (0 generated)
I did manual testing on this; - dropping links and non-links - following the different user choices (Okay, Cancel, Close, Loss of Focus) - ensured that the preference change was good and was synced As discussed on the bug entry, I created a confirmation dialog and used the Confirm Bubble so that it would not be modal. I am not a committer so I can't send this to Try.
We shouldn't ask for permission, we should ask for forgiveness. Dropping should set the homepage and allow you to undo that if you don't like it.
I have made the suggested change. The bubble now looks like so: [Icon] Home page [x] Your home page has been set. [Undo] I did experiment with including a link after the second row, but imo it did not add useful functionality. But I have a couple screenshots if you are interested in looking at them.
Yes, please (always) post screenshots to the bug in question.
Patch Set 3: Implements a simple confirmation bubble with the option to Undo as suggested in the issue discussion.
https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:155: public views::ButtonListener, Nit: All part declarations should be aligned https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:160: bool undo_value_is_ntp, Nit: All args should be aligned https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:161: const GURL &undo_url, Nit: & binds to type, not name https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:166: protected: Nit: Why protected? No one subclasses this class... this should probably be private. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:178: // views::ButtonListener overrides: Nit: I've been trying to standardize on just "// views::ButtonListener:" https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:185: // views::WidgetDelegate method. Nit: You don't directly subclass WidgetDelegate. Name this based on your direct parent, and declare in the same order you declared the parent classes above. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:188: static HomePageUndoBubble* sethomeundo_bubble_; Nit: No run-together words in variable names. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:190: enum HomePageUndoBubbleLinkIds { Nit: enums get declared atop the section... though in this case I'd omit the enum entirely, not tag the link, and not check the ID in LinkClicked(). Future modifiers can do that if we ever add another link. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:220: sethomeundo_bubble_->GetWidget()->Close(); Nit; Indent 2, not 3 https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:229: : BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_LEFT), Nit: Indent 4, not 8 https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:246: // Create two columns for the message and the undo link Nit: Trailing period https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:248: cs = layout->AddColumnSet(1); Make sure you test all this in RTL (try starting with --lang=he) and verify that the undo link is to the left of the label. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:249: cs->AddColumn(views::GridLayout::LEADING, views::GridLayout::CENTER, 0, I don't think you want CENTER vertical alignment for these. In that case, if the label and the link don't have the same height, they'll be offset in weird ways. I think you want BASELINE. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:258: message_label->SetMultiLine(true); If you're using a multiline label, you need to give the label a particular max width. I think maybe you don't want a multiline label here anyway, since the undo link will look weird in that case? https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:268: layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing); Why this? https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:271: void HomePageUndoBubble::ButtonPressed(views::Button* sender, Why do we need to override this function at all? https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:282: GetWidget()->Close(); Nit: Perhaps this should call HideBubble() instead so all closes go through the same codepath. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:288: // because we'll be destroyed asynchronously and the shown state will Nit: Extra space. Also "will" -> "could", I believe -- you might want to fill out the scenario where this gets checked. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:290: DCHECK_EQ(sethomeundo_bubble_, this); Nit: (expected, actual) https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:318: if (event.source_operations() & ui::DragDropTypes::DRAG_LINK) { Nit: {} not needed. You could also use ?: to make things even shorter. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:329: PrefService* preferences = browser_->profile()->GetPrefs(); Nit: |prefs| is more common a name (and might avoid some line wrapping) https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:330: Nit: Unnecessary newline https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:331: const bool old_is_ntp = preferences->GetBoolean( Nit: While I personally like them, Chrome code generally does not use const on locals. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar_view.h (right): https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.h:34: class HomeImageButton : public views::ImageButton { Nit: We should declare this in its own header, like we do with e.g. the reload button.
https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:268: layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing); On 2013/01/17 23:48:42, Peter Kasting wrote: > Why this? Oh, I see you have one at top too. I'd remove both of these. They make the bubble look weirdly tall and padded in your screenshot.
https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:155: public views::ButtonListener, On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: All part declarations should be aligned Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:160: bool undo_value_is_ntp, On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: All args should be aligned Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:161: const GURL &undo_url, On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: & binds to type, not name Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:166: protected: On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: Why protected? No one subclasses this class... this should probably be > private. Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:178: // views::ButtonListener overrides: On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: I've been trying to standardize on just "// views::ButtonListener:" Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:185: // views::WidgetDelegate method. On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: You don't directly subclass WidgetDelegate. Name this based on your direct > parent, and declare in the same order you declared the parent classes above. Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:188: static HomePageUndoBubble* sethomeundo_bubble_; On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: No run-together words in variable names. Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:190: enum HomePageUndoBubbleLinkIds { On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: enums get declared atop the section... though in this case I'd omit the > enum entirely, not tag the link, and not check the ID in LinkClicked(). Future Removed enum. Done. > modifiers can do that if we ever add another link. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:197: GURL new_home_page_; new_home_page_ was not used as we removed the hyperlink from the bubble. I removed the variable and the unnecessary parameters to the constructor. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:220: sethomeundo_bubble_->GetWidget()->Close(); On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit; Indent 2, not 3 Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:229: : BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_LEFT), On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: Indent 4, not 8 Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:246: // Create two columns for the message and the undo link On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: Trailing period Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:248: cs = layout->AddColumnSet(1); On 2013/01/17 23:48:42, Peter Kasting wrote: > Make sure you test all this in RTL (try starting with --lang=he) and verify that > the undo link is to the left of the label. Verified that is works. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:249: cs->AddColumn(views::GridLayout::LEADING, views::GridLayout::CENTER, 0, On 2013/01/17 23:48:42, Peter Kasting wrote: > I don't think you want CENTER vertical alignment for these. In that case, if > the label and the link don't have the same height, they'll be offset in weird > ways. I think you want BASELINE. Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:258: message_label->SetMultiLine(true); On 2013/01/17 23:48:42, Peter Kasting wrote: > If you're using a multiline label, you need to give the label a particular max > width. I think maybe you don't want a multiline label here anyway, since the > undo link will look weird in that case? This was remnants of an alternative approach. Fixed/Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:268: layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing); On 2013/01/17 23:49:46, Peter Kasting wrote: > On 2013/01/17 23:48:42, Peter Kasting wrote: > > Why this? > > Oh, I see you have one at top too. > > I'd remove both of these. They make the bubble look weirdly tall and padded in > your screenshot. I thought it needed some padding. Without the padding it looked 'anemic'. I have removed the padding. Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:271: void HomePageUndoBubble::ButtonPressed(views::Button* sender, On 2013/01/17 23:48:42, Peter Kasting wrote: > Why do we need to override this function at all? We do not, removed. Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:282: GetWidget()->Close(); On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: Perhaps this should call HideBubble() instead so all closes go through the > same codepath. Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:288: // because we'll be destroyed asynchronously and the shown state will On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: Extra space. Also "will" -> "could", I believe -- you might want to fill > out the scenario where this gets checked. This is a comment I copied from another singleton bubble. The "and the shown state will be checked before then." is not actually applicable in this class. But I have left the first part of the comment as it wouldn't be obvious to a 'reader'. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:290: DCHECK_EQ(sethomeundo_bubble_, this); On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: (expected, actual) Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:318: if (event.source_operations() & ui::DragDropTypes::DRAG_LINK) { On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: {} not needed. You could also use ?: to make things even shorter. Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:329: PrefService* preferences = browser_->profile()->GetPrefs(); On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: |prefs| is more common a name (and might avoid some line wrapping) Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:330: On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: Unnecessary newline Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:331: const bool old_is_ntp = preferences->GetBoolean( On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: While I personally like them, Chrome code generally does not use const on > locals. Done. https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar_view.h (right): https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.h:34: class HomeImageButton : public views::ImageButton { On 2013/01/17 23:48:42, Peter Kasting wrote: > Nit: We should declare this in its own header, like we do with e.g. the reload > button. Done.
Sorry about the extra patches and spam. I was trying to resolve the errors reported here and not in cpplint with Patch 5. - Missing newline at the end of the 2 new files. - Missing include of <set> And then when uploading Patch 6 I got an error "Base files missing" and to try again, hence Patch #7 Is there any way to run the lint that runs here before I upload? In any case, the patch is ready for review.
LGTM, but make sure you fix all instances of nits I point out. For example, the "'&' goes on type" nit applies in several places still. https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... File chrome/browser/ui/views/home_button.cc (right): https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:22: namespace { Nit: When there are multiple classes in a file, I tend to separate them with comment lines as follows: ...<previous file contents above here>... <blank line> <blank line> // Classname --------------- <out to 79 cols> <blank line> ...<class contents>... Repeat this for every distinct class. When one class is in an anonymous namespace I usually go ahead and treat the namespace opener and closer as "part of the class" too for this purpose. None of this is required, I just find it helps make the different classes stand out. https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:23: class HomePageUndoBubble : public views::BubbleDelegateView, Nit: Go ahead and add a blank line here. https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:34: bool undo_value_is_ntp, Nit: Args should be aligned https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:35: const GURL &undo_url, Nit: '&' and '*' go on type name, not variable name https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:39: // views::BubbleDelegateView methods. Nit: " methods." -> ":" for consistency with below https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:115: browser_->profile()->GetPrefs()->SetBoolean(prefs::kHomePageIsNewTabPage, Nit: I'd probably pull this out into a temp as you were doing before, since GetPrefs() is camel-case and thus we assume not cheap https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:124: // because we'll be destroyed asynchronously. Nit: OK, but why? (I think I know why -- so that triggering the bubble show while the bubble is in the midst of closing doesn't break things -- but you should be explicit. Oh, and don't just copy that and say "break things" either, flesh it out :) ) https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:142: int* formats, Nit: Indent 4, not 8 https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:155: ui::DragDropTypes::DRAG_NONE; Nit: Welcome to put this on previous line if it fits. https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... File chrome/browser/ui/views/home_button.h (right): https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.h:28: }; Nit: DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.h:31: Extra newline. I know you added this because Rietveld's lint complained before. Ignore it. As long as your local file has an LF as the last character, you're fine. Lint often spits out warnings about this that are totally wrong. (2 files)
https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... File chrome/browser/ui/views/home_button.cc (right): https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:22: namespace { On 2013/01/18 18:19:27, Peter Kasting wrote: > Nit: When there are multiple classes in a file, I tend to separate them with > comment lines as follows: > > ...<previous file contents above here>... > <blank line> > <blank line> > // Classname --------------- <out to 79 cols> > <blank line> > ...<class contents>... > > Repeat this for every distinct class. > > When one class is in an anonymous namespace I usually go ahead and treat the > namespace opener and closer as "part of the class" too for this purpose. > > None of this is required, I just find it helps make the different classes stand > out. Done. https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:23: class HomePageUndoBubble : public views::BubbleDelegateView, On 2013/01/18 18:19:27, Peter Kasting wrote: > Nit: Go ahead and add a blank line here. Done. https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:34: bool undo_value_is_ntp, On 2013/01/18 18:19:27, Peter Kasting wrote: > Nit: Args should be aligned I reread the style guides and have a better understanding. Question: The examples imply this but it isn't explicit. Is this a guideline: If you have to split parameters on multiple lines dedicate a line per parameter. Avoid: home_page_undo_bubble_ = new HomePageUndoBubble(browser, undo_value_is_ntp, undo_url, anchor_view); Prefer: home_page_undo_bubble_ = new HomePageUndoBubble(browser, undo_value_is_ntp, undo_url, anchor_view); (hopefully the spaces are respected in comments) Is there any difference between function calls and definitions? My guess is that this is not enforced because I have seen a both styles in the code. Thank you for the detailed review. My next CL to have *significantly* less style issues (hopefully none :) ). Done. https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:35: const GURL &undo_url, On 2013/01/18 18:19:27, Peter Kasting wrote: > Nit: '&' and '*' go on type name, not variable name Done. https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:39: // views::BubbleDelegateView methods. On 2013/01/18 18:19:27, Peter Kasting wrote: > Nit: " methods." -> ":" for consistency with below Done. https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:115: browser_->profile()->GetPrefs()->SetBoolean(prefs::kHomePageIsNewTabPage, On 2013/01/18 18:19:27, Peter Kasting wrote: > Nit: I'd probably pull this out into a temp as you were doing before, since > GetPrefs() is camel-case and thus we assume not cheap Done. https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:124: // because we'll be destroyed asynchronously. On 2013/01/18 18:19:27, Peter Kasting wrote: > Nit: OK, but why? (I think I know why -- so that triggering the bubble show > while the bubble is in the midst of closing doesn't break things -- but you > should be explicit. Oh, and don't just copy that and say "break things" either, > flesh it out :) ) After closer inspection the only thing that would happen is that the widget's Close() would be called twice. The widget itself guards against this situation so in actuality nothing "bad" would happen. However, I presume that multiple calls to the widget's Close() is breaking a contract with that class. I hope I have a good understanding of the situation and I have been able to communicate this adequately with the change to the comment. Done. https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:142: int* formats, On 2013/01/18 18:19:27, Peter Kasting wrote: > Nit: Indent 4, not 8 Done. https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:155: ui::DragDropTypes::DRAG_NONE; On 2013/01/18 18:19:27, Peter Kasting wrote: > Nit: Welcome to put this on previous line if it fits. Done. https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... File chrome/browser/ui/views/home_button.h (right): https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.h:28: }; On 2013/01/18 18:19:27, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.h:31: On 2013/01/18 18:19:27, Peter Kasting wrote: > Extra newline. I know you added this because Rietveld's lint complained before. > Ignore it. As long as your local file has an LF as the last character, you're > fine. Lint often spits out warnings about this that are totally wrong. > > (2 files) Done.
LGTM (again) https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... File chrome/browser/ui/views/home_button.cc (right): https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:34: bool undo_value_is_ntp, The only rule is, if you use multiple lines, the beginnings of all the lines should all be aligned. There is no rule about one-vs.-many args per line, nor about definitions being distinct from calls. We used to have a team style rule about not using multiple args per line at declarations/definitions (but not calls), but this was removed. I still tend to encourage people to use one arg per line at declarations/definitions for clarity. At callsites I don't really care. So my personal preference would be one arg per line here, but it's not mandatory. https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... File chrome/browser/ui/views/home_button.cc (right): https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:119: Nit: Newline probably not needed https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:127: // We have to reset |home_page_undo_bubble_| here, not in our destructor, Nit: How about this for clarity: We have to reset |home_page_undo_bubble_| here, not in our destructor, because we'll be hidden first, then destroyed asynchronously. If we wait to reset this, and the user triggers a call to ShowBubble() while the window is hidden but not destroyed, GetWidget()->Close() would be called twice.
Oh also! Since this is views-only, we need a bug or patches for the other platforms too, at least Mac. (We might be able to punt GTK since GTK is eventually moving to views.) You may be able to rope in a Mac person to doing that work...
I tried reaching out to see if a Mac contributor would assist. I have a little Mac development experience but my Mac is showing its age and is not able to to even compile the project. I will post an issue for the Mac implementation. Note, I do not have committer rights. I think I was a bit overzealous to work on this issue.
Note: I removed this "Note: I do not have committer rights." from the CL description. This kind of information doesn't to belong to our commit messages, it's just an information that you can say to reviewer, so he knows he has to land that for you and you can send that as a message here in rietveld.
https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... File chrome/browser/ui/views/home_button.cc (right): https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:73: if (home_page_undo_bubble_ != NULL) if (home_page_undo_bubble_) https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:118: PrefService* prefs = browser_->profile()->GetPrefs(); Better: PrefServiceBase* prefs = PrefServiceBase::FromBrowserContext(browser_->profile()); https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:143: views::ButtonListener* listener, I bet this will fit above! https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... File chrome/browser/ui/views/home_button.h (right): https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: new files are Copyright 2013. see crbug.com/140977 https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_HOME_BUTTON_H__ nit: CHROME_BROWSER_UI_VIEWS_HOME_BUTTON_H_ https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.h:8: #include <set> no need to include that. it's part of image_button.h interface. https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.h:19: virtual bool GetDropFormats( // views::ImageButton: as Peter prefers ;)
https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar_view.h (right): https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.h:16: #include "chrome/browser/ui/views/home_button.h" Don't include this here. Just forward declare instead: class HomeImageButton;
https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... File chrome/browser/ui/views/home_button.cc (right): https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:73: if (home_page_undo_bubble_ != NULL) On 2013/01/28 02:37:11, tfarina wrote: > if (home_page_undo_bubble_) Done. https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:118: PrefService* prefs = browser_->profile()->GetPrefs(); On 2013/01/28 02:37:11, tfarina wrote: > Better: > > PrefServiceBase* prefs = > PrefServiceBase::FromBrowserContext(browser_->profile()); Done. https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:119: On 2013/01/18 21:45:35, Peter Kasting wrote: > Nit: Newline probably not needed Done. https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:127: // We have to reset |home_page_undo_bubble_| here, not in our destructor, On 2013/01/18 21:45:35, Peter Kasting wrote: > Nit: How about this for clarity: > > We have to reset |home_page_undo_bubble_| here, not in our destructor, because > we'll be hidden first, then destroyed asynchronously. If we wait to reset this, > and the user triggers a call to ShowBubble() while the window is hidden but not > destroyed, GetWidget()->Close() would be called twice. Done. https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... File chrome/browser/ui/views/home_button.h (right): https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/01/28 02:37:11, tfarina wrote: > nit: new files are Copyright 2013. > see crbug.com/140977 Done. https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_HOME_BUTTON_H__ On 2013/01/28 02:37:11, tfarina wrote: > nit: CHROME_BROWSER_UI_VIEWS_HOME_BUTTON_H_ Done. https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.h:8: #include <set> On 2013/01/28 02:37:11, tfarina wrote: > no need to include that. it's part of image_button.h interface. This was specifically added to address the automated warning "Include what you use". https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.h:19: virtual bool GetDropFormats( On 2013/01/28 02:37:11, tfarina wrote: > // views::ImageButton: > > as Peter prefers ;) Done. https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar_view.h (right): https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.h:16: #include "chrome/browser/ui/views/home_button.h" On 2013/01/28 02:38:13, tfarina wrote: > Don't include this here. > > Just forward declare instead: > > class HomeImageButton; Done.
On 2013/01/28 02:28:58, Tom Cassiotis wrote: > I think I was a bit overzealous to work on this issue. I think you did a great job! I'd be happy to see further work from you.
Peter, I think he needs you to check the commit box to land this patch for him.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/11742003/38001
Presubmit check for 11742003-38001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
Presubmit checks took 1.5s to calculate.
Ben, you want to sign off on this as chrome OWNER and official UI behavior decider?
seems reasonable. lgtm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/11742003/38001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_aura. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
https://chromiumcodereview.appspot.com/11742003/diff/38001/chrome/browser/ui/... File chrome/browser/ui/views/home_button.cc (right): https://chromiumcodereview.appspot.com/11742003/diff/38001/chrome/browser/ui/... chrome/browser/ui/views/home_button.cc:92: ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance(); Linux Aura builder says this line is unused, remove it.
https://codereview.chromium.org/11742003/diff/38001/chrome/browser/ui/views/h... File chrome/browser/ui/views/home_button.cc (right): https://codereview.chromium.org/11742003/diff/38001/chrome/browser/ui/views/h... chrome/browser/ui/views/home_button.cc:92: ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance(); On 2013/01/30 00:08:35, Peter Kasting wrote: > Linux Aura builder says this line is unused, remove it. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/11742003/46002
Message was sent while issue was closed.
Change committed as 179526 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
