Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2553993003:
PlzNavigate: fix flaky TabManagerTest.TabManagerBasics browser test. (Closed)
DescriptionPlzNavigate: fix flaky TabManagerTest.TabManagerBasics browser test.
There were two issues:
- After we swapped render views and before the old one was destroyed, WebUI IPC
messages from the old render view would be incorrectly routed to the new one.
- The test itself was racy. In some places it waited for any navigation commit
notification but there could be multiple ongoing navigations at that point.
BUG=None
Committed: https://crrev.com/db7c3b163a71b7aa4ac3e41bba9e6a87e2ed5e5c
Cr-Commit-Position: refs/heads/master@{#437327}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 27 (15 generated)
The CQ bit was checked by yzshen@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...
yzshen@chromium.org changed reviewers: + georgesak@chromium.org, nasko@chromium.org
Hi, Nasko and George. Would you please take a look? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
chrome/browser/memory/tab_manager_browsertest.cc LGTM
The CQ bit was checked by yzshen@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.
My bad, I didn't notice that Yuzhu was looking at this test failure and I looked at the test as well. (I just saw this one while checking that Georges is available for reviews). https://codereview.chromium.org/2556393002/ is the patch. It didn't address WebUI fix listed here, but it changed the test to wait for navigations to complete which is what browser tests generally do (instead of waiting for commit). I defer to Yuzhu/Georges which version of changes to tab_manager_browsertest.cc we will land.
On 2016/12/08 16:07:09, jam wrote: > My bad, I didn't notice that Yuzhu was looking at this test failure and I looked > at the test as well. (I just saw this one while checking that Georges is > available for reviews). > > https://codereview.chromium.org/2556393002/ is the patch. It didn't address > WebUI fix listed here, but it changed the test to wait for navigations to > complete which is what browser tests generally do (instead of waiting for > commit). I defer to Yuzhu/Georges which version of changes to > tab_manager_browsertest.cc we will land. Hi, John. Now that you are here, would you please review the change to web_contents_impl? It seems Nasko is busy. Thanks! I will let Georges decide which version of changes to tab_manager_browsertest.cc to land.
jam@chromium.org changed reviewers: + jam@chromium.org
lgtm https://codereview.chromium.org/2553993003/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2553993003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:680: if (main_frame) { nit: are you sure you need this check?
https://codereview.chromium.org/2553993003/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2553993003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:680: if (main_frame) { On 2016/12/08 17:43:53, jam wrote: > nit: are you sure you need this check? Yes, there are cases where render_view_host->GetMainFrame() returns null. I didn't look deeper to see whether those are expected though. Do you think it is a bug? Thanks!
This implementation works for me and I've already LGTMed it, so let's go with this one.
The CQ bit was checked by yzshen@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by yzshen@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": 1481227282692770, "parent_rev":
"1e6f3ed7fa5a1b4eee11a888f584a06d377dece8", "commit_rev":
"cc76e86e3c74b22a63934fd62a975347a1663944"}
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: fix flaky TabManagerTest.TabManagerBasics browser test. There were two issues: - After we swapped render views and before the old one was destroyed, WebUI IPC messages from the old render view would be incorrectly routed to the new one. - The test itself was racy. In some places it waited for any navigation commit notification but there could be multiple ongoing navigations at that point. BUG=None ========== to ========== PlzNavigate: fix flaky TabManagerTest.TabManagerBasics browser test. There were two issues: - After we swapped render views and before the old one was destroyed, WebUI IPC messages from the old render view would be incorrectly routed to the new one. - The test itself was racy. In some places it waited for any navigation commit notification but there could be multiple ongoing navigations at that point. BUG=None Committed: https://crrev.com/db7c3b163a71b7aa4ac3e41bba9e6a87e2ed5e5c Cr-Commit-Position: refs/heads/master@{#437327} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/db7c3b163a71b7aa4ac3e41bba9e6a87e2ed5e5c Cr-Commit-Position: refs/heads/master@{#437327} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
