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

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:
1 year, 3 months ago by Tom Cassiotis
Modified:
1 year, 2 months ago
CC:
chromium-reviews_chromium.org, 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) Lint Patch
M chrome/app/generated_resources.grd View 1 chunk +3 lines, -0 lines 0 comments 0 errors 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 1 errors 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 1 errors 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 0 errors 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 0 errors Download
M chrome/chrome_browser_ui.gypi View 1 2 3 1 chunk +2 lines, -0 lines 0 comments 0 errors Download
Commit:

Messages

Total messages: 30
Tom Cassiotis
I did manual testing on this; - dropping links and non-links - following the different ...
1 year, 3 months ago #1
Peter Kasting
We shouldn't ask for permission, we should ask for forgiveness. Dropping should set the homepage ...
1 year, 3 months ago #2
Tom Cassiotis
I have made the suggested change. The bubble now looks like so: [Icon] Home page ...
1 year, 3 months ago #3
Peter Kasting
Yes, please (always) post screenshots to the bug in question.
1 year, 3 months ago #4
Tom Cassiotis
Patch Set 3: Implements a simple confirmation bubble with the option to Undo as suggested ...
1 year, 3 months ago #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 ...
1 year, 3 months ago #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: > ...
1 year, 3 months ago #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: > ...
1 year, 2 months ago #8
Tom Cassiotis
Sorry about the extra patches and spam. I was trying to resolve the errors reported ...
1 year, 2 months ago #9
Peter Kasting
LGTM, but make sure you fix all instances of nits I point out. For example, ...
1 year, 2 months ago #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: > ...
1 year, 2 months ago #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 ...
1 year, 2 months ago #12
Peter Kasting
Oh also! Since this is views-only, we need a bug or patches for the other ...
1 year, 2 months ago #13
Tom Cassiotis
I tried reaching out to see if a Mac contributor would assist. I have a ...
1 year, 2 months ago #14
tfarina
Note: I removed this "Note: I do not have committer rights." from the CL description. ...
1 year, 2 months ago #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* ...
1 year, 2 months ago #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 ...
1 year, 2 months ago #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: ...
1 year, 2 months ago #18
Peter Kasting
On 2013/01/28 02:28:58, Tom Cassiotis wrote: > I think I was a bit overzealous to ...
1 year, 2 months ago #19
tfarina
Peter, I think he needs you to check the commit box to land this patch ...
1 year, 2 months ago #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
1 year, 2 months ago #21
I haz the power (commit-bot)
Presubmit check for 11742003-38001 failed and returned exit status 1. Running presubmit commit checks ...
1 year, 2 months ago #22
Peter Kasting
Ben, you want to sign off on this as chrome OWNER and official UI behavior ...
1 year, 2 months ago #23
Ben Goodger (Google)
seems reasonable. lgtm.
1 year, 2 months ago #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
1 year, 2 months ago #25
I haz the power (commit-bot)
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
1 year, 2 months ago #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 ...
1 year, 2 months ago #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 ...
1 year, 2 months ago #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
1 year, 2 months ago #29
I haz the power (commit-bot)
1 year, 2 months ago #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 1280:2d3e6564b7b6