Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(503)

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.

Description

Drag 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 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+222 lines, -2 lines) Patch
M chrome/app/generated_resources.grd View 1 chunk +3 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/home_button.h View 1 2 3 4 5 6 7 8 1 chunk +33 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/home_button.cc View 1 2 3 4 5 6 7 8 9 1 chunk +180 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/toolbar_view.h View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/views/toolbar_view.cc View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -1 line 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 30 (0 generated)
Tom Cassiotis
I did manual testing on this; - dropping links and non-links - following the different ...
7 years, 11 months ago (2013-01-02 16:24:37 UTC) #1
Peter Kasting
We shouldn't ask for permission, we should ask for forgiveness. Dropping should set the homepage ...
7 years, 11 months ago (2013-01-02 18:41:30 UTC) #2
Tom Cassiotis
I have made the suggested change. The bubble now looks like so: [Icon] Home page ...
7 years, 11 months ago (2013-01-03 18:10:09 UTC) #3
Peter Kasting
Yes, please (always) post screenshots to the bug in question.
7 years, 11 months ago (2013-01-03 18:22:20 UTC) #4
Tom Cassiotis
Patch Set 3: Implements a simple confirmation bubble with the option to Undo as suggested ...
7 years, 11 months ago (2013-01-17 17:55:47 UTC) #5
Peter Kasting
https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/toolbar_view.cc File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/toolbar_view.cc#newcode155 chrome/browser/ui/views/toolbar_view.cc:155: public views::ButtonListener, Nit: All part declarations should be aligned ...
7 years, 11 months ago (2013-01-17 23:48:42 UTC) #6
Peter Kasting
https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/toolbar_view.cc File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/toolbar_view.cc#newcode268 chrome/browser/ui/views/toolbar_view.cc:268: layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing); On 2013/01/17 23:48:42, Peter Kasting wrote: > ...
7 years, 11 months ago (2013-01-17 23:49:46 UTC) #7
Tom Cassiotis
https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/toolbar_view.cc File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11742003/diff/12001/chrome/browser/ui/views/toolbar_view.cc#newcode155 chrome/browser/ui/views/toolbar_view.cc:155: public views::ButtonListener, On 2013/01/17 23:48:42, Peter Kasting wrote: > ...
7 years, 11 months ago (2013-01-18 15:18:49 UTC) #8
Tom Cassiotis
Sorry about the extra patches and spam. I was trying to resolve the errors reported ...
7 years, 11 months ago (2013-01-18 17:08:05 UTC) #9
Peter Kasting
LGTM, but make sure you fix all instances of nits I point out. For example, ...
7 years, 11 months ago (2013-01-18 18:19:27 UTC) #10
Tom Cassiotis
https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/home_button.cc File chrome/browser/ui/views/home_button.cc (right): https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/home_button.cc#newcode22 chrome/browser/ui/views/home_button.cc:22: namespace { On 2013/01/18 18:19:27, Peter Kasting wrote: > ...
7 years, 11 months ago (2013-01-18 21:24:19 UTC) #11
Peter Kasting
LGTM (again) https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/home_button.cc File chrome/browser/ui/views/home_button.cc (right): https://codereview.chromium.org/11742003/diff/26003/chrome/browser/ui/views/home_button.cc#newcode34 chrome/browser/ui/views/home_button.cc:34: bool undo_value_is_ntp, The only rule is, if ...
7 years, 11 months ago (2013-01-18 21:45:34 UTC) #12
Peter Kasting
Oh also! Since this is views-only, we need a bug or patches for the other ...
7 years, 11 months ago (2013-01-21 05:29:41 UTC) #13
Tom Cassiotis
I tried reaching out to see if a Mac contributor would assist. I have a ...
7 years, 10 months ago (2013-01-28 02:28:58 UTC) #14
tfarina
Note: I removed this "Note: I do not have committer rights." from the CL description. ...
7 years, 10 months ago (2013-01-28 02:32:17 UTC) #15
tfarina
https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/home_button.cc File chrome/browser/ui/views/home_button.cc (right): https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/home_button.cc#newcode73 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/home_button.cc#newcode118 chrome/browser/ui/views/home_button.cc:118: PrefService* ...
7 years, 10 months ago (2013-01-28 02:37:10 UTC) #16
tfarina
https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/toolbar_view.h File chrome/browser/ui/views/toolbar_view.h (right): https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/toolbar_view.h#newcode16 chrome/browser/ui/views/toolbar_view.h:16: #include "chrome/browser/ui/views/home_button.h" Don't include this here. Just forward declare ...
7 years, 10 months ago (2013-01-28 02:38:13 UTC) #17
Tom Cassiotis
https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/home_button.cc File chrome/browser/ui/views/home_button.cc (right): https://codereview.chromium.org/11742003/diff/18012/chrome/browser/ui/views/home_button.cc#newcode73 chrome/browser/ui/views/home_button.cc:73: if (home_page_undo_bubble_ != NULL) On 2013/01/28 02:37:11, tfarina wrote: ...
7 years, 10 months ago (2013-01-28 15:34:33 UTC) #18
Peter Kasting
On 2013/01/28 02:28:58, Tom Cassiotis wrote: > I think I was a bit overzealous to ...
7 years, 10 months ago (2013-01-28 22:39:12 UTC) #19
tfarina
Peter, I think he needs you to check the commit box to land this patch ...
7 years, 10 months ago (2013-01-28 22:42:17 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/11742003/38001
7 years, 10 months ago (2013-01-28 22:45:30 UTC) #21
commit-bot: I haz the power
Presubmit check for 11742003-38001 failed and returned exit status 1. Running presubmit commit checks ...
7 years, 10 months ago (2013-01-28 22:45:33 UTC) #22
Peter Kasting
Ben, you want to sign off on this as chrome OWNER and official UI behavior ...
7 years, 10 months ago (2013-01-28 22:53:29 UTC) #23
Ben Goodger (Google)
seems reasonable. lgtm.
7 years, 10 months ago (2013-01-29 22:25:48 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/11742003/38001
7 years, 10 months ago (2013-01-29 22:38:42 UTC) #25
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 10 months ago (2013-01-29 23:46:51 UTC) #26
Peter Kasting
https://chromiumcodereview.appspot.com/11742003/diff/38001/chrome/browser/ui/views/home_button.cc File chrome/browser/ui/views/home_button.cc (right): https://chromiumcodereview.appspot.com/11742003/diff/38001/chrome/browser/ui/views/home_button.cc#newcode92 chrome/browser/ui/views/home_button.cc:92: ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance(); Linux Aura builder says this ...
7 years, 10 months ago (2013-01-30 00:08:35 UTC) #27
Tom Cassiotis
https://codereview.chromium.org/11742003/diff/38001/chrome/browser/ui/views/home_button.cc File chrome/browser/ui/views/home_button.cc (right): https://codereview.chromium.org/11742003/diff/38001/chrome/browser/ui/views/home_button.cc#newcode92 chrome/browser/ui/views/home_button.cc:92: ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance(); On 2013/01/30 00:08:35, Peter Kasting ...
7 years, 10 months ago (2013-01-30 00:18:38 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/11742003/46002
7 years, 10 months ago (2013-01-30 00:30:28 UTC) #29
commit-bot: I haz the power
7 years, 10 months ago (2013-01-30 04:55:40 UTC) #30
Message was sent while issue was closed.
Change committed as 179526

Powered by Google App Engine
This is Rietveld 408576698