Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2899133004:
Reduce overdraw on bookmark bar (Closed)
DescriptionReduce overdraw on bookmark bar
The bookmark bar always experience 2 layers of overdraw because the layers
are marked as non-opaque. After research, I noticed that it is always opaque
for both attached and detached bookmark bar with or without theme, so the
top layer is now marked as opaque.
The bookmark bar and the toolbar is overlapped for the toolbar to control the
show/hide animation of the separator between them. This cl also updates the
location and the height of bookmark bar to avoid the overlapping.
BUG=725956
Review-Url: https://codereview.chromium.org/2899133004
Cr-Commit-Position: refs/heads/master@{#476569}
Committed: https://chromium.googlesource.com/chromium/src/+/7073ae5bc6af913c649500e015210992f9c7111a
Patch Set 1 : Reduce overdraw on bookmark bar #
Total comments: 2
Patch Set 2 : new approach #
Total comments: 3
Patch Set 3 : Update bookmark bar location and size #
Total comments: 2
Patch Set 4 : address comments #
Total comments: 2
Patch Set 5 : revert change in browserview #
Messages
Total messages: 63 (40 generated)
The CQ bit was checked by yiyix@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...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by yiyix@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.
Description was changed from ========== Reduce overdraw on bookmark bar remove layer BUG= ========== to ========== Reduce overdraw on bookmark bar The bookmark bar always contains 2 layers of overdraw. One of the overdraw layer is created to display the showing/hiding animation of bookmark bar only. This layer can be added before animation and destroyed afterwards to reduce raster effort. BUG=725956 ==========
yiyix@chromium.org changed reviewers: + sky@chromium.org
@sky, estade, could you please review this patch? Thank you.
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/2899133004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2899133004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:662: DestroyLayer(); I am surprised this works. You're destroying the layer almost as soon as it's being created. I would have thought you'd need to wait until AnimationEnded to destroy the layer. (also, I think it's safe to call DestroyLayer even if you don't have a layer.)
https://codereview.chromium.org/2899133004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2899133004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:662: DestroyLayer(); On 2017/05/24 17:34:30, Evan Stade wrote: > I am surprised this works. You're destroying the layer almost as soon as it's > being created. I would have thought you'd need to wait until AnimationEnded to > destroy the layer. > > (also, I think it's safe to call DestroyLayer even if you don't have a layer.) I think it would be better to key off whether any descendant views have layers. You can do this via OnChildLayerChanged(). I'm suggesting any time that function is called you check if any visible descendants have layers. If they don't, then the bookmarkbar shouldn't need a layer.
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
On 2017/05/24 21:32:05, sky wrote: > https://codereview.chromium.org/2899133004/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): > > https://codereview.chromium.org/2899133004/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:662: DestroyLayer(); > On 2017/05/24 17:34:30, Evan Stade wrote: > > I am surprised this works. You're destroying the layer almost as soon as it's > > being created. I would have thought you'd need to wait until AnimationEnded to > > destroy the layer. > > > > (also, I think it's safe to call DestroyLayer even if you don't have a layer.) > > I think it would be better to key off whether any descendant views have layers. > You can do this via OnChildLayerChanged(). I'm suggesting any time that function > is called you check if any visible descendants have layers. If they don't, then > the bookmarkbar shouldn't need a layer. I think what really happened is that i destroyed the layer and its child layers before the animation finishes. So the bug did not shown for me. I tried a new approach, please let me know what you think about it. @sky, some discussion on how to fix it can be found in the bug link, crbug/725956
https://codereview.chromium.org/2899133004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2899133004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:608: layer()->SetFillsBoundsOpaquely(true); I'm surprised this would work. Don't we need the the bookmark bar to show through in some cases? Same themes or animations or on the ntp when detached?
https://codereview.chromium.org/2899133004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2899133004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:226: view->GetLocalBounds(), false); Isn't this separator already being drawn by OpaqueBrowserFrameView[1]? And I believe you're moving the separator up by 1dip - 1px (i.e. 2px at 300%). I would think you should not be adding this here, but instead changing the layout of the bookmark bar so it's 1dip lower and 1 dip shorter so it doesn't overlap that other separator. [1] https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/opaque_bro...
@sky, please let me know if you have other concerns. https://codereview.chromium.org/2899133004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2899133004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:608: layer()->SetFillsBoundsOpaquely(true); On 2017/05/26 13:40:53, sky wrote: > I'm surprised this would work. Don't we need the the bookmark bar to show > through in some cases? Same themes or animations or on the ntp when detached? I believe that it's opaque for both attached and detached bookmark bars. For themes: For attached case, we first paint background color of the toolbar, then we paint the image from the theme for the bookmark bar. (as shown in https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... ). I believe the background of the toolbar is always opaque. I found this bug, which requests it to be transparent and marked as "won't fix", https://bugs.chromium.org/p/chromium/issues/detail?id=244892. I also tried to search transparent theme in the app store and could not find any. So I was convinced it's safe to mark the layer as opaque. For detached case, I found this comment, which explains the color for view must be painted opaquely and the layer can be semi-transparent, so it's safe to make the bookmark bar layer as opaque as well. https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi...
On 2017/05/26 15:03:41, Evan Stade wrote: > https://codereview.chromium.org/2899133004/diff/100001/chrome/browser/ui/view... > File chrome/browser/ui/views/frame/browser_view.cc (right): > > https://codereview.chromium.org/2899133004/diff/100001/chrome/browser/ui/view... > chrome/browser/ui/views/frame/browser_view.cc:226: view->GetLocalBounds(), > false); > Isn't this separator already being drawn by OpaqueBrowserFrameView[1]? And I > believe you're moving the separator up by 1dip - 1px (i.e. 2px at 300%). > > I would think you should not be adding this here, but instead changing the > layout of the bookmark bar so it's 1dip lower and 1 dip shorter so it doesn't > overlap that other separator. > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/opaque_bro... I was thinking bookmark bar should be able to print and control it's own separators on both top and bottom, so i added the draw commend. I guess your suggestion makes sense if the top separator belongs to the toolbar. Let me update the code.
On 2017/05/26 18:30:33, yiyix wrote: > @sky, please let me know if you have other concerns. > > https://codereview.chromium.org/2899133004/diff/100001/chrome/browser/ui/view... > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): > > https://codereview.chromium.org/2899133004/diff/100001/chrome/browser/ui/view... > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:608: > layer()->SetFillsBoundsOpaquely(true); > On 2017/05/26 13:40:53, sky wrote: > > I'm surprised this would work. Don't we need the the bookmark bar to show > > through in some cases? Same themes or animations or on the ntp when detached? > > I believe that it's opaque for both attached and detached bookmark bars. > For themes: > For attached case, we first paint background color of the toolbar, then we paint > the image from the theme for the bookmark bar. (as shown in > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... > ). I believe the background of the toolbar is always opaque. I found this bug, > which requests it to be transparent and marked as "won't fix", > https://bugs.chromium.org/p/chromium/issues/detail?id=244892. I also tried to > search transparent theme in the app store and could not find any. So I was > convinced it's safe to mark the layer as opaque. > > For detached case, I found this comment, which explains the color for view must > be painted opaquely and the layer can be semi-transparent, so it's safe to make > the bookmark bar layer as opaque as well. > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... I have confirmed with @bsep, who implements browser themes, that the toolbar color is always opaque (in https://cs.chromium.org/chromium/src/chrome/browser/themes/browser_theme_pack...) I am now 100% certain that I can update that line to layer()->SetFillsBoundsOpaquely(true); I will address @estade's comment and upload a new patch set.
The CQ bit was checked by yiyix@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #3 (id:120001) has been deleted
The CQ bit was checked by yiyix@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yiyix@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #4 (id:160001) has been deleted
The CQ bit was checked by yiyix@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...
Patchset #3 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@sky and @estade, could you please review this new patch?
estade@chromium.org changed reviewers: + pkasting@chromium.org
lgtm but I'd feel more comfortable if pkasting had a look at it as well. https://codereview.chromium.org/2899133004/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2899133004/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:608: layer()->SetFillsBoundsOpaquely(true); you can just remove this line as opacity is the default
@sky and @pkasting: could you please review this patch? Thank you. https://codereview.chromium.org/2899133004/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2899133004/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:608: layer()->SetFillsBoundsOpaquely(true); On 2017/05/30 21:39:33, Evan Stade wrote: > you can just remove this line as opacity is the default Done.
I defer to Peter on this, who is an owner.
I'm not seeing anywhere that creates/destroys layers as the CL description implies. Is that comment out of date or am I misinterpreting it? https://codereview.chromium.org/2899133004/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (left): https://codereview.chromium.org/2899133004/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:516: toolbar_bounds.Inset(-views::NonClientFrameView::kClientEdgeThickness, 0); Can you explain why you removed this horizontal outdent? Is it no longer needed in the MD top chrome world, or did you mistake it for a vertical overlap, or ____?
On 2017/05/31 02:16:03, Peter Kasting wrote: > I'm not seeing anywhere that creates/destroys layers as the CL description > implies. Is that comment out of date or am I misinterpreting it? > > https://codereview.chromium.org/2899133004/diff/200001/chrome/browser/ui/view... > File chrome/browser/ui/views/frame/browser_view.cc (left): > > https://codereview.chromium.org/2899133004/diff/200001/chrome/browser/ui/view... > chrome/browser/ui/views/frame/browser_view.cc:516: > toolbar_bounds.Inset(-views::NonClientFrameView::kClientEdgeThickness, 0); > Can you explain why you removed this horizontal outdent? Is it no longer needed > in the MD top chrome world, or did you mistake it for a vertical overlap, or > ____? The CL description is outdated. I forget to update it after using the new approach. Prior to this change, the toolbar and the bookmark bar is overlapped and tool bar draws the separator which is below the tool bar and on top of the bookmark bar. For example, the NTP bookmark bar has a height of 40 and we only draw its height as 39 on screen. (adjust the bookmark bar location at https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... and change bookmark bar height at https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi...) In this change, I moved the bookmark bar down by 1 px and reduce its height by 1 px. In the function that you are questioning, I am thinking the toolbar bounds are the real bounds now -- they don't need to display the separator outside of its bounds. So i removed the -1 insets for the horizontal separator. Am I misunderstand something? Thanks.
Description was changed from ========== Reduce overdraw on bookmark bar The bookmark bar always contains 2 layers of overdraw. One of the overdraw layer is created to display the showing/hiding animation of bookmark bar only. This layer can be added before animation and destroyed afterwards to reduce raster effort. BUG=725956 ========== to ========== Reduce overdraw on bookmark bar The bookmark bar always experience 2 layers of overdraw because the layers are marked as non-opaque. After research, I noticed that it is always opaque for both attached and detached bookmark bar with or without theme, so the top layer is now marked as opaque. The bookmark bar and the toolbar is overlapped for the toolbar to control the show/hide animation of the separator between them. This cl also updates the location and the height of bookmark bar to avoid the overlapping. BUG=725956 ==========
Description was changed from ========== Reduce overdraw on bookmark bar The bookmark bar always experience 2 layers of overdraw because the layers are marked as non-opaque. After research, I noticed that it is always opaque for both attached and detached bookmark bar with or without theme, so the top layer is now marked as opaque. The bookmark bar and the toolbar is overlapped for the toolbar to control the show/hide animation of the separator between them. This cl also updates the location and the height of bookmark bar to avoid the overlapping. BUG=725956 ========== to ========== Reduce overdraw on bookmark bar The bookmark bar always experience 2 layers of overdraw because the layers are marked as non-opaque. After research, I noticed that it is always opaque for both attached and detached bookmark bar with or without theme, so the top layer is now marked as opaque. The bookmark bar and the toolbar is overlapped for the toolbar to control the show/hide animation of the separator between them. This cl also updates the location and the height of bookmark bar to avoid the overlapping. BUG=725956 ==========
Description was changed from ========== Reduce overdraw on bookmark bar The bookmark bar always experience 2 layers of overdraw because the layers are marked as non-opaque. After research, I noticed that it is always opaque for both attached and detached bookmark bar with or without theme, so the top layer is now marked as opaque. The bookmark bar and the toolbar is overlapped for the toolbar to control the show/hide animation of the separator between them. This cl also updates the location and the height of bookmark bar to avoid the overlapping. BUG=725956 ========== to ========== Reduce overdraw on bookmark bar The bookmark bar always experience 2 layers of overdraw because the layers are marked as non-opaque. After research, I noticed that it is always opaque for both attached and detached bookmark bar with or without theme, so the top layer is now marked as opaque. The bookmark bar and the toolbar is overlapped for the toolbar to control the show/hide animation of the separator between them. This cl also updates the location and the height of bookmark bar to avoid the overlapping. BUG=725956 ==========
Small request: if possible, reply inline at the place where the question was asked, it makes the conversation easier to follow later. On 2017/05/31 15:32:58, yiyix wrote: > In the function that you are questioning, I am thinking the toolbar bounds are > the real bounds now -- they don't need to display the separator outside of its > bounds. So i removed the -1 insets for the horizontal separator. Am I > misunderstand something? I would be skeptical this is correct. The various places that draw the frame (e.g. the opaque frame and glass frame) look like they rely on this (checking the toolbar bounds and painting kClientEdgeThickness-wide rects and the like). I wouldn't be willing to remove this without either (a) verifying that this has no visible effect (and no unpainted area in a debug build) in the opaque and glass frames, or (b) adjusting those frames to handle this change appropriately. If you want to pursue the latter route, I would probably do it in a separate CL to unblock this one.
https://codereview.chromium.org/2899133004/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (left): https://codereview.chromium.org/2899133004/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:516: toolbar_bounds.Inset(-views::NonClientFrameView::kClientEdgeThickness, 0); On 2017/05/31 02:16:03, Peter Kasting wrote: > Can you explain why you removed this horizontal outdent? Is it no longer needed > in the MD top chrome world, or did you mistake it for a vertical overlap, or > ____? Sorry about it. I will pay more attention where I am replying. I will reply to this comment to help you to be in context. I did more research on those lines. You are right! I have mistake it for a vertical overlap! I should not remove those lines! These insets are added to draw the separator in between the tab and the toolbar in the right corner of the browser (right to the tab strips). I have reverted them. Thank you so much! @pkasting, could you please review this patch again? Thank you.
LGTM. I totally understand why this sort of thing is confusing, and FWIW, I'm not 100% convinced the existing code is correctly handling this in all cases. It may be possible to clean it up more along the lines of the change you made -- just probably not without also tweaking the frame drawing code to compensate.
On 2017/06/01 22:22:33, Peter Kasting wrote: > LGTM. > > I totally understand why this sort of thing is confusing, and FWIW, I'm not 100% > convinced the existing code is correctly handling this in all cases. It may be > possible to clean it up more along the lines of the change you made -- just > probably not without also tweaking the frame drawing code to compensate. fwiw this is why I dislike the gfx::Rect::Inset functions that don't just take a gfx::Insets. The order of params doesn't match the gfx::Insets ctors (or views::Border factory functions for that matter).
On 2017/06/01 23:22:56, Evan Stade wrote: > On 2017/06/01 22:22:33, Peter Kasting wrote: > > LGTM. > > > > I totally understand why this sort of thing is confusing, and FWIW, I'm not > 100% > > convinced the existing code is correctly handling this in all cases. It may > be > > possible to clean it up more along the lines of the change you made -- just > > probably not without also tweaking the frame drawing code to compensate. > > fwiw this is why I dislike the gfx::Rect::Inset functions that don't just take a > gfx::Insets. The order of params doesn't match the gfx::Insets ctors (or > views::Border factory functions for that matter). Amusingly enough, we were just discussing changing Insets' constructor args to match the order of everything else, over on the Harmony mailing list. I want to do it, but there wasn't enough widespread support for the idea. If you want to file a bug to remove all non-Insets overrides for Inset(), I'm fine with it. That one's not a windmill I'm going to personally tilt at :)
On 2017/06/01 23:29:46, Peter Kasting wrote: > On 2017/06/01 23:22:56, Evan Stade wrote: > > On 2017/06/01 22:22:33, Peter Kasting wrote: > > > LGTM. > > > > > > I totally understand why this sort of thing is confusing, and FWIW, I'm not > > 100% > > > convinced the existing code is correctly handling this in all cases. It may > > be > > > possible to clean it up more along the lines of the change you made -- just > > > probably not without also tweaking the frame drawing code to compensate. > > > > fwiw this is why I dislike the gfx::Rect::Inset functions that don't just take > a > > gfx::Insets. The order of params doesn't match the gfx::Insets ctors (or > > views::Border factory functions for that matter). > > Amusingly enough, we were just discussing changing Insets' constructor args to > match the order of everything else, over on the Harmony mailing list. I want to > do it, but there wasn't enough widespread support for the idea. > > If you want to file a bug to remove all non-Insets overrides for Inset(), I'm > fine with it. That one's not a windmill I'm going to personally tilt at :) A change in either direction (as long as it's towards consistency) is for the better. If you think the ordering of params for gfx::Insets is weird, it bears mentioning that most of the ctors match CSS property shorthand (e.g. margin, padding, etc.), except in CSS it's top right bottom left.
The CQ bit was checked by yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org Link to the patchset: https://codereview.chromium.org/2899133004/#ps220001 (title: "revert change in browserview")
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": 220001, "attempt_start_ts": 1496367964594660,
"parent_rev": "9939f3a8acf8565958bfc50d01ff7c0f7bd4ebe5", "commit_rev":
"7073ae5bc6af913c649500e015210992f9c7111a"}
Message was sent while issue was closed.
Description was changed from ========== Reduce overdraw on bookmark bar The bookmark bar always experience 2 layers of overdraw because the layers are marked as non-opaque. After research, I noticed that it is always opaque for both attached and detached bookmark bar with or without theme, so the top layer is now marked as opaque. The bookmark bar and the toolbar is overlapped for the toolbar to control the show/hide animation of the separator between them. This cl also updates the location and the height of bookmark bar to avoid the overlapping. BUG=725956 ========== to ========== Reduce overdraw on bookmark bar The bookmark bar always experience 2 layers of overdraw because the layers are marked as non-opaque. After research, I noticed that it is always opaque for both attached and detached bookmark bar with or without theme, so the top layer is now marked as opaque. The bookmark bar and the toolbar is overlapped for the toolbar to control the show/hide animation of the separator between them. This cl also updates the location and the height of bookmark bar to avoid the overlapping. BUG=725956 Review-Url: https://codereview.chromium.org/2899133004 Cr-Commit-Position: refs/heads/master@{#476569} Committed: https://chromium.googlesource.com/chromium/src/+/7073ae5bc6af913c649500e01521... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:220001) as https://chromium.googlesource.com/chromium/src/+/7073ae5bc6af913c649500e01521... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
