|
|
DescriptionRename 'warmCache' to 'warm_cache'
This CL renames an identifier 'warmCache' to 'warm_cache' as the same form with others.
BUG=chromium:736697
Review-Url: https://chromiumcodereview.appspot.com/3016563002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/e1aa3179fac3ded8df9856820f270694572a10d3
Patch Set 1 #
Total comments: 4
Patch Set 2 : do not remove ensure_diskcache #
Dependent Patchsets: Messages
Total messages: 21 (8 generated)
yukiy@google.com changed reviewers: + kouhei@chromium.org, nednguyen@google.com, shimazu@chromium.org
PTAL
Code lgtm. Please use a bit more detailed title like "Remove redundant tab preparation on initial run when ensuring cache temperature"?
Description was changed from ========== Remove 'ensure_diskcache' This CL removes 'ensure_diskcache'. It ensures there are any tabs, but browser always have at least one tab, so we do not have to execute this. Also renames 'warmCache' to 'warm_cache' as the same form with others. BUG=chromium:736697 ========== to ========== Remove redundant tab preparation on initial run when ensuring cache temperature This CL removes 'ensure_diskcache'. It ensures there are any tabs, but browser always have at least one tab, so we do not have to execute this. Also renames 'warmCache' to 'warm_cache' as the same form with others. BUG=chromium:736697 ==========
On 2017/09/19 07:57:44, shimazu wrote: > Code lgtm. > Please use a bit more detailed title like "Remove redundant tab preparation on > initial run when ensuring cache temperature"? I modified the title. Thanks!
lgtm On 2017/09/19 08:35:52, yukiy wrote: > On 2017/09/19 07:57:44, shimazu wrote: > > Code lgtm. > > Please use a bit more detailed title like "Remove redundant tab preparation on > > initial run when ensuring cache temperature"? > > I modified the title. Thanks! Description should be one line w/o line breaks.
https://codereview.chromium.org/3016563002/diff/1/telemetry/telemetry/page/ca... File telemetry/telemetry/page/cache_temperature.py (left): https://codereview.chromium.org/3016563002/diff/1/telemetry/telemetry/page/ca... telemetry/telemetry/page/cache_temperature.py:68: tab.Navigate("http://does.not.exist") Wait, where is this logic not needed any more?
https://codereview.chromium.org/3016563002/diff/1/telemetry/telemetry/page/ca... File telemetry/telemetry/page/cache_temperature.py (left): https://codereview.chromium.org/3016563002/diff/1/telemetry/telemetry/page/ca... telemetry/telemetry/page/cache_temperature.py:68: tab.Navigate("http://does.not.exist") On 2017/09/19 09:40:27, nednguyen wrote: > Wait, where is this logic not needed any more? This logic ensures that browser has any tab, but browser always has at least one tab according to TabListBackend. https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... kouhei@ is this explanetion correct?
https://codereview.chromium.org/3016563002/diff/1/telemetry/telemetry/page/ca... File telemetry/telemetry/page/cache_temperature.py (left): https://codereview.chromium.org/3016563002/diff/1/telemetry/telemetry/page/ca... telemetry/telemetry/page/cache_temperature.py:68: tab.Navigate("http://does.not.exist") On 2017/09/19 10:08:20, yukiy wrote: > On 2017/09/19 09:40:27, nednguyen wrote: > > Wait, where is this logic not needed any more? > > This logic ensures that browser has any tab, but browser always has at least one > tab according to TabListBackend. > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > kouhei@ is this explanetion correct? I recall it does more than that. Otherwise we wouldn't need the "Navigate("http://does.not.exist")"
On 2017/09/19 10:25:00, nednguyen wrote: > https://codereview.chromium.org/3016563002/diff/1/telemetry/telemetry/page/ca... > File telemetry/telemetry/page/cache_temperature.py (left): > > https://codereview.chromium.org/3016563002/diff/1/telemetry/telemetry/page/ca... > telemetry/telemetry/page/cache_temperature.py:68: > tab.Navigate("http://does.not.exist") > On 2017/09/19 10:08:20, yukiy wrote: > > On 2017/09/19 09:40:27, nednguyen wrote: > > > Wait, where is this logic not needed any more? > > > > This logic ensures that browser has any tab, but browser always has at least > one > > tab according to TabListBackend. > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > > kouhei@ is this explanetion correct? > > I recall it does more than that. Otherwise we wouldn't need the > "Navigate("http://does.not.exist")" I used git blame to get the history of this: https://codereview.chromium.org/1984973002/diff/20001/telemetry/telemetry/pag... That navigation was to ensure disk cache before we navigate
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Remove redundant tab preparation on initial run when ensuring cache temperature This CL removes 'ensure_diskcache'. It ensures there are any tabs, but browser always have at least one tab, so we do not have to execute this. Also renames 'warmCache' to 'warm_cache' as the same form with others. BUG=chromium:736697 ========== to ========== Rename 'warmCache' to 'warm_cache' This CL renames an identifier 'warmCache' to 'warm_cache' as the same form with others. BUG=chromium:736697 ==========
Sorry for late reply, PTAL! https://codereview.chromium.org/3016563002/diff/1/telemetry/telemetry/page/ca... File telemetry/telemetry/page/cache_temperature.py (left): https://codereview.chromium.org/3016563002/diff/1/telemetry/telemetry/page/ca... telemetry/telemetry/page/cache_temperature.py:68: tab.Navigate("http://does.not.exist") On 2017/09/19 10:25:00, nednguyen wrote: > On 2017/09/19 10:08:20, yukiy wrote: > > On 2017/09/19 09:40:27, nednguyen wrote: > > > Wait, where is this logic not needed any more? > > > > This logic ensures that browser has any tab, but browser always has at least > one > > tab according to TabListBackend. > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > > kouhei@ is this explanetion correct? > > I recall it does more than that. Otherwise we wouldn't need the > "Navigate("http://does.not.exist")" > I used git blame to get the history of this: > https://codereview.chromium.org/1984973002/diff/20001/telemetry/telemetry/pag... > > > That navigation was to ensure disk cache before we navigate Thanks ned, as you said this logic can't be removed. https://chromium.googlesource.com/external/github.com/catapult-project/catapu... So I want to do only renaming 'warmCache' to 'warm_cache' in this CL.
On 2017/09/21 07:20:17, yukiy wrote: > Sorry for late reply, PTAL! > > https://codereview.chromium.org/3016563002/diff/1/telemetry/telemetry/page/ca... > File telemetry/telemetry/page/cache_temperature.py (left): > > https://codereview.chromium.org/3016563002/diff/1/telemetry/telemetry/page/ca... > telemetry/telemetry/page/cache_temperature.py:68: > tab.Navigate("http://does.not.exist") > On 2017/09/19 10:25:00, nednguyen wrote: > > On 2017/09/19 10:08:20, yukiy wrote: > > > On 2017/09/19 09:40:27, nednguyen wrote: > > > > Wait, where is this logic not needed any more? > > > > > > This logic ensures that browser has any tab, but browser always has at least > > one > > > tab according to TabListBackend. > > > > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > > > > kouhei@ is this explanetion correct? > > > > I recall it does more than that. Otherwise we wouldn't need the > > "Navigate("http://does.not.exist")" > > > > I used git blame to get the history of this: > > > https://codereview.chromium.org/1984973002/diff/20001/telemetry/telemetry/pag... > > > > > > That navigation was to ensure disk cache before we navigate > > Thanks ned, as you said this logic can't be removed. > https://chromium.googlesource.com/external/github.com/catapult-project/catapu... > So I want to do only renaming 'warmCache' to 'warm_cache' in this CL. lgtm . Though can you add comment to explain what that navigation logic do? This is so we can avoid confusion in the future
On 2017/09/21 08:56:49, nednguyen wrote: > On 2017/09/21 07:20:17, yukiy wrote: > > Sorry for late reply, PTAL! > > > > > https://codereview.chromium.org/3016563002/diff/1/telemetry/telemetry/page/ca... > > File telemetry/telemetry/page/cache_temperature.py (left): > > > > > https://codereview.chromium.org/3016563002/diff/1/telemetry/telemetry/page/ca... > > telemetry/telemetry/page/cache_temperature.py:68: > > tab.Navigate("http://does.not.exist") > > On 2017/09/19 10:25:00, nednguyen wrote: > > > On 2017/09/19 10:08:20, yukiy wrote: > > > > On 2017/09/19 09:40:27, nednguyen wrote: > > > > > Wait, where is this logic not needed any more? > > > > > > > > This logic ensures that browser has any tab, but browser always has at > least > > > one > > > > tab according to TabListBackend. > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > > > > > > kouhei@ is this explanetion correct? > > > > > > I recall it does more than that. Otherwise we wouldn't need the > > > "Navigate("http://does.not.exist")" > > > > > > > I used git blame to get the history of this: > > > > > > https://codereview.chromium.org/1984973002/diff/20001/telemetry/telemetry/pag... > > > > > > > > > That navigation was to ensure disk cache before we navigate > > > > Thanks ned, as you said this logic can't be removed. > > > https://chromium.googlesource.com/external/github.com/catapult-project/catapu... > > So I want to do only renaming 'warmCache' to 'warm_cache' in this CL. > > lgtm . Though can you add comment to explain what that navigation logic do? This > is so we can avoid confusion in the future nvm, I saw you done it in another CL (which is the right way to do this :-)
The CQ bit was checked by yukiy@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from shimazu@chromium.org, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/3016563002/#ps40001 (title: "do not remove ensure_diskcache")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1505985001833420, "parent_rev": "195ce2a78d4cbf2d05a6f5242b993c6a5e38fa85", "commit_rev": "e1aa3179fac3ded8df9856820f270694572a10d3"}
Message was sent while issue was closed.
Description was changed from ========== Rename 'warmCache' to 'warm_cache' This CL renames an identifier 'warmCache' to 'warm_cache' as the same form with others. BUG=chromium:736697 ========== to ========== Rename 'warmCache' to 'warm_cache' This CL renames an identifier 'warmCache' to 'warm_cache' as the same form with others. BUG=chromium:736697 Review-Url: https://chromiumcodereview.appspot.com/3016563002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |