Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2560243002:
Add GetURL() and WaitForJSExpression() functions (Closed)
|
Created:
4 years ago by Robert Ogden Modified:
4 years ago Reviewers:
RyanSturm CC:
chromium-reviews, tbansal+watch-data-reduction-proxy_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd GetURL() and WaitForJSExpression() functions
BUG=669956
Committed: https://crrev.com/3dc7464b4a1d9f9139caa56f9b5c3363f0554e1f
Cr-Commit-Position: refs/heads/master@{#438255}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rework GetURL() to LoadURL() #
Total comments: 8
Patch Set 3 : Style nits. #
Messages
Total messages: 15 (6 generated)
robertogden@chromium.org changed reviewers: + ryansturm@chromium.org
https://codereview.chromium.org/2560243002/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2560243002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:222: def GetURL(self, url, timeout=30): I'd prefer LoadURL. This seems a little backwards based on the API of selenium. Read my other comment about removing LoadPage/SetURL. https://codereview.chromium.org/2560243002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:228: self.SetURL(url) For now, forcing us to use GetURL with a param everytime might be best. I don't think there is a strong need to introduce a convenience function, so maybe just keep it to one method. Let me know if there are other uses for self._url that I am not thinking of, but it seems unneccesary since the test will manage what urls it is navigating to. This use case is sort of ambiguously defined: SetURL("a") LoadPage() GetURL("b") LoadPage() I think I'd prefer the final LoadPage() to load the page from SetURL (i.e. "a"), but if we are forced to only use GetURL, it would have to look like: GetURL("a") GetURL("b") GetURL("a"/"b") (depending on what the consumer wanted)
https://codereview.chromium.org/2560243002/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2560243002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:222: def GetURL(self, url, timeout=30): On 2016/12/12 20:02:46, Ryan Sturm wrote: > I'd prefer LoadURL. This seems a little backwards based on the API of selenium. > > Read my other comment about removing LoadPage/SetURL. Done. https://codereview.chromium.org/2560243002/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:228: self.SetURL(url) On 2016/12/12 20:02:46, Ryan Sturm wrote: > For now, forcing us to use GetURL with a param everytime might be best. > > I don't think there is a strong need to introduce a convenience function, so > maybe just keep it to one method. > > Let me know if there are other uses for self._url that I am not thinking of, but > it seems unneccesary since the test will manage what urls it is navigating to. > > This use case is sort of ambiguously defined: > SetURL("a") > LoadPage() > GetURL("b") > LoadPage() > > I think I'd prefer the final LoadPage() to load the page from SetURL (i.e. "a"), > but if we are forced to only use GetURL, it would have to look like: > GetURL("a") > GetURL("b") > GetURL("a"/"b") (depending on what the consumer wanted) Acknowledged.
lgtm % style nits https://codereview.chromium.org/2560243002/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2560243002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:207: url: the URL to navigate to nit: s/the/The/ and s/to/to./ https://codereview.chromium.org/2560243002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:208: timeout: the time in seconds to load the page before timing out nit: s/the time/The time/ and s/out/out./ https://codereview.chromium.org/2560243002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:261: of min_poll and max_poll. nit: I'm not totally sure on python style, but chromium style in general would have min_poll surrounded by pipes. I.e., |min_poll| and |max_poll|. Look around at other python code to see what the chromium python standard is. https://codereview.chromium.org/2560243002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:264: expression: The Javascript expression to poll, as a string add periods to the end of these variable descriptions, and capitalize the first "the" on line 267
https://codereview.chromium.org/2560243002/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2560243002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:207: url: the URL to navigate to On 2016/12/13 16:49:08, Ryan Sturm wrote: > nit: s/the/The/ and s/to/to./ Done. https://codereview.chromium.org/2560243002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:208: timeout: the time in seconds to load the page before timing out On 2016/12/13 16:49:08, Ryan Sturm wrote: > nit: s/the time/The time/ and s/out/out./ Done. https://codereview.chromium.org/2560243002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:261: of min_poll and max_poll. On 2016/12/13 16:49:08, Ryan Sturm wrote: > nit: I'm not totally sure on python style, but chromium style in general would > have min_poll surrounded by pipes. I.e., |min_poll| and |max_poll|. Look around > at other python code to see what the chromium python standard is. Done. https://codereview.chromium.org/2560243002/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:264: expression: The Javascript expression to poll, as a string On 2016/12/13 16:49:08, Ryan Sturm wrote: > add periods to the end of these variable descriptions, and capitalize the first > "the" on line 267 Done.
The CQ bit was checked by robertogden@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org Link to the patchset: https://codereview.chromium.org/2560243002/#ps40001 (title: "Style nits.")
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": 40001, "attempt_start_ts": 1481656816643850,
"parent_rev": "cfc7ceb5606f849e7027bffa581451a3ea1da950", "commit_rev":
"146871090e016b81eb10cd8408244985aee9a7f5"}
Message was sent while issue was closed.
Description was changed from ========== Add GetURL() and WaitForJSExpression() functions BUG=669956 ========== to ========== Add GetURL() and WaitForJSExpression() functions BUG=669956 Review-Url: https://codereview.chromium.org/2560243002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add GetURL() and WaitForJSExpression() functions BUG=669956 Review-Url: https://codereview.chromium.org/2560243002 ========== to ========== Add GetURL() and WaitForJSExpression() functions BUG=669956 Committed: https://crrev.com/3dc7464b4a1d9f9139caa56f9b5c3363f0554e1f Cr-Commit-Position: refs/heads/master@{#438255} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3dc7464b4a1d9f9139caa56f9b5c3363f0554e1f Cr-Commit-Position: refs/heads/master@{#438255} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
