New issue
Advanced search Search tips

Issue 787500 link

Starred by 1 user
Status: Fixed
Owner:
nedngu...@google.com
Closed: Dec 2017
Cc:
perezju@chromium.org
pasko@chromium.org
achuith@chromium.org
Components:
EstimatedDays: ----
NextAction: 2017-12-12
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Deprecate credentials_path and credentials fields

Project Member Reported by nedngu...@google.com, Nov 21 2017 
The browser constructor currently does a bunch of logic that are not related to creating a browser, such as:

Handling page credential (through credentials_path  param)
Preparing the platform (line 44 to 56 in https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/browser/browser.py?rcl=37921f135dc61021b8ced4c73375339658ac9953&l=44)


In order to make it easy to support different mode of browser creation, we want to make sure that the browser creation is as lean as possible.

Comment 1 by nedngu...@google.com, Nov 21 2017

Components: Speed>Telemetry

Comment 2 by nedngu...@google.com, Nov 21 2017

Summary: Refactor Browser constructor to be leaner (was: Refactor Browser constructor to be cleaner)
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 22 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/fe4bc2834bd13f1fedb3a545d3fa104f702f93d3

commit fe4bc2834bd13f1fedb3a545d3fa104f702f93d3
Author: Nghia Nguyen <nednguyen@google.com>
Date: Wed Nov 22 11:03:18 2017

Throw exception if users try to record a page with credentials param set


Bug:  chromium:787500 
Change-Id: Iac1592ee74b3fab2ccc5c433e654882e1464cfbf
Reviewed-on: https://chromium-review.googlesource.com/783812
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>
Commit-Queue: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/fe4bc2834bd13f1fedb3a545d3fa104f702f93d3/telemetry/telemetry/page/shared_page_state.py
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 22 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/2126f67e1ca97ed0128fd8945a69e5f6df3ae3f0

commit 2126f67e1ca97ed0128fd8945a69e5f6df3ae3f0
Author: Nghia Nguyen <nednguyen@google.com>
Date: Wed Nov 22 11:40:27 2017

Remove support for credentials_path in Telemetry

Bug:  chromium:787500 
Change-Id: I99361411107f1ffc392278d2f6ca8d9a56daf2d3
Reviewed-on: https://chromium-review.googlesource.com/783668
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>
Commit-Queue: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/2126f67e1ca97ed0128fd8945a69e5f6df3ae3f0/telemetry/telemetry/internal/browser/possible_browser.py
[delete] https://crrev.com/fe4bc2834bd13f1fedb3a545d3fa104f702f93d3/telemetry/telemetry/internal/backends/codepen_credentials_backend_unittest.py
[modify] https://crrev.com/2126f67e1ca97ed0128fd8945a69e5f6df3ae3f0/telemetry/telemetry/page/page_run_end_to_end_unittest.py
[modify] https://crrev.com/2126f67e1ca97ed0128fd8945a69e5f6df3ae3f0/telemetry/telemetry/internal/backends/chrome/cros_browser_with_oobe.py
[modify] https://crrev.com/2126f67e1ca97ed0128fd8945a69e5f6df3ae3f0/telemetry/telemetry/testing/story_set_smoke_test.py
[delete] https://crrev.com/fe4bc2834bd13f1fedb3a545d3fa104f702f93d3/telemetry/telemetry/internal/backends/facebook_credentials_backend_unittest.py
[modify] https://crrev.com/2126f67e1ca97ed0128fd8945a69e5f6df3ae3f0/telemetry/telemetry/internal/backends/chrome/android_browser_finder.py
[modify] https://crrev.com/2126f67e1ca97ed0128fd8945a69e5f6df3ae3f0/telemetry/telemetry/internal/backends/chrome/cros_browser_finder.py
[delete] https://crrev.com/fe4bc2834bd13f1fedb3a545d3fa104f702f93d3/telemetry/telemetry/internal/browser/browser_credentials.py
[delete] https://crrev.com/fe4bc2834bd13f1fedb3a545d3fa104f702f93d3/telemetry/telemetry/internal/backends/google_credentials_backend_unittest.py
[delete] https://crrev.com/fe4bc2834bd13f1fedb3a545d3fa104f702f93d3/telemetry/telemetry/internal/browser/browser_credentials_unittest.py
[modify] https://crrev.com/2126f67e1ca97ed0128fd8945a69e5f6df3ae3f0/telemetry/telemetry/page/shared_page_state.py
[delete] https://crrev.com/fe4bc2834bd13f1fedb3a545d3fa104f702f93d3/telemetry/telemetry/internal/backends/facebook_credentials_backend.py
[modify] https://crrev.com/2126f67e1ca97ed0128fd8945a69e5f6df3ae3f0/telemetry/telemetry/internal/browser/browser.py
[modify] https://crrev.com/2126f67e1ca97ed0128fd8945a69e5f6df3ae3f0/telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py
[delete] https://crrev.com/fe4bc2834bd13f1fedb3a545d3fa104f702f93d3/telemetry/telemetry/internal/backends/google_credentials_backend.py
[delete] https://crrev.com/fe4bc2834bd13f1fedb3a545d3fa104f702f93d3/telemetry/telemetry/internal/backends/codepen_credentials_backend.py
[modify] https://crrev.com/2126f67e1ca97ed0128fd8945a69e5f6df3ae3f0/telemetry/telemetry/internal/browser/browser_unittest.py

Comment 5 by nedngu...@google.com, Nov 28 2017

Cc: pasko@chromium.org
I initially think about moving cache_flushing stuff out of browser.Browser constructor & put them in shared_page_state, right before we start the browser, but it's harder than I thought.

The problem is with these lines:

if self.platform.CanFlushIndividualFilesFromSystemCache():
          self.platform.FlushSystemCacheForDirectory(
              self._browser_backend.profile_directory)
          self.platform.FlushSystemCacheForDirectory(
              self._browser_backend.browser_directory)

Since in shared_page_state, we don't have access to browser_backend, the next logical step I was thinking is trying to move profile_directory & browser_directory out of browser_backend, but it's not simple either:

##### profile_directory #####

Android:
profile_directory = backend_settings.profile_dir, but profile_dir doesn't exist in https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/backends/android_browser_backend_settings.py, so I suspect that this is a bug. I need to run this & add logging to see what's going on there.

ChromeOS:
profile_directory = '/home/chronos/Default'

Dekstop:
profile_directory is a temp file (tmp_dir) generated when browser_backend is initated. Later browser is then started with --user-dir=tmp_dir flag.

##### browser_directory #####

Android: None

ChromeOS: Some dynamic logic to get browser directory based on ChromeProcess https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/backends/chrome/cros_browser_backend.py?rcl=65711d3e974c0960f2d130dd1289222e09b8bd75&l=136

Desktop: The logic is plumbed in here: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py?rcl=65711d3e974c0960f2d130dd1289222e09b8bd75&l=229


So now I am thinking about moving these browser cache flushing logic to possible_browser, which are the places we initialize the browser_backends, then the corresponding browser. 


Thoughts?

Comment 6 by nedngu...@google.com, Nov 28 2017

Labels: -Pri-3 Pri-1

Comment 7 by perezju@chromium.org, Nov 29 2017

Cc: achuith@chromium.org
Moving both profile_directory and browser_directory to possible_browser sgtm; those two sound like things that _should_ be possible to compute before the browser is started.

> ChromeOS: Some dynamic logic to get browser directory based on ChromeProcess

This could be a deal killer. +achuith, is it possible to figure out the browser_directory _before_ starting the browser?

I'm actually not quite sure how this works now. Today we FlushSystemCache for  browser_directory _before_ starting the browser and, at that time, browser_directory returns None?

Comment 8 by nedngu...@google.com, Nov 29 2017 

I found out why profile_directory of Android is buggy, yet we don't see any crash. The reason is the "if self.platform.CanFlushIndividualFilesFromSystemCache()" always return False on Android (https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/platform/android_platform_backend.py?rcl=cbba42a8e6eb4e5f35b02a1e1611e515e7f5013f&l=317)

Comment 9 by pasko@chromium.org, Nov 29 2017 

I also noticed that on Android profile_directory does not work (and it was indeed never called because of comment #8). I tried to take it from browser_options instead of _backend_settings, but it did not immediately help.  Did not look further yet.

Finding profile directory and cache directory is pretty important for benchmarking "coldish" start on Android. 

possible_browser sounds good to me. If we cannot determine profile_directory without starting a browser (on chromeos), we could potentially start the browser lazily once for shared_page_state and memoize the value?

Comment 12 by nedngu...@google.com, Dec 4 2017 

Juan: this bug is done enough to the point that it no longer blocks  issue 784319 . The rest left is deprecate the credential_paths & credential fields.

Comment 13 by perezju@chromium.org, Dec 5 2017

Blocking: -784319
Labels: -Pri-1 Pri-2
Summary: Deprecate credentials_path and credentials fields (was: Refactor Browser constructor to be leaner)
sgtm, thanks!
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 5 2017 

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

commit 71529f78e6399b691dcc0552f6032b574fe5a019
Author: Ned Nguyen <nednguyen@google.com>
Date: Tue Dec 05 11:23:06 2017

Ensure login_utils fetch credential file from cloud storage if not already exist

This allows us to remove credentials_path later on as it's currently handled
by Telemetry's page (https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/page/__init__.py?rcl=a12fb2d1ad961877f3fc1ca4b72965394270419d&l=47)

Bug:  787500 
Change-Id: I74217522183322fd533aaa0e84c029c9f4b9d2b8
Reviewed-on: https://chromium-review.googlesource.com/806802
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>
Commit-Queue: Ned Nguyen <nednguyen@google.com>
Cr-Commit-Position: refs/heads/master@{#521665}
[modify] https://crrev.com/71529f78e6399b691dcc0552f6032b574fe5a019/tools/perf/page_sets/login_helpers/login_utils.py
[modify] https://crrev.com/71529f78e6399b691dcc0552f6032b574fe5a019/tools/perf/page_sets/system_health/accessibility_stories.py
[modify] https://crrev.com/71529f78e6399b691dcc0552f6032b574fe5a019/tools/perf/page_sets/system_health/browsing_stories.py
[modify] https://crrev.com/71529f78e6399b691dcc0552f6032b574fe5a019/tools/perf/page_sets/system_health/loading_stories.py
[modify] https://crrev.com/71529f78e6399b691dcc0552f6032b574fe5a019/tools/perf/page_sets/system_health/long_running_stories.py
[modify] https://crrev.com/71529f78e6399b691dcc0552f6032b574fe5a019/tools/perf/page_sets/system_health/media_stories.py
[modify] https://crrev.com/71529f78e6399b691dcc0552f6032b574fe5a019/tools/perf/page_sets/system_health/system_health_story.py
Project Member

Comment 15 by bugdroid1@chromium.org, Dec 5 2017 

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

commit c0434db086a435fd14856fd7f430d5f049915957
Author: Ned Nguyen <nednguyen@google.com>
Date: Tue Dec 05 14:52:07 2017

Remove credentials & credential_path from Telemetry page

Bug:  787500 
Change-Id: I2966d90b7d9353c91f72b7bf09e1d5e6d120ebc3
Reviewed-on: https://chromium-review.googlesource.com/808544
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>
Commit-Queue: Ned Nguyen <nednguyen@google.com>
Cr-Commit-Position: refs/heads/master@{#521690}
[modify] https://crrev.com/c0434db086a435fd14856fd7f430d5f049915957/tools/perf/page_sets/google_pages.py
[modify] https://crrev.com/c0434db086a435fd14856fd7f430d5f049915957/tools/perf/page_sets/key_desktop_move_cases.py
[modify] https://crrev.com/c0434db086a435fd14856fd7f430d5f049915957/tools/perf/page_sets/key_mobile_sites_pages.py
[modify] https://crrev.com/c0434db086a435fd14856fd7f430d5f049915957/tools/perf/page_sets/key_mobile_sites_smooth.py
[modify] https://crrev.com/c0434db086a435fd14856fd7f430d5f049915957/tools/perf/page_sets/key_silk_cases.py
[modify] https://crrev.com/c0434db086a435fd14856fd7f430d5f049915957/tools/perf/page_sets/pathological_mobile_sites.py
[modify] https://crrev.com/c0434db086a435fd14856fd7f430d5f049915957/tools/perf/page_sets/simple_mobile_sites.py
[modify] https://crrev.com/c0434db086a435fd14856fd7f430d5f049915957/tools/perf/page_sets/top_10.py
[modify] https://crrev.com/c0434db086a435fd14856fd7f430d5f049915957/tools/perf/page_sets/top_25_smooth.py
[modify] https://crrev.com/c0434db086a435fd14856fd7f430d5f049915957/tools/perf/page_sets/top_pages.py
[modify] https://crrev.com/c0434db086a435fd14856fd7f430d5f049915957/tools/perf/page_sets/tough_compositor_cases.py
[modify] https://crrev.com/c0434db086a435fd14856fd7f430d5f049915957/tools/perf/page_sets/tough_pinch_zoom_cases.py
[modify] https://crrev.com/c0434db086a435fd14856fd7f430d5f049915957/tools/perf/page_sets/tough_scheduling_cases.py
[modify] https://crrev.com/c0434db086a435fd14856fd7f430d5f049915957/tools/perf/page_sets/typical_10_mobile.py

Comment 16 by nedngu...@google.com, Dec 7 2017

NextAction: 2017-12-12

Comment 17 by monor...@bugs.chromium.org, Dec 12 2017 

The NextAction date has arrived: 2017-12-12
Project Member

Comment 18 by bugdroid1@chromium.org, Dec 12 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/df4e6461e0051dd4af2c28970b80c80051c02e67

commit df4e6461e0051dd4af2c28970b80c80051c02e67
Author: nednguyen <nednguyen@google.com>
Date: Tue Dec 12 16:13:04 2017

Remove credentials_path & credentials related code in Telemetry

Bug:  chromium:787500 
Change-Id: I643b5e7d375217866eb26da7b09d22e3398380d6
Reviewed-on: https://chromium-review.googlesource.com/822211
Commit-Queue: Ned Nguyen <nednguyen@google.com>
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>

[modify] https://crrev.com/df4e6461e0051dd4af2c28970b80c80051c02e67/telemetry/telemetry/internal/testing/archive_files/test_simple_one_page_set.py
[modify] https://crrev.com/df4e6461e0051dd4af2c28970b80c80051c02e67/telemetry/telemetry/page/page_run_end_to_end_unittest.py
[delete] https://crrev.com/bf837d50904638c8f6893aa3649cb1a34f39f0d9/telemetry/telemetry/internal/backends/form_based_credentials_backend.py
[modify] https://crrev.com/df4e6461e0051dd4af2c28970b80c80051c02e67/telemetry/telemetry/page/__init__.py
[modify] https://crrev.com/df4e6461e0051dd4af2c28970b80c80051c02e67/telemetry/telemetry/page/shared_page_state.py
[modify] https://crrev.com/df4e6461e0051dd4af2c28970b80c80051c02e67/telemetry/telemetry/internal/testing/archive_files/test_simple_two_page_set.py
[modify] https://crrev.com/df4e6461e0051dd4af2c28970b80c80051c02e67/telemetry/telemetry/internal/browser/browser.py
[modify] https://crrev.com/df4e6461e0051dd4af2c28970b80c80051c02e67/telemetry/telemetry/testing/fakes/__init__.py
[modify] https://crrev.com/df4e6461e0051dd4af2c28970b80c80051c02e67/telemetry/telemetry/internal/testing/archive_files/test_page_set.py
[modify] https://crrev.com/df4e6461e0051dd4af2c28970b80c80051c02e67/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
[delete] https://crrev.com/bf837d50904638c8f6893aa3649cb1a34f39f0d9/telemetry/telemetry/internal/backends/form_based_credentials_backend_unittest_base.py

Comment 19 by nedngu...@google.com, Dec 13 2017

Status: Fixed (was: Assigned)

Comment 20 by benhenry@google.com, Jan 16 (6 days ago)

Components: Test>Telemetry

Comment 21 by benhenry@google.com, Jan 16 (6 days ago)

Components: -Speed>Telemetry
►

Sign in to add a comment