New issue
Advanced search Search tips

Issue 891150 link

Starred by 0 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocked on:
issue 789094



Sign in to add a comment

More QuickView tests and add <webview> scrolling tests

Project Member Reported by noel@chromium.org, Oct 2

Issue description

 Issue 883117  and no QuickView tests for it.  We should add some scroll tests for the quick view, how hard could it be?

Added first QuickView scroll test https://bugs.chromium.org/883117#c11 but since that bug was closed, continuing here.
 
 
Cc: lucmult@chromium.org joelhockey@chromium.org
> first QuickView scroll test https://bugs.chromium.org/883117#c11 

Ahem, correct link  https://crbug.com/883117#c11 

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1bfb3e9cfe3addb85420c07c79ef4fe5b5d6d803

commit 1bfb3e9cfe3addb85420c07c79ef4fe5b5d6d803
Author: Noel Gordon <noel@chromium.org>
Date: Tue Oct 02 05:18:47 2018

Add FilesApp test resource: tall.html

Add a tall html document for scrolling tests. No change in behavior so
no new tests, just adding a resource.

Bug: 891150
Change-Id: I8069e481c098aae21564e5e3ba43e52b12372a04
Reviewed-on: https://chromium-review.googlesource.com/1256463
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595729}
[add] https://crrev.com/1bfb3e9cfe3addb85420c07c79ef4fe5b5d6d803/chrome/test/data/chromeos/file_manager/tall.html
[modify] https://crrev.com/1bfb3e9cfe3addb85420c07c79ef4fe5b5d6d803/ui/file_manager/integration_tests/test_util.js

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b96523522d03c597199f170e2aeffd5ffced329

commit 4b96523522d03c597199f170e2aeffd5ffced329
Author: Noel Gordon <noel@chromium.org>
Date: Tue Oct 02 05:43:18 2018

Test FilesApp QuickView [open] attribute

FileApp QuickView has an [open] attribute, present when it (QuickView)
is open, and removed when it's not open. Verify the attribute appears,
and disappears, for all quick_view.js tests.

No change in behavior, no new tests.

Bug: 891150
Change-Id: If90ae81cd60cd350497dece3896173f432571ccc
Reviewed-on: https://chromium-review.googlesource.com/1256465
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595732}
[modify] https://crrev.com/4b96523522d03c597199f170e2aeffd5ffced329/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/67e12947007a4d2cc1975d0f366ddbd43b67ef8c

commit 67e12947007a4d2cc1975d0f366ddbd43b67ef8c
Author: Noel Gordon <noel@chromium.org>
Date: Tue Oct 02 07:12:14 2018

Make openQuickViewScrollText robust

The QuickView text <webview> provides no signal that it is loaded, but
it does have a src attribute. That can used as a proxy for loaded, and
help make a test that is robust (to load / script timing) on the bots.

 - make the text <webview> selector precise (.text-content class)
 - change the text scroll test to check the webview src attribute
   is non-empty before proceeding.
 - change to use scrollBy, and send incremental scrolls until the
   the scroll limit is reached [1].

[1] if the bot misses or drops a JS scrollBy request, no problem: just
ask for it again.

Bug: 891150
Change-Id: I0bb49c2b08b3535172819a3c1088fca5ba6fda94
Reviewed-on: https://chromium-review.googlesource.com/1256469
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595747}
[modify] https://crrev.com/67e12947007a4d2cc1975d0f366ddbd43b67ef8c/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0074bc43e28c672cdf9750e15ee70395c9cd5540

commit 0074bc43e28c672cdf9750e15ee70395c9cd5540
Author: Noel Gordon <noel@chromium.org>
Date: Tue Oct 02 11:12:29 2018

Add a Quick View html scrolling test

Teach the <file-safe-media> element more about the loaded state of its
child <webview>: make the <webview> send webview-loaded/cleared events
to its parent <file-safe-media>, and have the parent set that state in
the 'loaded' attribute of the child <webview>.

Add an html scroll test: wait for the <webview> to load its content by
waiting for the 'loaded' attribute. Then call window.scrollBy() on the
<webview> and verify that the <webview> was scrolled.

Minor: files_safe_media_webview_content.js: use document.querySelector
to get the #content element to silence a presubmit warning.

Test: browser_tests --gtest_filter="QuickView*openQuickViewScrollHtml"
Bug: 891150
Change-Id: I7bc10a47ff4e50a16ece910174959596615e6fd7
Reviewed-on: https://chromium-review.googlesource.com/1256399
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595791}
[modify] https://crrev.com/0074bc43e28c672cdf9750e15ee70395c9cd5540/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc
[modify] https://crrev.com/0074bc43e28c672cdf9750e15ee70395c9cd5540/ui/file_manager/file_manager/foreground/elements/files_safe_media.js
[modify] https://crrev.com/0074bc43e28c672cdf9750e15ee70395c9cd5540/ui/file_manager/file_manager/foreground/elements/files_safe_media_webview_content.js
[modify] https://crrev.com/0074bc43e28c672cdf9750e15ee70395c9cd5540/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7a8b31a5fa717bbb20d6ac50d0fd716328dfa073

commit 7a8b31a5fa717bbb20d6ac50d0fd716328dfa073
Author: Noel Gordon <noel@chromium.org>
Date: Wed Oct 03 11:46:54 2018

Add openQuickViewBackgroundColorHtml test

Untrusted content (HTML here) is rendered in Quick View in a <webview>
element. Its parent <file-safe-media> sets the background color of the
rendered HTML and that color must be solid white. Test that.

Bug: 891150
Change-Id: I10975df3e862b4d8746c97adc43aaf18b248dd4b
Reviewed-on: https://chromium-review.googlesource.com/c/1258670
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596183}
[modify] https://crrev.com/7a8b31a5fa717bbb20d6ac50d0fd716328dfa073/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc
[modify] https://crrev.com/7a8b31a5fa717bbb20d6ac50d0fd716328dfa073/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/025fa15c0d7bda7585afc5ab5decb9f873224372

commit 025fa15c0d7bda7585afc5ab5decb9f873224372
Author: Noel Gordon <noel@chromium.org>
Date: Thu Oct 04 05:54:00 2018

Force the QuickView background white when displaying text documents

In  issue 883117  an externally injected stylesheet changed scroll style
of QuickView text documents: scroll bars went missing.

Investigating more, I find the background color of QuickView <webview>
content is also not set, so it is currently possible for an externally
injected stlyesheet to override the background color of text documents
in QuickView by accident.

Prevent such accidents - set the <webview> content background color to
white and add a test to verify the background color.

Bug: 891150
Change-Id: Ic7881ae45b2f36ba94ed245bae3d5944cda0eb2e
Reviewed-on: https://chromium-review.googlesource.com/c/1260784
Reviewed-by: Stuart Langley <slangley@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596517}
[modify] https://crrev.com/025fa15c0d7bda7585afc5ab5decb9f873224372/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc
[modify] https://crrev.com/025fa15c0d7bda7585afc5ab5decb9f873224372/ui/file_manager/file_manager/foreground/elements/files_safe_text_webview_content.css
[modify] https://crrev.com/025fa15c0d7bda7585afc5ab5decb9f873224372/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d5e2217bc4dbc5e6d4271da1a8a286ae8fbd5784

commit d5e2217bc4dbc5e6d4271da1a8a286ae8fbd5784
Author: Noel Gordon <noel@chromium.org>
Date: Fri Oct 05 00:48:03 2018

Update FilesApp test resource tall.html size

The tall.html file size is actually 589 bytes.

Bug: 891150
Change-Id: I60e695a11dca5043312d6134257a6407612cefe2
Reviewed-on: https://chromium-review.googlesource.com/c/1263536
Reviewed-by: Stuart Langley <slangley@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596944}
[modify] https://crrev.com/d5e2217bc4dbc5e6d4271da1a8a286ae8fbd5784/ui/file_manager/integration_tests/test_util.js

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0e8e237c91c4dd964206a16a946e5d668044bf0e

commit 0e8e237c91c4dd964206a16a946e5d668044bf0e
Author: Noel Gordon <noel@chromium.org>
Date: Fri Oct 05 01:13:40 2018

Add FilesApp test resource: small.jpg

Add a small 100x100px jpeg test image. No change in behavior so no new
tests, just adding a resource.

Bug: 891150
Change-Id: I3ce460b3e8778b55fdd04fd9fd63891f32b7259f
Reviewed-on: https://chromium-review.googlesource.com/c/1263515
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Stuart Langley <slangley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596953}
[add] https://crrev.com/0e8e237c91c4dd964206a16a946e5d668044bf0e/chrome/test/data/chromeos/file_manager/small.jpg
[modify] https://crrev.com/0e8e237c91c4dd964206a16a946e5d668044bf0e/ui/file_manager/integration_tests/test_util.js

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/49779c17cc9558c4a112bc3b47f92ae0c9ca2194

commit 49779c17cc9558c4a112bc3b47f92ae0c9ca2194
Author: Noel Gordon <noel@chromium.org>
Date: Fri Oct 05 04:24:16 2018

Add Quick View image load test

Now we have the QuickView <webview> infra to detect the 'loaded' state
of thew <webview> content, add a <file-safe-media> image test and also
check the <webview> renders its content on a transparent black body.

Bug: 891150
Change-Id: I8e6aac85ce09e0fcbe0cbba660630d1427e2962b
Reviewed-on: https://chromium-review.googlesource.com/c/1263678
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596989}
[modify] https://crrev.com/49779c17cc9558c4a112bc3b47f92ae0c9ca2194/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 12 by bugdroid1@chromium.org, Oct 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c34fd4b0ad65336af6f966d789fd017e44a51212

commit c34fd4b0ad65336af6f966d789fd017e44a51212
Author: Noel Gordon <noel@chromium.org>
Date: Fri Oct 05 06:26:01 2018

Add FilesApp PDF test resource: tall.pdf

Add a tall PDF file (derived from tall.txt). No change in behavior, so
no new tests, just adding a resource.

Bug: 891150
Change-Id: Ib5b7f2faec713c8cae23183cc3c55a3f77188a4b
Reviewed-on: https://chromium-review.googlesource.com/c/1263680
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597012}
[add] https://crrev.com/c34fd4b0ad65336af6f966d789fd017e44a51212/chrome/test/data/chromeos/file_manager/tall.pdf
[modify] https://crrev.com/c34fd4b0ad65336af6f966d789fd017e44a51212/ui/file_manager/integration_tests/test_util.js

Project Member

Comment 13 by bugdroid1@chromium.org, Oct 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cad84481927a14dd0804d37ab374721fb3bc343f

commit cad84481927a14dd0804d37ab374721fb3bc343f
Author: Noel Gordon <noel@chromium.org>
Date: Fri Oct 05 07:30:11 2018

Actually enable the openQuickViewImage test ;)

Bug: 891150
Change-Id: I571b3ec2d402d9eee7bf49eae65d3bec8f6c2132
Reviewed-on: https://chromium-review.googlesource.com/c/1264096
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597024}
[modify] https://crrev.com/cad84481927a14dd0804d37ab374721fb3bc343f/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Oct 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24bab0142e61d0fdffcf454d2af2aba36ea679db

commit 24bab0142e61d0fdffcf454d2af2aba36ea679db
Author: Noel Gordon <noel@chromium.org>
Date: Fri Oct 05 08:05:35 2018

Add Quick View video load test

Now we have the QuickView <webview> infra to detect the 'loaded' state
of thew <webview> content, add a <file-safe-media> video test and also
check the <webview> renders its content on a transparent black body.

Bug: 891150
Change-Id: I634a60ba733bfff79ed72b0e75d7024da4c8182c
Reviewed-on: https://chromium-review.googlesource.com/c/1264097
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597028}
[modify] https://crrev.com/24bab0142e61d0fdffcf454d2af2aba36ea679db/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc
[modify] https://crrev.com/24bab0142e61d0fdffcf454d2af2aba36ea679db/ui/file_manager/integration_tests/file_manager/quick_view.js

Adding an <audio> load test should be as straight forward as the <video> #14 and <image> #11 cases.  Only DOM difference is the <audio> tag, however:

https://chromium-review.googlesource.com/c/chromium/src/+/1264138/2

_integration_: the audio test constantly crashes on the chrome-os CQ (RELEASE) in Blink's "inert" bit propagation to frames code.  That code is required per spec, but makes Blink's FlatTreeTraversal code unhappy ...

Received signal 6
#0 0x7f5f455fb8fd base::debug::StackTrace::StackTrace()
#1 0x7f5f4530afaa base::debug::StackTrace::StackTrace()
#2 0x7f5f455fb37f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#3 0x7f5f18cee0c0 <unknown>
#4 0x7f5f17423fcf gsignal
#5 0x7f5f174253fa abort
#6 0x7f5f455fabe6 base::debug::(anonymous namespace)::DebugBreak()
#7 0x7f5f455fabc8 base::debug::BreakDebugger()
#8 0x7f5f45376020 logging::LogMessage::~LogMessage()
#9 0x7f5f1fc3b915 blink::FlatTreeTraversal::AssertPrecondition()
#10 0x7f5f1fd9527c blink::FlatTreeTraversal::ContainsIncludingPseudoElement()
#11 0x7f5f1fdc6e62 blink::Node::IsInert()
#12 0x7f5f2018448a blink::LocalFrame::PropagateInertToChildFrames()
#13 0x7f5f20184415 blink::LocalFrame::SetIsInert()
#14 0x7f5f203b215e blink::InertSubtreesChanged()
#15 0x7f5f203b2697 blink::HTMLDialogElement::showModal()
#16 0x7f5f211c5bd5 blink::HTMLDialogElementV8Internal::showModalMethod()
#17 0x7f5f211c5b3a blink::V8HTMLDialogElement::showModalMethodCallback()
#18 0x7f5f41520a6d v8::internal::FunctionCallbackArguments::Call()
#19 0x7f5f4151f253 v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#20 0x7f5f4151d761 v8::internal::Builtin_Impl_HandleApiCall()
#21 0x7f5f4151d201 v8::internal::Builtin_HandleApiCall()
#22 0x7f5f4123fbb5 <unknown>

The stack trace appears in the unsolved issue: crbug.com/789094
Blockedon: 789094
Project Member

Comment 17 by bugdroid1@chromium.org, Oct 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f2a2e6b62589eb9c9831263080ee26d3253ece23

commit f2a2e6b62589eb9c9831263080ee26d3253ece23
Author: Noel Gordon <noel@chromium.org>
Date: Mon Oct 08 07:11:18 2018

Call <file-safe-media> webview contentChanged when content loads

Add JS content load handlers to the <file-safe-media> webview #content
element: onloadeddata for <video> and <audio>, and onload for the rest
which currently means <image> [1].

Call contentChanged when these load handlers fire: to confirm that the
audio|image|video loaded, and send the 'webview-loaded' message to the
parent <file-safe-media>. The parent <file-safe-media> responds to the
message by setting the <webview> 'loaded' attribute: see CL:1256469.

[1] The 'html' <file-safe-media> use-case already handles 'onload' via
the HTML fetch API, and invokes contentChanged when fetch returns HTML
text to render.

Test: browser_tests --gtest_filter="QuickView*FilesAppBrowserTest*"
Bug: 891150
Change-Id: I12d65a20be3cce570c90b5e0a8fd9e20bd77985c
Reviewed-on: https://chromium-review.googlesource.com/c/1267838
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597491}
[modify] https://crrev.com/f2a2e6b62589eb9c9831263080ee26d3253ece23/ui/file_manager/file_manager/foreground/elements/files_safe_media_webview_content.js

Project Member

Comment 18 by bugdroid1@chromium.org, Oct 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3aef0232407549a2a64df1d67ef95edede182ece

commit 3aef0232407549a2a64df1d67ef95edede182ece
Author: Noel Gordon <noel@chromium.org>
Date: Mon Oct 08 09:17:41 2018

Add Quick View PDF document load test

Add a Quick View loading test for a PDF document. For loading, wait on
the <webview> |src| then detect the PDF plugin <embed> type as a proxy
for "contentChanged".

Note: there's no need to add any PDF scrolling / backgroundColor tests
since those aspects are handled internally by Chrome's PDF viewer.

Bug: 891150
Change-Id: I8d3eb438f1cca6d2d249966fd107a4075c7b67b7
Reviewed-on: https://chromium-review.googlesource.com/c/1267916
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597499}
[modify] https://crrev.com/3aef0232407549a2a64df1d67ef95edede182ece/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc
[modify] https://crrev.com/3aef0232407549a2a64df1d67ef95edede182ece/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 19 by bugdroid1@chromium.org, Oct 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/de22efc00b1701160f546a279041d190de8dd9cd

commit de22efc00b1701160f546a279041d190de8dd9cd
Author: Noel Gordon <noel@chromium.org>
Date: Mon Oct 08 13:01:42 2018

Disable QuickView/FilesAppBrowserTest/openQuickViewPdf on MSAN

The FilesApp QuickView PDF browser test crashes the MSAN bot. Disable
the test in MSAN.

Tbr: slangley, lucmult
No-Presubmit: true
Bug: 768070,891150
Change-Id: I262418dd3b79df11824810c42749cd1b6d78edee
Reviewed-on: https://chromium-review.googlesource.com/c/1267597
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597542}
[modify] https://crrev.com/de22efc00b1701160f546a279041d190de8dd9cd/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Oct 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1a9c89e498d681525a7daadc3f4eeee11f041889

commit 1a9c89e498d681525a7daadc3f4eeee11f041889
Author: Noel Gordon <noel@chromium.org>
Date: Tue Oct 09 01:54:38 2018

Minor: remove extra PDF bug reference, fix a typo, one error check

Make the bug comment reference the crash bug, fix a typo tallPdf, and
no need to call checkIfNoErrorsOccured twice.

Bug: 891150
Change-Id: I40c1ce5ef82ebbb4f78e365310af3fa665ba7e6c
Reviewed-on: https://chromium-review.googlesource.com/c/1270195
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597774}
[modify] https://crrev.com/1a9c89e498d681525a7daadc3f4eeee11f041889/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc
[modify] https://crrev.com/1a9c89e498d681525a7daadc3f4eeee11f041889/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 21 by bugdroid1@chromium.org, Oct 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e57ddf05da94ac767fdd92244ea18eb2810f39b

commit 5e57ddf05da94ac767fdd92244ea18eb2810f39b
Author: Noel Gordon <noel@chromium.org>
Date: Tue Oct 09 04:04:46 2018

Add Quick View load test for a Linux files volume

Cargo-cult the MTP / USB test cases: mount Linux files (Crostini), and
open one of its files in QuickView.

QuickView involves <webview>, as we have seen in this bug. <webview>'s
are only available to chrome extensions and hence we should use a test
system that supports extensions to cover <webview> use.

Bug: 891150
Change-Id: I7fe3814c89125f5c06d4465726fa0cbe851a1659
Reviewed-on: https://chromium-review.googlesource.com/c/1270202
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597794}
[modify] https://crrev.com/5e57ddf05da94ac767fdd92244ea18eb2810f39b/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc
[modify] https://crrev.com/5e57ddf05da94ac767fdd92244ea18eb2810f39b/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 22 by bugdroid1@chromium.org, Oct 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ae8001971c6c5a694ebb7f8db6e6d8531da8400f

commit ae8001971c6c5a694ebb7f8db6e6d8531da8400f
Author: Noel Gordon <noel@chromium.org>
Date: Thu Oct 11 01:38:43 2018

Add Quick View load test for Android files

Add a test to open the Play files (Android) volume, select a file, and
display that file in Quick View.

Bug: 891150
Change-Id: I060027f94c5598de77b1231a57b96eb105ddc2b7
Reviewed-on: https://chromium-review.googlesource.com/c/1273415
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598623}
[modify] https://crrev.com/ae8001971c6c5a694ebb7f8db6e6d8531da8400f/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc
[modify] https://crrev.com/ae8001971c6c5a694ebb7f8db6e6d8531da8400f/ui/file_manager/integration_tests/file_manager/quick_view.js
[modify] https://crrev.com/ae8001971c6c5a694ebb7f8db6e6d8531da8400f/ui/file_manager/integration_tests/test_util.js

Project Member

Comment 23 by bugdroid1@chromium.org, Oct 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ae2fcf0ef323eb487edced1ab876a090b3f9ce1e

commit ae2fcf0ef323eb487edced1ab876a090b3f9ce1e
Author: Noel Gordon <noel@chromium.org>
Date: Fri Oct 12 07:04:47 2018

Add Quick View audio load test

Now we have the QuickView <webview> infra to detect the 'loaded' state
of the <webview> content, add a <file-safe-media> audio test, and also
check the <webview> renders its content on a transparent black body.

This test exposed an issue in Blink node code. When the modal <dialog>
is calling showModal(), its flat tree distribution can be out-of-date,
causing a DCHECK crash when testing if the Quick View <dialog> element
contains sibling <iframe id="command-dispatcher"> [1].

Fix that: FlatTreeTraversal::ContainsIncludingPseudoElement() requires
that its node arguments have up-to-date flat tree distribution. Ensure
the <dialog>'s flat tree distribution is up-of-date _before_ trying to
propagate the "inert" bit into sub-frames.

Add crash test: html/dialog/showmodal-shadow-sibling-frame-crash.html

[1] Known crash reports already exist: issue 789094  issue 804047 .

test: browser_tests --gtest_filter="QuickView*openQuickViewAudio"
Bug: 891150, 789094,  804047 
Change-Id: I714585272fc775c157f6d0bd97143af27bf2b961
Reviewed-on: https://chromium-review.googlesource.com/c/1264138
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Hayato Ito <hayato@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599125}
[modify] https://crrev.com/ae2fcf0ef323eb487edced1ab876a090b3f9ce1e/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc
[add] https://crrev.com/ae2fcf0ef323eb487edced1ab876a090b3f9ce1e/third_party/WebKit/LayoutTests/html/dialog/showmodal-shadow-sibling-frame-crash-expected.txt
[add] https://crrev.com/ae2fcf0ef323eb487edced1ab876a090b3f9ce1e/third_party/WebKit/LayoutTests/html/dialog/showmodal-shadow-sibling-frame-crash.html
[modify] https://crrev.com/ae2fcf0ef323eb487edced1ab876a090b3f9ce1e/third_party/blink/renderer/core/html/html_dialog_element.cc
[modify] https://crrev.com/ae2fcf0ef323eb487edced1ab876a090b3f9ce1e/ui/file_manager/integration_tests/file_manager/quick_view.js

Project Member

Comment 24 by bugdroid1@chromium.org, Oct 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4052e809d3bb03478e56c341cc877e49eed96225

commit 4052e809d3bb03478e56c341cc877e49eed96225
Author: Noel Gordon <noel@chromium.org>
Date: Sat Oct 13 00:18:09 2018

Partially back-off CL:1264138 to use the fix in CL:823582

CL:823582 used ForceLayoutForCentering() which updates the dialog flat
tree distribution as a side-effect. Hence, the forced dialog flat tree
distribution update in CL:1264138 is now redundant and can be removed.

Covered by layout tests:
  html/dialog/showmodal-shadow-sibling-frame-crash.html
  html/dialog/dialog-show-modal-inert-crash.html

Covered by Chrome OS browser test:
  browser_tests --gtest_filter="QuickView*openQuickViewAudio"

Tbr: hayato-san
Bug: 891150, 789094,  804047 
Change-Id: I84f5e0d29aab2c8956b689bb713a9c383471aa80
Reviewed-on: https://chromium-review.googlesource.com/c/1278075
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599436}
[modify] https://crrev.com/4052e809d3bb03478e56c341cc877e49eed96225/third_party/blink/renderer/core/html/html_dialog_element.cc

Cc: benwells@chromium.org
Nice that the last two changes detected / fixed a long-standing blink node code issues.  Integration testing is a two-way street -- keeps our stuff fixed _and_ helps find/fix chromium / blink issues outside of Files app.


Sign in to add a comment