Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2686223002:
Fix flaky password visibility browser test (Closed)
DescriptionFix flaky password visibility browser test
We inject dummy scripts to wait until password visiblity notifications have been
sent from the renderer to the browser, but in the case of password fields in
OOPIFs, we need to inject dummy scripts into the OOPIFs, not just the main
frame.
BUG=662485
Review-Url: https://codereview.chromium.org/2686223002
Cr-Commit-Position: refs/heads/master@{#449529}
Committed: https://chromium.googlesource.com/chromium/src/+/fc6825e49d0c5aa1a922cf3b260e67070835abfb
Patch Set 1 #
Total comments: 5
Patch Set 2 : meacer comments #
Total comments: 2
Patch Set 3 : fix typo #Messages
Total messages: 17 (9 generated)
estark@chromium.org changed reviewers: + meacer@chromium.org
The CQ bit was checked by estark@chromium.org to run a CQ dry run
meacer, PTAL? https://codereview.chromium.org/2686223002/diff/1/chrome/browser/ssl/security... File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right): https://codereview.chromium.org/2686223002/diff/1/chrome/browser/ssl/security... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1140: // Inject a dummy script into each frame to ensure that password notifications note: this test was not marked as flaky. I'm guessing this is because it's an OOPIF iframe, so we don't technically need to inject into all the frames, but I figured it was better for consistency.
Lgtm. Just curious, how did you figure out this was the problem? https://codereview.chromium.org/2686223002/diff/1/chrome/browser/ssl/security... File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right): https://codereview.chromium.org/2686223002/diff/1/chrome/browser/ssl/security... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1140: // Inject a dummy script into each frame to ensure that password notifications On 2017/02/10 00:55:34, estark wrote: > note: this test was not marked as flaky. I'm guessing this is because it's an > OOPIF iframe, so we don't technically need to inject into all the frames, but I > figured it was better for consistency. Would it make sense to move this code to InjectScript and make it take a WebContents so that it injects into all frames automatically? https://codereview.chromium.org/2686223002/diff/1/chrome/browser/ssl/security... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1179: // from any subframe have been sent. nit: any subframe or all subframes? They sound different to me.
> Just curious, how did you figure out this was the problem? "figure out" is a strong phrase. I'm kind of just guessing and will see if it starts flaking again. This test was disabled a while ago, and then I made an unrelated change in the renderer that made all the other similar tests start flaking in a very similar way, which I fixed with the InjectScript() calls, which made me think that these iframe tests are probably failing for the same reason but with frames involved. https://codereview.chromium.org/2686223002/diff/1/chrome/browser/ssl/security... File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right): https://codereview.chromium.org/2686223002/diff/1/chrome/browser/ssl/security... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1140: // Inject a dummy script into each frame to ensure that password notifications On 2017/02/10 01:07:40, Mustafa Emre Acer wrote: > On 2017/02/10 00:55:34, estark wrote: > > note: this test was not marked as flaky. I'm guessing this is because it's an > > OOPIF iframe, so we don't technically need to inject into all the frames, but > I > > figured it was better for consistency. > > Would it make sense to move this code to InjectScript and make it take a > WebContents so that it injects into all frames automatically? Done. https://codereview.chromium.org/2686223002/diff/1/chrome/browser/ssl/security... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1179: // from any subframe have been sent. On 2017/02/10 01:07:40, Mustafa Emre Acer wrote: > nit: any subframe or all subframes? They sound different to me. Probably should have been "all subframes", but n/a now
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org Link to the patchset: https://codereview.chromium.org/2686223002/#ps20001 (title: "meacer comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
And a missed typo before it lands. https://codereview.chromium.org/2686223002/diff/20001/chrome/browser/ssl/secu... File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right): https://codereview.chromium.org/2686223002/diff/20001/chrome/browser/ssl/secu... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:70: // notifcations have been sent. nit: notifications
The CQ bit was unchecked by estark@chromium.org
https://codereview.chromium.org/2686223002/diff/20001/chrome/browser/ssl/secu... File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right): https://codereview.chromium.org/2686223002/diff/20001/chrome/browser/ssl/secu... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:70: // notifcations have been sent. On 2017/02/10 01:50:55, Mustafa Emre Acer wrote: > nit: notifications Good eye, done!
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org Link to the patchset: https://codereview.chromium.org/2686223002/#ps40001 (title: "fix typo")
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": 1486691643343050,
"parent_rev": "992f50de218e93808825efbe1067b9968f01fec9", "commit_rev":
"fc6825e49d0c5aa1a922cf3b260e67070835abfb"}
Message was sent while issue was closed.
Description was changed from ========== Fix flaky password visibility browser test We inject dummy scripts to wait until password visiblity notifications have been sent from the renderer to the browser, but in the case of password fields in OOPIFs, we need to inject dummy scripts into the OOPIFs, not just the main frame. BUG=662485 ========== to ========== Fix flaky password visibility browser test We inject dummy scripts to wait until password visiblity notifications have been sent from the renderer to the browser, but in the case of password fields in OOPIFs, we need to inject dummy scripts into the OOPIFs, not just the main frame. BUG=662485 Review-Url: https://codereview.chromium.org/2686223002 Cr-Commit-Position: refs/heads/master@{#449529} Committed: https://chromium.googlesource.com/chromium/src/+/fc6825e49d0c5aa1a922cf3b260e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/fc6825e49d0c5aa1a922cf3b260e... |
