Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(21)

Issue 622793002: Group the different permission related methods in the content api. (Closed)

Created:
6 years, 2 months ago by Miguel Garcia
Modified:
5 years, 11 months ago
CC:
chromium-reviews, eme-reviews_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, Michael van Ouwerkerk, markusheintz_, wjia+watch_chromium.org, android-webview-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Group the different permission related methods in the content api. BUG=392145 Committed: https://crrev.com/b7beb08c377e86a8b9b224a5064a2cf0b737a679 Cr-Commit-Position: refs/heads/master@{#301338}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Total comments: 59

Patch Set 5 : #

Patch Set 6 : git cl format #

Patch Set 7 : Fix blink tests #

Total comments: 6

Patch Set 8 : #

Total comments: 26

Patch Set 9 : #

Patch Set 10 : Rebase #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+341 lines, -377 lines) Patch
M android_webview/browser/aw_content_browser_client.h View 1 2 3 4 5 1 chunk +6 lines, -17 lines 0 comments Download
M android_webview/browser/aw_content_browser_client.cc View 1 2 3 4 5 6 7 4 chunks +50 lines, -57 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.h View 1 2 3 4 5 6 7 8 9 2 chunks +10 lines, -24 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 5 chunks +113 lines, -80 lines 0 comments Download
M chrome/browser/content_settings/permission_context_base.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/content_settings/permission_context_uma_util.cc View 1 2 3 4 5 6 7 3 chunks +10 lines, -26 lines 0 comments Download
M chrome/browser/media/protected_media_identifier_permission_context.h View 1 2 3 4 5 2 chunks +5 lines, -7 lines 0 comments Download
M chrome/browser/media/protected_media_identifier_permission_context.cc View 1 chunk +1 line, -8 lines 0 comments Download
M chrome/browser/notifications/desktop_notification_service.h View 1 2 3 4 5 6 7 8 4 chunks +1 line, -13 lines 0 comments Download
M chrome/browser/notifications/desktop_notification_service.cc View 1 2 3 4 5 6 7 8 4 chunks +11 lines, -22 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.h View 1 2 3 4 5 6 7 8 9 2 chunks +1 line, -3 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 8 9 2 chunks +21 lines, -8 lines 0 comments Download
M content/browser/geolocation/geolocation_dispatcher_host.cc View 1 2 3 4 5 3 chunks +9 lines, -4 lines 0 comments Download
M content/browser/media/cdm/browser_cdm_manager.cc View 1 2 3 4 5 6 7 8 1 chunk +11 lines, -10 lines 1 comment Download
M content/browser/media/midi_dispatcher_host.h View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/media/midi_dispatcher_host.cc View 1 2 3 4 5 2 chunks +11 lines, -5 lines 0 comments Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M content/public/browser/content_browser_client.h View 1 2 3 4 5 6 3 chunks +11 lines, -41 lines 0 comments Download
M content/public/browser/content_browser_client.cc View 1 2 3 4 5 6 2 chunks +2 lines, -27 lines 0 comments Download
A content/public/browser/permission_type.h View 1 2 3 4 5 6 7 8 1 chunk +28 lines, -0 lines 0 comments Download
M content/shell/browser/layout_test/layout_test_content_browser_client.h View 1 2 3 4 5 6 7 8 9 2 chunks +8 lines, -5 lines 0 comments Download
M content/shell/browser/layout_test/layout_test_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 2 chunks +25 lines, -14 lines 0 comments Download
M content/shell/browser/layout_test/layout_test_notification_manager.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M content/shell/browser/layout_test/layout_test_notification_manager.cc View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -3 lines 0 comments Download

Messages

Total messages: 27 (7 generated)
Miguel Garcia
jam@ this is a follow up to https://codereview.chromium.org/459953002 where I promised I would merge the ...
6 years, 2 months ago (2014-10-20 14:34:34 UTC) #4
Bernhard Bauer
https://codereview.chromium.org/622793002/diff/100001/android_webview/browser/aw_content_browser_client.cc File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/622793002/diff/100001/android_webview/browser/aw_content_browser_client.cc#newcode409 android_webview/browser/aw_content_browser_client.cc:409: break; Do you want to fall through here? https://codereview.chromium.org/622793002/diff/100001/android_webview/browser/aw_content_browser_client.cc#newcode412 ...
6 years, 2 months ago (2014-10-20 14:57:24 UTC) #5
Peter Beverloo
First round of comments. Please run try-bots (including Blink layout ones!) since this will break ...
6 years, 2 months ago (2014-10-20 17:32:16 UTC) #6
jam
lgtm https://codereview.chromium.org/622793002/diff/100001/content/browser/media/midi_dispatcher_host.cc File content/browser/media/midi_dispatcher_host.cc (right): https://codereview.chromium.org/622793002/diff/100001/content/browser/media/midi_dispatcher_host.cc#newcode101 content/browser/media/midi_dispatcher_host.cc:101: content::PERMISSION_MIDI_SYSEX, nit: here and above, and elsewhere, no ...
6 years, 2 months ago (2014-10-20 20:13:57 UTC) #7
Miguel Garcia
Thanks for the reviews I have ran git cl format now, so all the indentation ...
6 years, 2 months ago (2014-10-21 17:17:13 UTC) #8
Bernhard Bauer
https://codereview.chromium.org/622793002/diff/100001/content/browser/media/cdm/browser_cdm_manager.cc File content/browser/media/cdm/browser_cdm_manager.cc (right): https://codereview.chromium.org/622793002/diff/100001/content/browser/media/cdm/browser_cdm_manager.cc#newcode262 content/browser/media/cdm/browser_cdm_manager.cc:262: true, On 2014/10/20 14:57:24, Bernhard Bauer wrote: > You ...
6 years, 2 months ago (2014-10-23 08:38:01 UTC) #9
Miguel Garcia
https://codereview.chromium.org/622793002/diff/100001/content/browser/media/cdm/browser_cdm_manager.cc File content/browser/media/cdm/browser_cdm_manager.cc (right): https://codereview.chromium.org/622793002/diff/100001/content/browser/media/cdm/browser_cdm_manager.cc#newcode262 content/browser/media/cdm/browser_cdm_manager.cc:262: true, On 2014/10/23 08:38:01, Bernhard Bauer wrote: > On ...
6 years, 2 months ago (2014-10-23 12:48:12 UTC) #10
Bernhard Bauer
LGTM! https://codereview.chromium.org/622793002/diff/100001/content/browser/media/cdm/browser_cdm_manager.cc File content/browser/media/cdm/browser_cdm_manager.cc (right): https://codereview.chromium.org/622793002/diff/100001/content/browser/media/cdm/browser_cdm_manager.cc#newcode262 content/browser/media/cdm/browser_cdm_manager.cc:262: true, On 2014/10/23 12:48:11, Miguel Garcia wrote: > ...
6 years, 2 months ago (2014-10-23 12:54:53 UTC) #11
Peter Beverloo
lgtm % the following comments https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_content_browser_client.cc#newcode1945 chrome/browser/chrome_content_browser_client.cc:1945: // so there is ...
6 years, 2 months ago (2014-10-24 14:37:28 UTC) #12
Miguel Garcia
+mukai and benm for remaining owners rubberstamp Thanks!
6 years, 2 months ago (2014-10-24 15:06:40 UTC) #14
Miguel Garcia
https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_content_browser_client.cc#newcode1945 chrome/browser/chrome_content_browser_client.cc:1945: // so there is no reason to implement it ...
6 years, 2 months ago (2014-10-24 16:05:33 UTC) #15
Jun Mukai
lgtm: chrome/browser/notifications
6 years, 2 months ago (2014-10-24 18:12:50 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/622793002/220001
6 years, 1 month ago (2014-10-27 10:08:05 UTC) #18
benm (inactive)
Michael should take a look at this from the WebView side.
6 years, 1 month ago (2014-10-27 10:44:20 UTC) #20
commit-bot: I haz the power
Committed patchset #10 (id:220001)
6 years, 1 month ago (2014-10-27 10:58:13 UTC) #21
commit-bot: I haz the power
Patchset 10 (id:??) landed as https://crrev.com/b7beb08c377e86a8b9b224a5064a2cf0b737a679 Cr-Commit-Position: refs/heads/master@{#301338}
6 years, 1 month ago (2014-10-27 10:58:58 UTC) #22
Peter Beverloo
On 2014/10/27 10:44:20, benm wrote: > Michael should take a look at this from the ...
6 years, 1 month ago (2014-10-27 11:16:54 UTC) #23
Miguel Garcia
Michael can you have look?, I apologize for having sent it with your review. I ...
6 years, 1 month ago (2014-10-27 11:28:58 UTC) #24
michaelbai
android_webview LGTM
6 years, 1 month ago (2014-10-27 16:46:05 UTC) #25
xhwang
5 years, 11 months ago (2015-01-21 05:43:20 UTC) #27
Message was sent while issue was closed.
https://codereview.chromium.org/622793002/diff/220001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (left):

https://codereview.chromium.org/622793002/diff/220001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:268:
cdm_cancel_permission_map_[GetId(render_frame_id, cdm_id)] = cancel_callback;
We used to have this code here to cancel the info bar when the CDM is destroyed.
This code now is removed and we are not calling CancelPermissionRequest(). I
suspect the info bar will persist even after we destroy the CDM that requested
the info bar.

I'll double check this tomorrow and see what's the best way to fix it.

Powered by Google App Engine
This is Rietveld 408576698