Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
497733002:
Declaring the weak_ptr_factory in proper order. (Closed)
|
Created:
6 years, 4 months ago by Anand Ratn (left samsung) Modified:
6 years, 3 months ago CC:
chromium-reviews, lgombos Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionDeclaring the weak_ptr_factory in proper order.
Cleaning up weak_ptr_factorydestruction order in "src/ash" module.
WeakPtrFactory should remain the last member so it'll be destroyed and
invalidate its weak pointers before any other members are destroyed.
BUG=303818
Committed: https://crrev.com/e8dea07883cf5cb9efa4c083495dd3de4abf3e14
Cr-Commit-Position: refs/heads/master@{#293450}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Changes are incorporated with the comments #
Total comments: 4
Patch Set 3 : Changes are incorporated in this patchset. #
Total comments: 2
Patch Set 4 : Reverting ash/shell.h #
Total comments: 2
Patch Set 5 : Adding instance method #Patch Set 6 : updating unittests file #Patch Set 7 : Removing Name Conflict #
Total comments: 4
Patch Set 8 : Updating method name #
Total comments: 2
Patch Set 9 : Maintaining function in proper order #
Messages
Total messages: 42 (7 generated)
oshima@chromium.org: Please review changes in src/ash module.
https://codereview.chromium.org/497733002/diff/1/ash/shell.h File ash/shell.h (left): https://codereview.chromium.org/497733002/diff/1/ash/shell.h#oldcode703 ash/shell.h:703: weak_display_manager_factory_; This is for display_manager_, and defined after display_manager_, so the order should be correct. Does this have to be moved to the end as well? (I don't have objection. Just checking) https://codereview.chromium.org/497733002/diff/1/ash/wm/workspace/workspace_w... File ash/wm/workspace/workspace_window_resizer.h (right): https://codereview.chromium.org/497733002/diff/1/ash/wm/workspace/workspace_w... ash/wm/workspace/workspace_window_resizer.h:221: static WorkspaceWindowResizer* instance_; can you move this to anonymous namespace in .cc ?
https://codereview.chromium.org/497733002/diff/1/ash/shell.h File ash/shell.h (left): https://codereview.chromium.org/497733002/diff/1/ash/shell.h#oldcode703 ash/shell.h:703: weak_display_manager_factory_; On 2014/08/26 15:31:05, oshima wrote: > This is for display_manager_, and defined after display_manager_, > so the order should be correct. > > Does this have to be moved to the end as well? (I don't have objection. Just > checking) It should be safe this way, and I think the Clang checker should not complain about it. (If it does, let me know). I'll leave it up to you two as to whether it should move.
On 2014/08/26 15:34:52, dmichael wrote: > https://codereview.chromium.org/497733002/diff/1/ash/shell.h > File ash/shell.h (left): > > https://codereview.chromium.org/497733002/diff/1/ash/shell.h#oldcode703 > ash/shell.h:703: weak_display_manager_factory_; > On 2014/08/26 15:31:05, oshima wrote: > > This is for display_manager_, and defined after display_manager_, > > so the order should be correct. > > > > Does this have to be moved to the end as well? (I don't have objection. Just > > checking) > It should be safe this way, and I think the Clang checker should not complain > about it. (If it does, let me know). > > I'll leave it up to you two as to whether it should move. If that's the case, I prefer to keep them closer.
On 2014/08/26 16:33:52, oshima wrote: > On 2014/08/26 15:34:52, dmichael wrote: > > https://codereview.chromium.org/497733002/diff/1/ash/shell.h > > File ash/shell.h (left): > > > > https://codereview.chromium.org/497733002/diff/1/ash/shell.h#oldcode703 > > ash/shell.h:703: weak_display_manager_factory_; > > On 2014/08/26 15:31:05, oshima wrote: > > > This is for display_manager_, and defined after display_manager_, > > > so the order should be correct. > > > > > > Does this have to be moved to the end as well? (I don't have objection. Just > > > checking) > > It should be safe this way, and I think the Clang checker should not complain > > about it. (If it does, let me know). > > > > I'll leave it up to you two as to whether it should move. > > If that's the case, I prefer to keep them closer. Thanks for reviewing. Review comments are incorporated. PTAL
PTAL. https://codereview.chromium.org/497733002/diff/1/ash/shell.h File ash/shell.h (left): https://codereview.chromium.org/497733002/diff/1/ash/shell.h#oldcode703 ash/shell.h:703: weak_display_manager_factory_; On 2014/08/26 15:34:52, dmichael (OOO 8.28-8.29) wrote: > On 2014/08/26 15:31:05, oshima wrote: > > This is for display_manager_, and defined after display_manager_, > > so the order should be correct. > > > > Does this have to be moved to the end as well? (I don't have objection. Just > > checking) > It should be safe this way, and I think the Clang checker should not complain > about it. (If it does, let me know). > > I'll leave it up to you two as to whether it should move. Done. https://codereview.chromium.org/497733002/diff/1/ash/shell.h#oldcode703 ash/shell.h:703: weak_display_manager_factory_; On 2014/08/26 15:31:05, oshima wrote: > This is for display_manager_, and defined after display_manager_, > so the order should be correct. > > Does this have to be moved to the end as well? (I don't have objection. Just > checking) Maintaining the order of display_manager_ in the new patchset. https://codereview.chromium.org/497733002/diff/1/ash/wm/workspace/workspace_w... File ash/wm/workspace/workspace_window_resizer.h (right): https://codereview.chromium.org/497733002/diff/1/ash/wm/workspace/workspace_w... ash/wm/workspace/workspace_window_resizer.h:221: static WorkspaceWindowResizer* instance_; On 2014/08/26 15:31:05, oshima wrote: > can you move this to anonymous namespace in .cc ? > Done.
https://codereview.chromium.org/497733002/diff/20001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/497733002/diff/20001/ash/shell.h#newcode760 ash/shell.h:760: weak_display_manager_factory_; sorry if I wasn't clear. Please keep them as it was before. https://codereview.chromium.org/497733002/diff/20001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_window_resizer.cc (right): https://codereview.chromium.org/497733002/diff/20001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer.cc:114: static WorkspaceWindowResizer* instance_ = NULL; nit: remove static.
On 2014/08/28 06:33:17, oshima wrote: > https://codereview.chromium.org/497733002/diff/20001/ash/shell.h > File ash/shell.h (right): > > https://codereview.chromium.org/497733002/diff/20001/ash/shell.h#newcode760 > ash/shell.h:760: weak_display_manager_factory_; > sorry if I wasn't clear. Please keep them as it was before. > > https://codereview.chromium.org/497733002/diff/20001/ash/wm/workspace/workspa... > File ash/wm/workspace/workspace_window_resizer.cc (right): > > https://codereview.chromium.org/497733002/diff/20001/ash/wm/workspace/workspa... > ash/wm/workspace/workspace_window_resizer.cc:114: static WorkspaceWindowResizer* > instance_ = NULL; > nit: remove static. Thanks for reviewing. Review comments are incorporated. PTAL
PTAL. https://codereview.chromium.org/497733002/diff/20001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/497733002/diff/20001/ash/shell.h#newcode760 ash/shell.h:760: weak_display_manager_factory_; On 2014/08/28 06:33:17, oshima wrote: > sorry if I wasn't clear. Please keep them as it was before. Sorry I misunderstood it previously. keeping it as it was before. https://codereview.chromium.org/497733002/diff/20001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_window_resizer.cc (right): https://codereview.chromium.org/497733002/diff/20001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer.cc:114: static WorkspaceWindowResizer* instance_ = NULL; On 2014/08/28 06:33:17, oshima wrote: > nit: remove static. Done.
https://codereview.chromium.org/497733002/diff/40001/ash/shell.h File ash/shell.h (left): https://codereview.chromium.org/497733002/diff/40001/ash/shell.h#oldcode703 ash/shell.h:703: weak_display_manager_factory_; sorry , I should have said, "please revert this file". no change is needed in this file.
On 2014/08/28 16:29:41, oshima wrote: > https://codereview.chromium.org/497733002/diff/40001/ash/shell.h > File ash/shell.h (left): > > https://codereview.chromium.org/497733002/diff/40001/ash/shell.h#oldcode703 > ash/shell.h:703: weak_display_manager_factory_; > sorry , I should have said, "please revert this file". > no change is needed in this file. Sorry I misunderstood it. changes in ash/shell.h reverted.
PTAL. https://codereview.chromium.org/497733002/diff/40001/ash/shell.h File ash/shell.h (left): https://codereview.chromium.org/497733002/diff/40001/ash/shell.h#oldcode703 ash/shell.h:703: weak_display_manager_factory_; On 2014/08/28 16:29:41, oshima wrote: > sorry , I should have said, "please revert this file". > no change is needed in this file. Done.
On 2014/08/28 16:29:41, oshima wrote: > https://codereview.chromium.org/497733002/diff/40001/ash/shell.h > File ash/shell.h (left): > > https://codereview.chromium.org/497733002/diff/40001/ash/shell.h#oldcode703 > ash/shell.h:703: weak_display_manager_factory_; > sorry , I should have said, "please revert this file". > no change is needed in this file. Thanks for the review. I have updated the patchset as per the discussion. PTAL.
lgtm
lgtm
The CQ bit was checked by anand.ratn@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/anand.ratn@samsung.com/497733002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2014/09/02 10:08:14, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Hi oshima, linux_chromium_chromeos_rel_swarming build failed and the reason is because of moving WorkspaceWindowResizer::instance_ to anonymous namespace in ash/wm/workspace/workspace_window_resizer.cc Please suggest me what can be done. I was thinking to put it back to ash/wm/workspace/workspace_window_resizer.h Regards, Anand Ratn
https://codereview.chromium.org/497733002/diff/60001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_window_resizer.h (right): https://codereview.chromium.org/497733002/diff/60001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer.h:169: Can you add static method to return the instance then? WorkspaceWindowResizer* instance(); sorry, I didn't know it's used by the test.
On 2014/09/03 00:04:03, oshima wrote: > https://codereview.chromium.org/497733002/diff/60001/ash/wm/workspace/workspa... > File ash/wm/workspace/workspace_window_resizer.h (right): > > https://codereview.chromium.org/497733002/diff/60001/ash/wm/workspace/workspa... > ash/wm/workspace/workspace_window_resizer.h:169: > Can you add static method to return the instance then? > > WorkspaceWindowResizer* instance(); > > sorry, I didn't know it's used by the test. Thanks for the review. I have added the instance method in this patchset. PTAL
PTAL. Changes are incorporated in this patchset. https://codereview.chromium.org/497733002/diff/60001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_window_resizer.h (right): https://codereview.chromium.org/497733002/diff/60001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer.h:169: I have Added the instance method in new patchset. PTAL. Thanks
On 2014/09/03 07:56:22, anand.ratn wrote: > PTAL. Changes are incorporated in this patchset. > > https://codereview.chromium.org/497733002/diff/60001/ash/wm/workspace/workspa... > File ash/wm/workspace/workspace_window_resizer.h (right): > > https://codereview.chromium.org/497733002/diff/60001/ash/wm/workspace/workspa... > ash/wm/workspace/workspace_window_resizer.h:169: > > I have Added the instance method in new patchset. PTAL. > Thanks Hi oshima. AFAIK and as per chromium standards, a member variable name should be end with _ (underscore). After moving instance_ to anonymous namespace, it was no more class member variable. so it should be "instance" only. The method I'm adding for getting the instance from workspace_window_resizer.h file has to be renamed, otherwise there will be ambiguity. Ex: WorkspaceWindowResizer* WorkspaceWindowResizer::instance() { return instance; --> this statement is ambiguous. //return instance_; } Please suggest if i can modify the instance method. InMyOpinion: I can modify the instance() function to GetInstance(). Thanks.
On 2014/09/03 14:10:54, anand.ratn wrote: > On 2014/09/03 07:56:22, anand.ratn wrote: > > PTAL. Changes are incorporated in this patchset. > > > > > https://codereview.chromium.org/497733002/diff/60001/ash/wm/workspace/workspa... > > File ash/wm/workspace/workspace_window_resizer.h (right): > > > > > https://codereview.chromium.org/497733002/diff/60001/ash/wm/workspace/workspa... > > ash/wm/workspace/workspace_window_resizer.h:169: > > > > I have Added the instance method in new patchset. PTAL. > > Thanks > > Hi oshima. > AFAIK and as per chromium standards, a member variable name should be end with _ > (underscore). After moving instance_ to anonymous namespace, it was no more > class member variable. so it should be "instance" only. The method I'm adding > for getting the instance from workspace_window_resizer.h file has to be renamed, > otherwise there will be ambiguity. > Ex: > WorkspaceWindowResizer* WorkspaceWindowResizer::instance() { > return instance; --> this statement is ambiguous. > //return instance_; > } > Please suggest if i can modify the instance method. > InMyOpinion: I can modify the instance() function to GetInstance(). > > Thanks. you can use g_instance. (it does not have to have g_, as it's filed scoped, but not global, but I'm fine if that resolve the name conflict) I just do not want to expose raw pointer when an accessor is enough.
The CQ bit was checked by anand.ratn@samsung.com
The CQ bit was unchecked by anand.ratn@samsung.com
On 2014/09/03 17:02:17, oshima wrote: > On 2014/09/03 14:10:54, anand.ratn wrote: > > On 2014/09/03 07:56:22, anand.ratn wrote: > > > PTAL. Changes are incorporated in this patchset. > > > > > > > > > https://codereview.chromium.org/497733002/diff/60001/ash/wm/workspace/workspa... > > > File ash/wm/workspace/workspace_window_resizer.h (right): > > > > > > > > > https://codereview.chromium.org/497733002/diff/60001/ash/wm/workspace/workspa... > > > ash/wm/workspace/workspace_window_resizer.h:169: > > > > > > I have Added the instance method in new patchset. PTAL. > > > Thanks > > > > Hi oshima. > > AFAIK and as per chromium standards, a member variable name should be end with > _ > > (underscore). After moving instance_ to anonymous namespace, it was no more > > class member variable. so it should be "instance" only. The method I'm adding > > for getting the instance from workspace_window_resizer.h file has to be > renamed, > > otherwise there will be ambiguity. > > Ex: > > WorkspaceWindowResizer* WorkspaceWindowResizer::instance() { > > return instance; --> this statement is ambiguous. > > //return instance_; > > } > > Please suggest if i can modify the instance method. > > InMyOpinion: I can modify the instance() function to GetInstance(). > > > > Thanks. > > you can use g_instance. (it does not have to have g_, as it's filed scoped, but > not global, > but I'm fine if that resolve the name conflict) I just do not want to expose raw > pointer > when an accessor is enough. I have added g_instance() method in new patchset. PTAL.
https://codereview.chromium.org/497733002/diff/120001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_window_resizer.h (right): https://codereview.chromium.org/497733002/diff/120001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_window_resizer.h:170: // Adding method to return WorkspaceWindowResizer *instance // Returns the currently used instance for test. https://codereview.chromium.org/497733002/diff/120001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_window_resizer.h:171: static WorkspaceWindowResizer* g_instance(); Sorry I meant "g_instance "as variable name. (It's pretty common among chromium developers, but I should not have expected that) On second thought, it's probably better to call it "GetInstanceForTest()". Let me be very specific this time to avoid confusion this time. Method: static WorkspaceWindowResizer* GetInstanceForTest(); variable: WorkspaceWindowResizer* instance = NULL; in anonymous namespace in the file "workspace_window_resizer.cc"
Thanks for the review. PTAL at new patchset. https://codereview.chromium.org/497733002/diff/120001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_window_resizer.h (right): https://codereview.chromium.org/497733002/diff/120001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_window_resizer.h:170: // Adding method to return WorkspaceWindowResizer *instance Done. https://codereview.chromium.org/497733002/diff/120001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_window_resizer.h:171: static WorkspaceWindowResizer* g_instance(); Thank you oshima for clearing my doubt . I have made the changes accordingly in new patchset.
lgtm thanks! https://codereview.chromium.org/497733002/diff/140001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_window_resizer.cc (right): https://codereview.chromium.org/497733002/diff/140001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_window_resizer.cc:255: } nit: keep this after static member variables. (after kScreenEdgeInset)
PTAL. https://codereview.chromium.org/497733002/diff/140001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_window_resizer.cc (right): https://codereview.chromium.org/497733002/diff/140001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_window_resizer.cc:255: } On 2014/09/04 09:31:11, oshima wrote: > nit: keep this after static member variables. (after kScreenEdgeInset) Done.
The CQ bit was checked by anand.ratn@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/anand.ratn@samsung.com/497733002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu_retina_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
The CQ bit was checked by anand.ratn@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/anand.ratn@samsung.com/497733002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as f01b7a8f22dda131d39ffa53a3dc31e818643062
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e8dea07883cf5cb9efa4c083495dd3de4abf3e14 Cr-Commit-Position: refs/heads/master@{#293450} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
