Chromium Code Reviews
Help | Chromium Project | Sign in
(16)

Issue 11742003: Implemented drop hander for the Home toolbar button that accepts (only) links (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 1 month ago by Tom Cassiotis
Modified:
2 years, 1 month 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 : #

Messages

Total messages: 30 (0 generated)
Tom Cassiotis
I did manual testing on this; - dropping links and non-links - following the different ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month ago (2013-01-03 18:10:09 UTC) #3
Peter Kasting
Yes, please (always) post screenshots to the bug in question.
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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: > ...
2 years, 1 month 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: > ...
2 years, 1 month 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 ...
2 years, 1 month 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, ...
2 years, 1 month 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: > ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month 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. ...
2 years, 1 month 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* ...
2 years, 1 month 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 ...
2 years, 1 month 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: ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month ago (2013-01-28 22:42:17 UTC) #20
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/11742003/38001
2 years, 1 month ago (2013-01-28 22:45:30 UTC) #21
I haz the power (commit-bot)
Presubmit check for 11742003-38001 failed and returned exit status 1. Running presubmit commit checks ...
2 years, 1 month 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 ...
2 years, 1 month ago (2013-01-28 22:53:29 UTC) #23
Ben Goodger (Google)
seems reasonable. lgtm.
2 years, 1 month ago (2013-01-29 22:25:48 UTC) #24
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/11742003/38001
2 years, 1 month ago (2013-01-29 22:38:42 UTC) #25
I haz the power (commit-bot)
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
2 years, 1 month 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 ...
2 years, 1 month 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 ...
2 years, 1 month ago (2013-01-30 00:18:38 UTC) #28
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tom.cassiotis@gmail.com/11742003/46002
2 years, 1 month ago (2013-01-30 00:30:28 UTC) #29
I haz the power (commit-bot)
2 years, 1 month ago (2013-01-30 04:55:40 UTC) #30
Message was sent while issue was closed.
Change committed as 179526
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld dd99357-tainted