Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
380613002:
Experimental app list: Added a current page indicator to the launcher. (Closed)
|
Created:
6 years, 5 months ago by kcarattini Modified:
6 years, 5 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionExperimental app list: Added a current page indicator to the launcher.
Adds a current page indicator to the launcher when the experimental app launcher is enabled.
BUG=391642
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282335
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282586
Patch Set 1 #
Total comments: 21
Patch Set 2 : Adjusted according to review comments. #Patch Set 3 : Fixed spelling mistake #
Total comments: 6
Patch Set 4 : Changes lanuch indicator change to start of transition animation #
Total comments: 2
Patch Set 5 : removed slow transition #
Total comments: 17
Patch Set 6 : Responded to review comments #
Total comments: 2
Patch Set 7 : Removed static initializer. #
Messages
Total messages: 21 (0 generated)
Views stuff looks mostly good. +mgiuca: Do you think ContentsSwitcherView should be made into its own PaginationModelObserver here? https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/app_list_b... File ui/app_list/views/app_list_background.cc (right): https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/app_list_b... ui/app_list/views/app_list_background.cc:92: // separator rect. nit: 'separator' can wrap to the previous line. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... File ui/app_list/views/contents_switcher_view.cc (right): https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:20: const int kPreferredHeight = 32 + kPageIndicatorHeight; Pull out 32 into kButtonImageSize. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:50: views::ImageButton* button = new views::ImageButton(this); We should have button->SetPreferredSize(gfx::Size(kButtonImageSize, kButtonImageSize)); https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:62: views::Background::CreateSolidBackground(0, 0, 255)); Use app_list::kPagerSelectedColor. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:63: indicator->SetFillsBoundsOpaquely(true); I don't think this does anything. This is an option for aura layers. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:66: views::View* indicator_container = new views::View(); // A container view that will consume space when its child is not visible. // TODO(calamity): Remove this once BoxLayout supports space-consuming invisible views. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:70: views::View* button_indicator_container = new views::View(); Maybe button_container? Either way, this should have a comment like: // View containing the indicator view and image button. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:86: void ContentsSwitcherView::Layout() { Ooh, I should update this to use BoxLayout with MAIN_AXIS_ALIGNMENT_CENTER. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:111: true); It would be better to make a private member // Owned by views hierarchy. std::vector<view::View*> page_active_indicators_; where you keep pointers to all the indicator views you create and then access them here. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_v... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_v... ui/app_list/views/contents_view.cc:80: contents_switcher_view_->SelectedPageChanged(-1, initial_page_index); Is this necessary? I would think the line above would trigger ContentsView::SelectedPageChanged() since the observer is registered in the constructor.
https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/app_list_b... File ui/app_list/views/app_list_background.cc (right): https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/app_list_b... ui/app_list/views/app_list_background.cc:92: // separator rect. On 2014/07/09 05:50:18, calamity wrote: > nit: 'separator' can wrap to the previous line. Done. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... File ui/app_list/views/contents_switcher_view.cc (right): https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:20: const int kPreferredHeight = 32 + kPageIndicatorHeight; On 2014/07/09 05:50:19, calamity wrote: > Pull out 32 into kButtonImageSize. Done. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:50: views::ImageButton* button = new views::ImageButton(this); On 2014/07/09 05:50:18, calamity wrote: > We should have > > button->SetPreferredSize(gfx::Size(kButtonImageSize, kButtonImageSize)); Done. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:62: views::Background::CreateSolidBackground(0, 0, 255)); On 2014/07/09 05:50:19, calamity wrote: > Use app_list::kPagerSelectedColor. Done. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:63: indicator->SetFillsBoundsOpaquely(true); On 2014/07/09 05:50:19, calamity wrote: > I don't think this does anything. This is an option for aura layers. Done. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:66: views::View* indicator_container = new views::View(); On 2014/07/09 05:50:19, calamity wrote: > // A container view that will consume space when its child is not visible. > // TODO(calamity): Remove this once BoxLayout supports space-consuming invisible > views. Done. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:70: views::View* button_indicator_container = new views::View(); On 2014/07/09 05:50:19, calamity wrote: > Maybe button_container? Either way, this should have a comment like: > // View containing the indicator view and image button. Done. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:86: void ContentsSwitcherView::Layout() { On 2014/07/09 05:50:18, calamity wrote: > Ooh, I should update this to use BoxLayout with MAIN_AXIS_ALIGNMENT_CENTER. Ack. Did you want me to add a TODO for you? https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:111: true); On 2014/07/09 05:50:18, calamity wrote: > It would be better to make a private member > > // Owned by views hierarchy. > std::vector<view::View*> page_active_indicators_; > > where you keep pointers to all the indicator views you create and then access > them here. Done. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_v... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_v... ui/app_list/views/contents_view.cc:80: contents_switcher_view_->SelectedPageChanged(-1, initial_page_index); On 2014/07/09 05:50:19, calamity wrote: > Is this necessary? I would think the line above would trigger > ContentsView::SelectedPageChanged() since the observer is registered in the > constructor. I would have thought so too, but it doesn't seem to work without it.
> +mgiuca: Do you think ContentsSwitcherView should be made into its own > PaginationModelObserver here? Yes, I do. (Unless it makes ownership more hairy, but it's worth giving it a try.) Make ContentsSwitcherView a PaginationModelObserver, and then we won't have ContentsView::SelectedPageChanged do CSV's bidding. CL description: Firstly there is a typo, it should be "when the", not "whent he". Secondly, the first row (72 chars) of the CL description should be a complete sentence that summarizes the entire commit, since it is often used as the name of the CL (see the heading in Reitveld). The second row should always be a blank line. Try to compress it down to 72 chars, and split the details out into a separate paragraph if necessary. Suggestion: "Experimental app list: Added a current page indicator to the launcher." Note that when changing the description of a CL you should also change the subject to match its first line. Terminology note: I use the term "launcher" to refer to the UI, because it isn't just an app launcher any more. (But I still use "experimental app list" as a prefix because that's the name of the flag.) https://codereview.chromium.org/380613002/diff/40001/ui/app_list/views/conten... File ui/app_list/views/contents_switcher_view.cc (right): https://codereview.chromium.org/380613002/diff/40001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.cc:52: button->SetPreferredSize(gfx::Size(kButtonImageSize, kButtonImageSize)); What does setting this do? (Why is it needed now and not before?) https://codereview.chromium.org/380613002/diff/40001/ui/app_list/views/conten... File ui/app_list/views/contents_switcher_view.h (right): https://codereview.chromium.org/380613002/diff/40001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.h:45: std::vector<views::View*> page_active_indicators_; I'm not sure if this is the right approach. We eventually want to have an animation showing a single blue indicator sliding between the buttons, which means ultimately we're going to want a single ContentsPageIndicatorView which moves (rather than an array of hidden ones, and one visible one). If it's possible to do this now, I think you should. It's debatable whether you should submit this now, and then change it over to the other approach, or whether it isn't worth submitting this change unless that is done (because the final approach will be very different). You can decide or we can discuss.
Hmm I thought I'd fixed the typo already. Is that not the case? Kendra On 09/07/2014 5:01 pm, <mgiuca@chromium.org> wrote: > +mgiuca: Do you think ContentsSwitcherView should be made into its own >> PaginationModelObserver here? >> > > Yes, I do. (Unless it makes ownership more hairy, but it's worth giving it > a > try.) Make ContentsSwitcherView a PaginationModelObserver, and then we > won't > have ContentsView::SelectedPageChanged do CSV's bidding. > > CL description: > Firstly there is a typo, it should be "when the", not "whent he". > > Secondly, the first row (72 chars) of the CL description should be a > complete > sentence that summarizes the entire commit, since it is often used as the > name > of the CL (see the heading in Reitveld). The second row should always be a > blank > line. Try to compress it down to 72 chars, and split the details out into a > separate paragraph if necessary. > > Suggestion: > "Experimental app list: Added a current page indicator to the launcher." > > Note that when changing the description of a CL you should also change the > subject to match its first line. > > Terminology note: I use the term "launcher" to refer to the UI, because it > isn't > just an app launcher any more. (But I still use "experimental app list" as > a > prefix because that's the name of the flag.) > > > https://codereview.chromium.org/380613002/diff/40001/ui/ > app_list/views/contents_switcher_view.cc > File ui/app_list/views/contents_switcher_view.cc (right): > > https://codereview.chromium.org/380613002/diff/40001/ui/ > app_list/views/contents_switcher_view.cc#newcode52 > ui/app_list/views/contents_switcher_view.cc:52: > button->SetPreferredSize(gfx::Size(kButtonImageSize, kButtonImageSize)); > What does setting this do? (Why is it needed now and not before?) > > https://codereview.chromium.org/380613002/diff/40001/ui/ > app_list/views/contents_switcher_view.h > File ui/app_list/views/contents_switcher_view.h (right): > > https://codereview.chromium.org/380613002/diff/40001/ui/ > app_list/views/contents_switcher_view.h#newcode45 > ui/app_list/views/contents_switcher_view.h:45: std::vector<views::View*> > page_active_indicators_; > I'm not sure if this is the right approach. We eventually want to have > an animation showing a single blue indicator sliding between the > buttons, which means ultimately we're going to want a single > ContentsPageIndicatorView which moves (rather than an array of hidden > ones, and one visible one). > > If it's possible to do this now, I think you should. It's debatable > whether you should submit this now, and then change it over to the other > approach, or whether it isn't worth submitting this change unless that > is done (because the final approach will be very different). You can > decide or we can discuss. > > https://codereview.chromium.org/380613002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/09 07:23:26, chromium-reviews wrote: > Hmm I thought I'd fixed the typo already. Is that not the case? Oh, you fixed it in the description, but not the subject. If I get time, I might see if I can fix Reitveld to automate all this, but in the meantime, manually keep the first line of the description and the subject field in sync. :(
One other thing, you should hook the change to TransitionStarted() so that the blue line changes on click rather than at the end of the contents view animation. https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... File ui/app_list/views/contents_switcher_view.cc (right): https://codereview.chromium.org/380613002/diff/1/ui/app_list/views/contents_s... ui/app_list/views/contents_switcher_view.cc:86: void ContentsSwitcherView::Layout() { On 2014/07/09 06:45:21, kcarattini wrote: > On 2014/07/09 05:50:18, calamity wrote: > > Ooh, I should update this to use BoxLayout with MAIN_AXIS_ALIGNMENT_CENTER. > > Ack. Did you want me to add a TODO for you? > Nah, it's cool. https://codereview.chromium.org/380613002/diff/40001/ui/app_list/views/conten... File ui/app_list/views/contents_switcher_view.cc (right): https://codereview.chromium.org/380613002/diff/40001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.cc:52: button->SetPreferredSize(gfx::Size(kButtonImageSize, kButtonImageSize)); On 2014/07/09 07:01:20, Matt Giuca wrote: > What does setting this do? (Why is it needed now and not before?) It should have been there before. It has no effect on the current app list. It's for semantic layout correctness. It tells the ImageButton what size to be when it has no image set. We can remove this if you really want. https://codereview.chromium.org/380613002/diff/40001/ui/app_list/views/conten... File ui/app_list/views/contents_switcher_view.h (right): https://codereview.chromium.org/380613002/diff/40001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.h:45: std::vector<views::View*> page_active_indicators_; On 2014/07/09 07:01:20, Matt Giuca wrote: > I'm not sure if this is the right approach. We eventually want to have an > animation showing a single blue indicator sliding between the buttons, which > means ultimately we're going to want a single ContentsPageIndicatorView which > moves (rather than an array of hidden ones, and one visible one). > > If it's possible to do this now, I think you should. It's debatable whether you > should submit this now, and then change it over to the other approach, or > whether it isn't worth submitting this change unless that is done (because the > final approach will be very different). You can decide or we can discuss. From a short discussion w/ Matt: We don't know what we want. We can defer this decision for now. https://codereview.chromium.org/380613002/diff/40001/ui/app_list/views/conten... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/380613002/diff/40001/ui/app_list/views/conten... ui/app_list/views/contents_view.cc:251: contents_switcher_view_->AddSwitcherButton(resource_id, page_index); Lines 79-80 can be removed if you move this above SetTotalPages(). This was happening because SetTotalPages() triggered the notification but AddSwitcherButton() hadn't added the indicator yet, causing the ContentsSwitcherView::SelectedPageChanged() to have no effect.
Dona all. PTAL. https://codereview.chromium.org/380613002/diff/40001/ui/app_list/views/conten... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/380613002/diff/40001/ui/app_list/views/conten... ui/app_list/views/contents_view.cc:251: contents_switcher_view_->AddSwitcherButton(resource_id, page_index); On 2014/07/09 08:02:54, calamity wrote: > Lines 79-80 can be removed if you move this above SetTotalPages(). This was > happening because SetTotalPages() triggered the notification but > AddSwitcherButton() hadn't added the indicator yet, causing the > ContentsSwitcherView::SelectedPageChanged() to have no effect. Done.
Excellent. Things will be much easier when we inevitably have to change the animation. https://codereview.chromium.org/380613002/diff/60001/ui/app_list/views/conten... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/380613002/diff/60001/ui/app_list/views/conten... ui/app_list/views/contents_view.cc:105: contents_switcher_view_ = contents_switcher_view; Better DCHECK(!contents_switcher_view_) here now since changing the ContentsSwitcherView will require a RemoveObserver. https://codereview.chromium.org/380613002/diff/60001/ui/app_list/views/conten... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/380613002/diff/60001/ui/app_list/views/conten... ui/app_list/views/contents_view.h:68: void set_contents_switcher_view(ContentsSwitcherView* contents_switcher_view); This should be SetContentsSwitcherView(...) now. unix_case for inlined direct getters/setters. CamelCase for anything at all more complex. https://codereview.chromium.org/380613002/diff/80001/ui/app_list/pagination_m... File ui/app_list/pagination_model.cc (right): https://codereview.chromium.org/380613002/diff/80001/ui/app_list/pagination_m... ui/app_list/pagination_model.cc:232: SetTransition(transition); Is this change still needed? https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... File ui/app_list/views/contents_switcher_view.cc (right): https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.cc:19: const int kPageIndicatorHeight = 1; Move this into app_list_constants.h as ContentsSwitcherSeparatorHeight and then use it in app_list_background.cc as both the separator rect height and the ContentsPageIndicatorView. https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.cc:80: views::BoxLayout::MAIN_AXIS_ALIGNMENT_CENTER); I don't think this line is necessary (which also means you can get rid of the layout_manager variable. https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.cc:142: page_active_indicators_[new_selected]->SetVisible(true); I think it's better to call SelectedPageChanged() here rather than duplicate the logic. https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_view.h:68: void set_contents_switcher_view(ContentsSwitcherView* contents_switcher_view); This should change to SetContentsSwitcherView now. unix_case and inlined in header for simple x_ = x; style setters and getters, CamelCase for anything at all more complex. https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_view.h:110: // Returns the pagination model for thei ContentsView. s/thei/the/
lgtm w nits. https://codereview.chromium.org/380613002/diff/80001/ui/app_list/pagination_m... File ui/app_list/pagination_model.cc (right): https://codereview.chromium.org/380613002/diff/80001/ui/app_list/pagination_m... ui/app_list/pagination_model.cc:232: SetTransition(transition); You can revert this change now that we are using TransitionChanged instead of TransitionStarted. (I'm not sure what other effects it will have; best to revert it.) https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... File ui/app_list/views/contents_switcher_view.cc (right): https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.cc:128: // Change the indicator at the beginning of the transition animation. Comment out of date. https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.cc:132: if (pm.IsRevertingCurrentTransition()) { // Swap the direction if the transition is reversed. https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.cc:142: page_active_indicators_[new_selected]->SetVisible(true); On 2014/07/10 03:59:36, calamity wrote: > I think it's better to call SelectedPageChanged() here rather than duplicate the > logic. Acknowledged.
https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... File ui/app_list/views/contents_switcher_view.cc (right): https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.cc:19: const int kPageIndicatorHeight = 1; On 2014/07/10 03:59:36, calamity wrote: > Move this into app_list_constants.h as ContentsSwitcherSeparatorHeight and then > use it in app_list_background.cc as both the separator rect height and the > ContentsPageIndicatorView. Done. https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.cc:80: views::BoxLayout::MAIN_AXIS_ALIGNMENT_CENTER); On 2014/07/10 03:59:36, calamity wrote: > I don't think this line is necessary (which also means you can get rid of the > layout_manager variable. Done. https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.cc:128: // Change the indicator at the beginning of the transition animation. Updated. On 2014/07/10 05:31:03, Matt Giuca wrote: > Comment out of date. https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.cc:132: if (pm.IsRevertingCurrentTransition()) { On 2014/07/10 05:31:03, Matt Giuca wrote: > // Swap the direction if the transition is reversed. Done. https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_switcher_view.cc:142: page_active_indicators_[new_selected]->SetVisible(true); On 2014/07/10 03:59:36, calamity wrote: > I think it's better to call SelectedPageChanged() here rather than duplicate the > logic. Done. https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_view.h:68: void set_contents_switcher_view(ContentsSwitcherView* contents_switcher_view); On 2014/07/10 03:59:36, calamity wrote: > This should change to SetContentsSwitcherView now. unix_case and inlined in > header for simple x_ = x; style setters and getters, CamelCase for anything at > all more complex. Done. https://codereview.chromium.org/380613002/diff/80001/ui/app_list/views/conten... ui/app_list/views/contents_view.h:110: // Returns the pagination model for thei ContentsView. On 2014/07/10 03:59:36, calamity wrote: > s/thei/the/ Done.
lgtm
The CQ bit was checked by kcarattini@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kcarattini@chromium.org/380613002/100001
Message was sent while issue was closed.
Change committed as 282335
Message was sent while issue was closed.
https://codereview.chromium.org/380613002/diff/100001/ui/app_list/views/conte... File ui/app_list/views/contents_switcher_view.cc (right): https://codereview.chromium.org/380613002/diff/100001/ui/app_list/views/conte... ui/app_list/views/contents_switcher_view.cc:21: kButtonImageSize + kContentsSwitcherSeparatorHeight; Since kButtonImageSize or kCntentsSwitcherSeparatorHeight aren't defined in header files, the compiler can't do this computation at compile time, so this adds a static initializer. Your options are to make sure that the compiler can know the value of the two things on the right at compile time here, or to make kPreferredHeight() a function instead of a global const.
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/384803003/ by thakis@chromium.org. The reason for reverting is: Caused a static initializer, see http://build.chromium.org/p/chromium/builders/Linux%20x64/builds/67534.
Message was sent while issue was closed.
https://codereview.chromium.org/380613002/diff/100001/ui/app_list/views/conte... File ui/app_list/views/contents_switcher_view.cc (right): https://codereview.chromium.org/380613002/diff/100001/ui/app_list/views/conte... ui/app_list/views/contents_switcher_view.cc:21: kButtonImageSize + kContentsSwitcherSeparatorHeight; On 2014/07/10 18:01:16, Nico (away) wrote: > Since kButtonImageSize or kCntentsSwitcherSeparatorHeight aren't defined in > header files, the compiler can't do this computation at compile time, so this > adds a static initializer. Your options are to make sure that the compiler can > know the value of the two things on the right at compile time here, or to make > kPreferredHeight() a function instead of a global const. Done.
The CQ bit was checked by kcarattini@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kcarattini@chromium.org/380613002/120001
Message was sent while issue was closed.
Change committed as 282586 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
