Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2951903004:
Added back button when in folder view. (Closed)
DescriptionAdded back button when in folder view.
Also fixed statechange from folder view in AppListView::SetState
BUG=735496
Review-Url: https://codereview.chromium.org/2951903004
Cr-Commit-Position: refs/heads/master@{#483097}
Committed: https://chromium.googlesource.com/chromium/src/+/dbb94859e4464ee4d6c07bd8c1f0b9b65c55db7a
Patch Set 1 #
Total comments: 8
Patch Set 2 : Added back button when in folder view. #
Total comments: 12
Patch Set 3 : Added back button when in folder view. #
Total comments: 6
Patch Set 4 : Added back button when in folder view. #
Total comments: 2
Patch Set 5 : Addressed comments and cached the feature flag. #
Messages
Total messages: 33 (19 generated)
Description was changed from ========== Added back button when in folder view. Also fixed statechange from folder view in AppListView::SetState BUG=735496 ========== to ========== Added back button when in folder view. Also fixed statechange from folder view in AppListView::SetState BUG=735496 ==========
newcomer@chromium.org changed reviewers: + vadimt@chromium.org, xiyuan@chromium.org
PTAL! -Alex
https://codereview.chromium.org/2951903004/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2951903004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:855: app_list_main_view() Add {} https://codereview.chromium.org/2951903004/diff/1/ui/app_list/views/contents_... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/2951903004/diff/1/ui/app_list/views/contents_... ui/app_list/views/contents_view.cc:213: bool folder_active = (state == AppListModel::STATE_APPS) state == AppListModel::STATE_APPS && ? apps_container_view_->IsInFolderView() https://codereview.chromium.org/2951903004/diff/1/ui/app_list/views/contents_... ui/app_list/views/contents_view.cc:223: app_list_main_view_->search_box_view()->SwapGoogleIconWithBackButton( {} https://codereview.chromium.org/2951903004/diff/1/ui/app_list/views/search_bo... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2951903004/diff/1/ui/app_list/views/search_bo... ui/app_list/views/search_box_view.cc:337: void SearchBoxView::SwapGoogleIconWithBackButton(bool show_back_button) { "ShowBackOrGoogleIcon" BTW, do we show Google icon if search provider is NOT Google?
PTAL! https://codereview.chromium.org/2951903004/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2951903004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:855: app_list_main_view() On 2017/06/22 00:36:12, vadimt wrote: > Add {} Done. https://codereview.chromium.org/2951903004/diff/1/ui/app_list/views/contents_... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/2951903004/diff/1/ui/app_list/views/contents_... ui/app_list/views/contents_view.cc:213: bool folder_active = (state == AppListModel::STATE_APPS) On 2017/06/22 00:36:12, vadimt wrote: > state == AppListModel::STATE_APPS && > ? apps_container_view_->IsInFolderView() Done. https://codereview.chromium.org/2951903004/diff/1/ui/app_list/views/contents_... ui/app_list/views/contents_view.cc:223: app_list_main_view_->search_box_view()->SwapGoogleIconWithBackButton( On 2017/06/22 00:36:12, vadimt wrote: > {} Done. https://codereview.chromium.org/2951903004/diff/1/ui/app_list/views/search_bo... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2951903004/diff/1/ui/app_list/views/search_bo... ui/app_list/views/search_box_view.cc:337: void SearchBoxView::SwapGoogleIconWithBackButton(bool show_back_button) { On 2017/06/22 00:36:12, vadimt wrote: > "ShowBackOrGoogleIcon" > > BTW, do we show Google icon if search provider is NOT Google? Done, and I opened a dialogue with our pm/ux crew.
https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/conte... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:213: bool folder_active = state == AppListModel::STATE_APPS && nit: bool -> const bool https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:223: app_list_main_view()->search_box_view())) { Saw a comment says the SearchBoxView is added to a child widget. Would this ever be true ? https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:348: content_container_->RemoveChildView(back_button_); The current code assumes |google_icon_| and |back_button_| are owned by the views hierarchy. Remove them from the hierachy leaves them unowned and could result in a memory leak. Can we manipulate visibility instead? If we want to do the add/remove trick, we need to make SearchBoxView owns the two.
https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:857: ->apps_container_view() Actually, please create a var for the common part: app_list_main_view() ->contents_view() ->apps_container_view() https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/conte... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:222: if (app_list_main_view()->Contains( Nit: app_list_main_view_ https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:195: content_container_->AddChildView(google_icon_); Google icon is supposed to be owned by view hierarchy, but you may never add it to the hierarchy, so it will be leaked? Please run with ASAN to confirm the leak, then consider unique_ptr + set_owned_by_client().
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Addressed comments. https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:857: ->apps_container_view() On 2017/06/22 18:28:08, vadimt wrote: > Actually, please create a var for the common part: app_list_main_view() > ->contents_view() > ->apps_container_view() Done. https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/conte... File ui/app_list/views/contents_view.cc (right): https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:213: bool folder_active = state == AppListModel::STATE_APPS && On 2017/06/22 17:33:24, xiyuan wrote: > nit: bool -> const bool Done. https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:222: if (app_list_main_view()->Contains( On 2017/06/22 18:28:08, vadimt wrote: > Nit: app_list_main_view_ Done. https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/conte... ui/app_list/views/contents_view.cc:223: app_list_main_view()->search_box_view())) { On 2017/06/22 17:33:24, xiyuan wrote: > Saw a comment says the SearchBoxView is added to a child widget. Would this ever > be true ? You're correct, that code was not getting ran. Thanks. https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:195: content_container_->AddChildView(google_icon_); I took xiyuan@'s suggestion of setting the visibility of the views. https://codereview.chromium.org/2951903004/diff/20001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:348: content_container_->RemoveChildView(back_button_); On 2017/06/22 17:33:24, xiyuan wrote: > The current code assumes |google_icon_| and |back_button_| are owned by the > views hierarchy. Remove them from the hierachy leaves them unowned and could > result in a memory leak. > > Can we manipulate visibility instead? If we want to do the add/remove trick, we > need to make SearchBoxView owns the two. I set the visibility as that makes more sense to me.
https://codereview.chromium.org/2951903004/diff/80001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2951903004/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:342: int google_icon_index = content_container_->GetIndexOf(google_icon_); Converting to index and back to view. https://codereview.chromium.org/2951903004/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:345: ->SetVisible(show_back_button ? false : true); !show_back_button https://codereview.chromium.org/2951903004/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:347: ->SetVisible(show_back_button ? true : false); show_back_button ? true : false => show_back_button
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Thanks. -Alex https://codereview.chromium.org/2951903004/diff/80001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2951903004/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:342: int google_icon_index = content_container_->GetIndexOf(google_icon_); On 2017/06/24 00:16:26, vadimt wrote: > Converting to index and back to view. Acknowledged. https://codereview.chromium.org/2951903004/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:345: ->SetVisible(show_back_button ? false : true); On 2017/06/24 00:16:26, vadimt wrote: > !show_back_button Done. https://codereview.chromium.org/2951903004/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:347: ->SetVisible(show_back_button ? true : false); On 2017/06/24 00:16:26, vadimt wrote: > show_back_button ? true : false => show_back_button Done.
The CQ bit was checked by newcomer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the late response... OOO yesterday. https://codereview.chromium.org/2951903004/diff/140001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2951903004/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:195: content_container_->AddChildView(google_icon_); Didn't we already AddChildView(google_icon_) in line 179 ? I would suggest to merge the "if" in line 175-185 with the "if" in line 194. Think it is more readable.
Patchset #5 (id:160001) has been deleted
Thanks, I addressed your comment and cached the feature flag. -Alex https://codereview.chromium.org/2951903004/diff/140001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2951903004/diff/140001/ui/app_list/views/sear... ui/app_list/views/search_box_view.cc:195: content_container_->AddChildView(google_icon_); On 2017/06/27 14:52:35, xiyuan wrote: > Didn't we already AddChildView(google_icon_) in line 179 ? I would suggest to > merge the "if" in line 175-185 with the "if" in line 194. Think it is more > readable. Correct and done.
lgtm
The CQ bit was checked by newcomer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by newcomer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimt@chromium.org Link to the patchset: https://codereview.chromium.org/2951903004/#ps180001 (title: "Addressed comments and cached the feature flag.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1498677512484790,
"parent_rev": "e479e7d2636670318fa4d7f685e1283384996c61", "commit_rev":
"dbb94859e4464ee4d6c07bd8c1f0b9b65c55db7a"}
Message was sent while issue was closed.
Description was changed from ========== Added back button when in folder view. Also fixed statechange from folder view in AppListView::SetState BUG=735496 ========== to ========== Added back button when in folder view. Also fixed statechange from folder view in AppListView::SetState BUG=735496 Review-Url: https://codereview.chromium.org/2951903004 Cr-Commit-Position: refs/heads/master@{#483097} Committed: https://chromium.googlesource.com/chromium/src/+/dbb94859e4464ee4d6c07bd8c1f0... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001) as https://chromium.googlesource.com/chromium/src/+/dbb94859e4464ee4d6c07bd8c1f0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
