Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2937183002:
Flip UseGroupedPermissionInfobars and UsePermissionManagerForMediaRequests (Closed)
|
Created:
3 years, 6 months ago by Timothy Loh Modified:
3 years, 6 months ago Reviewers:
raymes CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFlip UseGroupedPermissionInfobars and UsePermissionManagerForMediaRequests
This patch enables by default the flags UseGroupedPermissionInfobars and
UsePermissionManagerForMediaRequests. This means that media permissions
(mic, camera) will go through the PermissionManager like every other
permission, and Android will use the PermissionRequestManager instead of
the PermissionQueueController.
There are some minor UI changes here, mostly ones which users will never
notice, although of note is grouped mic+camera permissions on desktop
will now use two separate list items rather than one.
BUG=606138, 596786, 734889
Review-Url: https://codereview.chromium.org/2937183002
Cr-Commit-Position: refs/heads/master@{#481443}
Committed: https://chromium.googlesource.com/chromium/src/+/02958eaa8c765b0bc65c020bb1dd1d1fd0792de0
Patch Set 1 #Patch Set 2 : download wip #Patch Set 3 : update #
Total comments: 2
Patch Set 4 : no test changes now #
Messages
Total messages: 30 (24 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_...)
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 ========== Flip UseGroupedPermissionInfobars and UsePermissionManagerForMediaRequests BUG= ========== to ========== Flip UseGroupedPermissionInfobars and UsePermissionManagerForMediaRequests This patch enables by default the flags UseGroupedPermissionInfobars and UsePermissionManagerForMediaRequests. This means that media permissions (mic, camera) will go through the PermissionManager like every other permission, and Android will use the PermissionRequestManager instead of the PermissionQueueController. There are some minor UI changes here, mostly ones which users will never notice, although of note is grouped mic+camera permissions on desktop will now use two separate list items rather than one. BUG=606138,596786 ==========
timloh@chromium.org changed reviewers: + raymes@chromium.org
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...
Feel free to wait for an OK from MTV. PermissionNavigationTest change is because we now queue in the PRM instead of the PQC (Java), so I think we can't get into a state where the PQC queue is used. Download test change is because it seemed like too much work to make it work with WithParamInterface if we're just going to remove the old path soon enough anyway.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2937183002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/permissions/PermissionNavigationTest.java (left): https://codereview.chromium.org/2937183002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/permissions/PermissionNavigationTest.java:51: }); Let's just delete this in a separate CL along with the getQueueLengthForTesting function. https://codereview.chromium.org/2937183002/diff/40001/chrome/browser/download... File chrome/browser/download/download_request_limiter_unittest.cc (left): https://codereview.chromium.org/2937183002/diff/40001/chrome/browser/download... chrome/browser/download/download_request_limiter_unittest.cc:53: ResetCounts(); I think we should just put if (PermissionRequestManager::IsEnabled()) checks in each of the functions and merge the two classes. it will be a bit messy but it won't be code that lives for long.
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...
On 2017/06/20 04:13:46, raymes wrote: > https://codereview.chromium.org/2937183002/diff/40001/chrome/android/javatest... > File > chrome/android/javatests/src/org/chromium/chrome/browser/permissions/PermissionNavigationTest.java > (left): > > https://codereview.chromium.org/2937183002/diff/40001/chrome/android/javatest... > chrome/android/javatests/src/org/chromium/chrome/browser/permissions/PermissionNavigationTest.java:51: > }); > Let's just delete this in a separate CL along with the getQueueLengthForTesting > function. > > https://codereview.chromium.org/2937183002/diff/40001/chrome/browser/download... > File chrome/browser/download/download_request_limiter_unittest.cc (left): > > https://codereview.chromium.org/2937183002/diff/40001/chrome/browser/download... > chrome/browser/download/download_request_limiter_unittest.cc:53: ResetCounts(); > I think we should just put if (PermissionRequestManager::IsEnabled()) checks in > each of the functions and merge the two classes. it will be a bit messy but it > won't be code that lives for long. Test changes have been pulled out and landed in https://codereview.chromium.org/2946953003/ and https://codereview.chromium.org/2947013003/, this CL is now just the flip of the two features.
lgtm
Description was changed from ========== Flip UseGroupedPermissionInfobars and UsePermissionManagerForMediaRequests This patch enables by default the flags UseGroupedPermissionInfobars and UsePermissionManagerForMediaRequests. This means that media permissions (mic, camera) will go through the PermissionManager like every other permission, and Android will use the PermissionRequestManager instead of the PermissionQueueController. There are some minor UI changes here, mostly ones which users will never notice, although of note is grouped mic+camera permissions on desktop will now use two separate list items rather than one. BUG=606138,596786 ========== to ========== Flip UseGroupedPermissionInfobars and UsePermissionManagerForMediaRequests This patch enables by default the flags UseGroupedPermissionInfobars and UsePermissionManagerForMediaRequests. This means that media permissions (mic, camera) will go through the PermissionManager like every other permission, and Android will use the PermissionRequestManager instead of the PermissionQueueController. There are some minor UI changes here, mostly ones which users will never notice, although of note is grouped mic+camera permissions on desktop will now use two separate list items rather than one. BUG=606138,596786,734889 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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": 60001, "attempt_start_ts": 1498107204584900,
"parent_rev": "1913d5748e9730be549efe5a3f2d2248c4a425de", "commit_rev":
"9d933cc340081fc222fa05669d4e8a067a72b319"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498107204584900,
"parent_rev": "48387a5dce3fcb5504bfaa6e83dcf67ea14fa506", "commit_rev":
"359b5c2c345d229bd2d0991a8293e2d28ce33b4a"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498107204584900,
"parent_rev": "26dcbacdbed227e6845ef45148e292f5b7def680", "commit_rev":
"02958eaa8c765b0bc65c020bb1dd1d1fd0792de0"}
Message was sent while issue was closed.
Description was changed from ========== Flip UseGroupedPermissionInfobars and UsePermissionManagerForMediaRequests This patch enables by default the flags UseGroupedPermissionInfobars and UsePermissionManagerForMediaRequests. This means that media permissions (mic, camera) will go through the PermissionManager like every other permission, and Android will use the PermissionRequestManager instead of the PermissionQueueController. There are some minor UI changes here, mostly ones which users will never notice, although of note is grouped mic+camera permissions on desktop will now use two separate list items rather than one. BUG=606138,596786,734889 ========== to ========== Flip UseGroupedPermissionInfobars and UsePermissionManagerForMediaRequests This patch enables by default the flags UseGroupedPermissionInfobars and UsePermissionManagerForMediaRequests. This means that media permissions (mic, camera) will go through the PermissionManager like every other permission, and Android will use the PermissionRequestManager instead of the PermissionQueueController. There are some minor UI changes here, mostly ones which users will never notice, although of note is grouped mic+camera permissions on desktop will now use two separate list items rather than one. BUG=606138,596786,734889 Review-Url: https://codereview.chromium.org/2937183002 Cr-Commit-Position: refs/heads/master@{#481443} Committed: https://chromium.googlesource.com/chromium/src/+/02958eaa8c765b0bc65c020bb1dd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/02958eaa8c765b0bc65c020bb1dd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
