Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2876993003:
Test PermissionRequestManager in GeolocationPermissionContext unit tests (Closed)
|
Created:
3 years, 7 months ago by Timothy Loh Modified:
3 years, 7 months ago Reviewers:
raymes CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, mcasas+geolocation_chromium.org, raymes+watch_chromium.org, mlamouri+watch-geolocation_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionTest PermissionRequestManager on Android in GeolocationPermissionContext unit tests
This patch adds tests for the PermissionRequestManager on Android in the
GeolocationPermissionContext unit tests via testing::WithParamInterface.
The existing tests are refactored slightly to make this possible. In
particular, the #ifs which switched between checking the PRM and the
InfoBarService are replaced with checks on GetParam(), and as much as
possible this logic is pulled into AcceptPrompt() and so on.
BUG=671052, 606138
Review-Url: https://codereview.chromium.org/2876993003
Cr-Commit-Position: refs/heads/master@{#473491}
Committed: https://chromium.googlesource.com/chromium/src/+/746d11013697cfc620b5a1dfcf76a745dfa5182d
Patch Set 1 #Patch Set 2 : wipwip #Patch Set 3 : it works?!?!?? #Patch Set 4 : :d #
Total comments: 10
Patch Set 5 : address comments #Patch Set 6 : disable too #Messages
Total messages: 29 (22 generated)
The CQ bit was checked by timloh@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 timloh@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by timloh@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.
Description was changed from ========== wip not for review BUG= ========== to ========== Test PermissionRequestManager on Android in GeolocationPermissionContext unit tests This patch adds tests for the PermissionRequestManager on Android in the GeolocationPermissionContext unit tests via testing::WithParamInterface. The existing tests are refactored slightly to make this possible. In particular, the #ifs which switched between checking the PRM and the InfoBarService are replaced with checks on GetParam(), and as much as possible repeated logic is pulled into AcceptPrompt() and so on. BUG=671052, 606138 ==========
timloh@chromium.org changed reviewers: + raymes@chromium.org
Description was changed from ========== Test PermissionRequestManager on Android in GeolocationPermissionContext unit tests This patch adds tests for the PermissionRequestManager on Android in the GeolocationPermissionContext unit tests via testing::WithParamInterface. The existing tests are refactored slightly to make this possible. In particular, the #ifs which switched between checking the PRM and the InfoBarService are replaced with checks on GetParam(), and as much as possible repeated logic is pulled into AcceptPrompt() and so on. BUG=671052, 606138 ========== to ========== Test PermissionRequestManager on Android in GeolocationPermissionContext unit tests This patch adds tests for the PermissionRequestManager on Android in the GeolocationPermissionContext unit tests via testing::WithParamInterface. The existing tests are refactored slightly to make this possible. In particular, the #ifs which switched between checking the PRM and the InfoBarService are replaced with checks on GetParam(), and as much as possible this logic is pulled into AcceptPrompt() and so on. BUG=671052, 606138 ==========
Thanks! https://codereview.chromium.org/2876993003/diff/60001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (left): https://codereview.chromium.org/2876993003/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:531: EXPECT_TRUE(closed_infobar_tracker_.Contains(infobar)); Is this now covered somewhere else? https://codereview.chromium.org/2876993003/diff/60001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/2876993003/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:203: void EnableFeature(base::test::ScopedFeatureList& scoped_feature_list, in Chrome we never pass arguments by non-const reference. Prefer a pointer. https://codereview.chromium.org/2876993003/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:474: infobar_service->RemoveInfoBar(infobar); Question: do you know why all this removing happens? Is it just necessary cleanup for the test? Should we be doing this in Deny/Close? https://codereview.chromium.org/2876993003/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:485: infobars::InfoBar* infobar_1 = infobar_service()->infobar_at(0); nit: why infobar_1? https://codereview.chromium.org/2876993003/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:1056: return; Maybe remove the #ifdef above and just rely on this check? Then we could move the TODO above this line and it would be more descriptive for someone looking to fix it.
The CQ bit was checked by timloh@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...
Latest patch set also changes size_t GetNumberOfPrompts() to bool HasActivePrompt(), which is more accurate. https://codereview.chromium.org/2876993003/diff/60001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (left): https://codereview.chromium.org/2876993003/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:531: EXPECT_TRUE(closed_infobar_tracker_.Contains(infobar)); On 2017/05/17 04:24:49, raymes wrote: > Is this now covered somewhere else? This test and SinglePermissionBubble (renamed bubble->prompt) are basically the same now. https://codereview.chromium.org/2876993003/diff/60001/chrome/browser/geolocat... File chrome/browser/geolocation/geolocation_permission_context_unittest.cc (right): https://codereview.chromium.org/2876993003/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:203: void EnableFeature(base::test::ScopedFeatureList& scoped_feature_list, On 2017/05/17 04:24:49, raymes wrote: > in Chrome we never pass arguments by non-const reference. Prefer a pointer. Done. https://codereview.chromium.org/2876993003/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:474: infobar_service->RemoveInfoBar(infobar); On 2017/05/17 04:24:49, raymes wrote: > Question: do you know why all this removing happens? Is it just necessary > cleanup for the test? Should we be doing this in Deny/Close? Yeah, clean-up for a couple of tests which queue multiple requests (e.g. QueuedPermission). In theory we should do it in deny/close too but these branches are going away eventually anyway. Technically the PRM code-path might want to clean up the infobars too, but when testing with the PRM we don't look at the infobar system for assertions, only the PRM. https://codereview.chromium.org/2876993003/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:485: infobars::InfoBar* infobar_1 = infobar_service()->infobar_at(0); On 2017/05/17 04:24:49, raymes wrote: > nit: why infobar_1? Bad copy-paste... fixed! https://codereview.chromium.org/2876993003/diff/60001/chrome/browser/geolocat... chrome/browser/geolocation/geolocation_permission_context_unittest.cc:1056: return; On 2017/05/17 04:24:49, raymes wrote: > Maybe remove the #ifdef above and just rely on this check? Then we could move > the TODO above this line and it would be more descriptive for someone looking to > fix it. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Latest patch set also disabled PRM when testing PQC so we won't need to immediately update this when enabling the feature in the future.
lgtm
The CQ bit was checked by timloh@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": 100001, "attempt_start_ts": 1495417575574260,
"parent_rev": "124eb0b5af215ce2df373be6552228d79090256e", "commit_rev":
"746d11013697cfc620b5a1dfcf76a745dfa5182d"}
Message was sent while issue was closed.
Description was changed from ========== Test PermissionRequestManager on Android in GeolocationPermissionContext unit tests This patch adds tests for the PermissionRequestManager on Android in the GeolocationPermissionContext unit tests via testing::WithParamInterface. The existing tests are refactored slightly to make this possible. In particular, the #ifs which switched between checking the PRM and the InfoBarService are replaced with checks on GetParam(), and as much as possible this logic is pulled into AcceptPrompt() and so on. BUG=671052, 606138 ========== to ========== Test PermissionRequestManager on Android in GeolocationPermissionContext unit tests This patch adds tests for the PermissionRequestManager on Android in the GeolocationPermissionContext unit tests via testing::WithParamInterface. The existing tests are refactored slightly to make this possible. In particular, the #ifs which switched between checking the PRM and the InfoBarService are replaced with checks on GetParam(), and as much as possible this logic is pulled into AcceptPrompt() and so on. BUG=671052, 606138 Review-Url: https://codereview.chromium.org/2876993003 Cr-Commit-Position: refs/heads/master@{#473491} Committed: https://chromium.googlesource.com/chromium/src/+/746d11013697cfc620b5a1dfcf76... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/746d11013697cfc620b5a1dfcf76... |
