Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2617503002:
[telemetry] Let ScrollToElement callers pass root (Closed)
Description[Telemetry] Support specifying container element for action_runner.ScrollToElement API
Allows callers of ScrollToElement specify which element should be
scrolled (previously this was hard coded to document.scrollingElement
or document.body if scrollingElement was not set).
Review-Url: https://codereview.chromium.org/2617503002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/e6b04fef1997055d90390256fe96282bbbd98a51
Patch Set 1 #
Total comments: 3
Patch Set 2 : Improve docs. #Patch Set 3 : Rebase. #
Total comments: 1
Patch Set 4 : Update comments. #Patch Set 5 : please ignore #Patch Set 6 : add tests #Patch Set 7 : fix typo #
Total comments: 12
Patch Set 8 : fixes #Patch Set 9 : add coverage in action_runner_unittest #Patch Set 10 : disable on android #
Messages
Total messages: 32 (14 generated)
Description was changed from ========== [telemetry] Let ScrollToElement callers pass root Allows callers of ScrollToElement specify which element should be scrolled (previously this was hard coded to document.scrollingElement or document.body if scrollingElement was not set). ========== to ========== [telemetry] Let ScrollToElement callers pass root Allows callers of ScrollToElement specify which element should be scrolled (previously this was hard coded to document.scrollingElement or document.body if scrollingElement was not set). ==========
hjd@chromium.org changed reviewers: + nednguyen@google.com, perezju@chromium.org
ptal, thanks! +nednguyen for owners stamp
https://codereview.chromium.org/2617503002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/actions/action_runner.py (right): https://codereview.chromium.org/2617503002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/actions/action_runner.py:419: scroll_selector=None, scroll_element_function=None, Maybe these should be called container_selector and container_element_function. Then the one-liner doc should read something like: "Perform scroll gesture on a container until an element is in view". You can then also add a note before the args explaining that both container and element can be given by a CSS selector or a js function, as well as explaining the default cases for the container. https://codereview.chromium.org/2617503002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/actions/scroll_to_element.py (right): https://codereview.chromium.org/2617503002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/actions/scroll_to_element.py:13: scroll_selector=None, scroll_element_function=None, ditto on variable names
Updated, thanks! https://codereview.chromium.org/2617503002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/actions/action_runner.py (right): https://codereview.chromium.org/2617503002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/actions/action_runner.py:419: scroll_selector=None, scroll_element_function=None, On 2017/01/04 14:57:31, perezju wrote: > Maybe these should be called container_selector and container_element_function. > > Then the one-liner doc should read something like: "Perform scroll gesture on a > container until an element is in view". You can then also add a note before the > args explaining that both container and element can be given by a CSS selector > or a js function, as well as explaining the default cases for the container. Those do seem like better names! Also updated comments.
lgtm w/nit thanks! https://codereview.chromium.org/2617503002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/action_runner.py (right): https://codereview.chromium.org/2617503002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/action_runner.py:429: xor a JavaScript function which returns an element provided as a string. nit: for clarity I would rewrite as "a JavaScript function, provided as a string, which returns an element."
On 2017/01/04 15:37:59, perezju wrote: > lgtm w/nit > > thanks! > > https://codereview.chromium.org/2617503002/diff/40001/telemetry/telemetry/int... > File telemetry/telemetry/internal/actions/action_runner.py (right): > > https://codereview.chromium.org/2617503002/diff/40001/telemetry/telemetry/int... > telemetry/telemetry/internal/actions/action_runner.py:429: xor a JavaScript > function which returns an element provided as a string. > nit: for clarity I would rewrite as "a JavaScript function, provided as a > string, which returns an element." Done.
On 2017/01/04 16:37:04, hjd wrote: > On 2017/01/04 15:37:59, perezju wrote: > > lgtm w/nit > > > > thanks! > > > > > https://codereview.chromium.org/2617503002/diff/40001/telemetry/telemetry/int... > > File telemetry/telemetry/internal/actions/action_runner.py (right): > > > > > https://codereview.chromium.org/2617503002/diff/40001/telemetry/telemetry/int... > > telemetry/telemetry/internal/actions/action_runner.py:429: xor a JavaScript > > function which returns an element provided as a string. > > nit: for clarity I would rewrite as "a JavaScript function, provided as a > > string, which returns an element." > > Done. friendly ping :) Could I get an owners stamp for this please Ned, I think it's done now.
On 2017/01/10 14:26:38, hjd wrote: > On 2017/01/04 16:37:04, hjd wrote: > > On 2017/01/04 15:37:59, perezju wrote: > > > lgtm w/nit > > > > > > thanks! > > > > > > > > > https://codereview.chromium.org/2617503002/diff/40001/telemetry/telemetry/int... > > > File telemetry/telemetry/internal/actions/action_runner.py (right): > > > > > > > > > https://codereview.chromium.org/2617503002/diff/40001/telemetry/telemetry/int... > > > telemetry/telemetry/internal/actions/action_runner.py:429: xor a JavaScript > > > function which returns an element provided as a string. > > > nit: for clarity I would rewrite as "a JavaScript function, provided as a > > > string, which returns an element." > > > > Done. > > friendly ping :) Could I get an owners stamp for this please Ned, I think it's > done now. Oops, sorry for missing this. a Can you add unittest? Also please update the description to "[Telemetry] Support specifying container element for action_runner.ScrollToElement API"
Description was changed from ========== [telemetry] Let ScrollToElement callers pass root Allows callers of ScrollToElement specify which element should be scrolled (previously this was hard coded to document.scrollingElement or document.body if scrollingElement was not set). ========== to ========== [Telemetry] Support specifying container element for action_runner.ScrollToElement API Allows callers of ScrollToElement specify which element should be scrolled (previously this was hard coded to document.scrollingElement or document.body if scrollingElement was not set). ==========
On 2017/01/10 14:32:19, nednguyen wrote: > On 2017/01/10 14:26:38, hjd wrote: > > On 2017/01/04 16:37:04, hjd wrote: > > > On 2017/01/04 15:37:59, perezju wrote: > > > > lgtm w/nit > > > > > > > > thanks! > > > > > > > > > > > > > > https://codereview.chromium.org/2617503002/diff/40001/telemetry/telemetry/int... > > > > File telemetry/telemetry/internal/actions/action_runner.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2617503002/diff/40001/telemetry/telemetry/int... > > > > telemetry/telemetry/internal/actions/action_runner.py:429: xor a > JavaScript > > > > function which returns an element provided as a string. > > > > nit: for clarity I would rewrite as "a JavaScript function, provided as a > > > > string, which returns an element." > > > > > > Done. > > > > friendly ping :) Could I get an owners stamp for this please Ned, I think it's > > done now. > > Oops, sorry for missing this. > a > Can you add unittest? > Also please update the description to "[Telemetry] Support specifying container > element for action_runner.ScrollToElement API" Thanks! Updated title and added tests for ScrollToElement also found a bug in gesture_common which I think I introduced when fixing the original so bug in gesture_common so I rewrote that function to be a lot simpler. ptal
The CQ bit was checked by hjd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
still lgtm, but adding a few small comments https://codereview.chromium.org/2617503002/diff/120001/telemetry/telemetry/in... File telemetry/telemetry/internal/actions/scroll_to_element_unittest.py (right): https://codereview.chromium.org/2617503002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/actions/scroll_to_element_unittest.py:8: from telemetry.util import js_template nit: add extra blank line here (should be 2 lines between top level elements) https://codereview.chromium.org/2617503002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/actions/scroll_to_element_unittest.py:13: # TODO(catapult:#3028): Render in JavaScript method when supported by API. thanks for adding these! https://codereview.chromium.org/2617503002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/actions/scroll_to_element_unittest.py:45: def _InsertElement(self, theid='element', container_selector='container'): 'container' -> '#container' https://codereview.chromium.org/2617503002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/actions/scroll_to_element_unittest.py:65: self._EvaluateJavaScript('document.scrollingElement.scrollTop'), 0) What is that assertion checking for? Could you add a few comments explaining what is being checked? https://codereview.chromium.org/2617503002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/actions/scroll_to_element_unittest.py:77: self._InsertElement(theid='element', container_selector='#container') omit values if they are the defaults https://codereview.chromium.org/2617503002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/actions/scroll_to_element_unittest.py:79: self._tab.EvaluateJavaScript('document.scrollingElement.scrollTop'), 0) is this also missing? self.assertEquals(self._VisibleAreaOfElement(selector='#element'), 0)
https://codereview.chromium.org/2617503002/diff/120001/telemetry/telemetry/in... File telemetry/telemetry/internal/actions/scroll_to_element_unittest.py (right): https://codereview.chromium.org/2617503002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/actions/scroll_to_element_unittest.py:8: from telemetry.util import js_template On 2017/01/11 13:05:00, perezju wrote: > nit: add extra blank line here (should be 2 lines between top level elements) Done, thanks! https://codereview.chromium.org/2617503002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/actions/scroll_to_element_unittest.py:13: # TODO(catapult:#3028): Render in JavaScript method when supported by API. On 2017/01/11 13:05:00, perezju wrote: > thanks for adding these! Acknowledged. https://codereview.chromium.org/2617503002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/actions/scroll_to_element_unittest.py:45: def _InsertElement(self, theid='element', container_selector='container'): On 2017/01/11 13:05:00, perezju wrote: > 'container' -> '#container' oops, thanks! https://codereview.chromium.org/2617503002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/actions/scroll_to_element_unittest.py:65: self._EvaluateJavaScript('document.scrollingElement.scrollTop'), 0) On 2017/01/11 13:05:00, perezju wrote: > What is that assertion checking for? Could you add a few comments explaining > what is being checked? Done. https://codereview.chromium.org/2617503002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/actions/scroll_to_element_unittest.py:77: self._InsertElement(theid='element', container_selector='#container') On 2017/01/11 13:05:00, perezju wrote: > omit values if they are the defaults Done. https://codereview.chromium.org/2617503002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/actions/scroll_to_element_unittest.py:79: self._tab.EvaluateJavaScript('document.scrollingElement.scrollTop'), 0) On 2017/01/11 13:05:00, perezju wrote: > is this also missing? > > self.assertEquals(self._VisibleAreaOfElement(selector='#element'), 0) yep, nice catch thanks! :)
lgtm Can you also add action_runner_unittest.ActionRunnerTest.TestScrollToElement2 ? action_runner.XYZ is the top most API that users will be using
On 2017/01/11 14:17:25, nednguyen wrote: > lgtm > > Can you also add action_runner_unittest.ActionRunnerTest.TestScrollToElement2 ? > action_runner.XYZ is the top most API that users will be using Thanks! I updated action_runner_unittest.ActionRunnerTest.TestScrollToElement to cover container element in its second scroll.
On 2017/01/11 15:49:32, hjd wrote: > On 2017/01/11 14:17:25, nednguyen wrote: > > lgtm > > > > Can you also add action_runner_unittest.ActionRunnerTest.TestScrollToElement2 > ? > > action_runner.XYZ is the top most API that users will be using > > Thanks! I updated action_runner_unittest.ActionRunnerTest.TestScrollToElement to > cover container element in its second scroll. that sgtm but you forgot to upload your latest patch?
On 2017/01/11 15:50:20, nednguyen wrote: > On 2017/01/11 15:49:32, hjd wrote: > > On 2017/01/11 14:17:25, nednguyen wrote: > > > lgtm > > > > > > Can you also add > action_runner_unittest.ActionRunnerTest.TestScrollToElement2 > > ? > > > action_runner.XYZ is the top most API that users will be using > > > > Thanks! I updated action_runner_unittest.ActionRunnerTest.TestScrollToElement > to > > cover container element in its second scroll. > > that sgtm but you forgot to upload your latest patch? yep, just uploaded now
The CQ bit was checked by hjd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2617503002/#ps160001 (title: "add coverage in action_runner_unittest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...)
The CQ bit was checked by hjd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2617503002/#ps180001 (title: "disable on android")
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": 180001, "attempt_start_ts": 1484309445761650,
"parent_rev": "4bd198f6a85c700acbbd23c0b666c846636526d2", "commit_rev":
"e6b04fef1997055d90390256fe96282bbbd98a51"}
Message was sent while issue was closed.
Description was changed from ========== [Telemetry] Support specifying container element for action_runner.ScrollToElement API Allows callers of ScrollToElement specify which element should be scrolled (previously this was hard coded to document.scrollingElement or document.body if scrollingElement was not set). ========== to ========== [Telemetry] Support specifying container element for action_runner.ScrollToElement API Allows callers of ScrollToElement specify which element should be scrolled (previously this was hard coded to document.scrollingElement or document.body if scrollingElement was not set). Review-Url: https://codereview.chromium.org/2617503002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
