Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2844563002:
OOPIF: Clear focused frame before advancing to remote frame. (Closed)
DescriptionOOPIF: Clear focused frame before advancing to remote frame.
There's a race where we could receive a new click event before receving
notification of a remote frame becoming focused as a result of advancing
(using the tab key for example) to that frame.
If we already think we're focused we would never reply back to the
browser to refocus this frame.
BUG=713977
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2844563002
Cr-Commit-Position: refs/heads/master@{#467580}
Committed: https://chromium.googlesource.com/chromium/src/+/c23fcf744de8b51e5936649cfe985cfa9c058398
Patch Set 1 #
Messages
Total messages: 31 (15 generated)
The CQ bit was checked by avallee@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...
avallee@chromium.org changed reviewers: + thakis@chromium.org
PTAL.
Do you want to reenable TabAndMouseFocusNavigation too?
Do you want to reenable TabAndMouseFocusNavigation too?
Sorry, keyboard shortcut fail, didn't mean to send this yet. I also wanted to say: I'm not qualified to review this. It looks like this code is from https://codereview.chromium.org/1500873002 originally, so please get a review from either alexmos or creis or dcheng. Happy to owner-stamp after that if still needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
avallee@chromium.org changed reviewers: + haraken@chromium.org - thakis@chromium.org
haraken: PTAL, nasko suggested you can review this while dcheng is out. Thanks.
On 2017/04/26 01:04:42, avallee wrote: > haraken: PTAL, nasko suggested you can review this while dcheng is out. Thanks. Did you see my comment about reenabling the test?
On 2017/04/26 01:10:28, Nico wrote: > On 2017/04/26 01:04:42, avallee wrote: > > haraken: PTAL, nasko suggested you can review this while dcheng is out. > Thanks. > > Did you see my comment about reenabling the test? @thakis: I re-enabled the test for windows yesterday looking for the flake with some logging in there. Follow up in a few days to remove the logging if this makes the flakiness go away.
On 2017/04/26 01:17:17, avallee wrote: > On 2017/04/26 01:10:28, Nico wrote: > > On 2017/04/26 01:04:42, avallee wrote: > > > haraken: PTAL, nasko suggested you can review this while dcheng is out. > > Thanks. > > > > Did you see my comment about reenabling the test? > > @thakis: I re-enabled the test for windows yesterday looking for the flake with > some logging in there. Follow up in a few days to remove the logging if this > makes the flakiness go away. Why don't you re-enable the test in this CL?
On 2017/04/26 02:36:27, haraken wrote: > On 2017/04/26 01:17:17, avallee wrote: > > On 2017/04/26 01:10:28, Nico wrote: > > > On 2017/04/26 01:04:42, avallee wrote: > > > > haraken: PTAL, nasko suggested you can review this while dcheng is out. > > > Thanks. > > > > > > Did you see my comment about reenabling the test? > > > > @thakis: I re-enabled the test for windows yesterday looking for the flake > with > > some logging in there. Follow up in a few days to remove the logging if this > > makes the flakiness go away. > > Why don't you re-enable the test in this CL? Test was enabled (lightly flaky) in this CL https://codereview.chromium.org/2837213002/
On 2017/04/26 02:46:41, avallee wrote: > On 2017/04/26 02:36:27, haraken wrote: > > On 2017/04/26 01:17:17, avallee wrote: > > > On 2017/04/26 01:10:28, Nico wrote: > > > > On 2017/04/26 01:04:42, avallee wrote: > > > > > haraken: PTAL, nasko suggested you can review this while dcheng is out. > > > > Thanks. > > > > > > > > Did you see my comment about reenabling the test? > > > > > > @thakis: I re-enabled the test for windows yesterday looking for the flake > > with > > > some logging in there. Follow up in a few days to remove the logging if this > > > makes the flakiness go away. > > > > Why don't you re-enable the test in this CL? > > Test was enabled (lightly flaky) in this CL > https://codereview.chromium.org/2837213002/ I'm confused. Is your plan to deflake the test in this CL?
On 2017/04/26 02:53:24, haraken wrote: > On 2017/04/26 02:46:41, avallee wrote: > > On 2017/04/26 02:36:27, haraken wrote: > > > On 2017/04/26 01:17:17, avallee wrote: > > > > On 2017/04/26 01:10:28, Nico wrote: > > > > > On 2017/04/26 01:04:42, avallee wrote: > > > > > > haraken: PTAL, nasko suggested you can review this while dcheng is > out. > > > > > Thanks. > > > > > > > > > > Did you see my comment about reenabling the test? > > > > > > > > @thakis: I re-enabled the test for windows yesterday looking for the flake > > > with > > > > some logging in there. Follow up in a few days to remove the logging if > this > > > > makes the flakiness go away. > > > > > > Why don't you re-enable the test in this CL? > > > > Test was enabled (lightly flaky) in this CL > > https://codereview.chromium.org/2837213002/ > > I'm confused. Is your plan to deflake the test in this CL? That's correct. Since the flake rate is low, I did not disable the test. This CL should elimitate the flakiness from the test.
On 2017/04/26 19:37:10, avallee wrote: > On 2017/04/26 02:53:24, haraken wrote: > > On 2017/04/26 02:46:41, avallee wrote: > > > On 2017/04/26 02:36:27, haraken wrote: > > > > On 2017/04/26 01:17:17, avallee wrote: > > > > > On 2017/04/26 01:10:28, Nico wrote: > > > > > > On 2017/04/26 01:04:42, avallee wrote: > > > > > > > haraken: PTAL, nasko suggested you can review this while dcheng is > > out. > > > > > > Thanks. > > > > > > > > > > > > Did you see my comment about reenabling the test? > > > > > > > > > > @thakis: I re-enabled the test for windows yesterday looking for the > flake > > > > with > > > > > some logging in there. Follow up in a few days to remove the logging if > > this > > > > > makes the flakiness go away. > > > > > > > > Why don't you re-enable the test in this CL? > > > > > > Test was enabled (lightly flaky) in this CL > > > https://codereview.chromium.org/2837213002/ > > > > I'm confused. Is your plan to deflake the test in this CL? > > That's correct. Since the flake rate is low, I did not disable the test. This CL > should elimitate the flakiness from the test. LGTM
The CQ bit was checked by avallee@chromium.org
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by avallee@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 avallee@chromium.org
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": 1, "attempt_start_ts": 1493267074877040, "parent_rev":
"8a4c38833ae890a2b12fb416d4c9375a6de5c051", "commit_rev":
"c23fcf744de8b51e5936649cfe985cfa9c058398"}
Message was sent while issue was closed.
Description was changed from ========== OOPIF: Clear focused frame before advancing to remote frame. There's a race where we could receive a new click event before receving notification of a remote frame becoming focused as a result of advancing (using the tab key for example) to that frame. If we already think we're focused we would never reply back to the browser to refocus this frame. BUG=713977 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== OOPIF: Clear focused frame before advancing to remote frame. There's a race where we could receive a new click event before receving notification of a remote frame becoming focused as a result of advancing (using the tab key for example) to that frame. If we already think we're focused we would never reply back to the browser to refocus this frame. BUG=713977 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2844563002 Cr-Commit-Position: refs/heads/master@{#467580} Committed: https://chromium.googlesource.com/chromium/src/+/c23fcf744de8b51e5936649cfe98... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/c23fcf744de8b51e5936649cfe98... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
