Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
955653003:
Adding Maps page to page_sets for telemetry. (Closed)
DescriptionAdding Maps page to page_sets for telemetry.
This CL adds google maps page to telemetry, and runs a synthetic drag
gesture on the page. This helps testing of maps page in telemetry to
track the performance on desktop chrome.
BUG=457148
Committed: https://crrev.com/f502701b2bcc43c05d82e5780b2d6cf2d326bc23
Cr-Commit-Position: refs/heads/master@{#319135}
Committed: https://crrev.com/694ab0859755c71bf2dcb457f427344d3961e2d0
Cr-Commit-Position: refs/heads/master@{#319441}
Patch Set 1 : Single drag. #Patch Set 2 : Multiple actions on Maps benchmark. #
Total comments: 29
Patch Set 3 : Disabled for android. #
Total comments: 16
Patch Set 4 : Added unittest. #
Total comments: 23
Patch Set 5 : Addressed comments. #
Total comments: 6
Patch Set 6 : Fixed nits. #Patch Set 7 : Fixing the archive file. #
Total comments: 1
Patch Set 8 : Relaxing the test. #
Total comments: 3
Patch Set 9 : Addressed comments. #
Total comments: 1
Patch Set 10 : Made changes. #Patch Set 11 : Fixed bugs. #
Total comments: 4
Patch Set 12 : Made changes. #Messages
Total messages: 67 (25 generated)
Patchset #4 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #2 (id:100001) has been deleted
https://codereview.chromium.org/955653003/diff/120001/tools/perf/page_sets/to... File tools/perf/page_sets/top_25_smooth.py (right): https://codereview.chromium.org/955653003/diff/120001/tools/perf/page_sets/to... tools/perf/page_sets/top_25_smooth.py:96: action_runner.Wait(5) I had to include wait here because the HasReachedQuiescence works only once for a page.
ssid@chromium.org changed reviewers: + petrcermak@chromium.org, picksi@chromium.org, skyostil@chromium.org
PTAL.
A bunch of Python and JavaScript comments. Petr https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/action_runner.py (right): https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/action_runner.py:245: """Perform drag gesture on the page. nit: "... a drag gesture ..." https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/action_runner.py:248: height respectively (see drag.js for full implementation). I don't think "respectively" belongs here. It suggests that the start point should be specified in ratios of page width and the end point should be specified in ratios of page height. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/action_runner.py:252: gesture, as a ratio of the visible bounding rectangle for nit: [Take this only as an opinion] I would personally put "as a ratio ... document.body" in parentheses (instead of separating it with a comma). https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/action_runner.py:267: left_start_ratio=left_start_ratio, top_start_ratio=top_start_ratio, I would suggest putting every argument on a single line to make this more readable. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag.js (right): https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.js:13: if (opt_options) { Is there any reason to make the argument optional if you always provide it? https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.js:37: var self = this; No need for "self" because you don't use it in any closure. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.js:40: this.endMeasuringHook = function() {} You don't seem to be really using these hooks anywhere (except for calling the empty functions). I don't think you need to include this functionality now. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.js:47: // Assign this.element_ here instead of constructor, because the constructor Which constructor? The DragAction constructor certainly doesn't. It seems that you must call it explicitly. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag.py (right): https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. Could you provide a short module or class docstring describing the functionality (for example, see seek.py in the same folder)? https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:4: import os There should be a blank line before this line. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:10: # TODO(chrishenry): Ignore attributes, to be deleted when usage in I don't think you should add a TODO for someone else when you add a new file. To be honest, I don't even know what it means. Also, there should be a blank line before the constructor. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:50: tab.ExecuteJavaScript(""" You're mixing single and double quotes in this file. Single quotes are typically preferable. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:53: % (done_callback)) Firstly, you don't need the brackets around "done_callback" because it's a single argument (plus to create a tuple with one item, there's a special syntax: you need to add a comma before the closing bracket). Secondly and more importantly, why do you factor the callback out when it doesn't change? You could simply include it in the string. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:56: if (self._selector is None and self._text is None and Do you expect that these could be empty (i.e. '')? If not, there's no need to use "X is None". You can use "not X" instead, which is shorter. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:61: gesture_source_type = 'chrome.gpuBenchmarking.TOUCH_INPUT' If you wanted to shorten this, you could do the following instead: gesture_source_type = 'chrome.gpuBenchmarking.%s_INPUT' % ( 'TOUCH' if self._use_touch else 'MOUSE')
Patchset #3 (id:130001) has been deleted
Patchset #3 (id:150001) has been deleted
Patchset #3 (id:170001) has been deleted
Patchset #3 (id:190001) has been deleted
Thanks Petr. PTAL. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/action_runner.py (right): https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/action_runner.py:245: """Perform drag gesture on the page. On 2015/02/25 11:35:48, petrcermak wrote: > nit: "... a drag gesture ..." Done. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/action_runner.py:248: height respectively (see drag.js for full implementation). On 2015/02/25 11:35:48, petrcermak wrote: > I don't think "respectively" belongs here. It suggests that the start point > should be specified in ratios of page width and the end point should be > specified in ratios of page height. Done. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/action_runner.py:252: gesture, as a ratio of the visible bounding rectangle for On 2015/02/25 11:35:48, petrcermak wrote: > nit: [Take this only as an opinion] I would personally put "as a ratio ... > document.body" in parentheses (instead of separating it with a comma). keeping this consistent with the file. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/action_runner.py:267: left_start_ratio=left_start_ratio, top_start_ratio=top_start_ratio, On 2015/02/25 11:35:48, petrcermak wrote: > I would suggest putting every argument on a single line to make this more > readable. Keeping this consistent with the style in file. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag.js (right): https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.js:13: if (opt_options) { On 2015/02/25 11:35:48, petrcermak wrote: > Is there any reason to make the argument optional if you always provide it? you are right, it isn't used. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.js:37: var self = this; On 2015/02/25 11:35:48, petrcermak wrote: > No need for "self" because you don't use it in any closure. Done. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag.py (right): https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2015/02/25 11:35:49, petrcermak wrote: > Could you provide a short module or class docstring describing the functionality > (for example, see seek.py in the same folder)? Done. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:4: import os On 2015/02/25 11:35:49, petrcermak wrote: > There should be a blank line before this line. Done. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:10: # TODO(chrishenry): Ignore attributes, to be deleted when usage in On 2015/02/25 11:35:49, petrcermak wrote: > I don't think you should add a TODO for someone else when you add a new file. To > be honest, I don't even know what it means. > > Also, there should be a blank line before the constructor. Done. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:50: tab.ExecuteJavaScript(""" On 2015/02/25 11:35:48, petrcermak wrote: > You're mixing single and double quotes in this file. Single quotes are typically > preferable. Double quotes are needed at few places. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:53: % (done_callback)) On 2015/02/25 11:35:49, petrcermak wrote: > Firstly, you don't need the brackets around "done_callback" because it's a > single argument (plus to create a tuple with one item, there's a special syntax: > you need to add a comma before the closing bracket). > > Secondly and more importantly, why do you factor the callback out when it > doesn't change? You could simply include it in the string. It is separated because the line becomes too large, and too keep some formatting in javascript code run. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:56: if (self._selector is None and self._text is None and On 2015/02/25 11:35:49, petrcermak wrote: > Do you expect that these could be empty (i.e. '')? If not, there's no need to > use "X is None". You can use "not X" instead, which is shorter. Maybe if someone wants to test with empty string. Not very sure. https://codereview.chromium.org/955653003/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:61: gesture_source_type = 'chrome.gpuBenchmarking.TOUCH_INPUT' On 2015/02/25 11:35:49, petrcermak wrote: > If you wanted to shorten this, you could do the following instead: > > gesture_source_type = 'chrome.gpuBenchmarking.%s_INPUT' % ( > 'TOUCH' if self._use_touch else 'MOUSE') This looks more readable, I think.
Looks pretty good. Added some comments. https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... File tools/perf/page_sets/top_25_smooth.py (right): https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... tools/perf/page_sets/top_25_smooth.py:80: """ Why: productivity, top google properties; drag and drop anywhere """ Not sure what you mean by "drag and drop anywhere"? https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... tools/perf/page_sets/top_25_smooth.py:86: action_runner.Wait(2) All this waiting should be done in RunNavigateSteps for the page since we're really waiting for the page to load and that is common for all types of interactions. https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... tools/perf/page_sets/top_25_smooth.py:97: interaction.End() Could you add a TODO here about doing a zoom gesture too which isn't currently possible? https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... File tools/perf/page_sets/top_pages.py (right): https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... tools/perf/page_sets/top_pages.py:101: # Disabled on android since the desktop record of maps don't load in android. nit: s/don't/doesn't/ https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... tools/perf/page_sets/top_pages.py:104: """ Why: productivity, top google properties; drag and drop anywhere """ Same question about the last part of this. https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... tools/perf/page_sets/top_pages.py:113: if (browser_info._browser._platform_backend.platform.GetOSName() == nit: this could be shortened to return ... GetOSName() != 'android' https://codereview.chromium.org/955653003/diff/210001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag.py (right): https://codereview.chromium.org/955653003/diff/210001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:26: class DragAction(page_action.PageAction): We should add a unit test for this too (see scroll_unittest.py).
https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... File tools/perf/page_sets/top_25_smooth.py (right): https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... tools/perf/page_sets/top_25_smooth.py:80: """ Why: productivity, top google properties; drag and drop anywhere """ On 2015/02/27 15:33:05, Sami wrote: > Not sure what you mean by "drag and drop anywhere"? I wrote drag and drop anywhere in sense that you can drag and drop on any point on the page. That is the point of adding this page I guess. Suggestions are welcome.
https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... File tools/perf/page_sets/top_25_smooth.py (right): https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... tools/perf/page_sets/top_25_smooth.py:80: """ Why: productivity, top google properties; drag and drop anywhere """ On 2015/02/27 15:52:04, ssid wrote: > On 2015/02/27 15:33:05, Sami wrote: > > Not sure what you mean by "drag and drop anywhere"? > > I wrote drag and drop anywhere in sense that you can drag and drop on any point > on the page. That is the point of adding this page I guess. Suggestions are > welcome. Sorry, I was just confused because "drag & drop" means something different than just dragging. Maybe just say "supports drag gestures"?
Added unittest. PTAL. https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... File tools/perf/page_sets/top_25_smooth.py (right): https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... tools/perf/page_sets/top_25_smooth.py:80: """ Why: productivity, top google properties; drag and drop anywhere """ On 2015/02/27 16:02:32, Sami wrote: > On 2015/02/27 15:52:04, ssid wrote: > > On 2015/02/27 15:33:05, Sami wrote: > > > Not sure what you mean by "drag and drop anywhere"? > > > > I wrote drag and drop anywhere in sense that you can drag and drop on any > point > > on the page. That is the point of adding this page I guess. Suggestions are > > welcome. > > Sorry, I was just confused because "drag & drop" means something different than > just dragging. Maybe just say "supports drag gestures"? Done. https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... tools/perf/page_sets/top_25_smooth.py:86: action_runner.Wait(2) On 2015/02/27 15:33:05, Sami wrote: > All this waiting should be done in RunNavigateSteps for the page since we're > really waiting for the page to load and that is common for all types of > interactions. Done. https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... tools/perf/page_sets/top_25_smooth.py:97: interaction.End() On 2015/02/27 15:33:05, Sami wrote: > Could you add a TODO here about doing a zoom gesture too which isn't currently > possible? Done. https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... File tools/perf/page_sets/top_pages.py (right): https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... tools/perf/page_sets/top_pages.py:101: # Disabled on android since the desktop record of maps don't load in android. On 2015/02/27 15:33:06, Sami wrote: > nit: s/don't/doesn't/ Done. https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... tools/perf/page_sets/top_pages.py:104: """ Why: productivity, top google properties; drag and drop anywhere """ On 2015/02/27 15:33:06, Sami wrote: > Same question about the last part of this. Done. https://codereview.chromium.org/955653003/diff/210001/tools/perf/page_sets/to... tools/perf/page_sets/top_pages.py:113: if (browser_info._browser._platform_backend.platform.GetOSName() == On 2015/02/27 15:33:06, Sami wrote: > nit: this could be shortened to return ... GetOSName() != 'android' Done. https://codereview.chromium.org/955653003/diff/210001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag.py (right): https://codereview.chromium.org/955653003/diff/210001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:26: class DragAction(page_action.PageAction): On 2015/02/27 15:33:06, Sami wrote: > We should add a unit test for this too (see scroll_unittest.py). Done.
Patchset #4 (id:230001) has been deleted
Neat, thanks! https://codereview.chromium.org/955653003/diff/250001/tools/perf/page_sets/to... File tools/perf/page_sets/top_pages.py (right): https://codereview.chromium.org/955653003/diff/250001/tools/perf/page_sets/to... tools/perf/page_sets/top_pages.py:101: # Disabled on android since the desktop record of maps doesn't load in android. Please move this comment to CanRunOnBrowser https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag_unittest.py (right): https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:8: nit: two blank lines between top-level entries. https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:10: def testDragAction(self): Does this test also work on Android with the mouse events? https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:44: self.assertTrue(div_position_x == div_width / 4, This looks like it might fail if anything adds rounding errors to the coordinates. We should probably make this a little less strict and only make sure we are within one CSS pixel of the result (i.e., cast to an int). Also, could we use assertEquals? https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/unittes... File tools/telemetry/unittest_data/draggable.html (right): https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/unittes... tools/telemetry/unittest_data/draggable.html:30: <body id ="body" onmouseup="drop(event)"> nit: no need for "(event)" here and below.
picksi@google.com changed reviewers: + picksi@google.com
Looking good. I have a couple of nits. https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag.js (right): https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.js:47: rect.left + rect.width * this.options_.left_start_ratio_; nit: calculate... var pixel_width = rect.width * this.options_.left_start_ratio_ and 'pixel_height' (might want to choose a better name!) instead of re-calculating each value twice. https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.js:52: var end_top = nit: Are start_top/end_top, start_left/end_left the correct names for these? Are they really tops & lefts or should they be start_X/end_X and start_Y/end_Y? Ideally we'd keep these in a vector (if such a thing exists in this bit of the code), and then we could use start.x, start.y, end.x, end.y ... but I may be misunderstanding what these values are used for. https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag.py (right): https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.py:100: gesture_source_type) nice! https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag_unittest.py (right): https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:27: window.__dragAction.beginMeasuringHook = function() { You use single quotes around embedded code in drag.py, should probably be consistent. I believe single quotes are preferred. https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:43: # 0.25 is the ratio of displacement to the initial size. nit: Change 0.25 in the comment to '4' or (probably better) change /4 to *0.25 in the code.
PTAL. https://codereview.chromium.org/955653003/diff/250001/tools/perf/page_sets/to... File tools/perf/page_sets/top_pages.py (right): https://codereview.chromium.org/955653003/diff/250001/tools/perf/page_sets/to... tools/perf/page_sets/top_pages.py:101: # Disabled on android since the desktop record of maps doesn't load in android. On 2015/03/02 14:03:56, Sami wrote: > Please move this comment to CanRunOnBrowser Done. https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag.js (right): https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.js:47: rect.left + rect.width * this.options_.left_start_ratio_; On 2015/03/02 14:11:05, picksi wrote: > nit: calculate... > > var pixel_width = rect.width * this.options_.left_start_ratio_ > > and 'pixel_height' (might want to choose a better name!) instead of > re-calculating each value twice. As we discussed, added brackets and these are different variables, and not re-calculated. https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.js:52: var end_top = On 2015/03/02 14:11:05, picksi wrote: > nit: Are start_top/end_top, start_left/end_left the correct names for these? Are > they really tops & lefts or should they be start_X/end_X and start_Y/end_Y? > > Ideally we'd keep these in a vector (if such a thing exists in this bit of the > code), and then we could use start.x, start.y, end.x, end.y ... but I may be > misunderstanding what these values are used for. It is named top and left because the html style properties are named top and left. There is no vector operation being done here. The function of this file is to call the extension API(in c++) using these values, by getting the values from python module to run benchmark. I don't see clearly how to make all of these as vectors, since we can only pass int between them. What do you think? https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag_unittest.py (right): https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:8: On 2015/03/02 14:03:56, Sami wrote: > nit: two blank lines between top-level entries. Done. https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:10: def testDragAction(self): On 2015/03/02 14:03:56, Sami wrote: > Does this test also work on Android with the mouse events? Added touch events to work on android. https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:27: window.__dragAction.beginMeasuringHook = function() { On 2015/03/02 14:11:05, picksi wrote: > You use single quotes around embedded code in drag.py, should probably be > consistent. I believe single quotes are preferred. Is this your suggestion? 'window.__dragAction.beginMeasuringHook = function() {' + ' window.__didBeginMeasuring = true;' + '};' + 'window.__dragAction.endMeasuringHook = function() {' + ' window.__didEndMeasuring = true;' + '};' Isnt it easier to understand the code with the former? https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:43: # 0.25 is the ratio of displacement to the initial size. On 2015/03/02 14:11:05, picksi wrote: > nit: Change 0.25 in the comment to '4' or (probably better) change /4 to *0.25 > in the code. Changed. https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:44: self.assertTrue(div_position_x == div_width / 4, On 2015/03/02 14:03:56, Sami wrote: > This looks like it might fail if anything adds rounding errors to the > coordinates. We should probably make this a little less strict and only make > sure we are within one CSS pixel of the result (i.e., cast to an int). Also, > could we use assertEquals? It converts the result to int because both the operands are ints. But I had to take floor value. Is this correct now? https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/unittes... File tools/telemetry/unittest_data/draggable.html (right): https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/unittes... tools/telemetry/unittest_data/draggable.html:30: <body id ="body" onmouseup="drop(event)"> On 2015/03/02 14:03:56, Sami wrote: > nit: no need for "(event)" here and below. Done.
Looks good. A couple of questions/answers https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag.js (right): https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.js:47: rect.left + rect.width * this.options_.left_start_ratio_; OK. This looks clear with brackets! Thanks. https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag.js:52: var end_top = If these top/left naming reflects their usage this is fine. Also fine if these aren't actually vectors! https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag_unittest.py (right): https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:27: window.__dragAction.beginMeasuringHook = function() { I'm suggesting you either stick with """ or ''', currently you use both (""" here and ''' at line 81 in drag.py) https://codereview.chromium.org/955653003/diff/270001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag_unittest.py (right): https://codereview.chromium.org/955653003/diff/270001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:46: expected_x = math.floor(div_width * -0.25) Where has the minus signed come from? I'm confused!
lgtm with a couple of nits and once Simon is happy. https://codereview.chromium.org/955653003/diff/270001/tools/telemetry/unittes... File tools/telemetry/unittest_data/draggable.html (right): https://codereview.chromium.org/955653003/diff/270001/tools/telemetry/unittes... tools/telemetry/unittest_data/draggable.html:4: body { margin:0; } nit: add a space after the ':' (here and below). https://codereview.chromium.org/955653003/diff/270001/tools/telemetry/unittes... tools/telemetry/unittest_data/draggable.html:47: document.getElementById('body').addEventListener("mouseup", drop); nit: Use single quotes consistently (here and below).
https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag_unittest.py (right): https://codereview.chromium.org/955653003/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:27: window.__dragAction.beginMeasuringHook = function() { On 2015/03/03 13:36:06, picksi wrote: > I'm suggesting you either stick with """ or ''', currently you use both (""" > here and ''' at line 81 in drag.py) Yes! now I I get it. Thanks. https://codereview.chromium.org/955653003/diff/270001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag_unittest.py (right): https://codereview.chromium.org/955653003/diff/270001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:46: expected_x = math.floor(div_width * -0.25) On 2015/03/03 13:36:06, picksi wrote: > Where has the minus signed come from? I'm confused! I changed the ratio moved to drag up instead of down, since in android drag down means refresh page, and disabling that will complicate the code. So made it drag up. https://codereview.chromium.org/955653003/diff/270001/tools/telemetry/unittes... File tools/telemetry/unittest_data/draggable.html (right): https://codereview.chromium.org/955653003/diff/270001/tools/telemetry/unittes... tools/telemetry/unittest_data/draggable.html:4: body { margin:0; } On 2015/03/03 13:46:15, Sami wrote: > nit: add a space after the ':' (here and below). Done. https://codereview.chromium.org/955653003/diff/270001/tools/telemetry/unittes... tools/telemetry/unittest_data/draggable.html:47: document.getElementById('body').addEventListener("mouseup", drop); On 2015/03/03 13:46:15, Sami wrote: > nit: Use single quotes consistently (here and below). Done.
Great! Thanks. LGTM!
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/955653003/#ps290001 (title: "Fixed nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/955653003/290001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ssid@chromium.org changed reviewers: - picksi@google.com
There was a change in the name of archive file by someone, which caused the error. Moved it to the new file now. PTAL. https://codereview.chromium.org/955653003/diff/310001/tools/perf/page_sets/da... File tools/perf/page_sets/data/top_25_smooth.json (right): https://codereview.chromium.org/955653003/diff/310001/tools/perf/page_sets/da... tools/perf/page_sets/data/top_25_smooth.json:9: ], This was moved to different file after the CL crrev.com/966283002 . So, moving this.
Great, thanks for figuring out the problem. Still lgtm.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from picksi@google.com Link to the patchset: https://codereview.chromium.org/955653003/#ps310001 (title: "Fixing the archive file.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/955653003/310001
Message was sent while issue was closed.
Committed patchset #7 (id:310001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f502701b2bcc43c05d82e5780b2d6cf2d326bc23 Cr-Commit-Position: refs/heads/master@{#319135}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:310001) has been created in https://codereview.chromium.org/982683003/ by xhwang@chromium.org. The reason for reverting is: Failed tests in drag_unittest.py: http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac10.9%20Tests&....
Message was sent while issue was closed.
cut-n-paste from the log, the failure is:
[1/1] telemetry.page.actions.drag_unittest.DragActionTest.testDragAction failed
unexpectedly:
Traceback (most recent call last):
File
"/Volumes/data/b/build/slave/Mac10_9_Tests/build/src/tools/telemetry/telemetry/page/actions/drag_unittest.py",
line 54, in testDragAction
(div_position_y, expected_y))
AssertionError: Moved element's top coordinate: -175, expected: -176
Looks like a rounding error... ?
Message was sent while issue was closed.
Relaxed the check, since it rounds off pixels. PTAL
Message was sent while issue was closed.
https://codereview.chromium.org/955653003/diff/330001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag_unittest.py (right): https://codereview.chromium.org/955653003/diff/330001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:16: return expected * (1 + error) <= value <= expected * (1 - error) nit: this could be simplified to "abs((value - expected) / expected) <= error". Also, assert that expected is non-zero.
Message was sent while issue was closed.
picksi@google.com changed reviewers: + picksi@google.com
Message was sent while issue was closed.
I like the idea for the fix. I have some unformed thoughts below, but I might be over thinking the whole thing :) https://codereview.chromium.org/955653003/diff/330001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag_unittest.py (right): https://codereview.chromium.org/955653003/diff/330001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:16: return expected * (1 + error) <= value <= expected * (1 - error) I agree with Sami's simplification, this could be re-arranged to avoid the division by 0 (bikeshed...bikeshed...) into error_range = expected * error return abs(value - expected) < error_range Also you should probably rename 'error' to be something like error_bar_ratio or error_range... something that is more descriptive. And a final thought, does this actually need to be a ratio? Could it just be an absolute pixel number? Are we really happy for a 10 pixel error across a 100 pixel drag? https://codereview.chromium.org/955653003/diff/330001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:56: self.assertTrue(self.checkWithinRange(div_position_x, expected_x, 0.1), I would suggest removing the #10%... comment and replacing it with a well-named var, e.g. error_range=0.1 instead. This way the code says what it's doing rather than the comments. This variable should also be named the same as the name of the parameter inside the function. Hmmm. Does it actually need to be passed at all? Could it just be a hard coded 0.1 inside the function?
Message was sent while issue was closed.
On 2015/03/05 12:02:26, picksi wrote: > I like the idea for the fix. I have some unformed thoughts below, but I might be > over thinking the whole thing :) > > https://codereview.chromium.org/955653003/diff/330001/tools/telemetry/telemet... > File tools/telemetry/telemetry/page/actions/drag_unittest.py (right): > > https://codereview.chromium.org/955653003/diff/330001/tools/telemetry/telemet... > tools/telemetry/telemetry/page/actions/drag_unittest.py:16: return expected * (1 > + error) <= value <= expected * (1 - error) > I agree with Sami's simplification, this could be re-arranged to avoid the > division by 0 (bikeshed...bikeshed...) into > error_range = expected * error > return abs(value - expected) < error_range > > Also you should probably rename 'error' to be something like error_bar_ratio or > error_range... something that is more descriptive. > > And a final thought, does this actually need to be a ratio? Could it just be an > absolute pixel number? Are we really happy for a 10 pixel error across a 100 > pixel drag? An absolute pixel error would work fine too I think, and it would be yet simpler. > https://codereview.chromium.org/955653003/diff/330001/tools/telemetry/telemet... > tools/telemetry/telemetry/page/actions/drag_unittest.py:56: > self.assertTrue(self.checkWithinRange(div_position_x, expected_x, 0.1), > I would suggest removing the #10%... comment and replacing it with a well-named > var, e.g. error_range=0.1 instead. This way the code says what it's doing rather > than the comments. This variable should also be named the same as the name of > the parameter inside the function. > > Hmmm. Does it actually need to be passed at all? Could it just be a hard coded > 0.1 inside the function?
Message was sent while issue was closed.
Patchset #9 (id:350001) has been deleted
Message was sent while issue was closed.
Made changes. Thanks
Message was sent while issue was closed.
Great thanks, LGTM!
Message was sent while issue was closed.
Great thanks, LGTM!
Message was sent while issue was closed.
Thanks, lgtm with a nit. https://codereview.chromium.org/955653003/diff/370001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag_unittest.py (right): https://codereview.chromium.org/955653003/diff/370001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:12: def checkWithinRange(self, value, expected, error_ratio): CamelCase naming please.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from picksi@google.com, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/955653003/#ps390001 (title: "Made changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/955653003/390001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Fix for the previous CL. https://codereview.chromium.org/955653003/diff/410001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/action_runner.py (right): https://codereview.chromium.org/955653003/diff/410001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/action_runner.py:245: selector=None, text=None, element_function=None): These are not used in this test. Added for usage in next test for scrolling. https://codereview.chromium.org/955653003/diff/410001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/drag_unittest.py (right): https://codereview.chromium.org/955653003/diff/410001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/drag_unittest.py:30: left_end_ratio=0.25, top_end_ratio=0.25) Changed because of the error in previous CL. See crrev.com/980403003
Looks good!
https://codereview.chromium.org/955653003/diff/410001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/action_runner.py (right): https://codereview.chromium.org/955653003/diff/410001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/action_runner.py:245: selector=None, text=None, element_function=None): On 2015/03/05 21:12:22, ssid wrote: > These are not used in this test. Added for usage in next test for scrolling. Let's leave these for the next patch instead of bunching unrelated changes into a single patch.
Sorry for many changes. https://codereview.chromium.org/955653003/diff/410001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/action_runner.py (right): https://codereview.chromium.org/955653003/diff/410001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/action_runner.py:245: selector=None, text=None, element_function=None): On 2015/03/06 12:11:20, Sami wrote: > On 2015/03/05 21:12:22, ssid wrote: > > These are not used in this test. Added for usage in next test for scrolling. > > Let's leave these for the next patch instead of bunching unrelated changes into > a single patch. I thought it was part of this patch. Changed it.
Thank you, lgtm.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from picksi@google.com Link to the patchset: https://codereview.chromium.org/955653003/#ps430001 (title: "Made changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/955653003/430001
Message was sent while issue was closed.
Committed patchset #12 (id:430001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/694ab0859755c71bf2dcb457f427344d3961e2d0 Cr-Commit-Position: refs/heads/master@{#319441} |
