Telemetry: Look for a page's display name when replaying it.

Telemetry: Look for a page's display name when replaying it.

When recording a page, we identify it by its "display name" (name if it has one,
otherwise URL unless it's a file). When replaying a page, however, we always use
its URL to look up the correct WPR file. So pages with names cannot be found.

R=tonyg@chromium.org
BUG=344380
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252052

[Dominik Grewe, 2014-02-17 18:00:38]

PTAL

https://codereview.chromium.org/169873002/diff/30001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_set_archive_info.py (right):

https://codereview.chromium.org/169873002/diff/30001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_set_archive_info.py:61: wpr_file =
self._page_name_to_wpr_file.get(page.display_name, None)
This is the only line that really matters. Everything else is just renaming to
indicate we're now dealing with page names and not URLs.

[tonyg, 2014-02-18 00:03:08]

lgtm

I recommend adding a test case which demonstrates the bug.

[Dominik Grewe, 2014-02-18 13:09:24]

On 2014/02/18 00:03:08, tonyg wrote:
> lgtm
> 
> I recommend adding a test case which demonstrates the bug.

I've updated the tests to better reflect how we're actually recording pages,
i.e. using a page's display_name to identify it in the WPR file rather than it's
URL. Adding a name to some of the example pages then exposed the bug. PTAL.

[tonyg, 2014-02-18 15:59:59]

lgtm

[Dominik Grewe, 2014-02-18 16:13:02]

I just noticed a problem. Older page sets still use the URL instead of the
display_name in the WPR file. If those pages have names we now won't find them
(e.g. Blogger in top_25). Do you have a better idea than replacing all of those
URLs with the page names? Or should we change the scripts to handle this (i.e.
look for URL if you can't find display_name)?

[tonyg, 2014-02-18 16:36:20]

On 2014/02/18 16:13:02, Dominik Grewe wrote:
> I just noticed a problem. Older page sets still use the URL instead of the
> display_name in the WPR file. If those pages have names we now won't find them
> (e.g. Blogger in top_25). Do you have a better idea than replacing all of
those
> URLs with the page names? Or should we change the scripts to handle this (i.e.
> look for URL if you can't find display_name)?

Yeah, I think display_name should automatically fall back to an escaped version
of the URL (but it is possible I'm not considering some subtlety as there are a
few of them here).

[Dominik Grewe, 2014-02-18 17:45:27]

On 2014/02/18 16:36:20, tonyg wrote:
> On 2014/02/18 16:13:02, Dominik Grewe wrote:
> > I just noticed a problem. Older page sets still use the URL instead of the
> > display_name in the WPR file. If those pages have names we now won't find
them
> > (e.g. Blogger in top_25). Do you have a better idea than replacing all of
> those
> > URLs with the page names? Or should we change the scripts to handle this
(i.e.
> > look for URL if you can't find display_name)?
> 
> Yeah, I think display_name should automatically fall back to an escaped
version
> of the URL (but it is possible I'm not considering some subtlety as there are
a
> few of them here).

display_name falls back to the URL if there's no name. But if a page has a name,
it returns that. The problem is that in some older WPR JSON files the URL is
used to identify a page even if it has a name.
To work around that we could make PageSetArchiveInfo.WprFilePathForPage fall
back to the URL if a search for display_name didn't return a result. I've
updated the patch.

[tonyg, 2014-02-19 00:35:59]

lgtm

[Dominik Grewe, 2014-02-19 10:42:23]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-02-19 10:42:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/169873002/160001

[commit-bot: I haz the power, 2014-02-19 14:56:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/169873002/160001

[commit-bot: I haz the power, 2014-02-19 15:28:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/169873002/160001

[commit-bot: I haz the power, 2014-02-19 19:54:09]

Change committed as 252052