Add methods for cache_temperature: ClearCache, WarmCache

Add methods for cache_temperature: ClearCache, WarmCache
(refactoring of cache_temperature)

This CL refactors cache_temperature in order to add new
cache_temperature state 'HOT' in the following CL:
https://codereview.chromium.org/3011263002/

BUG=chromium:736697
Review-Url: https://chromiumcodereview.appspot.com/3013213002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/eb2e641ee3d16241847ea7b96ec54e992fb55b75

[yukiy, 2017-09-19 08:32:49]

yukiy@google.com changed reviewers:
+ kouhei@chromium.org, nednguyen@google.com, shimazu@chromium.org

[yukiy, 2017-09-19 08:32:50]

This is a following CL of https://codereview.chromium.org/3016563002/
PTAL;-)

[shimazu, 2017-09-19 09:37:32]

Code lgtm with nits.

Regarding the title, this patch is factoring out or cleaning up the code rather
than adding something, so something like "Clean up cache_temparature by wrapping
code into methods" would be better.

https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca...
File telemetry/telemetry/page/cache_temperature.py (right):

https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca...
telemetry/telemetry/page/cache_temperature.py:59: any_tab = browser.tabs[0]
nit: |tab| seems enough.

https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca...
telemetry/telemetry/page/cache_temperature.py:63: if '#' in page.url:
nit: The opposite condition might be better to reduce indent.

if not '#' in page.url:
  return

[kouhei (in TOK), 2017-09-19 09:38:51]

https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca...
File telemetry/telemetry/page/cache_temperature.py (right):

https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca...
telemetry/telemetry/page/cache_temperature.py:63: if '#' in page.url:
Would you mind keeping AvoidDoubleHashNavigation as is?
Having this here in EnsurePageCacheTemperature is a *hack* and I think this
should ideally live in tab.py Navigate. Factoring out feels like making the hack
permanent.

[nednguyen, 2017-09-19 09:50:05]

https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca...
File telemetry/telemetry/page/cache_temperature.py (right):

https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca...
telemetry/telemetry/page/cache_temperature.py:63: if '#' in page.url:
On 2017/09/19 09:38:51, kouhei (in TOK) wrote:
> Would you mind keeping AvoidDoubleHashNavigation as is?
> Having this here in EnsurePageCacheTemperature is a *hack* and I think this
> should ideally live in tab.py Navigate. Factoring out feels like making the
hack
> permanent.

+1. If the purpose is to make the EnsurePageCacheTemperature code not too
clustered, I would suggest refactor as follow:


class CacheManipulator(object):
    TEMPERATURE = None
    def  PrepareCache(self, previous_page, page, browser):
       raise UnimplementedError

Then you can subclass: AnyCache(CacheManipulator), ColdCache(CacheManipulator),
WarmCache(CacheManipulator), HotCache(CacheManipulator).

The code in EnsurePageCacheTemperature then would be:

 def EnsurePageCacheTemperature(page, browser, previous_page=None):
     for c in all_cache_manipulators:
        if page.temperature == c.TEMPERATURE: 
            c.PrepareCache(previous_page, page, browser)
            return 
     raise UnimplementedError('Unrecognized cache temperature %s' %
page.temperature)

This way, you rely on polymorphism to branch the cache handling logic instead of
if-else.

[yukiy, 2017-09-21 08:19:20]

Description was changed from

==========
Add three methods for cache_temperature: ClearCache, AvoidDoubleHashNavigation,
WarmCache
(refactoring of cache_temperature)

This CL refactors cache_temperature in order to add new
cache_temperature state 'HOT' in the following CL:
https://codereview.chromium.org/3011263002/

BUG=chromium:736697
==========

to

==========
Add three methods for cache_temperature: ClearCache, WarmCache
(refactoring of cache_temperature)

This CL refactors cache_temperature in order to add new
cache_temperature state 'HOT' in the following CL:
https://codereview.chromium.org/3011263002/

BUG=chromium:736697
==========

[yukiy, 2017-09-21 08:23:44]

PTAL

https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca...
File telemetry/telemetry/page/cache_temperature.py (right):

https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca...
telemetry/telemetry/page/cache_temperature.py:59: any_tab = browser.tabs[0]
On 2017/09/19 09:37:31, shimazu wrote:
> nit: |tab| seems enough.

Done.

https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca...
telemetry/telemetry/page/cache_temperature.py:63: if '#' in page.url:
On 2017/09/19 09:37:31, shimazu wrote:
> nit: The opposite condition might be better to reduce indent.
> 
> if not '#' in page.url:
>   return

Acknowledged.

https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca...
telemetry/telemetry/page/cache_temperature.py:63: if '#' in page.url:
On 2017/09/19 09:38:51, kouhei (in TOK) wrote:
> Would you mind keeping AvoidDoubleHashNavigation as is?
> Having this here in EnsurePageCacheTemperature is a *hack* and I think this
> should ideally live in tab.py Navigate. Factoring out feels like making the
hack
> permanent.

Done.

https://codereview.chromium.org/3013213002/diff/1/telemetry/telemetry/page/ca...
telemetry/telemetry/page/cache_temperature.py:63: if '#' in page.url:
On 2017/09/19 09:50:05, nednguyen wrote:
> On 2017/09/19 09:38:51, kouhei (in TOK) wrote:
> > Would you mind keeping AvoidDoubleHashNavigation as is?
> > Having this here in EnsurePageCacheTemperature is a *hack* and I think this
> > should ideally live in tab.py Navigate. Factoring out feels like making the
> hack
> > permanent.
> 
> +1. If the purpose is to make the EnsurePageCacheTemperature code not too
> clustered, I would suggest refactor as follow:
> 
> 
> class CacheManipulator(object):
>     TEMPERATURE = None
>     def  PrepareCache(self, previous_page, page, browser):
>        raise UnimplementedError
> 
> Then you can subclass: AnyCache(CacheManipulator),
ColdCache(CacheManipulator),
> WarmCache(CacheManipulator), HotCache(CacheManipulator).
> 
> The code in EnsurePageCacheTemperature then would be:
> 
>  def EnsurePageCacheTemperature(page, browser, previous_page=None):
>      for c in all_cache_manipulators:
>         if page.temperature == c.TEMPERATURE: 
>             c.PrepareCache(previous_page, page, browser)
>             return 
>      raise UnimplementedError('Unrecognized cache temperature %s' %
> page.temperature)
> 
> This way, you rely on polymorphism to branch the cache handling logic instead
of
> if-else.

I chatted with kouhei@, and decided not to do this now (because my internship
will end at 09/29/2017, sadly).
This will be done in follow-up CLs.

[nednguyen, 2017-09-21 08:58:49]

lgtm

[kouhei (in TOK), 2017-09-22 03:46:08]

lgtm

[yukiy, 2017-09-22 03:47:00]

Description was changed from

==========
Add three methods for cache_temperature: ClearCache, WarmCache
(refactoring of cache_temperature)

This CL refactors cache_temperature in order to add new
cache_temperature state 'HOT' in the following CL:
https://codereview.chromium.org/3011263002/

BUG=chromium:736697
==========

to

==========
Add methods for cache_temperature: ClearCache, WarmCache
(refactoring of cache_temperature)

This CL refactors cache_temperature in order to add new
cache_temperature state 'HOT' in the following CL:
https://codereview.chromium.org/3011263002/

BUG=chromium:736697
==========

[yukiy, 2017-09-22 03:48:14]

The CQ bit was checked by yukiy@google.com

[yukiy, 2017-09-22 03:48:15]

The patchset sent to the CQ was uploaded after l-g-t-m from shimazu@chromium.org
Link to the patchset: https://codereview.chromium.org/3013213002/#ps20001
(title: "remove AvoidDoubleHashNavigation()")

[commit-bot: I haz the power, 2017-09-22 03:48:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-09-22 04:20:53]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1506052094503570,
"parent_rev": "b233ea0e0e23e18000a6e7b6c7232cb67dbee7de", "commit_rev":
"eb2e641ee3d16241847ea7b96ec54e992fb55b75"}

[commit-bot: I haz the power, 2017-09-22 04:20:57]

Description was changed from

==========
Add methods for cache_temperature: ClearCache, WarmCache
(refactoring of cache_temperature)

This CL refactors cache_temperature in order to add new
cache_temperature state 'HOT' in the following CL:
https://codereview.chromium.org/3011263002/

BUG=chromium:736697
==========

to

==========
Add methods for cache_temperature: ClearCache, WarmCache
(refactoring of cache_temperature)

This CL refactors cache_temperature in order to add new
cache_temperature state 'HOT' in the following CL:
https://codereview.chromium.org/3011263002/

BUG=chromium:736697

Review-Url: https://chromiumcodereview.appspot.com/3013213002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-09-22 04:20:58]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...