Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
433873002:
Loading screenshots and simple 1-to-1 comparison of screenshots (Closed)
|
Created:
6 years, 4 months ago by Lisa Ignatyeva Modified:
6 years, 4 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org, Denis Kuznetsov (DE-MUC), dzhioev (left Google) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionContinues https://codereview.chromium.org/405093003, so general description is there.
Now the testing mode has appeared: ScreenshotTester can now load the golden
screenshot from the disk and compare it to the screenshot taken right now.
It still should be turned on with switches. To turn testing with screenshots on,
--enable-screenshot-testing-with-mode=MODE is used, where MODE can be "test" or "update".
"update" updates golden screenshot and requires --golden-screenshots-dir=DIR: place to store golden screenshots. "test" runs comparing golden
screenshot with current one, and --artifacts-dir=DIR can be used, where DIR is
the place to store screenshots that are different from golden ones if they exist,
and also an image representing difference between screenshots. If no
--artifacts-dir switch is found, these bugproofs are saved to the directory with
golden screenshots.
BUG=395653
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286880
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286981
Patch Set 1 #Patch Set 2 : "switch files updated" #
Total comments: 22
Patch Set 3 : minor fixes #Patch Set 4 : some DCHECKs replaced with CHECKs #Patch Set 5 : LOGs replaced with CHECKs #
Total comments: 14
Patch Set 6 : minor style fixes #Patch Set 7 : compiler error fixed #
Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/login_ui_browsertest.cc (right): https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/login_ui_browsertest.cc:146: nit: blank line is not needed here. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/screenshot_tester.cc (right): https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:8: #include <string> nit: remove <string> inclusion since you already included it in a header file. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:56: DCHECK(false); I thought it's better to return false instead of DCHECK(false). The same for line 61. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:67: if (mode == "test") { Could you please define a couple of string constants in an anonymous namespace for "update" and "test", like, for instance: namespace { const char kUpdateMode[] = "update"; const char kTestMode[] = "test"; } // namespace and compare |mode| with them? https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:111: DCHECK(false); It's better add return value for SaveImage() and return false from the method instead of DCHECK(). The same for line #118. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:126: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); nit: replace this DCHECK() by DCHECK_CURRENTLY_ON(). The same for lines #150 and #173. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:156: DCHECK(false); No need in DCHECK(), since path does not exists and following call to GetFileSize will return incorrect value. It's better to return from the method. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:175: ASSERT_FALSE(model == NULL); nit: ASSERT_TRUE(model.get()); ASSERT_TRUE(sample.get()); https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:212: ASSERT_TRUE(screenshots_match); This assertion contradicts with if's condition.
https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/login_ui_browsertest.cc (right): https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/login_ui_browsertest.cc:146: On 2014/07/31 13:19:14, ygorshenin1 wrote: > nit: blank line is not needed here. Done. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/screenshot_tester.cc (right): https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:8: #include <string> On 2014/07/31 13:19:14, ygorshenin1 wrote: > nit: remove <string> inclusion since you already included it in a header file. Done. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:56: DCHECK(false); On 2014/07/31 13:19:14, ygorshenin1 wrote: > I thought it's better to return false instead of DCHECK(false). The same for > line 61. That's actually a question of strictness. I believe that wrong usage of these switches (==usage that makes screenshot testing impossible to work) should cause an immediate fail of the whole process. Returning false will change that to just turning screenshot testing off if something went wrong during initialization, and by now I think that makes less sense. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:67: if (mode == "test") { On 2014/07/31 13:19:14, ygorshenin1 wrote: > Could you please define a couple of string constants in an anonymous namespace > for "update" and "test", like, for instance: > > namespace { > const char kUpdateMode[] = "update"; > const char kTestMode[] = "test"; > } // namespace > > and compare |mode| with them? Done. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:111: DCHECK(false); On 2014/07/31 13:19:14, ygorshenin1 wrote: > It's better add return value for SaveImage() and return false from the method > instead of DCHECK(). The same for line #118. Done. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:126: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2014/07/31 13:19:14, ygorshenin1 wrote: > nit: replace this DCHECK() by DCHECK_CURRENTLY_ON(). The same for lines #150 and > #173. Done. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:156: DCHECK(false); On 2014/07/31 13:19:14, ygorshenin1 wrote: > No need in DCHECK(), since path does not exists and following call to > GetFileSize will return incorrect value. It's better to return from the method. I can return NULL instead of loaded file if something goes wrong, but that seems strange to me. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:175: ASSERT_FALSE(model == NULL); On 2014/07/31 13:19:14, ygorshenin1 wrote: > nit: ASSERT_TRUE(model.get()); > ASSERT_TRUE(sample.get()); Done. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:212: ASSERT_TRUE(screenshots_match); On 2014/07/31 13:19:14, ygorshenin1 wrote: > This assertion contradicts with if's condition. Yes, that was done intentionally. The test is failed when |screenshot_match| is false, but we have to do some logging if the test is failed. So, here is actually written something like: "if we found a mistake, log errors and then fail". I've moved the assertion, now it should look less weird.
https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/screenshot_tester.cc (right): https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:56: DCHECK(false); On 2014/07/31 16:37:39, Lisa Ignatyeva wrote: > On 2014/07/31 13:19:14, ygorshenin1 wrote: > > I thought it's better to return false instead of DCHECK(false). The same for > > line 61. > That's actually a question of strictness. I believe that wrong usage of these > switches (==usage that makes screenshot testing impossible to work) should cause > an immediate fail of the whole process. Returning false will change that to just > turning screenshot testing off if something went wrong during initialization, > and by now I think that makes less sense. So you should replace DCHECK() by CHECK(), because DCHECK's are triggered only in debug builds. Moreover, you could merge two lines to one: CHECK(false) << "No directory ..."; https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:156: DCHECK(false); I suggest you to replace DCHECK by CHECK, as I explained above. On 2014/07/31 16:37:38, Lisa Ignatyeva wrote: > On 2014/07/31 13:19:14, ygorshenin1 wrote: > > No need in DCHECK(), since path does not exists and following call to > > GetFileSize will return incorrect value. It's better to return from the > method. > > I can return NULL instead of loaded file if something goes wrong, but that seems > strange to me.
https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/screenshot_tester.cc (right): https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:56: DCHECK(false); On 2014/07/31 16:50:17, ygorshenin1 wrote: > On 2014/07/31 16:37:39, Lisa Ignatyeva wrote: > > On 2014/07/31 13:19:14, ygorshenin1 wrote: > > > I thought it's better to return false instead of DCHECK(false). The same for > > > line 61. > > That's actually a question of strictness. I believe that wrong usage of these > > switches (==usage that makes screenshot testing impossible to work) should > cause > > an immediate fail of the whole process. Returning false will change that to > just > > turning screenshot testing off if something went wrong during initialization, > > and by now I think that makes less sense. > > So you should replace DCHECK() by CHECK(), because DCHECK's are triggered only > in debug builds. Moreover, you could merge two lines to one: CHECK(false) << "No > directory ..."; Done. https://codereview.chromium.org/433873002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:156: DCHECK(false); On 2014/07/31 16:50:17, ygorshenin1 wrote: > I suggest you to replace DCHECK by CHECK, as I explained above. > > On 2014/07/31 16:37:38, Lisa Ignatyeva wrote: > > On 2014/07/31 13:19:14, ygorshenin1 wrote: > > > No need in DCHECK(), since path does not exists and following call to > > > GetFileSize will return incorrect value. It's better to return from the > > method. > > > > I can return NULL instead of loaded file if something goes wrong, but that > seems > > strange to me. > Done.
lgtm, but please resolve new bunch of comments. https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/screenshot_tester.cc (right): https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:7: #include <algorithm> Where are you using it? https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:30: // Sets test mode for screenshot testing. nit: add a blank line before comment. https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:35: } // namespace nit: add a blank line before "} // namespace" https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:63: } nit: curly braces are not needed here. https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:65: if (!command_line.HasSwitch(chromeos::switches::kGoldenScreenshotsDir)) { nit: curly braces are not needed here. https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:95: return; nit: return is redundant here. https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:100: DCHECK(SaveImage("golden_screenshot", golden_screenshots_dir_, png_data)); Replace DCHECK() by CHECK(), otherwise in release builds call to SaveImage() will be thrown away. https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:162: } nit: curly braces are not needed here.
https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/screenshot_tester.cc (right): https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:7: #include <algorithm> On 2014/07/31 17:28:18, ygorshenin1 wrote: > Where are you using it? It was used before, but I've forgot to remove it :( Removed. https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:30: // Sets test mode for screenshot testing. On 2014/07/31 17:28:18, ygorshenin1 wrote: > nit: add a blank line before comment. Done. https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:35: } // namespace On 2014/07/31 17:28:18, ygorshenin1 wrote: > nit: add a blank line before "} // namespace" Done. https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:63: } On 2014/07/31 17:28:18, ygorshenin1 wrote: > nit: curly braces are not needed here. But I can use them here, as I understand. https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:95: return; On 2014/07/31 17:28:18, ygorshenin1 wrote: > nit: return is redundant here. Done. https://codereview.chromium.org/433873002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/screenshot_tester.cc:100: DCHECK(SaveImage("golden_screenshot", golden_screenshots_dir_, png_data)); On 2014/07/31 17:28:18, ygorshenin1 wrote: > Replace DCHECK() by CHECK(), otherwise in release builds call to SaveImage() > will be thrown away. Done.
The CQ bit was checked by elizavetai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/elizavetai@chromium.org/433873002/100001
Message was sent while issue was closed.
Change committed as 286880
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/423713006/ by scheib@chromium.org. The reason for reverting is: Compile failures: http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2... From log: FAILED: /b/build/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/chrome/browser/chromeos/login/interactive_ui_tests.screenshot_tester.o.d -DV8_DEPRECATION_WARNINGS -DBLINK_SCALE_FILTERS_AT_RECORD_TIME -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DCR_CLANG_REVISION=214024 -DTOOLKIT_VIEWS=1 -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_ASH=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_X11=1 -DUSE_XI2_MT=2 -DIMAGE_LOADER_EXTENSION=1 -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DUSE_PROPRIETARY_CODECS -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DUSE_UDEV -DENABLE_EGLIMAGE=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DCLD2_DATA_SOURCE=static -DENABLE_FULL_PRINTING=1 -DENABLE_PRINTING=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_MANAGED_USERS=1 -DENABLE_MDNS=1 -DENABLE_SERVICE_DISCOVERY=1 -DHAS_OUT_OF_PROC_TEST_RUNNER -DGL_GLEXT_PROTOTYPES -DMOJO_USE_SYSTEM_IMPL -DLIBPEERCONNECTION_LIB=1 -DUSE_BRLAPI -DGTEST_HAS_POSIX_RE=0 -DNACL_WINDOWS=0 -DNACL_LINUX=1 -DNACL_OSX=0 -DNACL_ANDROID=0 -DNACL_TARGET_SUBARCH=64 -DNACL_TARGET_ARCH=x86 -DNACL_BUILD_SUBARCH=64 -DNACL_BUILD_ARCH=x86 -DAUTOFILL_ENABLE_SYNC -DSK_ENABLE_INST_COUNT=0 -DSK_SUPPORT_GPU=1 '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"' -DSK_ENABLE_LEGACY_API_ALIASING=1 -DSK_ATTR_DEPRECATED=SK_NOTHING_ARG1 -DGR_GL_IGNORE_ES3_MSAA=0 -DSK_WILL_NEVER_DRAW_PERSPECTIVE_TEXT -DSK_SUPPORT_LEGACY_PICTURE_CLONE -DSK_SUPPORT_LEGACY_GETDEVICE -DSK_IGNORE_ETC1_SUPPORT -DSK_IGNORE_GPU_DITHER -DSK_USE_POSIX_THREADS -DSK_DEFERRED_CANVAS_USES_FACTORIES=1 -DPROTOBUF_USE_DLLS -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DUNIT_TEST -DGTEST_HAS_RTTI=0 -DHUNSPELL_STATIC -DHUNSPELL_CHROME_CLIENT -DUSE_HUNSPELL -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DCHROME_PNG_WRITE_SUPPORT -DPNG_USER_CONFIG -DUSE_NSS=1 -DOS_CHROMEOS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -Igen -I../.. -I../../skia/config -I../../third_party/khronos -I../../gpu -I../../third_party/WebKit/Source -I../../third_party/WebKit -Igen/chrome -I../../third_party/WebKit -I../../v8/include -I../../third_party/libwebp -I../../third_party/ots/include -I../../third_party/qcms/src -I../../third_party/iccjpeg -I../../third_party/libjpeg -Igen/policy -Igen/protoc_out -Igen/net -I../../third_party/skia/src/core -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../skia/ext -I../../third_party/protobuf -I../../third_party/protobuf/src -I../../testing/gmock/include -I../../testing/gtest/include -I../../third_party/icu/source/i18n -I../../third_party/icu/source/common -I../../third_party/libpng -I../../third_party/npapi -I../../third_party/npapi/bindings -I../../third_party/zlib -Igen/webkit -I../../net/third_party/nss/ssl -Igen/ash/resources -Werror -pthread -fno-exceptions -fno-strict-aliasing -Wall -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC -Wheader-hygiene -Wno-char-subscripts -Wno-unneeded-internal-declaration -Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing -Wno-reserved-user-defined-literal -Wno-deprecated-register -Xclang -load -Xclang /b/build/slave/Linux_ChromiumOS_Builder/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -add-plugin -Xclang find-bad-constructs -fcolor-diagnostics -B/b/build/slave/Linux_ChromiumOS_Builder/build/src/third_party/binutils/Linux_x64/Release/bin -pthread -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/nss -I/usr/include/nspr -Wno-header-guard -m64 -march=x86-64 -O2 -fdata-sections -ffunction-sections -fno-slp-vectorize -funwind-tables -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -Wsign-compare -std=gnu++11 -c ../../chrome/browser/chromeos/login/screenshot_tester.cc -o obj/chrome/browser/chromeos/login/interactive_ui_tests.screenshot_tester.o ../../chrome/browser/chromeos/login/screenshot_tester.cc:33:37:error: expected ';' after top level declarator const char kUpdateMode[] = "update;" ^ ; ../../chrome/browser/chromeos/login/screenshot_tester.cc:167:30:error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare] if (golden_screenshot_size == -1) { ~~~~~~~~~~~~~~~~~~~~~~ ^ ~~ ../../chrome/browser/chromeos/login/screenshot_tester.cc:194:26:error: no member named 'config' in 'SkBitmap' ASSERT_EQ(model_bitmap.config(), sample_bitmap.config()); ~~~~~~~~~~~~ ^ ../../testing/gtest/include/gtest/gtest.h:1954:48: note: expanded from macro 'ASSERT_EQ' # define ASSERT_EQ(val1, val2) GTEST_ASSERT_EQ(val1, val2) ^ ../../testing/gtest/include/gtest/gtest.h:1937:55: note: expanded from macro 'GTEST_ASSERT_EQ' EqHelper<GTEST_IS_NULL_LITERAL_(expected)>::Compare, \ ^ ..
Message was sent while issue was closed.
Compiler error is fixed and now it seems to be working.
lgtm
The CQ bit was checked by elizavetai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/elizavetai@chromium.org/433873002/120001
Message was sent while issue was closed.
Change committed as 286981 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
