Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2771213003:
OOPIF: Add test to for tab and mouse focus changes. (Closed)
DescriptionOOPIF: Add test to for tab and mouse focus changes.
Test ensures that focus can transition to parent or sibling frames when
the target element to be focused was clicked just before the frame was
focused.
Test currently fails due to http://crbug.com/702330.
BUG=702330
Review-Url: https://codereview.chromium.org/2771213003
Cr-Commit-Position: refs/heads/master@{#459998}
Committed: https://chromium.googlesource.com/chromium/src/+/28b32ffa898387ee0decdd02a17fd0e45f095772
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address alexmos comments. #Patch Set 3 : rename variable origin->empty_offset #Messages
Total messages: 16 (10 generated)
avallee@chromium.org changed reviewers: + alexmos@chromium.org, lfg@chromium.org, wjmaclean@chromium.org
avallee@chromium.org changed required reviewers: + alexmos@chromium.org
This is just the test. It currently fails due to the double focus events when tabbing into a remote frame.
Awesome test, thanks for being thorough. LGTM with nits. https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:284: // TODO(http://crbug.com/702330): Enable this test. nit: https https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:310: // have an <input>, then two <iframe> elements, then another <input>. Perhaps also mention what it sends back. It's kind of difficult to follow all the coordinates floating around, and a sample returned string would help grasp how the test works. https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:363: gfx::Point origin; is there a better name? I'm thinking of the document origin here, and this is obviously not that :) https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:372: auto main_points = parsed.first; nit: I'd rename to something like main_frame_input_coords, child1_points -> child1_input_coords, etc., to clarify which coords these are for. You could also declare iframe1_offset = parsed.second[0] and similarly for second frame to improve readability. https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:383: LOG(ERROR) << "SimulateKeyPress"; Did you want to leave these and the console.log() in on purpose, to help debug any failures on trybots? If you keep them, INFO might be better than ERROR. https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:393: content::DOMMessageQueue msg_queue{web_contents}; Why the {} and not just ()? And do you need it at all? (The one above doesn't use the web_contents) https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:432: LOG(ERROR) << "before fail"; remove? (also below)
https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:284: // TODO(http://crbug.com/702330): Enable this test. On 2017/03/24 23:55:39, alexmos wrote: > nit: https Done. https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:310: // have an <input>, then two <iframe> elements, then another <input>. On 2017/03/24 23:55:39, alexmos wrote: > Perhaps also mention what it sends back. It's kind of difficult to follow all > the coordinates floating around, and a sample returned string would help grasp > how the test works. Done. https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:363: gfx::Point origin; On 2017/03/24 23:55:39, alexmos wrote: > is there a better name? I'm thinking of the document origin here, and this is > obviously not that :) Well both the input points and the offset points for the iframes are to be offset by nothing (we're already in the top document). Maybe no_offset or empty_offset. https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:372: auto main_points = parsed.first; On 2017/03/24 23:55:39, alexmos wrote: > nit: I'd rename to something like main_frame_input_coords, child1_points -> > child1_input_coords, etc., to clarify which coords these are for. You could > also declare iframe1_offset = parsed.second[0] and similarly for second frame to > improve readability. Done. https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:383: LOG(ERROR) << "SimulateKeyPress"; On 2017/03/24 23:55:39, alexmos wrote: > Did you want to leave these and the console.log() in on purpose, to help debug > any failures on trybots? If you keep them, INFO might be better than ERROR. nope, missed a couple. https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:393: content::DOMMessageQueue msg_queue{web_contents}; On 2017/03/24 23:55:39, alexmos wrote: > Why the {} and not just ()? And do you need it at all? (The one above doesn't > use the web_contents) Gone, this was trying to fix the failure that I didn't understand at the time. The plain queue make sense here. https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:432: LOG(ERROR) << "before fail"; On 2017/03/24 23:55:39, alexmos wrote: > remove? (also below) Done.
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.
Thanks, LGTM https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/2771213003/diff/1/chrome/browser/site_per_pro... chrome/browser/site_per_process_interactive_browsertest.cc:363: gfx::Point origin; On 2017/03/27 19:54:24, avallee wrote: > On 2017/03/24 23:55:39, alexmos wrote: > > is there a better name? I'm thinking of the document origin here, and this is > > obviously not that :) > > Well both the input points and the offset points for the iframes are to be > offset by nothing (we're already in the top document). Maybe no_offset or > empty_offset. Yes, either one of those sounds fine.
The CQ bit was checked by avallee@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/2771213003/#ps40001 (title: "rename variable origin->empty_offset")
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": 1490669018103590,
"parent_rev": "f2e73a29defb5173967c94a19606bc302468e61e", "commit_rev":
"28b32ffa898387ee0decdd02a17fd0e45f095772"}
Message was sent while issue was closed.
Description was changed from ========== OOPIF: Add test to for tab and mouse focus changes. Test ensures that focus can transition to parent or sibling frames when the target element to be focused was clicked just before the frame was focused. Test currently fails due to http://crbug.com/702330. BUG=702330 ========== to ========== OOPIF: Add test to for tab and mouse focus changes. Test ensures that focus can transition to parent or sibling frames when the target element to be focused was clicked just before the frame was focused. Test currently fails due to http://crbug.com/702330. BUG=702330 Review-Url: https://codereview.chromium.org/2771213003 Cr-Commit-Position: refs/heads/master@{#459998} Committed: https://chromium.googlesource.com/chromium/src/+/28b32ffa898387ee0decdd02a17f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/28b32ffa898387ee0decdd02a17f... |
