Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12310109:
Add a shortcut to open the Apps page from the bookmark bar. (Closed)
|
Created:
7 years, 10 months ago by MAD Modified:
7 years, 9 months ago CC:
chromium-reviews, browser-components-watch_chromium.org, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a shortcut to open the Apps page from the bookmark bar.
Moved over to: https://codereview.chromium.org/12386088/
BUG=177377
TEST=Make sure that an app shortcut is available in the bookmark bar when instant extended is enabled, and that this button can be hidden/shown from the bookmark bar context menu.
Patch Set 1 : #
Total comments: 16
Patch Set 2 : First round of CR comments. #Patch Set 3 : pre-sync #Patch Set 4 : Sync'd to ToT. #Patch Set 5 : Addressed CR comments. #Patch Set 6 : Adding the missing test file... #
Total comments: 20
Patch Set 7 : Other CR comments addressed. #
Total comments: 8
Patch Set 8 : OWNERS review comments addressed. #Patch Set 9 : Sync'd to ToT. #Patch Set 10 : Changed the icon to match the new favicon for the apps page. #Patch Set 11 : Sync'd to ToT. #
Total comments: 11
Messages
Total messages: 32 (0 generated)
Can you please pre-OWNER review this? I'm probably going to ask you to implement the Mac version afterward... :-) Thanks! BYE MAD
Can you write a basic test for this - e.g. just testing that the criteria to make the button visible/not is working right? https://codereview.chromium.org/12310109/diff/2001/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/12310109/diff/2001/chrome/app/generated_resou... chrome/app/generated_resources.grd:5883: + <message name="IDS_POLICY_VALIDATION_BAD_INITIAL_SIGNATURE" desc="Message indicating a bad signature on policy validation using the initial key."> I don't think you meant to have all these changes in your diff? https://codereview.chromium.org/12310109/diff/2001/chrome/app/generated_resou... chrome/app/generated_resources.grd:9297: + <message name="IDS_BOOKMARK_BAR_SHOW_APPS_SHORTCUT" desc="In Title Case: Name shown in the context menu to hide/show the apps shortcut in the bookmakr bar"> This one isn't In Title Case. https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:274: class ShortcutButton : public views::TextButton { Should this implement GetTooltipText()? https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:283: if (bookmark_utils::IsBookmarkBarViewAnimationsDisabled()) { It seems most of this logic is identical to BookmarkButton. What do you think of adding a common parent class (BookmarkButtonBase?) to avoid the code duplication? https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right): https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:359: bool ShouldShowAppsShortcut(); Can this be const? https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:364: PrefChangeRegistrar profile_pref_registrar_; Comment mentioning which pref this is for. https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc (right): https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc:150: PrefService* prefs = PrefServiceFromBrowserContext(profile_); Shouldn't this use profile_->GetPrefs() like elsewhere in this file? https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc:202: prefs::kShowAppsShortcutInBookmarkBar); What calls this? I am wondering if this should be returning false if InstantEnabled is off. Or is it not an issue when the item wasn't added in that case? (If so, perhaps add a dcheck that instantextended isn't on.)
Addressed most of the comments... Still need to write the test and resolve the code reuse issue... Thanks! BYE MAD https://codereview.chromium.org/12310109/diff/2001/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/12310109/diff/2001/chrome/app/generated_resou... chrome/app/generated_resources.grd:5883: + <message name="IDS_POLICY_VALIDATION_BAD_INITIAL_SIGNATURE" desc="Message indicating a bad signature on policy validation using the initial key."> On 2013/02/25 21:13:29, Alexei Svitkine wrote: > I don't think you meant to have all these changes in your diff? Arg... I thought I resolved these... Some git mistakes messed that up... Now I did... https://codereview.chromium.org/12310109/diff/2001/chrome/app/generated_resou... chrome/app/generated_resources.grd:9297: + <message name="IDS_BOOKMARK_BAR_SHOW_APPS_SHORTCUT" desc="In Title Case: Name shown in the context menu to hide/show the apps shortcut in the bookmakr bar"> On 2013/02/25 21:13:29, Alexei Svitkine wrote: > This one isn't In Title Case. D'Ho!ne. :-) https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:274: class ShortcutButton : public views::TextButton { On 2013/02/25 21:13:29, Alexei Svitkine wrote: > Should this implement GetTooltipText()? No need for tooltip, the name of the shortcut is all we need. https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:283: if (bookmark_utils::IsBookmarkBarViewAnimationsDisabled()) { On 2013/02/25 21:13:29, Alexei Svitkine wrote: > It seems most of this logic is identical to BookmarkButton. > > What do you think of adding a common parent class (BookmarkButtonBase?) to avoid > the code duplication? I thought about it, but I recently got slapped on the wrist for using inheritance to share code like this... And I'm not sure we would gain much by using composition here... Though maybe this case is not quite the same as the I got slapped for... I'll think about it some more... https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right): https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:359: bool ShouldShowAppsShortcut(); On 2013/02/25 21:13:29, Alexei Svitkine wrote: > Can this be const? Done. https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:364: PrefChangeRegistrar profile_pref_registrar_; On 2013/02/25 21:13:29, Alexei Svitkine wrote: > Comment mentioning which pref this is for. Done. https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc (right): https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc:150: PrefService* prefs = PrefServiceFromBrowserContext(profile_); On 2013/02/25 21:13:29, Alexei Svitkine wrote: > Shouldn't this use profile_->GetPrefs() like elsewhere in this file? Done. https://codereview.chromium.org/12310109/diff/2001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc:202: prefs::kShowAppsShortcutInBookmarkBar); On 2013/02/25 21:13:29, Alexei Svitkine wrote: > What calls this? I am wondering if this should be returning false if > InstantEnabled is off. Or is it not an issue when the item wasn't added in that > case? (If so, perhaps add a dcheck that instantextended isn't on.) Done.
One other thing, can you add a histogram and/or count to see how often this gets triggered?
On 2013/02/25 22:00:00, Alexei Svitkine wrote: > One other thing, can you add a histogram and/or count to see how often this gets > triggered? ho, yes, I had one initially and it got lost in my git mishap... Thanks for reminding me...
OK, added a unit test and finally decided to create a base class for text buttons on the bookmark bar. Let me know if there is anything else that needs to be addressed... Thanks! BYE MAD
You forgot to add bookmark_bar_view_unittest.cc to the CL.
Oopssss.... Sorry about that... I was sure I did a git add, seems like I didn't... I should use git status more... It's there now. On Tue, Feb 26, 2013 at 1:39 PM, <asvitkine@chromium.org> wrote: > You forgot to add bookmark_bar_view_unittest.cc to the CL. > > https://codereview.chromium.**org/12310109/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right): https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. btw we already have bookmark_bar_view_test.cc! Yeah, I plan to rename it to _unittest.cc, but never get around to do it yet. https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:10: #include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h" make this the first include. https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:16: can you wrap in unnamed namespace?
LGTM with a few more nits. https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right): https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:369: bool IsAppsShortcutVisible() const; Nit: Suffix with "ForTesting". https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right): https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:61: EXPECT_TRUE(bookmark_bar_view.IsAppsShortcutVisible()); Nit: Set the bool to false again to make sure it can transition from true -> false as well. https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc (right): https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc:200: // This should only be available when instant extended is enabled. Nit: "be available" -> "be called".
https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right): https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:10: #include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h" On 2013/02/26 19:27:51, tfarina wrote: > make this the first include. No, that's incorrect - against the style guide in fact. Test files aren't special in this regard, you only have a first include if there's a .h file with the same name (e.g. if there was a bookmarks_bar_view_unittest.h). Yes, some tests break that, but we've slowly been moving away from that.
https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right): https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:10: #include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h" On 2013/02/26 19:32:30, Alexei Svitkine wrote: > On 2013/02/26 19:27:51, tfarina wrote: > > make this the first include. > > No, that's incorrect - against the style guide in fact. Test files aren't > special in this regard, you only have a first include if there's a .h file with > the same name (e.g. if there was a bookmarks_bar_view_unittest.h). > > Yes, some tests break that, but we've slowly been moving away from that. No, actually we have been moving the other way around. At least that is the way I'm used and reviewers haven't complained about doing it.
https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right): https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:10: #include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h" On 2013/02/26 19:35:26, tfarina wrote: > https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... > File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right): > > https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... > chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:10: #include > "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h" > On 2013/02/26 19:32:30, Alexei Svitkine wrote: > > On 2013/02/26 19:27:51, tfarina wrote: > > > make this the first include. > > > > No, that's incorrect - against the style guide in fact. Test files aren't > > special in this regard, you only have a first include if there's a .h file > with > > the same name (e.g. if there was a bookmarks_bar_view_unittest.h). > > > > Yes, some tests break that, but we've slowly been moving away from that. > > No, actually we have been moving the other way around. At least that is the way > I'm used and reviewers haven't complained about doing it. Well, all the tests I've reviewed recently didn't use this convention. I haven't seen a discussion about it and as it stands, it's contrary to the style guide. This suggests that it's the wrong direction. (i.e. we want to follow the style guide as close as possible) The reason that we include the corresponding header file first in the impl file is to ensure that the header can compile on its own and doesn't have a hidden dependency against some previous include. (The origin of this came from google3 when they made this transition per this motivation.) Considering that for any test file, there's already a corresponding impl file that does include the header first, there's no reason to mandate this in the test file too, since that's already tested. So I don't see a good reason to diverge from the style guide here.
On Tue, Feb 26, 2013 at 4:48 PM, <asvitkine@chromium.org> wrote: > > https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... > File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc > (right): > > https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... > chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:10: > #include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h" > On 2013/02/26 19:35:26, tfarina wrote: > > https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... >> >> File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc > > (right): > > > https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... >> >> chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:10: > > #include >> >> "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h" >> On 2013/02/26 19:32:30, Alexei Svitkine wrote: >> > On 2013/02/26 19:27:51, tfarina wrote: >> > > make this the first include. >> > >> > No, that's incorrect - against the style guide in fact. Test files > > aren't >> >> > special in this regard, you only have a first include if there's a > > .h file >> >> with >> > the same name (e.g. if there was a bookmarks_bar_view_unittest.h). >> > >> > Yes, some tests break that, but we've slowly been moving away from > > that. > >> No, actually we have been moving the other way around. At least that > > is the way >> >> I'm used and reviewers haven't complained about doing it. > > > Well, all the tests I've reviewed recently didn't use this convention. > > I haven't seen a discussion about it and as it stands, it's contrary to > the style guide. This suggests that it's the wrong direction. (i.e. we > want to follow the style guide as close as possible) > May be we are reading different style guides? Or am I understanding the following wrong? In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or test the stuff in dir2/foo2.h, order your includes as follows: dir2/foo2.h (preferred location — see details below). C system files. C++ system files. Other libraries' .h files. Your project's .h files. With the preferred ordering, if dir2/foo2.h omits any necessary includes, the build of dir/foo.cc or dir/foo_test.cc will break. Thus, this rule ensures that build breaks show up first for the people working on these files, not for innocent people in other packages. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... It cites the same reasons you said here for foo.h/foo.cc -- Thiago
On 2013/02/26 19:54:28, tfarina wrote: > May be we are reading different style guides? Or am I understanding > the following wrong? > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... > > It cites the same reasons you said here for foo.h/foo.cc Interesting, that's not how I remember that section the last time I looked at it. I'm pretty sure it used to only say to do it for "foo.cc" and didn't mention "foo_test.cc". Anyway, you're right - good to know.
How about this? Thanks again! BYE MAD... https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right): https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:369: bool IsAppsShortcutVisible() const; On 2013/02/26 19:29:51, Alexei Svitkine wrote: > Nit: Suffix with "ForTesting". Done. https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right): https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/02/26 19:27:51, tfarina wrote: > no (c) Done. https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/02/26 19:27:51, tfarina wrote: > btw we already have bookmark_bar_view_test.cc! > > Yeah, I plan to rename it to _unittest.cc, but never get around to do it yet. But the other one is in interactive_ui_tests, you want to convert it as a unittest? https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:10: #include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h" On 2013/02/26 19:27:51, tfarina wrote: > make this the first include. Done. https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:16: On 2013/02/26 19:27:51, tfarina wrote: > can you wrap in unnamed namespace? If I do so, I can't set the test as friend of the bookmark bar view and can't access the private method. Would you rather I make the method public? https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:61: EXPECT_TRUE(bookmark_bar_view.IsAppsShortcutVisible()); On 2013/02/26 19:29:51, Alexei Svitkine wrote: > Nit: Set the bool to false again to make sure it can transition from true -> > false as well. Done.
https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right): https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/02/26 20:55:53, MAD wrote: > On 2013/02/26 19:27:51, tfarina wrote: > > btw we already have bookmark_bar_view_test.cc! > > > > Yeah, I plan to rename it to _unittest.cc, but never get around to do it yet. > > But the other one is in interactive_ui_tests, you want to convert it as a > unittest? Ah, in that case I'd rename to _uitest.cc https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:16: On 2013/02/26 20:55:53, MAD wrote: > On 2013/02/26 19:27:51, tfarina wrote: > > can you wrap in unnamed namespace? > > If I do so, I can't set the test as friend of the bookmark bar view and can't > access the private method. Would you rather I make the method public? No, private is better, leave that way so ;)
So do I get your LGTM for this? If so, I'll ask for OWNERS approval now... Thanks! BYE MAD https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right): https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/02/26 21:14:21, tfarina wrote: > On 2013/02/26 20:55:53, MAD wrote: > > On 2013/02/26 19:27:51, tfarina wrote: > > > btw we already have bookmark_bar_view_test.cc! > > > > > > Yeah, I plan to rename it to _unittest.cc, but never get around to do it > yet. > > > > But the other one is in interactive_ui_tests, you want to convert it as a > > unittest? > Ah, in that case I'd rename to _uitest.cc Do you want me to do it in this CL, or you'll do it later?
Actually, might as well ask for OWNERS review now... sky@ are you OK with reviewing this? Would you prefer I ask other owners? You are the intersection of all these folders, but I could ask separate OWNERS if you have too much on your plate... Thanks! BYE MAD
you have my informal LGTM. The bat is with Scott now ;) https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right): https://codereview.chromium.org/12310109/diff/1028/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/02/26 21:20:06, MAD wrote: > On 2013/02/26 21:14:21, tfarina wrote: > > On 2013/02/26 20:55:53, MAD wrote: > > > On 2013/02/26 19:27:51, tfarina wrote: > > > > btw we already have bookmark_bar_view_test.cc! > > > > > > > > Yeah, I plan to rename it to _unittest.cc, but never get around to do it > > yet. > > > > > > But the other one is in interactive_ui_tests, you want to convert it as a > > > unittest? > > Ah, in that case I'd rename to _uitest.cc > > Do you want me to do it in this CL, or you'll do it later? No, leave it to me. I'll consider doing it in another CL.
I spoke with Glen about this too and he voiced some concerns about this. Please bring this up to UI review before proceeding here. https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:660: apps_page_shortcut_->GetPreferredSize(); Shouldn't you check visibility here? https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1084: if (view == other_bookmarked_button_ || view == apps_page_shortcut_) { Why does the app shortcut button show the contents of the other folder? I suspect you never hit this code path since apps_page_shortcut_ is not a MenuButton. https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1628: bool has_other_children = !model_->other_node()->empty(); Don't you need to update this too? https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1782: return chrome::search::IsInstantExtendedAPIEnabled(browser_->profile()) && We should also hide this is I don't have any apps, right?
Comments addressed/answered and email sent to UI leads... Thanks! BYE MAD https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:660: apps_page_shortcut_->GetPreferredSize(); On 2013/02/26 23:56:10, sky wrote: > Shouldn't you check visibility here? Then should I also check the visibility of the other bookmarked button too? https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1084: if (view == other_bookmarked_button_ || view == apps_page_shortcut_) { On 2013/02/26 23:56:10, sky wrote: > Why does the app shortcut button show the contents of the other folder? I > suspect you never hit this code path since apps_page_shortcut_ is not a > MenuButton. D'Ho!ne. https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1628: bool has_other_children = !model_->other_node()->empty(); On 2013/02/26 23:56:10, sky wrote: > Don't you need to update this too? Why? This is for the visibility of the other bookmarks button whether there are other bookmarks or not. This should not affect the visibility of the apps shortcut, shouldn't it? https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1782: return chrome::search::IsInstantExtendedAPIEnabled(browser_->profile()) && On 2013/02/26 23:56:10, sky wrote: > We should also hide this is I don't have any apps, right? Not sure... Even without apps, we still have the chrome webstore link to install apps... I'll ask the PMs/UI leads...
Ping this once the ui folks are happy and I'll take another look. -Scott On Tue, Feb 26, 2013 at 4:55 PM, <mad@chromium.org> wrote: > Comments addressed/answered and email sent to UI leads... > > Thanks! > > BYE > MAD > > > > > https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): > > https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:660: > apps_page_shortcut_->GetPreferredSize(); > On 2013/02/26 23:56:10, sky wrote: >> >> Shouldn't you check visibility here? > > > Then should I also check the visibility of the other bookmarked button > too? > > > https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1084: if (view == > other_bookmarked_button_ || view == apps_page_shortcut_) { > On 2013/02/26 23:56:10, sky wrote: >> >> Why does the app shortcut button show the contents of the other > > folder? I >> >> suspect you never hit this code path since apps_page_shortcut_ is not > > a >> >> MenuButton. > > > D'Ho!ne. > > > https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1628: bool > has_other_children = !model_->other_node()->empty(); > On 2013/02/26 23:56:10, sky wrote: >> >> Don't you need to update this too? > > > Why? This is for the visibility of the other bookmarks button whether > there are other bookmarks or not. This should not affect the visibility > of the apps shortcut, shouldn't it? > > > https://codereview.chromium.org/12310109/diff/25/chrome/browser/ui/views/book... > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1782: return > chrome::search::IsInstantExtendedAPIEnabled(browser_->profile()) && > On 2013/02/26 23:56:10, sky wrote: >> >> We should also hide this is I don't have any apps, right? > > > Not sure... Even without apps, we still have the chrome webstore link to > install apps... I'll ask the PMs/UI leads... > > https://codereview.chromium.org/12310109/
So Glen just gave this a go... Do you have other comments? Thanks! BYE MAD
https://codereview.chromium.org/12310109/diff/18001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/12310109/diff/18001/chrome/app/generated_reso... chrome/app/generated_resources.grd:8896: + Apps nit: indent this and 'Opens apps ...' https://codereview.chromium.org/12310109/diff/18001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_utils.cc (right): https://codereview.chromium.org/12310109/diff/18001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_utils.cc:443: true, Shouldn't the value of this be based on whether I have any apps? If I haven't installed any apps I don't want to see this button. https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1317: button->SetEnabled(true); Enabled is this default. https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1636: bookmarks_separator_view_->SetVisible(has_other_children); Shouldn't the visibility of the separator view be based on whether the apps button is visible too, eg bookmarks_separator_view_->SetVisible(has_other_children || apps_age_shortcut_->is_visible()) ? https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right): https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:369: bool IsAppsShortcutVisibleForTesting() const; Since you're using a friend nuke this and check apps_page_shortcut_ directly.
On 2013/02/28 21:50:31, sky wrote: > https://codereview.chromium.org/12310109/diff/18001/chrome/app/generated_reso... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/12310109/diff/18001/chrome/app/generated_reso... > chrome/app/generated_resources.grd:8896: + Apps > nit: indent this and 'Opens apps ...' > > https://codereview.chromium.org/12310109/diff/18001/chrome/browser/bookmarks/... > File chrome/browser/bookmarks/bookmark_utils.cc (right): > > https://codereview.chromium.org/12310109/diff/18001/chrome/browser/bookmarks/... > chrome/browser/bookmarks/bookmark_utils.cc:443: true, > Shouldn't the value of this be based on whether I have any apps? If I haven't > installed any apps I don't want to see this button. > > https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): > > https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1317: > button->SetEnabled(true); > Enabled is this default. > > https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1636: > bookmarks_separator_view_->SetVisible(has_other_children); > Shouldn't the visibility of the separator view be based on whether the apps > button is visible too, eg > bookmarks_separator_view_->SetVisible(has_other_children || > apps_age_shortcut_->is_visible()) ? > > https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right): > > https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... > chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:369: bool > IsAppsShortcutVisibleForTesting() const; > Since you're using a friend nuke this and check apps_page_shortcut_ directly. This will be continued on https://codereview.chromium.org/12386088/
Comments adressed in patchset 2 of: https://codereview.chromium.org/12386088/ https://codereview.chromium.org/12310109/diff/18001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/12310109/diff/18001/chrome/app/generated_reso... chrome/app/generated_resources.grd:8896: + Apps On 2013/02/28 21:50:31, sky wrote: > nit: indent this and 'Opens apps ...' Done. https://codereview.chromium.org/12310109/diff/18001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_utils.cc (right): https://codereview.chromium.org/12310109/diff/18001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_utils.cc:443: true, On 2013/02/28 21:50:31, sky wrote: > Shouldn't the value of this be based on whether I have any apps? If I haven't > installed any apps I don't want to see this button. Isn't this the only entry-point to the CWS? If it's not there, how are you going to install Apps? https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1317: button->SetEnabled(true); On 2013/02/28 21:50:31, sky wrote: > Enabled is this default. Done. https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1636: bookmarks_separator_view_->SetVisible(has_other_children); On 2013/02/28 21:50:31, sky wrote: > Shouldn't the visibility of the separator view be based on whether the apps > button is visible too, eg > bookmarks_separator_view_->SetVisible(has_other_children || > apps_age_shortcut_->is_visible()) ? Extracted it to its own method since its visibility needs to be set correctly at initialization too. Done. https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right): https://codereview.chromium.org/12310109/diff/18001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:369: bool IsAppsShortcutVisibleForTesting() const; On 2013/02/28 21:50:31, sky wrote: > Since you're using a friend nuke this and check apps_page_shortcut_ directly. Done.
https://codereview.chromium.org/12310109/diff/18001/chrome/browser/bookmarks/... File chrome/browser/bookmarks/bookmark_utils.cc (right): https://codereview.chromium.org/12310109/diff/18001/chrome/browser/bookmarks/... chrome/browser/bookmarks/bookmark_utils.cc:443: true, On 2013/03/04 23:30:14, beaudoin wrote: > On 2013/02/28 21:50:31, sky wrote: > > Shouldn't the value of this be based on whether I have any apps? If I haven't > > installed any apps I don't want to see this button. > > Isn't this the only entry-point to the CWS? If it's not there, how are you going > to install Apps? Not sure. I was assuming we still have a link to the CWS as we do today. Is that not the case?
I think you need to push a new patchset too.
On 2013/03/05 01:32:46, sky wrote: > I think you need to push a new patchset too. Sky, there's no more link to the CWS on the 1993 NTP.
On 2013/03/05 02:18:07, gideonwald wrote: > On 2013/03/05 01:32:46, sky wrote: > > I think you need to push a new patchset too. > > Sky, there's no more link to the CWS on the 1993 NTP. @Sky the patchset has been pushed to https://codereview.chromium.org/12310109/ (patchset 2) this is suboptimal, but I don't think I can take over MAD's CR so starting another one is the best I could do. :(
On 2013/03/05 03:09:56, beaudoin wrote: > On 2013/03/05 02:18:07, gideonwald wrote: > > On 2013/03/05 01:32:46, sky wrote: > > > I think you need to push a new patchset too. > > > > Sky, there's no more link to the CWS on the 1993 NTP. > > @Sky the patchset has been pushed to https://codereview.chromium.org/12310109/ > (patchset 2) > this is suboptimal, but I don't think I can take over MAD's CR so starting > another one is the best I could do. :( Wrong link, sorry: https://codereview.chromium.org/12386088/ |
