New issue
Advanced search Search tips

Issue 903669 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 903670



Sign in to add a comment

Trim down unused JS in CrOS' ui/file_manager/* apps

Project Member Reported by tapted@chromium.org, Nov 9

Issue description

Chrome Version       : 72.0.3595.2

This applies to CrOS files app, gallery, video_player, audio_player

Currently they have a lot of JS that isn't used in release, or is redundant E.g.
 - test code, or
 - code used by other apps that comes in via shared libraries, or
 - code included in the background page, and included again in each foreground window
 
Blocking: 903670
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 9

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

commit ac464a7ca158d1559ec1620cfb041ee1d4592878
Author: Trent Apted <tapted@chromium.org>
Date: Fri Nov 09 04:56:39 2018

CrOS Files: Don't bake-in so much test code to background_scripts.js.

Instead, load it lazily the first time a testing extension connects
and wants to use the remote call APIs.

This avoids having to parse the testing code every time any of the
ui/file_manager apps start up (in release, or in tests). Note that
current tests (e.g. gallery) may start up a background page with this
test code up to *four times* for each test. After this change, only
the app under test will load the testing code.

There may still be some added latency. To help balance that, this
CL caches the result of RemoteCall.isStepByStepEnabled(). That makes
it consistent with the newly added function anyway.

This still distributes the testing code in release, which is not ideal.
Loading from a filesystem:// URL might avoid that in future. This should
probably also use an ES6 module.. Baby steps.

Bug: 903669
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I7acf6b55b10fa775d40bae48b81b0cfd1859df56
Reviewed-on: https://chromium-review.googlesource.com/c/1322340
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606742}
[modify] https://crrev.com/ac464a7ca158d1559ec1620cfb041ee1d4592878/ui/file_manager/file_manager/background/js/BUILD.gn
[add] https://crrev.com/ac464a7ca158d1559ec1620cfb041ee1d4592878/ui/file_manager/file_manager/background/js/runtime_loaded_test_util.js
[modify] https://crrev.com/ac464a7ca158d1559ec1620cfb041ee1d4592878/ui/file_manager/file_manager/background/js/test_util_base.js
[modify] https://crrev.com/ac464a7ca158d1559ec1620cfb041ee1d4592878/ui/file_manager/file_manager/manifest.json
[modify] https://crrev.com/ac464a7ca158d1559ec1620cfb041ee1d4592878/ui/file_manager/file_manager/test/js/BUILD.gn
[modify] https://crrev.com/ac464a7ca158d1559ec1620cfb041ee1d4592878/ui/file_manager/file_manager/test/scripts/create_test_main.py
[modify] https://crrev.com/ac464a7ca158d1559ec1620cfb041ee1d4592878/ui/file_manager/file_manager_resources.grd
[modify] https://crrev.com/ac464a7ca158d1559ec1620cfb041ee1d4592878/ui/file_manager/integration_tests/remote_call.js

Cc: lucmult@chromium.org noel@chromium.org
^^^ The above was pretty good.  Only one test complained on ASAN only [1] with a TIMEOUT PASS flake.

[1] LauncherSearch/FilesAppBrowserTest.Test/launcherOpenSearchResult

Looking at the logs of that test, I noticed the output of this line added in the above CL

  throw new Error('Expected enableTesting');

Also, lucmult@ mentioned that the browser test step-by-step flag wasn't working correctly when a 'step()' command was entered into the DevTools console _after about 30 secs_.

The 30 seconds tipped us off: the background page gets killed, and is reloaded
when someone sends it a chrome.test.sendMessage.  When that happens, the state of the window.IN_TEST and other JS state get reset.

We have fix coming for this, so no worries.





Project Member

Comment 4 by bugdroid1@chromium.org, Nov 14

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

commit 5f770283367ccbd002dafe0079f571a4fa7151a3
Author: Luciano Pacheco <lucmult@chromium.org>
Date: Wed Nov 14 02:03:39 2018

Files app: Fix test stepper with async load of test functions

After crrev.com/c/1322340 which changed test functions to be lazy
loaded (asynchronously), running tests in step-by-step mode wasn't
working if tried to step after Files app background page was put to
sleep for inactivity.

There were 2 issues:

1. The logic was relying on a global state window.IN_TEST which gets
lost when extension goes to sleep.

2 Any queued response was replied with sendResponse(true), which was
skipping the execution of the test function that it requested to run.

This CL fixes both of these issues by doing:

1. Instead of relying on window.IN_TEST, it checks if test functions
are loaded with: "test.util.executeTestMessage !== null", if so runs
the requested function, otherwise en-queues the request and async-loads
the test functions.

2. Change to en-queue both the request and the sendResponse to be able
to run the requested functions once the test functions are fully
loaded.

This new design, made |enableTesting| and |ensureLoaded_| routines not
necessary anymore and they were removed.

Test: Manually run tests with the flags: --ui-test-action-timeout=100000
--enable-file-manager-step-by-step-tests  --remote-debugging-port=9222
and waited all extensions to disappear before issuing a step. Also
triggered MSAN and ASAN bots in addition to normal bots.

Bug: 903669
Change-Id: Ia66f7ce7cd920a7a815340119591ff5ff957603c
Reviewed-on: https://chromium-review.googlesource.com/c/1333177
Commit-Queue: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607842}
[modify] https://crrev.com/5f770283367ccbd002dafe0079f571a4fa7151a3/ui/file_manager/file_manager/background/js/test_util_base.js
[modify] https://crrev.com/5f770283367ccbd002dafe0079f571a4fa7151a3/ui/file_manager/integration_tests/remote_call.js

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 14

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

commit 9ecc4552b89f948973fe97a0fdb3bb53b8426d97
Author: Luciano Pacheco <lucmult@chromium.org>
Date: Wed Nov 14 08:17:46 2018

Files app: propagate IN_TEST to dialogs too

Files app dialogs (file picker / save as) aren't opened by background
page, thus they didn't get the IN_TEST state forwarded to them. This CL
changes Files app background to attach IN_TEST to the dialog during
dialog registration. This happens before the UI initialization.

This fixes the issue where test functions can't use test-only attributes
to select elements in the page, for example when trying to use
|navigateWithDirectoryTree| which relies on |full-path-for-testing|
attribute.

Bug: 903669
Change-Id: I8ad19965cddbc6bd6779c9c6f2710872b830754c
Reviewed-on: https://chromium-review.googlesource.com/c/1334992
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607935}
[modify] https://crrev.com/9ecc4552b89f948973fe97a0fdb3bb53b8426d97/ui/file_manager/file_manager/background/js/background.js

All good here. #4 made FilesAppBrowserTest.Test/launcherOpenSearchResult solid green: no flakes.

Sign in to add a comment