Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781903002:
Modify a test to wait for compositor frame of OOPIF synced with CSS modifications in parent process (Closed)
DescriptionModify a test to wait for compositor frame of OOPIF synced with CSS modifications in parent process
A recent test which compares bounds reported from a focused node inside
OOPIF against those reported for the same element throught AutofillAgent
falkes 15% of the time. The reason seems to be that transforming the
node bounds from child frame coordinates to root frame coordinates is
failing for the focused node (which comes first and before autofill's
update). The root cause is most probably a delayed updated in compositor
frame's metadata for the OOPIF.
This CL will modify the test so that we will wait for a known transform
to happen, that is, for a fixed position frame and no border,
transforming (0, 0) to root view should (almost) return the left and top
properties of the frame (which we set).
BUG=704943
Review-Url: https://codereview.chromium.org/2781903002
Cr-Commit-Position: refs/heads/master@{#460219}
Committed: https://chromium.googlesource.com/chromium/src/+/0d112490bb36604ec3f89951188d41fa111ea248
Patch Set 1 #
Total comments: 7
Patch Set 2 : Addressing comments #
Total comments: 2
Patch Set 3 : Rebase to ToT and Activate the Test #Patch Set 4 : Remove a stale comment #Messages
Total messages: 20 (13 generated)
ekaramad@chromium.org changed reviewers: + alexmos@chromium.org
Hello Alex, Unfortunately the test still flakes and seems like moving to interactive tests is not the fix. But I stil think the test should stay here instead of browser tests since it involves simulating keyboard <input>. The reason was that when I focused <input> to get the first bounds, the bounds were reported before the compositor frame updates its meta data on the browser side. To make sure that happens I used a function as you can observer in the CL. Alternatively, I could expose SurfaceHitTestReadyNotifier from internal to public and use that method instead. But this is less work and the method I think is more to-the-point. I'd be happy for any suggestions for either approach. Thanks!
https://codereview.chromium.org/2781903002/diff/1/chrome/browser/site_per_pro... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/2781903002/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:1089: scoped_refptr<content::MessageLoopRunner> loop_runner_ = content::MessageLoopRunner is marked as deprecated. Could you use base::RunLoop instead, similarly to what's done in https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser... ? https://codereview.chromium.org/2781903002/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:1093: base::TimeDelta::FromMilliseconds(kCheckAgainDelay)); nit: you can use TestTimeouts::tiny_timeout() https://codereview.chromium.org/2781903002/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:1118: "iframe.style.left = '%dpx';", I wonder if declaring the CSS statically before navigation could avoid this problem too - e.g., similarly to page_with_positioned_frame.html.
The CQ bit was checked by ekaramad@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...
Thanks Alex! Please, take another look. https://codereview.chromium.org/2781903002/diff/1/chrome/browser/site_per_pro... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/2781903002/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:1089: scoped_refptr<content::MessageLoopRunner> loop_runner_ = On 2017/03/28 18:54:25, alexmos wrote: > content::MessageLoopRunner is marked as deprecated. Could you use base::RunLoop > instead, similarly to what's done in > https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser... > ? Sure. Thanks for pointing this out. Done. https://codereview.chromium.org/2781903002/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:1093: base::TimeDelta::FromMilliseconds(kCheckAgainDelay)); On 2017/03/28 18:54:25, alexmos wrote: > nit: you can use TestTimeouts::tiny_timeout() Done. https://codereview.chromium.org/2781903002/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:1118: "iframe.style.left = '%dpx';", On 2017/03/28 18:54:24, alexmos wrote: > I wonder if declaring the CSS statically before navigation could avoid this > problem too - e.g., similarly to page_with_positioned_frame.html. It might stop flaking but we are not removing the underlying issue which is the compositor frame update. It could theoretically still flake if the SurfaceId for the OOPIF is not registered in time. Also looking at a helper method which uses the file you mentioned: https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser... it seems like we also wait for surface to be ready (which is I guess what the wait method below is doing indirectly).
LGTM (one more thing below) https://codereview.chromium.org/2781903002/diff/1/chrome/browser/site_per_pro... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/2781903002/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:1118: "iframe.style.left = '%dpx';", On 2017/03/28 19:37:46, EhsanK wrote: > On 2017/03/28 18:54:24, alexmos wrote: > > I wonder if declaring the CSS statically before navigation could avoid this > > problem too - e.g., similarly to page_with_positioned_frame.html. > > It might stop flaking but we are not removing the underlying issue which is the > compositor frame update. It could theoretically still flake if the SurfaceId for > the OOPIF is not registered in time. > > Also looking at a helper method which uses the file you mentioned: > https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser... > it seems like we also wait for surface to be ready (which is I guess what the > wait method below is doing indirectly). Acknowledged. https://codereview.chromium.org/2781903002/diff/20001/chrome/browser/site_per... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/2781903002/diff/20001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:1098: PasswordAutofillPopupPositionInsideOOPIF) { Looks like the test was disabled in https://bugs.chromium.org/p/chromium/issues/detail?id=705914, so please either rebase and reenable it here, or revert the CL that disabled it after this lands.
The CQ bit was checked by ekaramad@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...
Thanks Alex! I rebased and will try to CQ soon. https://codereview.chromium.org/2781903002/diff/20001/chrome/browser/site_per... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/2781903002/diff/20001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:1098: PasswordAutofillPopupPositionInsideOOPIF) { On 2017/03/28 19:48:13, alexmos wrote: > Looks like the test was disabled in > https://bugs.chromium.org/p/chromium/issues/detail?id=705914, so please either > rebase and reenable it here, or revert the CL that disabled it after this lands. Thanks for reminding me. Done!
The CQ bit was checked by ekaramad@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: This issue passed the CQ dry run.
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2781903002/#ps60001 (title: "Remove a stale comment")
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": 60001, "attempt_start_ts": 1490737809363980,
"parent_rev": "c1028928eaec4fd84d29a81c462152f3e6fbf08c", "commit_rev":
"0d112490bb36604ec3f89951188d41fa111ea248"}
Message was sent while issue was closed.
Description was changed from ========== Modify a test to wait for compositor frame of OOPIF synced with CSS modifications in parent process A recent test which compares bounds reported from a focused node inside OOPIF against those reported for the same element throught AutofillAgent falkes 15% of the time. The reason seems to be that transforming the node bounds from child frame coordinates to root frame coordinates is failing for the focused node (which comes first and before autofill's update). The root cause is most probably a delayed updated in compositor frame's metadata for the OOPIF. This CL will modify the test so that we will wait for a known transform to happen, that is, for a fixed position frame and no border, transforming (0, 0) to root view should (almost) return the left and top properties of the frame (which we set). BUG=704943 ========== to ========== Modify a test to wait for compositor frame of OOPIF synced with CSS modifications in parent process A recent test which compares bounds reported from a focused node inside OOPIF against those reported for the same element throught AutofillAgent falkes 15% of the time. The reason seems to be that transforming the node bounds from child frame coordinates to root frame coordinates is failing for the focused node (which comes first and before autofill's update). The root cause is most probably a delayed updated in compositor frame's metadata for the OOPIF. This CL will modify the test so that we will wait for a known transform to happen, that is, for a fixed position frame and no border, transforming (0, 0) to root view should (almost) return the left and top properties of the frame (which we set). BUG=704943 Review-Url: https://codereview.chromium.org/2781903002 Cr-Commit-Position: refs/heads/master@{#460219} Committed: https://chromium.googlesource.com/chromium/src/+/0d112490bb36604ec3f89951188d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/0d112490bb36604ec3f89951188d... |
