Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
956403003:
Fix the issue that wallpaper changes twice when both "onChanged" and "onAlarm" are fired. (Closed)
|
Created:
5 years, 10 months ago by xdai1 Modified:
5 years, 9 months ago Reviewers:
bshe CC:
chromium-reviews, stevenjb+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix the issue that wallpaper changes twice when both "onChanged" and "onAlarm" are fired.
Suppose we have two Chromebook A and B with the same account and with surprise me enabled.
This CL handles two edge cases when "onChanged" and "onAlarm" are both fired on Chromebook B.
1) The last surprise wallpaper was set on A yesterday. Log into B today:
Don't sync with A. A new surprise wallpaper should set for B.
2) The last surprise wallpaper was set on A today. Log into B later the same day:
Don't set a surprise wallpaper for B anymore, instead B should sync the same wallpaper with A.
BUG=373682
TEST=Tested on devices
Committed: https://crrev.com/4a36585c7c380782e999a259c74edd6c06fea793
Cr-Commit-Position: refs/heads/master@{#320119}
Patch Set 1 #
Total comments: 1
Patch Set 2 : #
Total comments: 15
Patch Set 3 : Address the code review comments. #Patch Set 4 : Address the code review comments. #Patch Set 5 : Address the code review comments. #Patch Set 6 : Address the code review comments. #
Total comments: 6
Patch Set 7 : Address the multiple trigger of onAlarm() issue. #Patch Set 8 : Fix the failed wallpaper sync test. #
Messages
Total messages: 21 (4 generated)
xdai@chromium.org changed reviewers: + bshe@chromium.org
Biao, could you review the CL please? thanks!
Interesting case. What if the alarm fired first then the sync envoked. We should also figure out a way to handle it. https://codereview.chromium.org/956403003/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/956403003/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:308: dateString, true); dateString seems undefined?
On 2015/02/27 19:31:19, bshe wrote: > Interesting case. What if the alarm fired first then the sync envoked. We should > also figure out a way to handle it. > > https://codereview.chromium.org/956403003/diff/1/chrome/browser/resources/chr... > File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js > (right): > > https://codereview.chromium.org/956403003/diff/1/chrome/browser/resources/chr... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:308: > dateString, true); > dateString seems undefined? Biao, please take another look at the CL, thanks! I've tested the two edge cases on my devices with the alarm of 1 minute interval (see the description of the CL). I'll test them again with the alarm of 1 day interval tomorrow.
So this CL wont take care of the case that onAlarm fires first then sync info coming in? https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:151: Constants.WallpaperSourceEnum.Online); nit: 4 spaces indention for new line. https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:205: Constants.WallpaperSyncStorage.get( why do you want to move the check to here? Looks like we skipped checking from wallpaper from rss this way, I worry it might cause problem. https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:293: enabledItems[Constants.AccessSurpriseMeEnabledKey]; nit: 4 spaces indention https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:296: Constants.AccessLastSurpriseWallpaperChangedDate, function(items) { ditto https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:297: var syncLastSurpriseMeChangedDate = nit: function implementation should have 2 space indention. https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:305: syncLastSurpriseMeChangedDate == today)) { nit: indention is off. Consider this: if (!syncSurpriseMeEnabled || (syncSurpriseMeEnabled && syncLastSurpriseMeChangedDate == today)) { .... } https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:309: infoItems[Constants.AccessLocalWallpaperInfoKey]; nit: 4 space indention
It does take care of the case that onAlarm fires first (since in the two cases that in the CL description, onAlarm and onChanged are both fired, sometimes onAlarm fires first, sometime onChanged fires first.) On 2015/03/04 19:45:40, bshe wrote: > So this CL wont take care of the case that onAlarm fires first then sync info > coming in? > > https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... > File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js > (right): > > https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:151: > Constants.WallpaperSourceEnum.Online); > nit: 4 spaces indention for new line. > > https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:205: > Constants.WallpaperSyncStorage.get( > why do you want to move the check to here? Looks like we skipped checking from > wallpaper from rss this way, I worry it might cause problem. > We won't miss rss wallpaper this way. After the fix for the onChanged() function and setRandomWallpaper_() function as in Patch 2, these two ways (checking AccessLastSurpriseWallpaperChangedDate here or in updateRandomWallpaper_() function) does exactly the same thing, but I think it's more clear to check here than the old way.
Biao, please take another look, thanks! I've tested on my devices, and it works as expected. But I just found another edge case: what if onAlarm() is trigger more than once? Seems it will also change the wallpaper more than once... https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:151: Constants.WallpaperSourceEnum.Online); On 2015/03/04 19:45:39, bshe wrote: > nit: 4 spaces indention for new line. Done. https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:205: Constants.WallpaperSyncStorage.get( On 2015/03/04 19:45:39, bshe wrote: > why do you want to move the check to here? Looks like we skipped checking from > wallpaper from rss this way, I worry it might cause problem. Since when surprise me is enabled, we only change wallpaper once a day (no matter it's rss special wallpaper or a random surprise wallpaper). So it won't miss rss wallpaper here. Both way will do the same thing. But I think it might be more clear to put the check here? https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:293: enabledItems[Constants.AccessSurpriseMeEnabledKey]; On 2015/03/04 19:45:39, bshe wrote: > nit: 4 spaces indention Done. https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:296: Constants.AccessLastSurpriseWallpaperChangedDate, function(items) { On 2015/03/04 19:45:39, bshe wrote: > ditto Done. https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:297: var syncLastSurpriseMeChangedDate = On 2015/03/04 19:45:39, bshe wrote: > nit: function implementation should have 2 space indention. I think it already has 2 space indention? https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:305: syncLastSurpriseMeChangedDate == today)) { On 2015/03/04 19:45:39, bshe wrote: > nit: indention is off. Consider this: > if (!syncSurpriseMeEnabled || > (syncSurpriseMeEnabled && syncLastSurpriseMeChangedDate == today)) { > .... > } Done. https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:309: infoItems[Constants.AccessLocalWallpaperInfoKey]; On 2015/03/04 19:45:39, bshe wrote: > nit: 4 space indention Done.
> It does take care of the case that onAlarm fires first (since in the two cases > that in the CL description, onAlarm and onChanged are both fired, sometimes > onAlarm fires first, sometime onChanged fires first.) I mean this case: The last surprise wallpaper was set on A today. Log into B today: Before sync with A, a new surprise wallpaper might be set on B due to onAlarm fired before sync? Not sure if this is going to happen, but when it happen, the wallpaper on Chromebook B might be different from the one on A. This is an edge case so I wont worry too much about it. https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:205: Constants.WallpaperSyncStorage.get( On 2015/03/04 21:32:39, xdai1 wrote: > On 2015/03/04 19:45:39, bshe wrote: > > why do you want to move the check to here? Looks like we skipped checking from > > wallpaper from rss this way, I worry it might cause problem. > > Since when surprise me is enabled, we only change wallpaper once a day (no > matter it's rss special wallpaper or a random surprise wallpaper). So it won't > miss rss wallpaper here. Both way will do the same thing. But I think it might > be more clear to put the check here? Rss wallpaper is not saved to sync storage and we prefer to use the rss wallpaper over random online wallpaper. Set rss wallpaper might not be success on Chromebook on first try. We dont want to block the second try (1 hour later, see retryLater_ function) in this case. If there is no bug that you are trying to fix with this move, I would prefer to keep the check in the old place.
On 2015/03/04 21:32:39, xdai1 wrote: > Biao, please take another look, thanks! I've tested on my devices, and it works > as expected. But I just found another edge case: what if onAlarm() is trigger > more than once? Seems it will also change the wallpaper more than once... Not sure why onAlarm is triggered more than once. If we failed to get rss from server, we might try to request it again 1 hour later. In that case, there might be more than one alarm one day. I do notice another unrelated bug with the rss though. Since we dont have rss file on the server yet, request to RSS always failed with 404. We should not call retryLater_ in event_page.js in this 404 case. > > https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... > File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js > (right): > > https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:151: > Constants.WallpaperSourceEnum.Online); > On 2015/03/04 19:45:39, bshe wrote: > > nit: 4 spaces indention for new line. > > Done. > > https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:205: > Constants.WallpaperSyncStorage.get( > On 2015/03/04 19:45:39, bshe wrote: > > why do you want to move the check to here? Looks like we skipped checking from > > wallpaper from rss this way, I worry it might cause problem. > > Since when surprise me is enabled, we only change wallpaper once a day (no > matter it's rss special wallpaper or a random surprise wallpaper). So it won't > miss rss wallpaper here. Both way will do the same thing. But I think it might > be more clear to put the check here? > > https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:293: > enabledItems[Constants.AccessSurpriseMeEnabledKey]; > On 2015/03/04 19:45:39, bshe wrote: > > nit: 4 spaces indention > > Done. > > https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:296: > Constants.AccessLastSurpriseWallpaperChangedDate, function(items) { > On 2015/03/04 19:45:39, bshe wrote: > > ditto > > Done. > > https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:297: var > syncLastSurpriseMeChangedDate = > On 2015/03/04 19:45:39, bshe wrote: > > nit: function implementation should have 2 space indention. > > I think it already has 2 space indention? > > https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:305: > syncLastSurpriseMeChangedDate == today)) { > On 2015/03/04 19:45:39, bshe wrote: > > nit: indention is off. Consider this: > > if (!syncSurpriseMeEnabled || > > (syncSurpriseMeEnabled && syncLastSurpriseMeChangedDate == today)) { > > .... > > } > > Done. > > https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:309: > infoItems[Constants.AccessLocalWallpaperInfoKey]; > On 2015/03/04 19:45:39, bshe wrote: > > nit: 4 space indention > > Done.
On 2015/03/05 14:53:20, bshe wrote: > > It does take care of the case that onAlarm fires first (since in the two cases > > that in the CL description, onAlarm and onChanged are both fired, sometimes > > onAlarm fires first, sometime onChanged fires first.) > > I mean this case: > The last surprise wallpaper was set on A today. Log into B today: > Before sync with A, a new surprise wallpaper might be set on B due to onAlarm > fired before sync? > Not sure if this is going to happen, but when it happen, the wallpaper on > Chromebook B might be different from the one on A. This is an edge case so I > wont worry too much about it. > In this case the onAlarm() function on B won't change the wallpaper since the last surprise wallpaper was modified today, and it won't be allowed to change the wallpaper twice. > https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... > File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js > (right): > > https://codereview.chromium.org/956403003/diff/20001/chrome/browser/resources... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:205: > Constants.WallpaperSyncStorage.get( > On 2015/03/04 21:32:39, xdai1 wrote: > > On 2015/03/04 19:45:39, bshe wrote: > > > why do you want to move the check to here? Looks like we skipped checking > from > > > wallpaper from rss this way, I worry it might cause problem. > > > > Since when surprise me is enabled, we only change wallpaper once a day (no > > matter it's rss special wallpaper or a random surprise wallpaper). So it won't > > miss rss wallpaper here. Both way will do the same thing. But I think it might > > be more clear to put the check here? > > Rss wallpaper is not saved to sync storage and we prefer to use the rss > wallpaper over random online wallpaper. Set rss wallpaper might not be success > on Chromebook on first try. We dont want to block the second try (1 hour later, > see retryLater_ function) in this case. If there is no bug that you are trying > to fix with this move, I would prefer to keep the check in the old place. OK, I brought back the old way of checking last modified date of surprise wallpaper. It shouldn't make any differences. I'll test on my devices. But in the old way, A and B might fetch different rss wallpapers since Rss wallpaper isn't saved to sync storage.
Biao, please take another look at the CL, thanks! Also addressed the multiple trigger of onAlarm() because of retrying to change wallpaper (404 failure). Tested on devices based on 1 hour interval alarm, and it worked as expected. Will also test it again based on 1 day interval alarm tomorrow. On 2015/03/05 14:57:24, bshe wrote: > On 2015/03/04 21:32:39, xdai1 wrote: > > Biao, please take another look, thanks! I've tested on my devices, and it > works > > as expected. But I just found another edge case: what if onAlarm() is trigger > > more than once? Seems it will also change the wallpaper more than once... > Not sure why onAlarm is triggered more than once. If we failed to get rss from > server, > we might try to request it again 1 hour later. In that case, there might be more > than one > alarm one day. > I do notice another unrelated bug with the rss though. Since we dont have rss > file on the > server yet, request to RSS always failed with 404. We should not call > retryLater_ in > event_page.js in this 404 case. > > > > >
https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:35: var onNotFound = function() { If not found, I think we should just use a random ONLINE wallpaper instead of using local copy of RSS. NOT_FOUND is a good indication of no RSS wallpaper. https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:108: self.updateRandomWallpaper_(); would be nice to handle the not found case in failure. https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:313: url, type, onSuccess, onFailure, onNotFound, opt_xhr) { I think it is probably better to handle onNotFound in onFailure. Not all callers care about onNotFound case.
Patchset #7 (id:120001) has been deleted
Biao, thanks for the review. Please take another look, thanks! https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:35: var onNotFound = function() { On 2015/03/09 14:16:09, bshe wrote: > If not found, I think we should just use a random ONLINE wallpaper instead of > using local copy of RSS. NOT_FOUND is a good indication of no RSS wallpaper. If not found (404), we randomly set a wallpaper displayed in wallpaper picker. If other type error occurs, we try to use local rss first, and if we still got error, set an alarm to retry 1 hour later. https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:108: self.updateRandomWallpaper_(); On 2015/03/09 14:16:09, bshe wrote: > would be nice to handle the not found case in failure. Done. https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:313: url, type, onSuccess, onFailure, onNotFound, opt_xhr) { On 2015/03/09 14:16:09, bshe wrote: > I think it is probably better to handle onNotFound in onFailure. Not all callers > care about onNotFound case. Done.
On 2015/03/09 21:07:10, xdai1 wrote: > Biao, thanks for the review. Please take another look, thanks! > > https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... > File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js > (right): > > https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:35: var > onNotFound = function() { > On 2015/03/09 14:16:09, bshe wrote: > > If not found, I think we should just use a random ONLINE wallpaper instead of > > using local copy of RSS. NOT_FOUND is a good indication of no RSS wallpaper. > > If not found (404), we randomly set a wallpaper displayed in wallpaper picker. > If other type error occurs, we try to use local rss first, and if we still got > error, set an alarm to retry 1 hour later. > > https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:108: > self.updateRandomWallpaper_(); > On 2015/03/09 14:16:09, bshe wrote: > > would be nice to handle the not found case in failure. > > Done. > > https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... > File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): > > https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:313: url, type, > onSuccess, onFailure, onNotFound, opt_xhr) { > On 2015/03/09 14:16:09, bshe wrote: > > I think it is probably better to handle onNotFound in onFailure. Not all > callers > > care about onNotFound case. > > Done. lgtm
On 2015/03/10 18:19:21, bshe wrote: > On 2015/03/09 21:07:10, xdai1 wrote: > > Biao, thanks for the review. Please take another look, thanks! > > > > > https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... > > File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js > > (right): > > > > > https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... > > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:35: var > > onNotFound = function() { > > On 2015/03/09 14:16:09, bshe wrote: > > > If not found, I think we should just use a random ONLINE wallpaper instead > of > > > using local copy of RSS. NOT_FOUND is a good indication of no RSS wallpaper. > > > > If not found (404), we randomly set a wallpaper displayed in wallpaper picker. > > If other type error occurs, we try to use local rss first, and if we still got > > error, set an alarm to retry 1 hour later. > > > > > https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... > > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:108: > > self.updateRandomWallpaper_(); > > On 2015/03/09 14:16:09, bshe wrote: > > > would be nice to handle the not found case in failure. > > > > Done. > > > > > https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... > > File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): > > > > > https://codereview.chromium.org/956403003/diff/100001/chrome/browser/resource... > > chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:313: url, type, > > onSuccess, onFailure, onNotFound, opt_xhr) { > > On 2015/03/09 14:16:09, bshe wrote: > > > I think it is probably better to handle onNotFound in onFailure. Not all > > callers > > > care about onNotFound case. > > > > Done. > > lgtm Thanks Biao!
The CQ bit was checked by xdai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org Link to the patchset: https://codereview.chromium.org/956403003/#ps160001 (title: "Fix the failed wallpaper sync test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/956403003/160001
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4a36585c7c380782e999a259c74edd6c06fea793 Cr-Commit-Position: refs/heads/master@{#320119} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
