Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1855393007:
MD Downloads: ensure the DOM is ready before trying to find-in-page on it (Closed)
DescriptionMD Downloads: ensure the DOM is ready before trying to find-in-page on it
This unblocks a Polymer roll: https://codereview.chromium.org/1862213002/
Which, in turn, fixes a functional issue on chrome://md-settings
R=sky@chromium.org
BUG=600887
Committed: https://crrev.com/a0e06acbb33eb8e191ab5c33ea6875859f579247
Cr-Commit-Position: refs/heads/master@{#385870}
Patch Set 1 #
Total comments: 5
Messages
Total messages: 17 (5 generated)
Description was changed from ========== MD Downloads: ensure the DOM is ready before trying to find-in-page on it This unblocks a Polymer roll: https://codereview.chromium.org/1862213002/ Which, in turn, fixes a functional issue on chrome://md-settings R=sky@chromium.org BUG=600887 ========== to ========== MD Downloads: ensure the DOM is ready before trying to find-in-page on it This unblocks a Polymer roll: https://codereview.chromium.org/1862213002/ Which, in turn, fixes a functional issue on chrome://md-settings R=sky@chromium.org BUG=600887 ==========
/cc dpapad@ I'm mildly amazed this test has succeeded, as we use an "virtual, infinite list"[1] to render downloads now, which renders asynchronously and probably shouldn't really be counted on the "physical" DOM within it. [1] https://elements.polymer-project.org/elements/iron-list
dpapad@chromium.org changed reviewers: + dpapad@chromium.org
lgtm https://codereview.chromium.org/1855393007/diff/1/chrome/browser/ui/find_bar/... File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right): https://codereview.chromium.org/1855393007/diff/1/chrome/browser/ui/find_bar/... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:339: ASSERT_TRUE(content::ExecuteScript(web_contents, "Polymer.dom.flush();")); Nit(optional): Perhaps adding a comment to the effect of "Force some async operations to happen synchronously" would help future readers.
https://codereview.chromium.org/1855393007/diff/1/chrome/browser/ui/find_bar/... File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right): https://codereview.chromium.org/1855393007/diff/1/chrome/browser/ui/find_bar/... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:339: ASSERT_TRUE(content::ExecuteScript(web_contents, "Polymer.dom.flush();")); On 2016/04/07 00:59:55, dpapad wrote: > Nit(optional): Perhaps adding a comment to the effect of > "Force some async operations to happen synchronously" would help future readers. well, it's named Polymer.dom.flush(), i.e. flush Polymer-based DOM. also, just googling is probably better / more future proof.
LGTM
https://codereview.chromium.org/1855393007/diff/1/chrome/browser/ui/find_bar/... File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right): https://codereview.chromium.org/1855393007/diff/1/chrome/browser/ui/find_bar/... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:339: ASSERT_TRUE(content::ExecuteScript(web_contents, "Polymer.dom.flush();")); On 2016/04/07 01:02:35, Dan Beam wrote: > On 2016/04/07 00:59:55, dpapad wrote: > > Nit(optional): Perhaps adding a comment to the effect of > > "Force some async operations to happen synchronously" would help future > readers. > > well, it's named Polymer.dom.flush(), i.e. flush Polymer-based DOM. also, just > googling is probably better / more future proof. Actually, one question. Why isn't ui_test_utils::NavigateToURL enough?
https://codereview.chromium.org/1855393007/diff/1/chrome/browser/ui/find_bar/... File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right): https://codereview.chromium.org/1855393007/diff/1/chrome/browser/ui/find_bar/... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:339: ASSERT_TRUE(content::ExecuteScript(web_contents, "Polymer.dom.flush();")); On 2016/04/07 19:11:38, sky wrote: > On 2016/04/07 01:02:35, Dan Beam wrote: > > On 2016/04/07 00:59:55, dpapad wrote: > > > Nit(optional): Perhaps adding a comment to the effect of > > > "Force some async operations to happen synchronously" would help future > > readers. > > > > well, it's named Polymer.dom.flush(), i.e. flush Polymer-based DOM. also, > just > > googling is probably better / more future proof. > > Actually, one question. Why isn't ui_test_utils::NavigateToURL enough? because the list is rendered after onload
sorry for the wall... https://codereview.chromium.org/1855393007/diff/1/chrome/browser/ui/find_bar/... File chrome/browser/ui/find_bar/find_bar_host_browsertest.cc (right): https://codereview.chromium.org/1855393007/diff/1/chrome/browser/ui/find_bar/... chrome/browser/ui/find_bar/find_bar_host_browsertest.cc:339: ASSERT_TRUE(content::ExecuteScript(web_contents, "Polymer.dom.flush();")); On 2016/04/07 19:12:21, Dan Beam wrote: > On 2016/04/07 19:11:38, sky wrote: > > On 2016/04/07 01:02:35, Dan Beam wrote: > > > On 2016/04/07 00:59:55, dpapad wrote: > > > > Nit(optional): Perhaps adding a comment to the effect of > > > > "Force some async operations to happen synchronously" would help future > > > readers. > > > > > > well, it's named Polymer.dom.flush(), i.e. flush Polymer-based DOM. also, > > just > > > googling is probably better / more future proof. > > > > Actually, one question. Why isn't ui_test_utils::NavigateToURL enough? > > because the list is rendered after onload onload calls chrome.send()[1][2], which does CallJavascriptFunction()[3][4], IPCs the data via JS[5] and asynchronously queues rendering[6][7][8][9][10][11]. I have no idea how this worked with the new downloads page this long, but the very last step ("asynchronously queues rendering") became slightly more async recently[12] and the house of cards crumbled (this test started failing in that CL). [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... [2] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... [3] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... [4] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... [5] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... [6] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/polyme... [7] https://github.com/Polymer/polymer/blob/b30f962d3950ee2fbb9be5a63ec442885af6c... [8] https://github.com/Polymer/polymer/blob/edb59eb6283074ce54f374b30b884c9f9fcf9... [9] https://github.com/Polymer/polymer/blob/b30f962d3950ee2fbb9be5a63ec442885af6c... [10] https://github.com/Polymer/polymer/blob/b30f962d3950ee2fbb9be5a63ec442885af6c... [11] https://github.com/Polymer/polymer/blob/b30f962d3950ee2fbb9be5a63ec442885af6c... [12] https://codereview.chromium.org/1862213002/diff/40001/third_party/polymer/v1_...
SLGTM
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855393007/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855393007/1
Message was sent while issue was closed.
Description was changed from ========== MD Downloads: ensure the DOM is ready before trying to find-in-page on it This unblocks a Polymer roll: https://codereview.chromium.org/1862213002/ Which, in turn, fixes a functional issue on chrome://md-settings R=sky@chromium.org BUG=600887 ========== to ========== MD Downloads: ensure the DOM is ready before trying to find-in-page on it This unblocks a Polymer roll: https://codereview.chromium.org/1862213002/ Which, in turn, fixes a functional issue on chrome://md-settings R=sky@chromium.org BUG=600887 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== MD Downloads: ensure the DOM is ready before trying to find-in-page on it This unblocks a Polymer roll: https://codereview.chromium.org/1862213002/ Which, in turn, fixes a functional issue on chrome://md-settings R=sky@chromium.org BUG=600887 ========== to ========== MD Downloads: ensure the DOM is ready before trying to find-in-page on it This unblocks a Polymer roll: https://codereview.chromium.org/1862213002/ Which, in turn, fixes a functional issue on chrome://md-settings R=sky@chromium.org BUG=600887 Committed: https://crrev.com/a0e06acbb33eb8e191ab5c33ea6875859f579247 Cr-Commit-Position: refs/heads/master@{#385870} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/a0e06acbb33eb8e191ab5c33ea6875859f579247 Cr-Commit-Position: refs/heads/master@{#385870} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
