Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2934513004:
Changed static variable flag to be a bool in the anon namespace. (Closed)
DescriptionChanged static variable flag to be a bool in the anon namespace so that it properly changes between tests.
BUG=731998
Review-Url: https://codereview.chromium.org/2934513004
Cr-Commit-Position: refs/heads/master@{#479068}
Committed: https://chromium.googlesource.com/chromium/src/+/f5e2fd64538b4e801e017cc36ba324af81ad1d71
Patch Set 1 #
Total comments: 4
Patch Set 2 : In AppListView: Moved function and variable out of the global scope. In SearchBoxView: Moved variab… #
Total comments: 12
Patch Set 3 : addressed comments. #
Total comments: 4
Patch Set 4 : addressed comments. #
Total comments: 2
Patch Set 5 : addressed comments. #
Messages
Total messages: 39 (24 generated)
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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== Changed static variable flag to be a bool in the anon namespace. BUG= ========== to ========== Changed static variable flag to be a bool in the anon namespace so that it properly changes between tests. BUG=731998 ==========
newcomer@chromium.org changed reviewers: + vadimt@chromium.org
newcomer@chromium.org changed reviewers: + xiyuan@chromium.org
PTAL, removed the static variable after discussing the consequences with vadimt@
https://codereview.chromium.org/2934513004/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2934513004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:80: bool is_fullscreen_app_list_enabled; I'd say make this a member of the class that needs to check this frequently. If the cost of features::IsFullscreenAppListEnabled() is not big, just call it when needed.
https://codereview.chromium.org/2934513004/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2934513004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:80: bool is_fullscreen_app_list_enabled; +1. Please don't add non-const global variables.
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 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.
addressed comments, PTAL! https://codereview.chromium.org/2934513004/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2934513004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:80: bool is_fullscreen_app_list_enabled; On 2017/06/12 18:16:57, vadimt wrote: > +1. Please don't add non-const global variables. Done. https://codereview.chromium.org/2934513004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:80: bool is_fullscreen_app_list_enabled; On 2017/06/12 17:24:38, xiyuan wrote: > I'd say make this a member of the class that needs to check this frequently. > > If the cost of features::IsFullscreenAppListEnabled() is not big, just call it > when needed. Done.
https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:716: void AppListView::IsFullscreenAppListEnabled() { Get rid of this function and just initialize |is_fullscreen_app_list_enabled_| in ctor. https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:58: bool IsFullscreenAppListEnabled() { We probably can get rid of this. https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:59: // Cache this value to avoid repeated lookup. nit: Wrong comment if we keep the function. https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:147: is_fullscreen_app_list_enabled_ = IsFullscreenAppListEnabled(); nit: make |is_fullscreen_app_list_enabled_| a const bool member and give it initial value in the initialization list above.
https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:185: // The switch that is checked to determine if the fullscreen app list feature // Whether the fullscreen app list feature is enabled. https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:187: bool is_fullscreen_app_list_enabled_ = false; const.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
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...
addressed comments! https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:716: void AppListView::IsFullscreenAppListEnabled() { On 2017/06/12 22:33:15, xiyuan wrote: > Get rid of this function and just initialize |is_fullscreen_app_list_enabled_| > in ctor. Done. https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:185: // The switch that is checked to determine if the fullscreen app list feature On 2017/06/12 23:29:35, vadimt wrote: > // Whether the fullscreen app list feature is enabled. Done. https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:187: bool is_fullscreen_app_list_enabled_ = false; On 2017/06/12 23:29:35, vadimt wrote: > const. Done. https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:58: bool IsFullscreenAppListEnabled() { On 2017/06/12 22:33:15, xiyuan wrote: > We probably can get rid of this. Done. https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:59: // Cache this value to avoid repeated lookup. On 2017/06/12 22:33:15, xiyuan wrote: > nit: Wrong comment if we keep the function. Done. https://codereview.chromium.org/2934513004/diff/20001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:147: is_fullscreen_app_list_enabled_ = IsFullscreenAppListEnabled(); On 2017/06/12 22:33:15, xiyuan wrote: > nit: make |is_fullscreen_app_list_enabled_| a const bool member and give it > initial value in the initialization list above. Done.
https://codereview.chromium.org/2934513004/diff/80001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2934513004/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:117: void IsFullscreenAppListEnabled(); Remove. https://codereview.chromium.org/2934513004/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:185: // The switch that is checked to determine whether the fullscreen app list // The switch that is checked to determine -- should be removed.
Thanks, Alex https://codereview.chromium.org/2934513004/diff/80001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2934513004/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:117: void IsFullscreenAppListEnabled(); On 2017/06/12 23:58:49, vadimt wrote: > Remove. Done. https://codereview.chromium.org/2934513004/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:185: // The switch that is checked to determine whether the fullscreen app list On 2017/06/12 23:58:49, vadimt wrote: > // The switch that is checked to determine > -- > should be removed. Done.
https://codereview.chromium.org/2934513004/diff/100001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2934513004/diff/100001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:127: // The switch that is checked to determine if the fullscreen app list feature Again, shorten to "whether XXX"
Thanks, Alex https://codereview.chromium.org/2934513004/diff/100001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2934513004/diff/100001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:127: // The switch that is checked to determine if the fullscreen app list feature On 2017/06/13 00:21:59, vadimt wrote: > Again, shorten to "whether XXX" 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.
lgtm
The CQ bit was checked by newcomer@chromium.org
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": 120001, "attempt_start_ts": 1497377117720140,
"parent_rev": "091a11b45e72e81bb0ecefc0fb5b73d2f2e546cd", "commit_rev":
"f5e2fd64538b4e801e017cc36ba324af81ad1d71"}
Message was sent while issue was closed.
Description was changed from ========== Changed static variable flag to be a bool in the anon namespace so that it properly changes between tests. BUG=731998 ========== to ========== Changed static variable flag to be a bool in the anon namespace so that it properly changes between tests. BUG=731998 Review-Url: https://codereview.chromium.org/2934513004 Cr-Commit-Position: refs/heads/master@{#479068} Committed: https://chromium.googlesource.com/chromium/src/+/f5e2fd64538b4e801e017cc36ba3... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f5e2fd64538b4e801e017cc36ba3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
