Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2419763002:
Enable ExtensionTabsTest.QueryLastFocusedWindowTabs test (Closed)
|
Created:
4 years, 2 months ago by Qiang(Joe) Xu Modified:
4 years, 1 month ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable ExtensionTabsTest.QueryLastFocusedWindowTabs test
Changes include:
(1) Suspect content::RunAllPendingInMessageLoop is the reason for flaky, since BrowserWindow::IsActive is asynchronous on Mac and Linux.
(2) Construct a non-empty url as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?type=cs&sq=package:chromium&rcl=1476314269&l=530
(3) function->set_extension as there is a DCHECK here:
https://cs.chromium.org/chromium/src/chrome/browser/extensions/browser_extension_window_controller.cc?type=cs&q=browser_extension_window_controller.cc&sq=package:chromium&l=87
BUG=136562
TEST=change is on test itself
If flaky is still observed, please disable the test again.
Committed: https://crrev.com/7adee5e6cd95c133832c1d2b10a0f64e530f9d1f
Cr-Commit-Position: refs/heads/master@{#427214}
Patch Set 1 #Patch Set 2 : disable the test on Mac only #
Total comments: 2
Patch Set 3 : utilizing BrowserActivationWaiter test support #Messages
Total messages: 32 (22 generated)
Description was changed from ========== Enable ExtensionTabsTest.QueryLastFocusedWindowTabs BUG=136562 TEST=change is on test itself ========== to ========== Enable ExtensionTabsTest.QueryLastFocusedWindowTabs Changes include: (1) Suspect content::RunAllPendingInMessageLoop is the reason for flaky, since BrowserWindow::IsActive is asynchronous on Mac and Linux. Changed to using WidgetActivationWaiter. (2) Construct a non-empty url as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t... (3) function->set_extension as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/extensions/browser_extens... BUG=136562 TEST=change is on test itself If flaky is still observed, please disable the test again. ==========
warx@chromium.org changed reviewers: + asargent@chromium.org, sky@chromium.org
The CQ bit was checked by warx@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by warx@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...
Description was changed from ========== Enable ExtensionTabsTest.QueryLastFocusedWindowTabs Changes include: (1) Suspect content::RunAllPendingInMessageLoop is the reason for flaky, since BrowserWindow::IsActive is asynchronous on Mac and Linux. Changed to using WidgetActivationWaiter. (2) Construct a non-empty url as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t... (3) function->set_extension as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/extensions/browser_extens... BUG=136562 TEST=change is on test itself If flaky is still observed, please disable the test again. ========== to ========== Enable ExtensionTabsTest.QueryLastFocusedWindowTabs except on Mac Changes include: (1) Suspect content::RunAllPendingInMessageLoop is the reason for flaky, since BrowserWindow::IsActive is asynchronous on Mac and Linux. Changed to using WidgetActivationWaiter. And since Mac doesn't support views::Widget, disable the test on Mac only. (2) Construct a non-empty url as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t... (3) function->set_extension as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/extensions/browser_extens... BUG=136562 TEST=change is on test itself If flaky is still observed, please disable the test again. ==========
Hi Antony, Scott, could you PTAL? I picked up this issue because it was close to an issue I am working on. Disable it on Mac for the reasoning from this: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_... Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2419763002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_interactive_test.cc (right): https://codereview.chromium.org/2419763002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_interactive_test.cc:90: GURL url("chrome://settings"); Out of curiosity, any particular reason you used chrome://settings here, as opposed to say, about:blank ?
sky@chromium.org changed reviewers: + avi@chromium.org
+avi in case he has ideas as to what should be used on Mac.
On 2016/10/17 20:40:18, sky wrote: > +avi in case he has ideas as to what should be used on Mac. Why would something like BrowserActivationObserver (currently in https://cs.chromium.org/chromium/src/chrome/browser/ui/blocked_content/popup_... ) not work on the Mac? If so, we should move it to some browsertest util file.
On 2016/10/17 21:04:32, Avi wrote: > On 2016/10/17 20:40:18, sky wrote: > > +avi in case he has ideas as to what should be used on Mac. > > Why would something like BrowserActivationObserver (currently in > https://cs.chromium.org/chromium/src/chrome/browser/ui/blocked_content/popup_... > ) not work on the Mac? If so, we should move it to some browsertest util file. BrowserActivationObserver sounds promising. I didn't know this (thanks for pointing out). Let me see how it goes.
The CQ bit was checked by warx@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...
Description was changed from ========== Enable ExtensionTabsTest.QueryLastFocusedWindowTabs except on Mac Changes include: (1) Suspect content::RunAllPendingInMessageLoop is the reason for flaky, since BrowserWindow::IsActive is asynchronous on Mac and Linux. Changed to using WidgetActivationWaiter. And since Mac doesn't support views::Widget, disable the test on Mac only. (2) Construct a non-empty url as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t... (3) function->set_extension as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/extensions/browser_extens... BUG=136562 TEST=change is on test itself If flaky is still observed, please disable the test again. ========== to ========== Enable ExtensionTabsTest.QueryLastFocusedWindowTabs test Changes include: (1) Suspect content::RunAllPendingInMessageLoop is the reason for flaky, since BrowserWindow::IsActive is asynchronous on Mac and Linux. (2) Construct a non-empty url as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t... (3) function->set_extension as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/extensions/browser_extens... BUG=136562 TEST=change is on test itself If flaky is still observed, please disable the test again. ==========
Updated based on BrowserActivationWaiter. PTAL, thanks! https://codereview.chromium.org/2419763002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_interactive_test.cc (right): https://codereview.chromium.org/2419763002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_interactive_test.cc:90: GURL url("chrome://settings"); On 2016/10/17 20:33:14, Antony Sargent wrote: > Out of curiosity, any particular reason you used chrome://settings here, as > opposed to say, about:blank ? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by warx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asargent@chromium.org Link to the patchset: https://codereview.chromium.org/2419763002/#ps60001 (title: "utilizing BrowserActivationWaiter test support")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Enable ExtensionTabsTest.QueryLastFocusedWindowTabs test Changes include: (1) Suspect content::RunAllPendingInMessageLoop is the reason for flaky, since BrowserWindow::IsActive is asynchronous on Mac and Linux. (2) Construct a non-empty url as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t... (3) function->set_extension as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/extensions/browser_extens... BUG=136562 TEST=change is on test itself If flaky is still observed, please disable the test again. ========== to ========== Enable ExtensionTabsTest.QueryLastFocusedWindowTabs test Changes include: (1) Suspect content::RunAllPendingInMessageLoop is the reason for flaky, since BrowserWindow::IsActive is asynchronous on Mac and Linux. (2) Construct a non-empty url as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t... (3) function->set_extension as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/extensions/browser_extens... BUG=136562 TEST=change is on test itself If flaky is still observed, please disable the test again. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Enable ExtensionTabsTest.QueryLastFocusedWindowTabs test Changes include: (1) Suspect content::RunAllPendingInMessageLoop is the reason for flaky, since BrowserWindow::IsActive is asynchronous on Mac and Linux. (2) Construct a non-empty url as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t... (3) function->set_extension as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/extensions/browser_extens... BUG=136562 TEST=change is on test itself If flaky is still observed, please disable the test again. ========== to ========== Enable ExtensionTabsTest.QueryLastFocusedWindowTabs test Changes include: (1) Suspect content::RunAllPendingInMessageLoop is the reason for flaky, since BrowserWindow::IsActive is asynchronous on Mac and Linux. (2) Construct a non-empty url as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t... (3) function->set_extension as there is a DCHECK here: https://cs.chromium.org/chromium/src/chrome/browser/extensions/browser_extens... BUG=136562 TEST=change is on test itself If flaky is still observed, please disable the test again. Committed: https://crrev.com/7adee5e6cd95c133832c1d2b10a0f64e530f9d1f Cr-Commit-Position: refs/heads/master@{#427214} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7adee5e6cd95c133832c1d2b10a0f64e530f9d1f Cr-Commit-Position: refs/heads/master@{#427214} |
