Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2538253002: Don't show a modal permission prompt if the requesting WebContents has no tab. (Closed)

Created:
3 years, 11 months ago by dominickn
Modified:
3 years, 11 months ago
Reviewers:
raymes
CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't show a modal permission prompt if the requesting WebContents has no tab. Contextual search on Android spins up a WebContents which talks to Google search. If Google search chooses to request a permission inside this WebContents, the modal prompt crashes due to a lack of a containing tab. This is not an issue with the standard infobar permission prompts as they do not have a dependency on a tab for display. This CL changes a DCHECK for a TabAndroid to an explicit conditional, which acts as though the permission request was dismissed if there is no active Tab for the requesting WebContents. BUG=668640 Committed: https://crrev.com/c5c663618e3b8aa61bc71b1e39f4313ef6b41567 Cr-Commit-Position: refs/heads/master@{#435556}

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -17 lines) Patch
M chrome/browser/permissions/permission_dialog_delegate.h View 4 chunks +6 lines, -4 lines 0 comments Download
M chrome/browser/permissions/permission_dialog_delegate.cc View 6 chunks +25 lines, -13 lines 3 comments Download

Messages

Total messages: 17 (10 generated)
dominickn
PTAL, thanks!
3 years, 11 months ago (2016-11-30 23:24:39 UTC) #5
raymes
Do you know if things also act like they are dismissed in the non-modal (infobar) ...
3 years, 11 months ago (2016-12-01 00:31:08 UTC) #8
dominickn
On 2016/12/01 00:31:08, raymes wrote: > Do you know if things also act like they ...
3 years, 11 months ago (2016-12-01 01:11:54 UTC) #9
raymes
lgtm - but it would be good to understand how the infobar case fails, if ...
3 years, 11 months ago (2016-12-01 03:22:25 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2538253002/1
3 years, 11 months ago (2016-12-01 05:00:10 UTC) #12
commit-bot: I haz the power
Committed patchset #1 (id:1)
3 years, 11 months ago (2016-12-01 05:04:50 UTC) #15
commit-bot: I haz the power
3 years, 11 months ago (2016-12-01 05:08:10 UTC) #17
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/c5c663618e3b8aa61bc71b1e39f4313ef6b41567
Cr-Commit-Position: refs/heads/master@{#435556}

Powered by Google App Engine
This is Rietveld 408576698