Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
223823004:
Improving the user transition to add special cases for maximized windows and make the transition "m… (Closed)
|
Created:
6 years, 8 months ago by Mr4D (OOO till 08-26) Modified:
6 years, 8 months ago CC:
chromium-reviews, kalyank, ben+ash_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImproving the user transition to add special cases for maximized windows and make the transition "more fluid"
There were user complaints with the old animation which said that the animation looked janky. This was in part because various transitions (e.g. maximized->maximized) were showing the background animating in the middle of the animation which looked like "flashing") and because there are some real causes where the CPU gets 100% hogged and the UI thread has not time processing the animation.
This is addressing the first part: Improving the animation by removing the "flash" in the middle and instead transitioning the desktop cleanly from one state to another.
The shelf animation stays the same, but the windows will cross dissolve. When switching from maximize to maximize, the desktop will not shine through and when switching from / or to maximize, the desktop background will only show the desktop background from the user which is not maximized. This way the intermediate frames do not show "some state".
BUG=357852
TEST=unittest & visual checking
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261944
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262024
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262174
Patch Set 1 #
Total comments: 15
Patch Set 2 : Refactored #
Total comments: 21
Patch Set 3 : Addressed #Patch Set 4 : Fixing build problem #
Total comments: 2
Patch Set 5 : . #Patch Set 6 : Fixed preferences browser test [disabling animations] #
Total comments: 1
Messages
Total messages: 37 (0 generated)
Please have a look!
animation management code seems to be big enough to be refactored by now? https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc (right): https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:66: static int kMinimalAnimationTime = 1; kMinimalAnimationTimeMs (assuming it's milliseconds) can you nuke static(s) and use const instead? https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:69: bool IsUserEvent(ui::Event* e) { const ui::Event* https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:437: base::Unretained(this)), can you change to use weak pointer? 110 ms is a bit long to assume the pointer will remain valid. (There were crashes during login transition due to this) https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:765: if (animation_step_ == ANIMATION_STEP_FINALIZE) { optional: using switch would make it easy to understand the flow. https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:834: ash::SHELF_AUTO_HIDE_ALWAYS_HIDDEN, *iter); q: how/where the user's shelf setting is restored? https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:842: scoped_ptr<UserChangeActionDisabler> disabler(new UserChangeActionDisabler); This can be just created on stack ? https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:923: if (mru_list.size()) { !empty() https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h (right): https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.h:173: }; I like this design.
https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc (right): https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:437: base::Unretained(this)), On 2014/04/03 19:21:41, oshima wrote: > can you change to use weak pointer? 110 ms is a bit long to assume the pointer > will remain valid. (There were crashes during login transition due to this) never mind. I somehow thought it's passed to message loop.
All done & refactored. I thought about the refactor this morning, but thought that due to the dependencies it is more work then worth - but done! https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc (right): https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:66: static int kMinimalAnimationTime = 1; On 2014/04/03 19:21:41, oshima wrote: > kMinimalAnimationTimeMs (assuming it's milliseconds) > > can you nuke static(s) and use const instead? Done. https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:69: bool IsUserEvent(ui::Event* e) { On 2014/04/03 19:21:41, oshima wrote: > const ui::Event* Done. https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:765: if (animation_step_ == ANIMATION_STEP_FINALIZE) { On 2014/04/03 19:21:41, oshima wrote: > optional: using switch would make it easy to understand the flow. Done. https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:834: ash::SHELF_AUTO_HIDE_ALWAYS_HIDDEN, *iter); Added comment to explain. https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:842: scoped_ptr<UserChangeActionDisabler> disabler(new UserChangeActionDisabler); On 2014/04/03 19:21:41, oshima wrote: > This can be just created on stack ? Done. https://codereview.chromium.org/223823004/diff/1/chrome/browser/ui/ash/multi_... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:923: if (mru_list.size()) { On 2014/04/03 19:21:41, oshima wrote: > !empty() Done.
lg https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc (right): https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc:84: // Full screen covers the screen naturally. Maximized however can be smaller Work area can be smaller than fullscreen, so we do not check maximized mode... Also work area differs between users, so it's not reliable. https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc:91: GetDisplayNearestWindow(window).work_area(); don't you want to check with bounds instead of work area? https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc:163: // Some unit tests have no ChromeLauncherController. do you know why? https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc:176: for (ash::Shell::RootWindowControllerList::iterator it1 = controller.begin(); it/itr/iter ? (it1 sounds a bit strange as there is not 0 or 2) https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc:188: // ChromeLauncherController::ActiveUserChanged() to the new users value. and this will not update the preference right? https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.h (right): https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.h:26: class MultiUserWindowManagerAnimChromeOS { Can we use a bit shorter name? Here are a couple of candidates: UserSwitchAnimatorChromeOS UserSwitcherChromeOS ('cause it may or may not animate) https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.h:38: bool animation_disabled); indent https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.h:39: virtual ~MultiUserWindowManagerAnimChromeOS(); you don't need virtual, although I don't mind. Up to you. https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.h:50: const std::string& GetWallaperUserIdForTest() { return wallpaper_user_id_; } walllpaper_user_id_for_test()
Please have another look! https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc (right): https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc:84: // Full screen covers the screen naturally. Maximized however can be smaller Actually this is exactly what I have meant. The workarea excludes the shelf area and as such it is what I was looking for. https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc:91: GetDisplayNearestWindow(window).work_area(); No, this is okay. E.g.: Shelf is not inclusive. https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc:163: // Some unit tests have no ChromeLauncherController. Yes, because they do try to be minimalistic and initialize only what they need for testing. https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc:176: for (ash::Shell::RootWindowControllerList::iterator it1 = controller.begin(); On 2014/04/03 23:52:29, oshima wrote: > it/itr/iter ? (it1 sounds a bit strange as there is not 0 or 2) Done. https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc:188: // ChromeLauncherController::ActiveUserChanged() to the new users value. Correct. added comment https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.h (right): https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.h:26: class MultiUserWindowManagerAnimChromeOS { On 2014/04/03 23:52:29, oshima wrote: > Can we use a bit shorter name? Here are a couple of candidates: > > UserSwitchAnimatorChromeOS > UserSwitcherChromeOS ('cause it may or may not animate) > Done. https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.h:38: bool animation_disabled); On 2014/04/03 23:52:29, oshima wrote: > indent Done. https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.h:39: virtual ~MultiUserWindowManagerAnimChromeOS(); On 2014/04/03 23:52:29, oshima wrote: > you don't need virtual, although I don't mind. Up to you. Done. https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.h:50: const std::string& GetWallaperUserIdForTest() { return wallpaper_user_id_; } On 2014/04/03 23:52:29, oshima wrote: > walllpaper_user_id_for_test() Done.
one more nit about comment. https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc (right): https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc:163: // Some unit tests have no ChromeLauncherController. On 2014/04/04 14:21:32, Mr4D wrote: > Yes, because they do try to be minimalistic and initialize only what they need > for testing. I looked into it and I believe this is probably a bug (at least not intentional) because this test is using delegates in ash/test, which are designed either for ash/*tests, or a browser related tests that doesn't depend on the implementation of delegates. unit_tests under chrome/browser that test ash related features which involves the implementation of ash delegates should use delegates in chrome/browser/ui/ash but not one in ash/test because they do not work each other. Fixing it probably requires more work, so this is OK for now. IMO, changing production code to make test pass should be discouraged (I understand it's necessary sometimes) because you may not be testing what should be covered, and makes the test less meaningful. I've seen a case where a test passes, but the feature fails in production because the test is doing something different and we should minimize such case. https://codereview.chromium.org/223823004/diff/60001/chrome/browser/ui/ash/mu... File chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc (right): https://codereview.chromium.org/223823004/diff/60001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc:84: // then the work area - so we do not check for that mode, but instead check if maximized window should have the same size as work area, so this needs to be updated. How about something like: A normal window can have the same size as work area, so just check the bounds against work_area.
forgot to say, lgtm once the comment is fixed.
Please have another look! https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc (right): https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc:163: // Some unit tests have no ChromeLauncherController. As you said - the required work to make this one call work seems quite big. Some unit testes simply did not instantiate the ChromeLauncherController (which is not a bug) since it would require more interfaces/interactions to be present. [At a certain test framework size you start then to test the testing framework - but not the real code you want to test.] As an example: This particular call will require then the profile manager, multiple profiles and many more things. I did implement in the past dummy interfaces (like a dummy ChromeLauncherController) to address this, but some reviewers didn't like that either and opted for these kind of tests. If this is however a problem we should possibly try to do a "fixit" to get rid of these call suppressors since I have seen many of these checks in various places. https://codereview.chromium.org/223823004/diff/60001/chrome/browser/ui/ash/mu... File chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc (right): https://codereview.chromium.org/223823004/diff/60001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc:84: // then the work area - so we do not check for that mode, but instead check if On 2014/04/04 17:10:19, oshima wrote: > maximized window should have the same size as work area, so this needs to be > updated. How about something like: > > A normal window can have the same size as work area, so just check the bounds > against work_area. Done.
https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc (right): https://codereview.chromium.org/223823004/diff/20001/chrome/browser/ui/ash/mu... chrome/browser/ui/ash/multi_user/multi_user_window_manager_anim_chromeos.cc:163: // Some unit tests have no ChromeLauncherController. On 2014/04/04 17:47:17, Mr4D wrote: > As you said - the required work to make this one call work seems quite big. I don't think getting this return a valid instance isn't big change. What I'm worried is that doing so may have some (valid) side effects to other tests and that'd be the outside of the scope of this CL. > > Some unit testes simply did not instantiate the ChromeLauncherController (which > is not a bug) since it would require more interfaces/interactions to be present. > > [At a certain test framework size you start then to test the testing framework - > but not the real code you want to test.] > > As an example: This particular call will require then the profile manager, > multiple profiles and many more things. > > I did implement in the past dummy interfaces (like a dummy > ChromeLauncherController) to address this, but some reviewers didn't like that > either and opted for these kind of tests. This is valid solution for unit tests, and I'm ok with it (depending on the test, of course). My main point is that changing production code in order for a test to pass isn't good idea, and this returning NULL isn't intentional (at least for tests in c/b/ui/ash). > If this is however a problem we should possibly try to do a "fixit" to get rid > of these call suppressors since I have seen many of these checks in various > places. I worked on AshBaseHelper so I'll look into it first.
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/223823004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/223823004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/223823004/80001
Message was sent while issue was closed.
Change committed as 261944
The CL got reverted from someone, but the given revert reason could not have been related to this patch - so re-submitting.
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/223823004/80001
Message was sent while issue was closed.
Change committed as 262024
Message was sent while issue was closed.
On 2014/04/05 18:18:22, Mr4D wrote: > The CL got reverted from someone, but the given revert reason could not have > been related to this patch - so re-submitting. Previously reverted in http://crrev.com/261971 Looks like the same test (PreferencesTest.MultiProfiles) has been failing again in several cros bots since relanding this CL: http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2... http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2... http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20... Do you mind taking another look?
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/227243002/ by nhiroki@chromium.org. The reason for reverting is: As sadrul@ mentioned on the original review[1], this seems to be breaking PreferencesTest.MultiProfiles test. Please let me revert... [1] https://codereview.chromium.org/223823004.
Message was sent while issue was closed.
On 2014/04/05 18:18:22, Mr4D wrote: > The CL got reverted from someone, but the given revert reason could not have > been related to this patch - so re-submitting. Seriously? I spent an hour bisecting to this change to revert, and it reproduces 100% of the time. You could at least try running the test before breaking the tree again.
Wow. I was going through the failing valgrind and membot tests and have seen some unrelated printing tests failing. They couldn't have been the problems of this patch. Beside that I did *not* get any notifications for breakages and my build bots were green. Dont we not send out "normal breakage" notifications anymore? I did not see the failing tests pointed out by Sadrul - they sound very well be related and I will investigate. So - sorry for this.
Nkostylev - could you please do an owners review for the preferences_browsertest.cc? Thanks!
lgtm
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/223823004/100001
Message was sent while issue was closed.
Change committed as 262174
Message was sent while issue was closed.
https://codereview.chromium.org/223823004/diff/100001/chrome/browser/ui/ash/m... File chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc (right): https://codereview.chromium.org/223823004/diff/100001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc:71: base::Unretained(this)), Why Unretained? I believe we should have a weak ptr here. This causes a crash during shutdown.
Message was sent while issue was closed.
On 2014/04/08 18:35:08, mtomasz wrote: > https://codereview.chromium.org/223823004/diff/100001/chrome/browser/ui/ash/m... > File chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc (right): > > https://codereview.chromium.org/223823004/diff/100001/chrome/browser/ui/ash/m... > chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc:71: > base::Unretained(this)), > Why Unretained? I believe we should have a weak ptr here. This causes a crash > during shutdown. Crash stacktrace: Program received signal SIGSEGV, Segmentation fault. 0x000000000271e348 in ash::RootWindowController::GetShelfLayoutManager() () #0 0x000000000271e348 in ash::RootWindowController::GetShelfLayoutManager() () #1 0x0000000001f2aa7c in chrome::UserSwichAnimatorChromeOS::TransitionUserShelf(chrome::UserSwichAnimatorChromeOS::AnimationStep) () #2 0x0000000001f2b22f in chrome::UserSwichAnimatorChromeOS::AdvanceUserTransitionAnimation() () #3 0x0000000001f2b310 in chrome::UserSwichAnimatorChromeOS::FinalizeAnimation() () #4 0x0000000001f2b338 in chrome::UserSwichAnimatorChromeOS::~UserSwichAnimatorChromeOS() () #5 0x0000000001f28928 in chrome::MultiUserWindowManagerChromeOS::~MultiUserWindowManagerChromeOS() () #6 0x0000000001f28ae1 in chrome::MultiUserWindowManagerChromeOS::~MultiUserWindowManagerChromeOS() () #7 0x0000000001f26686 in chrome::MultiUserWindowManager::DeleteInstance() () #8 0x0000000001f1e9d9 in ChromeLauncherController::~ChromeLauncherController() () #9 0x0000000001f1ec11 in ChromeLauncherController::~ChromeLauncherController() () #10 0x00000000027326cf in ash::Shell::~Shell() () #11 0x00000000027331e1 in ash::Shell::~Shell() () #12 0x00000000027320f6 in ash::Shell::DeleteInstance() ()
Message was sent while issue was closed.
Unretained does not help. I think that this is part of the destruction process (order of it) since the animation gets finished when the system is going down. The right way to address this is to suppress completion upon system shut down. I will add that in a separate patch. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
