Fix extension id calculation to take into account Windows encoding.

Fix extension id calculation to take into account Windows encoding.

BUG=243532
TEST=updated and ran telemetry unit tests
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204380

[Tim Song, 2013-06-04 03:26:48]

This should fix the test failures on Windows, but I'm not sure if this is the
best approach. Please take a look.

[achuithb, 2013-06-04 18:33:29]

https://codereview.chromium.org/16346003/diff/1/tools/crx_id/crx_id.py
File tools/crx_id/crx_id.py (right):

https://codereview.chromium.org/16346003/diff/1/tools/crx_id/crx_id.py#newcod...
tools/crx_id/crx_id.py:113: def GetPublicKey(filename, from_file_path,
encode_path_for_windows=False):
encode_path_for_windows seems overly verbose. is_win_path?

https://codereview.chromium.org/16346003/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/extension_to_load.py (right):

https://codereview.chromium.org/16346003/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/extension_to_load.py:15: def __init__(self, path,
is_component=False, browser_type=None):
perhaps browser_type should not have a default?

https://codereview.chromium.org/16346003/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/extension_to_load.py:26:
self._is_platform_windows = os.name == 'nt'
Perhaps something shorter? is_win?

https://codereview.chromium.org/16346003/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/extension_to_load.py:32:
self._is_platform_windows = browser_type == 'system'
This doesn't seem right? What if browser_type is debug or release? I think we
want to check that --remote is not set (for the cros case), and whatever the
equivalent android check is, right?

[tonyg, 2013-06-04 18:40:16]

Looks reasonable to me

https://codereview.chromium.org/16346003/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/extension_to_load.py (right):

https://codereview.chromium.org/16346003/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/extension_to_load.py:32:
self._is_platform_windows = browser_type == 'system'
On 2013/06/04 18:33:29, achuith.bhandarkar wrote:
> This doesn't seem right? What if browser_type is debug or release? I think we
> want to check that --remote is not set (for the cros case), and whatever the
> equivalent android check is, right?

Elsewhere we've used:
not browser_type.startswith('android') and not browser_type.startswith('cros')

[Tim Song, 2013-06-05 18:36:27]

https://codereview.chromium.org/16346003/diff/1/tools/crx_id/crx_id.py
File tools/crx_id/crx_id.py (right):

https://codereview.chromium.org/16346003/diff/1/tools/crx_id/crx_id.py#newcod...
tools/crx_id/crx_id.py:113: def GetPublicKey(filename, from_file_path,
encode_path_for_windows=False):
On 2013/06/04 18:33:29, achuith.bhandarkar wrote:
> encode_path_for_windows seems overly verbose. is_win_path?

Done.

https://codereview.chromium.org/16346003/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/extension_to_load.py (right):

https://codereview.chromium.org/16346003/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/extension_to_load.py:15: def __init__(self, path,
is_component=False, browser_type=None):
On 2013/06/04 18:33:29, achuith.bhandarkar wrote:
> perhaps browser_type should not have a default?

This is probably a good idea, so we don't get hard to track down errors in the
future if someone forgets to specify the browser_type when using extensions with
telemetry. Done.

https://codereview.chromium.org/16346003/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/extension_to_load.py:32:
self._is_platform_windows = browser_type == 'system'
On 2013/06/04 18:33:29, achuith.bhandarkar wrote:
> This doesn't seem right? What if browser_type is debug or release? I think we
> want to check that --remote is not set (for the cros case), and whatever the
> equivalent android check is, right?

Done.

[achuithb, 2013-06-05 19:16:46]

lgtm from me. Please wait for Tony

[tonyg, 2013-06-05 21:44:31]

lgtm

[commit-bot: I haz the power, 2013-06-06 00:00:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/16346003/17001

[commit-bot: I haz the power, 2013-06-06 01:28:07]

Change committed as 204380