Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15389003:
Loosen a bit the HTML hierarchy required for Tabs to work, Improved UX of the dev tools (Closed)
|
Created:
7 years, 7 months ago by dvh Modified:
7 years, 6 months ago CC:
chromium-reviews, arv+watch_chromium.org, oshima+watch_chromium.org, Gaurav Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description- Loosen a bit the HTML hierarchy required for Tabs to work.
- Improved UX of the dev tools.
a/ "Load unpacked", "Pack" and "Update now" buttons have been moved to the top
b/ The header is now sticky like in chrome://extensions
c/ The list of extensions/apps is centered when the window is expanded
BUG=242747
BUG=242743
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202759
Patch Set 1 #Patch Set 2 : Removed call to console.trace() #
Total comments: 50
Patch Set 3 : Feedback from dbeam. #Patch Set 4 : Remove 'mainThis'. #
Total comments: 6
Patch Set 5 : Introduce RTL, changed non-selected tab color, applied feedback from dbeam. #
Messages
Total messages: 18 (0 generated)
PTAL. It looks like there's no test coverage for tabs.js. Is it worth creating some? tabs.js is used in certificate_viewer (settings > manage certificate) and there's a test but it looks like the test is disabled.
Forgot to set the reviewers.
On 2013/05/23 20:45:30, dvh wrote: > Forgot to set the reviewers. For now, I manually tested in all components using tabs. Still there's a component called "predictors" where it's used. I don't know how to show that UI.
yes, tests are good https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... File chrome/browser/resources/apps_debugger/css/items.css (right): https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:27: margin: 5px auto 0 auto; 5px auto 0 https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:83: padding: 3px 10px 8px 10px; 3px 10px 8px https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:90: margin-left: 50px; RTL? -webkit-margin-start: 50px; might be what you're looking for https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:92: top: 3px; ^ why are you using relative + top? wouldn't margin-top work for this or something? https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:96: background: linear-gradient(to bottom, rgba(255, 255, 255, 1.0), does this work without -webkit? also rgba(255, 255, 255, 1.0) -> white https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:99: left: 0; RTL? just adding right: 0; may be enough... https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:100: position: fixed; ^ does this app ever scroll? https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:107: margin-right: 0; ^ RTL https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... File chrome/browser/resources/apps_debugger/main.html (right): https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:32: i18n-content="appsDevtoolLoadUnpackedButton"></button> 4 \s indent or lining up attributes -- be consistent (below you use 4 \s, here you're lining up) https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:41: placeholder="Search"> l10n? https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:49: <div id="header-bottom-gradient"> nit: <div id="header-bottom-gradient"></div> https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:51: </div> <!-- #header --> ^ we don't really have a precedent for <!-- doing this -->, but I guess it's OK... https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... File ui/webui/resources/js/cr/ui/tabs.js (right): https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:13: while (el != null) { nit: while (el) { https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:14: if (el.nodeName == 'TABBOX') { nit: no curlies https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:19: return el; use https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... instead https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:56: }); }.bind(this); (and get rid of mainThis) https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:67: if (this.tagName == tagName) { nit: no curlies https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:81: function getFirstElementByTagNameFromRoot(tagName) { don't use |this| as the root, change to this pass in an argument as the root and use findAncestor() in ui/webui/resources/js/util.js https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:84: if (elements.length > 0) { nit: no curlies https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:85: result = elements[0]; return elements[0]; https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:87: return result; return null; https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:98: if (element != null) { if (element) { https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:100: child.selected = i == selectedIndex; is this a dom attribute? if so, you can probably use querySelector('tabs[selected]') or something https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:105: if (element != null) { if (element) {
dbeam: Fixed lots of those and some questions. Next steps: - I'm working on some unit tests for tabs.js - I'll fix the RTL once I understood how it works for webui https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... File chrome/browser/resources/apps_debugger/css/items.css (right): https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:27: margin: 5px auto 0 auto; On 2013/05/23 23:06:48, Dan Beam wrote: > 5px auto 0 Done. https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:83: padding: 3px 10px 8px 10px; On 2013/05/23 23:06:48, Dan Beam wrote: > 3px 10px 8px Done. https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:90: margin-left: 50px; On 2013/05/23 23:06:48, Dan Beam wrote: > RTL? -webkit-margin-start: 50px; might be what you're looking for Done. https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:92: top: 3px; On 2013/05/23 23:06:48, Dan Beam wrote: > ^ why are you using relative + top? wouldn't margin-top work for this or > something? I just tried it and since buttons and the header-title will try to keep the same baseline, I won't achieve my result. relative + top just work. https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:96: background: linear-gradient(to bottom, rgba(255, 255, 255, 1.0), On 2013/05/23 23:06:48, Dan Beam wrote: > does this work without -webkit? also rgba(255, 255, 255, 1.0) -> white I found that it works in practice. http://dev.w3.org/csswg/css-images-3/#linear-gradients Is it safe to use that? https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:99: left: 0; On 2013/05/23 23:06:48, Dan Beam wrote: > RTL? just adding right: 0; may be enough... Removed left:0. https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:100: position: fixed; On 2013/05/23 23:06:48, Dan Beam wrote: > ^ does this app ever scroll? The "toolbar" + the gradient at the bottom of the toolbar of this app should stick to the top while the bottom part will scroll. Here's a preview. http://i.imgur.com/k4Zzkii.png https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/css/items.css:107: margin-right: 0; On 2013/05/23 23:06:48, Dan Beam wrote: > ^ RTL I removed "margin-right: 0". I'll investigate "float: right" though. https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... File chrome/browser/resources/apps_debugger/main.html (right): https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:2: <html> I tried it to change to the following: <html i18n-values="dir:textdirection;" class="loading"> in order to support RTL but it failed with an exception. "Uncaught TypeError: Cannot call method 'getValue' of undefined", source: chrome-extension://lphgohfeebnhcpiohjndkgbhhkoapkjc/js/main_scripts.js (1363) It failed on the following code (see last line). Is there anything obvious that I missed? << 'i18n-values': function(element, attributeAndKeys, dictionary) { var parts = attributeAndKeys.replace(/\s/g, '').split(/;/); parts.forEach(function(part) { if (!part) return; var attributeAndKeyPair = part.match(/^([^:]+):(.+)$/); if (!attributeAndKeyPair) throw new Error('malformed i18n-values: ' + attributeAndKeys); var propName = attributeAndKeyPair[1]; var propExpr = attributeAndKeyPair[2]; var value = dictionary.getValue(propExpr); // <- here >> https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:32: i18n-content="appsDevtoolLoadUnpackedButton"></button> On 2013/05/23 23:06:48, Dan Beam wrote: > 4 \s indent or lining up attributes -- be consistent (below you use 4 \s, here > you're lining up) Done. https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:41: placeholder="Search"> On 2013/05/23 23:06:48, Dan Beam wrote: > l10n? I removed placeholder="Search". I think that's a glitch. https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:49: <div id="header-bottom-gradient"> On 2013/05/23 23:06:48, Dan Beam wrote: > nit: <div id="header-bottom-gradient"></div> Done. https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:51: </div> <!-- #header --> On 2013/05/23 23:06:48, Dan Beam wrote: > ^ we don't really have a precedent for <!-- doing this -->, but I guess it's > OK... I'll remove that anyway. https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... File ui/webui/resources/js/cr/ui/tabs.js (right): https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:13: while (el != null) { On 2013/05/23 23:06:48, Dan Beam wrote: > nit: while (el) { Used findAncestor() instead. https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:14: if (el.nodeName == 'TABBOX') { On 2013/05/23 23:06:48, Dan Beam wrote: > nit: no curlies Used findAncestor() instead. https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:19: return el; On 2013/05/23 23:06:48, Dan Beam wrote: > use > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > instead Done. https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:56: }); On 2013/05/23 23:06:48, Dan Beam wrote: > }.bind(this); (and get rid of mainThis) Done. https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:67: if (this.tagName == tagName) { On 2013/05/23 23:06:48, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:84: if (elements.length > 0) { On 2013/05/23 23:06:48, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:85: result = elements[0]; On 2013/05/23 23:06:48, Dan Beam wrote: > return elements[0]; Done. https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:87: return result; On 2013/05/23 23:06:48, Dan Beam wrote: > return null; Done. https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:98: if (element != null) { On 2013/05/23 23:06:48, Dan Beam wrote: > if (element) { Done. https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:100: child.selected = i == selectedIndex; On 2013/05/23 23:06:48, Dan Beam wrote: > is this a dom attribute? if so, you can probably use > querySelector('tabs[selected]') or something Actually, it needs to add the selected attribute on a given child and remove the selected attribute on other children too. I'm not sure querySelector would fit that usage. https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:105: if (element != null) { On 2013/05/23 23:06:48, Dan Beam wrote: > if (element) { Done.
https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... File chrome/browser/resources/apps_debugger/main.html (right): https://codereview.chromium.org/15389003/diff/3001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:2: <html> On 2013/05/24 04:33:42, dvh wrote: > I tried it to change to the following: > <html i18n-values="dir:textdirection;" class="loading"> > in order to support RTL but it failed with an exception. > > "Uncaught TypeError: Cannot call method 'getValue' of undefined", source: > chrome-extension://lphgohfeebnhcpiohjndkgbhhkoapkjc/js/main_scripts.js (1363) > > It failed on the following code (see last line). > Is there anything obvious that I missed? > > << > 'i18n-values': function(element, attributeAndKeys, dictionary) { > var parts = attributeAndKeys.replace(/\s/g, '').split(/;/); > parts.forEach(function(part) { > if (!part) > return; > > var attributeAndKeyPair = part.match(/^([^:]+):(.+)$/); > if (!attributeAndKeyPair) > throw new Error('malformed i18n-values: ' + attributeAndKeys); > > var propName = attributeAndKeyPair[1]; > var propExpr = attributeAndKeyPair[2]; > > var value = dictionary.getValue(propExpr); // <- here > >> you probably need to include load_time_data.js and/or run this https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/web_ui_ut... on your i18n dictionary (that you pass from C++ to JS) https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... File ui/webui/resources/js/cr/ui/tabs.js (right): https://codereview.chromium.org/15389003/diff/3001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/tabs.js:100: child.selected = i == selectedIndex; On 2013/05/24 04:33:42, dvh wrote: > On 2013/05/23 23:06:48, Dan Beam wrote: > > is this a dom attribute? if so, you can probably use > > querySelector('tabs[selected]') or something > > Actually, it needs to add the selected attribute on a given child and remove the > selected attribute on other children too. > I'm not sure querySelector would fit that usage. oh, yeah, you're setting it on them... sorry, yeah, keep this as is
Introduced support of LTR. I also tested manually in predictors (chrome://predictors) to make sure that there was no obvious things that I broke. Now, working on unit tests for tabs.js.
lgtm https://codereview.chromium.org/15389003/diff/10005/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/css/items.css (right): https://codereview.chromium.org/15389003/diff/10005/chrome/browser/resources/... chrome/browser/resources/apps_debugger/css/items.css:127: html[dir='rtl'] #search { ^ please move this under the #search {} selector https://codereview.chromium.org/15389003/diff/10005/ui/webui/resources/js/cr/... File ui/webui/resources/js/cr/ui/tabs.js (right): https://codereview.chromium.org/15389003/diff/10005/ui/webui/resources/js/cr/... ui/webui/resources/js/cr/ui/tabs.js:45: tagNames.forEach(function(tagName) { nit: Object.keys(map).forEach(function(tagName) { https://codereview.chromium.org/15389003/diff/10005/ui/webui/resources/js/cr/... ui/webui/resources/js/cr/ui/tabs.js:61: element = this.getElementsByTagName('TABS')[0]; nit: this.querySelector('tabs');
1. -webkit-margin-start I tested: -webkit-margin-start: 50px With top-of-tree, it doesn't work with RTL. In RTL, I had to uncheck/check the "-webkit-margin-start" property to have the correct layout. In LTR, it works properly. Maybe it's because the attributes of <html> is mutated and not taken in account immediately. Is it expected? I filed: crbug/243973 2. unit tests for tabs. I have to figure out how to create a unit tests for tabs. I tried to do something similar to list_selection_model_test.html. I tried to include tabs.js but it seems that decorate() was not called on the whole DOM tree. Am I missing something? I could not find similar UI tests in ui/webui/resources/js/cr/ui (with HTML and JS). Also I could not find how list_selection_model_test.html is referred in the unit tests to run. I could not find easily any valuable documentation on chromium.org. I filed: crbug/243971 Two final questions: Do you think I can create the unit test for tabs.js in a separate CL? Is the "-webkit-margin-start issue" something that I need to investigate before committing this change? https://codereview.chromium.org/15389003/diff/10005/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/css/items.css (right): https://codereview.chromium.org/15389003/diff/10005/chrome/browser/resources/... chrome/browser/resources/apps_debugger/css/items.css:127: html[dir='rtl'] #search { On 2013/05/25 02:43:33, Dan Beam wrote: > ^ please move this under the #search {} selector Done. https://codereview.chromium.org/15389003/diff/10005/ui/webui/resources/js/cr/... File ui/webui/resources/js/cr/ui/tabs.js (right): https://codereview.chromium.org/15389003/diff/10005/ui/webui/resources/js/cr/... ui/webui/resources/js/cr/ui/tabs.js:45: tagNames.forEach(function(tagName) { On 2013/05/25 02:43:33, Dan Beam wrote: > nit: Object.keys(map).forEach(function(tagName) { Done. https://codereview.chromium.org/15389003/diff/10005/ui/webui/resources/js/cr/... ui/webui/resources/js/cr/ui/tabs.js:61: element = this.getElementsByTagName('TABS')[0]; On 2013/05/25 02:43:33, Dan Beam wrote: > nit: this.querySelector('tabs'); Done.
On 2013/05/25 03:53:22, dvh wrote: > 1. -webkit-margin-start > I tested: -webkit-margin-start: 50px > With top-of-tree, it doesn't work with RTL. > In RTL, I had to uncheck/check the "-webkit-margin-start" property to have the > correct layout. > In LTR, it works properly. > > Maybe it's because the attributes of <html> is mutated and not taken in account > immediately. > Is it expected? > > I filed: crbug/243973 > > 2. unit tests for tabs. > I have to figure out how to create a unit tests for tabs. > I tried to do something similar to list_selection_model_test.html. I tried to > include tabs.js but it seems that decorate() was not called on the whole DOM > tree. > Am I missing something? > I could not find similar UI tests in ui/webui/resources/js/cr/ui (with HTML and > JS). > Also I could not find how list_selection_model_test.html is referred in the unit > tests to run. > I could not find easily any valuable documentation on http://chromium.org. > I filed: crbug/243971 > > Two final questions: > Do you think I can create the unit test for tabs.js in a separate CL? Yes. > Is the "-webkit-margin-start issue" something that I need to investigate before > committing this change? I think it's a known issue. > > https://codereview.chromium.org/15389003/diff/10005/chrome/browser/resources/... > File chrome/browser/resources/apps_debugger/css/items.css (right): > > https://codereview.chromium.org/15389003/diff/10005/chrome/browser/resources/... > chrome/browser/resources/apps_debugger/css/items.css:127: html[dir='rtl'] > #search { > On 2013/05/25 02:43:33, Dan Beam wrote: > > ^ please move this under the #search {} selector > > Done. > > https://codereview.chromium.org/15389003/diff/10005/ui/webui/resources/js/cr/... > File ui/webui/resources/js/cr/ui/tabs.js (right): > > https://codereview.chromium.org/15389003/diff/10005/ui/webui/resources/js/cr/... > ui/webui/resources/js/cr/ui/tabs.js:45: tagNames.forEach(function(tagName) { > On 2013/05/25 02:43:33, Dan Beam wrote: > > nit: Object.keys(map).forEach(function(tagName) { > > Done. > > https://codereview.chromium.org/15389003/diff/10005/ui/webui/resources/js/cr/... > ui/webui/resources/js/cr/ui/tabs.js:61: element = > this.getElementsByTagName('TABS')[0]; > On 2013/05/25 02:43:33, Dan Beam wrote: > > nit: this.querySelector('tabs'); > > Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dvh@chromium.org/15389003/18001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Added asargent@ as reviewer for chrome/browser/extensions/api/developer_private/developer_private_api.cc
chrome/browser/extensions/api/developer_private/developer_private_api.cc LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dvh@chromium.org/15389003/18001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dvh@chromium.org/15389003/18001
Message was sent while issue was closed.
Change committed as 202759 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
