Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1871033002:
Remove code for pregenerated profile handling from SharedPageState (Closed)
DescriptionRemove code for pregenerated profile handling from SharedPageState
The CL in https://codereview.chromium.org/1884493002/ creates a new
child class with logic to handle pregenerated profile. This CL removes this logic
from SharedPageState.
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/69f4fceb524f320d1afafd6c1e3f3f78939a1a8d
Patch Set 1 #Patch Set 2 : Remove code for pregenerated profile handling from SharedPageState #Patch Set 3 : Resolve conflicts #Messages
Total messages: 23 (11 generated)
Description was changed from ========== Use temporary directory for pregenerated profile extraction. Now the downloaded profile is extracted to the fixed directory inside the repository tree, which might lead to bugs if multiple tests want to use the same profile simultaneously. This CL extracts the profile to a temporary directory to avoid these problems. BUG=catapult:# ========== to ========== Use temporary directory for pregenerated profile extraction. Now the downloaded profile is extracted to the fixed directory inside the repository tree, which might lead to bugs if multiple tests want to use the same profile simultaneously. This CL extracts the profile to a temporary directory to avoid these problems. BUG=catapult:# ==========
gurrrik@yandex-team.ru changed reviewers: + zhenw@chromium.org
gurrrik@yandex-team.ru changed reviewers: + nednguyen@google.com
On 2016/04/08 13:12:42, gurrrik wrote: > mailto:gurrrik@yandex-team.ru changed reviewers: > + mailto:nednguyen@google.com Please take a look.
nednguyen@google.com changed reviewers: + eakuefner@chromium.org - zhenw@chromium.org
+Ethan: can you take a pass at this?
I think the right approach here is to first move this profile migration/downloading functionality out into possible_browser so that we don't just keep piling on shared_page_state. Ned, WDYT? gurrrik: Will you be willing to move the code related to profile management into possible_browser before doing this fix?
On 2016/04/11 at 18:35:47, eakuefner wrote: > I think the right approach here is to first move this profile migration/downloading functionality out into possible_browser so that we don't just keep piling on shared_page_state. > > Ned, WDYT? > gurrrik: Will you be willing to move the code related to profile management into possible_browser before doing this fix? Actually, we realized that the only place this code is really used is in https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/page_se... -- this is probably where the code should be migrated.
On 2016/04/11 19:52:04, eakuefner wrote: > On 2016/04/11 at 18:35:47, eakuefner wrote: > > I think the right approach here is to first move this profile > migration/downloading functionality out into possible_browser so that we don't > just keep piling on shared_page_state. > > > > Ned, WDYT? > > gurrrik: Will you be willing to move the code related to profile management > into possible_browser before doing this fix? > > Actually, we realized that the only place this code is really used is in > https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/page_se... > -- this is probably where the code should be migrated. Sure, I'll move the code. Should I create a new review for that?
On 2016/04/12 08:23:03, gurrrik wrote: > On 2016/04/11 19:52:04, eakuefner wrote: > > On 2016/04/11 at 18:35:47, eakuefner wrote: > > > I think the right approach here is to first move this profile > > migration/downloading functionality out into possible_browser so that we don't > > just keep piling on shared_page_state. > > > > > > Ned, WDYT? > > > gurrrik: Will you be willing to move the code related to profile management > > into possible_browser before doing this fix? > > > > Actually, we realized that the only place this code is really used is in > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/page_se... > > -- this is probably where the code should be migrated. > > Sure, I'll move the code. Should I create a new review for that? Oh, that code is inside chromium repo, so obviously it will be in another review.
Description was changed from ========== Use temporary directory for pregenerated profile extraction. Now the downloaded profile is extracted to the fixed directory inside the repository tree, which might lead to bugs if multiple tests want to use the same profile simultaneously. This CL extracts the profile to a temporary directory to avoid these problems. BUG=catapult:# ========== to ========== Remove code for pregenerated profile handling from SharedPageState The CL in https://codereview.chromium.org/1884493002/ creates a new child class with logic to handle pregenerated profile. This CL removes this logic from SharedPageState. BUG= ==========
Description was changed from ========== Remove code for pregenerated profile handling from SharedPageState The CL in https://codereview.chromium.org/1884493002/ creates a new child class with logic to handle pregenerated profile. This CL removes this logic from SharedPageState. BUG= ========== to ========== Remove code for pregenerated profile handling from SharedPageState The CL in https://codereview.chromium.org/1884493002/ creates a new child class with logic to handle pregenerated profile. This CL removes this logic from SharedPageState. ==========
I've uploaded a new patchset which removes the code from SharedPageState class, since it is moved to a new class in https://codereview.chromium.org/1884493002/ Please take a look.
lgtm
The CQ bit was checked by gurrrik@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1871033002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871033002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...) Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)
The CQ bit was checked by gurrrik@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1871033002/#ps40001 (title: "Resolve conflicts")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1871033002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871033002/40001
Message was sent while issue was closed.
Description was changed from ========== Remove code for pregenerated profile handling from SharedPageState The CL in https://codereview.chromium.org/1884493002/ creates a new child class with logic to handle pregenerated profile. This CL removes this logic from SharedPageState. ========== to ========== Remove code for pregenerated profile handling from SharedPageState The CL in https://codereview.chromium.org/1884493002/ creates a new child class with logic to handle pregenerated profile. This CL removes this logic from SharedPageState. Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
