Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2770423002:
Do not show permission bubble when browser is not active (Closed)
|
Created:
3 years, 9 months ago by Qiang(Joe) Xu Modified:
3 years, 7 months ago CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, raymes+watch_chromium.org, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not show permission bubble when browser is not active
Changes:
Right now, the permission request is added when active tab changed and bubble showing is scheduled when DocumentOnLoadCompletedInMainFrame and DocumentLoadedInFrame. There is nothing to do whether the container browser is active or not. This change is to fix that: do not trigger showing bubble when browser is not active, and schedule showing bubble when browser becomes active if there are still pending requests.
BUG=704350
TEST=test reporter's bug case, it is fixed.
Patch Set 1 #
Total comments: 2
Patch Set 2 : BrowserListObserver #
Total comments: 3
Patch Set 3 : feedback #Patch Set 4 : mac tests #Patch Set 5 : rebase #
Messages
Total messages: 92 (67 generated)
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: This issue passed the CQ dry run.
Description was changed from ========== Do not show permission bubble when browser is not visible BUG=704350 ========== to ========== Do not show permission bubble when browser is not visible Changes: Right now, the permission request is added when active tab changed and bubble showing is scheduled when DocumentOnLoadCompletedInMainFrame and DocumentLoadedInFrame. There is nothing to do with the web contents's host's visibility, like browser view's visibility. This change notifies active web contents associated PermissionRequestManager about Browser's visibility. BUG=704350 TEST=test reporter's bug case, it is fixed. ==========
warx@chromium.org changed reviewers: + mlamouri@chromium.org, sky@chromium.org
Hi all, PTAL, thanks! This fix is only dealing with browser window. mlamouri@, are there any other cases that may still be missing?
https://codereview.chromium.org/2770423002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2770423002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:1824: void BrowserView::OnWidgetVisibilityChanged(views::Widget* widget, I think this logic belongs closer to the permissions code rather than BrowserView. I think the permissions should be the one determining if the browser containing the webcontents is the active browser, and it not install a BrowserListObserver. Once the browser is active *and* the webcontents is active then show the bubble.
Description was changed from ========== Do not show permission bubble when browser is not visible Changes: Right now, the permission request is added when active tab changed and bubble showing is scheduled when DocumentOnLoadCompletedInMainFrame and DocumentLoadedInFrame. There is nothing to do with the web contents's host's visibility, like browser view's visibility. This change notifies active web contents associated PermissionRequestManager about Browser's visibility. BUG=704350 TEST=test reporter's bug case, it is fixed. ========== to ========== Do not show permission bubble when browser is not active Changes: Right now, the permission request is added when active tab changed and bubble showing is scheduled when DocumentOnLoadCompletedInMainFrame and DocumentLoadedInFrame. There is nothing to do whether the container browser is active or not. This change is to fix that: do not trigger showing bubble when browser is not active, and schedule showing bubble when browser becomes active if there are still pending requests. BUG=704350 TEST=test reporter's bug case, it is fixed. ==========
Description was changed from ========== Do not show permission bubble when browser is not active Changes: Right now, the permission request is added when active tab changed and bubble showing is scheduled when DocumentOnLoadCompletedInMainFrame and DocumentLoadedInFrame. There is nothing to do whether the container browser is active or not. This change is to fix that: do not trigger showing bubble when browser is not active, and schedule showing bubble when browser becomes active if there are still pending requests. BUG=704350 TEST=test reporter's bug case, it is fixed. ========== to ========== Do not show permission bubble when browser is not active Changes: Right now, the permission request is added when active tab changed and bubble showing is scheduled when DocumentOnLoadCompletedInMainFrame and DocumentLoadedInFrame. There is nothing to do whether the container browser is active or not. This change is to fix that: do not trigger showing bubble when browser is not active, and schedule showing bubble when browser becomes active if there are still pending requests. BUG=704350 TEST=test reporter's bug case, it is fixed. Add PermissionRequestManagerBrowserTest.PermissionRequestManagerBrowserTest test coverage. ==========
Patchset #2 (id:20001) has been deleted
HI, PTAL, thanks! https://codereview.chromium.org/2770423002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2770423002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:1824: void BrowserView::OnWidgetVisibilityChanged(views::Widget* widget, On 2017/03/27 15:47:51, sky wrote: > I think this logic belongs closer to the permissions code rather than > BrowserView. I think the permissions should be the one determining if the > browser containing the webcontents is the active browser, and it not install a > BrowserListObserver. Once the browser is active *and* the webcontents is active > then show the bubble. Yeah, I agree. And checking activation instead of visibility seems more reasonable. It also covers more cases, like a permission bubble is scheduled but another browser is created and set as active. https://codereview.chromium.org/2770423002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_manager_browsertest.cc (right): https://codereview.chromium.org/2770423002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager_browsertest.cc:310: BrowserList::GetInstance()->NotifyBrowserNoLongerActive(browser()); This is a work around for testing this, otherwise we need native widget deactivation for sending this signal. WDYT?
https://codereview.chromium.org/2770423002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_manager.cc (right): https://codereview.chromium.org/2770423002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager.cc:84: browser_active_(false), Why do you need to cache browser_active_ ? Can't TriggerShowBubble check active?
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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:80001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
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: Exceeded global retry quota
Patchset #4 (id:100001) has been deleted
Patchset #3 (id:60001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #4 (id:140001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:180001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #3 (id:120001) 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...
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_...)
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: This issue passed the CQ dry run.
Can you use WebContentsObserver callbacks? I assume that when the browser isn't active/in the background the tab is marked as hidden? WebContents should reflect that, right? Can you use WO::WasShown to check that the tab is back into view?
Description was changed from ========== Do not show permission bubble when browser is not active Changes: Right now, the permission request is added when active tab changed and bubble showing is scheduled when DocumentOnLoadCompletedInMainFrame and DocumentLoadedInFrame. There is nothing to do whether the container browser is active or not. This change is to fix that: do not trigger showing bubble when browser is not active, and schedule showing bubble when browser becomes active if there are still pending requests. BUG=704350 TEST=test reporter's bug case, it is fixed. Add PermissionRequestManagerBrowserTest.PermissionRequestManagerBrowserTest test coverage. ========== to ========== Do not show permission bubble when browser is not active Changes: Right now, the permission request is added when active tab changed and bubble showing is scheduled when DocumentOnLoadCompletedInMainFrame and DocumentLoadedInFrame. There is nothing to do whether the container browser is active or not. This change is to fix that: do not trigger showing bubble when browser is not active, and schedule showing bubble when browser becomes active if there are still pending requests. BUG=704350 TEST=test reporter's bug case, it is fixed. ==========
On 2017/03/30 13:23:04, mlamouri wrote: > Can you use WebContentsObserver callbacks? I assume that when the browser isn't > active/in the background the tab is marked as hidden? WebContents should reflect > that, right? Can you use WO::WasShown to check that the tab is back into view? It depends on whether we are fixing "showing prompt on inactive window" or "showing prompt on invisible window". Both will fix the reporter's bug but WO::WasShown cannot fix the case such as "prompt is going to be shown while a new window is created". I am going to send ps#4 out for the above question, before making more changes. ps#3 has a lot of mac test failures. I just know this fact [1], so I disable check for MAC. [1] https://cs.chromium.org/chromium/src/chrome/browser/ui/startup/startup_browse...
On 2017/03/30 at 18:55:07, warx wrote: > On 2017/03/30 13:23:04, mlamouri wrote: > > Can you use WebContentsObserver callbacks? I assume that when the browser isn't > > active/in the background the tab is marked as hidden? WebContents should reflect > > that, right? Can you use WO::WasShown to check that the tab is back into view? > > It depends on whether we are fixing "showing prompt on inactive window" or "showing prompt on invisible window". Both will fix the reporter's bug but WO::WasShown cannot fix the case such as "prompt is going to be shown while a new window is created". I'm no chrome/ owner and I know nothing about UI but as a permissions/ owner, I would prefer a generic solution based on WebContents. What do you mean by "prompt is going to be shown while a new window is created"?
On 2017/03/30 20:49:53, mlamouri wrote: > On 2017/03/30 at 18:55:07, warx wrote: > > On 2017/03/30 13:23:04, mlamouri wrote: > > > Can you use WebContentsObserver callbacks? I assume that when the browser > isn't > > > active/in the background the tab is marked as hidden? WebContents should > reflect > > > that, right? Can you use WO::WasShown to check that the tab is back into > view? > > > > It depends on whether we are fixing "showing prompt on inactive window" or > "showing prompt on invisible window". Both will fix the reporter's bug but > WO::WasShown cannot fix the case such as "prompt is going to be shown while a > new window is created". > > I'm no chrome/ owner and I know nothing about UI but as a permissions/ owner, I > would prefer a generic solution based on WebContents. > > What do you mean by "prompt is going to be shown while a new window is created"? That means a browser is visible but not active because activation belongs to the new created window. Shall we show prompt on this visible but not active browser?
On 2017/03/30 20:49:53, mlamouri wrote: > On 2017/03/30 at 18:55:07, warx wrote: > > On 2017/03/30 13:23:04, mlamouri wrote: > > > Can you use WebContentsObserver callbacks? I assume that when the browser > isn't > > > active/in the background the tab is marked as hidden? WebContents should > reflect > > > that, right? Can you use WO::WasShown to check that the tab is back into > view? > > > > It depends on whether we are fixing "showing prompt on inactive window" or > "showing prompt on invisible window". Both will fix the reporter's bug but > WO::WasShown cannot fix the case such as "prompt is going to be shown while a > new window is created". > > I'm no chrome/ owner and I know nothing about UI but as a permissions/ owner, I > would prefer a generic solution based on WebContents. > > What do you mean by "prompt is going to be shown while a new window is created"? WasShown reflects when the WebContents is shown. This does not necessarily correspond to shown *and* in the active browser. For example, consider two browsers B and C. Both are visible, but C is in the foreground. Further tab 1 and 2 are in B with 1 selected. If 1 closes 2 gets WasShown, even though it is not in the active browser. The problem should only shown once the browser is active.
On 2017/03/31 04:27:54, sky wrote: > On 2017/03/30 20:49:53, mlamouri wrote: > > On 2017/03/30 at 18:55:07, warx wrote: > > > On 2017/03/30 13:23:04, mlamouri wrote: > > > > Can you use WebContentsObserver callbacks? I assume that when the browser > > isn't > > > > active/in the background the tab is marked as hidden? WebContents should > > reflect > > > > that, right? Can you use WO::WasShown to check that the tab is back into > > view? > > > > > > It depends on whether we are fixing "showing prompt on inactive window" or > > "showing prompt on invisible window". Both will fix the reporter's bug but > > WO::WasShown cannot fix the case such as "prompt is going to be shown while a > > new window is created". > > > > I'm no chrome/ owner and I know nothing about UI but as a permissions/ owner, > I > > would prefer a generic solution based on WebContents. > > > > What do you mean by "prompt is going to be shown while a new window is > created"? > > WasShown reflects when the WebContents is shown. This does not necessarily > correspond to shown *and* in the active browser. For example, consider two > browsers B and C. Both are visible, but C is in the foreground. Further tab 1 > and 2 are in B with 1 selected. If 1 closes 2 gets WasShown, even though it is > not in the active browser. The problem should only shown once the browser is > active. That should be, the 'bubble' should only be shown if the webcontents is visible and in the active browser.
On 2017/03/31 at 04:27:54, sky wrote: > On 2017/03/30 20:49:53, mlamouri wrote: > > On 2017/03/30 at 18:55:07, warx wrote: > > > On 2017/03/30 13:23:04, mlamouri wrote: > > > > Can you use WebContentsObserver callbacks? I assume that when the browser > > isn't > > > > active/in the background the tab is marked as hidden? WebContents should > > reflect > > > > that, right? Can you use WO::WasShown to check that the tab is back into > > view? > > > > > > It depends on whether we are fixing "showing prompt on inactive window" or > > "showing prompt on invisible window". Both will fix the reporter's bug but > > WO::WasShown cannot fix the case such as "prompt is going to be shown while a > > new window is created". > > > > I'm no chrome/ owner and I know nothing about UI but as a permissions/ owner, I > > would prefer a generic solution based on WebContents. > > > > What do you mean by "prompt is going to be shown while a new window is created"? > > WasShown reflects when the WebContents is shown. This does not necessarily correspond to shown *and* in the active browser. For example, consider two browsers B and C. Both are visible, but C is in the foreground. Further tab 1 and 2 are in B with 1 selected. If 1 closes 2 gets WasShown, even though it is not in the active browser. The problem should only shown once the browser is active. Would WebContents be focused in this case? Can we figure out this information by checking for visible and focused?
Not necessarily. For example, if focus is on the omnibox then the active webcontents in the active browser is not focused. -Scott On Fri, Mar 31, 2017 at 1:22 AM, <mlamouri@chromium.org> wrote: > On 2017/03/31 at 04:27:54, sky wrote: > > On 2017/03/30 20:49:53, mlamouri wrote: > > > On 2017/03/30 at 18:55:07, warx wrote: > > > > On 2017/03/30 13:23:04, mlamouri wrote: > > > > > Can you use WebContentsObserver callbacks? I assume that when the > browser > > > isn't > > > > > active/in the background the tab is marked as hidden? WebContents > should > > > reflect > > > > > that, right? Can you use WO::WasShown to check that the tab is > back into > > > view? > > > > > > > > It depends on whether we are fixing "showing prompt on inactive > window" or > > > "showing prompt on invisible window". Both will fix the reporter's bug > but > > > WO::WasShown cannot fix the case such as "prompt is going to be shown > while > a > > > new window is created". > > > > > > I'm no chrome/ owner and I know nothing about UI but as a permissions/ > owner, I > > > would prefer a generic solution based on WebContents. > > > > > > What do you mean by "prompt is going to be shown while a new window is > created"? > > > > WasShown reflects when the WebContents is shown. This does not > necessarily > correspond to shown *and* in the active browser. For example, consider two > browsers B and C. Both are visible, but C is in the foreground. Further > tab 1 > and 2 are in B with 1 selected. If 1 closes 2 gets WasShown, even though > it is > not in the active browser. The problem should only shown once the browser > is > active. > > Would WebContents be focused in this case? Can we figure out this > information by > checking for visible and focused? > > https://codereview.chromium.org/2770423002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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...
Patchset #6 (id:260001) 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...
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_...)
raymes@chromium.org changed reviewers: + dominickn@chromium.org, raymes@chromium.org, timloh@chromium.org
FYI dominickn and timloh (who are doing some permission prompt code refactoring)
Patchset #6 (id:280001) has been deleted
Patchset #5 (id:240001) has been deleted
I am stucked in tests for some time. Right now, most of the permissionprompt in tests are mock ones which don't rely on real UI, compared to the production usage of PermissionPromptImpl. But some tests like PermissionDialogTest actaully uses real permissionprompt [1]. For tests that use the real permissionprompt, mac needs to be excluded since BrowserWindow::IsActive() always return false in tests on MAC, which leads to the current version that has no test coverage and can pass existing tests. On this CL, permission prompt relies on browser UI activation. I feel some work needs to be done for test framework. What do you guys think? Thanks! [1] https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_re... https://codereview.chromium.org/2770423002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_manager.cc (right): https://codereview.chromium.org/2770423002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager.cc:84: browser_active_(false), On 2017/03/27 22:43:45, sky wrote: > Why do you need to cache browser_active_ ? Can't TriggerShowBubble check active? Yes, we can eliminate the usage of browser_active_. I was thinking that they need to be balanced.
I'm not familiar with the tests for this code. If there are locks of mocks, can you change the return value to what you need to trigger the bug? On Thu, Apr 6, 2017 at 3:22 PM, <warx@chromium.org> wrote: > I am stucked in tests for some time. Right now, most of the > permissionprompt in > tests are mock ones which don't rely on real UI, compared to the production > usage of PermissionPromptImpl. But some tests like PermissionDialogTest > actaully > uses real permissionprompt [1]. > > For tests that use the real permissionprompt, mac needs to be excluded > since > BrowserWindow::IsActive() always return false in tests on MAC, which leads > to > the current version that has no test coverage and can pass existing tests. > > On this CL, permission prompt relies on browser UI activation. I feel some > work > needs to be done for test framework. What do you guys think? Thanks! > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/ > permissions/permission_request_manager_browsertest.cc?q= > permissionrequestmanagerbrow&l=117 > > > https://codereview.chromium.org/2770423002/diff/40001/ > chrome/browser/permissions/permission_request_manager.cc > File chrome/browser/permissions/permission_request_manager.cc (right): > > https://codereview.chromium.org/2770423002/diff/40001/ > chrome/browser/permissions/permission_request_manager.cc#newcode84 > chrome/browser/permissions/permission_request_manager.cc:84: > browser_active_(false), > On 2017/03/27 22:43:45, sky wrote: > > Why do you need to cache browser_active_ ? Can't TriggerShowBubble > check active? > > Yes, we can eliminate the usage of browser_active_. I was thinking that > they need to be balanced. > > https://codereview.chromium.org/2770423002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Does this bug occur on platforms other than crOS? I wonder if it makes more sense to push this down into the relevant PermissionPrompt subclass instead of adding more #if's into the PermissionRequestManager.
On 2017/04/11 03:56:55, Timothy Loh wrote: > Does this bug occur on platforms other than crOS? I wonder if it makes more > sense to push this down into the relevant PermissionPrompt subclass instead of > adding more #if's into the PermissionRequestManager. Permission shown on desktop (reporter's bug, that means showing on invisible browser window) is Chrome OS only. But permission bubble shown on inactive browser window is common on all platforms. I started thinking that maybe permission bubble shown on inactive window is not a bad thing. It is just silently showing there, which doesn't cause any UI bad look. sky@, what do you think?
On 2017/04/11 05:43:26, Qiang(Joe) Xu wrote: > On 2017/04/11 03:56:55, Timothy Loh wrote: > > Does this bug occur on platforms other than crOS? I wonder if it makes more > > sense to push this down into the relevant PermissionPrompt subclass instead of > > adding more #if's into the PermissionRequestManager. > > Permission shown on desktop (reporter's bug, that means showing on invisible > browser window) is Chrome OS only. > But permission bubble shown on inactive browser window is common on all > platforms. I started thinking that maybe > permission bubble shown on inactive window is not a bad thing. It is just > silently showing there, which doesn't cause > any UI bad look. sky@, what do you think? IMO that results in a bad UI. Why would we want to show the bubble if the browser isn't active?
On 2017/04/11 16:47:57, sky wrote: > On 2017/04/11 05:43:26, Qiang(Joe) Xu wrote: > > On 2017/04/11 03:56:55, Timothy Loh wrote: > > > Does this bug occur on platforms other than crOS? I wonder if it makes more > > > sense to push this down into the relevant PermissionPrompt subclass instead > of > > > adding more #if's into the PermissionRequestManager. > > > > Permission shown on desktop (reporter's bug, that means showing on invisible > > browser window) is Chrome OS only. > > But permission bubble shown on inactive browser window is common on all > > platforms. I started thinking that maybe > > permission bubble shown on inactive window is not a bad thing. It is just > > silently showing there, which doesn't cause > > any UI bad look. sky@, what do you think? > > IMO that results in a bad UI. Why would we want to show the bubble if the > browser isn't active? What does it mean for the browser not to be "active"? Does it mean that the window is minimized, or doesn't have focus?
Doesn't have focus. This could mean another chrome window, or possibly another application has focus. -Scott On Tue, Apr 11, 2017 at 5:07 PM, <raymes@chromium.org> wrote: > On 2017/04/11 16:47:57, sky wrote: > > On 2017/04/11 05:43:26, Qiang(Joe) Xu wrote: > > > On 2017/04/11 03:56:55, Timothy Loh wrote: > > > > Does this bug occur on platforms other than crOS? I wonder if it > makes > more > > > > sense to push this down into the relevant PermissionPrompt subclass > instead > > of > > > > adding more #if's into the PermissionRequestManager. > > > > > > Permission shown on desktop (reporter's bug, that means showing on > invisible > > > browser window) is Chrome OS only. > > > But permission bubble shown on inactive browser window is common on all > > > platforms. I started thinking that maybe > > > permission bubble shown on inactive window is not a bad thing. It is > just > > > silently showing there, which doesn't cause > > > any UI bad look. sky@, what do you think? > > > > IMO that results in a bad UI. Why would we want to show the bubble if the > > browser isn't active? > > What does it mean for the browser not to be "active"? Does it mean that the > window is minimized, or doesn't have focus? > > https://codereview.chromium.org/2770423002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/04/12 03:40:30, sky wrote: > Doesn't have focus. This could mean another chrome window, or possibly > another application has focus. > > -Scott > > On Tue, Apr 11, 2017 at 5:07 PM, <mailto:raymes@chromium.org> wrote: > > > On 2017/04/11 16:47:57, sky wrote: > > > On 2017/04/11 05:43:26, Qiang(Joe) Xu wrote: > > > > On 2017/04/11 03:56:55, Timothy Loh wrote: > > > > > Does this bug occur on platforms other than crOS? I wonder if it > > makes > > more > > > > > sense to push this down into the relevant PermissionPrompt subclass > > instead > > > of > > > > > adding more #if's into the PermissionRequestManager. > > > > > > > > Permission shown on desktop (reporter's bug, that means showing on > > invisible > > > > browser window) is Chrome OS only. > > > > But permission bubble shown on inactive browser window is common on all > > > > platforms. I started thinking that maybe > > > > permission bubble shown on inactive window is not a bad thing. It is > > just > > > > silently showing there, which doesn't cause > > > > any UI bad look. sky@, what do you think? > > > > > > IMO that results in a bad UI. Why would we want to show the bubble if the > > > browser isn't active? > > > > What does it mean for the browser not to be "active"? Does it mean that the > > window is minimized, or doesn't have focus? > > > > https://codereview.chromium.org/2770423002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Hmm I see - yeah I guess I don't feel too strongly about that. It doesn't seem too problematic to me if a prompt shows up on a window that doesn't have focus, but I'm not a UX expert :)
On 2017/04/12 at 05:46:11, raymes wrote: > On 2017/04/12 03:40:30, sky wrote: > > Doesn't have focus. This could mean another chrome window, or possibly > > another application has focus. > > > > -Scott > > > > On Tue, Apr 11, 2017 at 5:07 PM, <mailto:raymes@chromium.org> wrote: > > > > > On 2017/04/11 16:47:57, sky wrote: > > > > On 2017/04/11 05:43:26, Qiang(Joe) Xu wrote: > > > > > On 2017/04/11 03:56:55, Timothy Loh wrote: > > > > > > Does this bug occur on platforms other than crOS? I wonder if it > > > makes > > > more > > > > > > sense to push this down into the relevant PermissionPrompt subclass > > > instead > > > > of > > > > > > adding more #if's into the PermissionRequestManager. > > > > > > > > > > Permission shown on desktop (reporter's bug, that means showing on > > > invisible > > > > > browser window) is Chrome OS only. > > > > > But permission bubble shown on inactive browser window is common on all > > > > > platforms. I started thinking that maybe > > > > > permission bubble shown on inactive window is not a bad thing. It is > > > just > > > > > silently showing there, which doesn't cause > > > > > any UI bad look. sky@, what do you think? > > > > > > > > IMO that results in a bad UI. Why would we want to show the bubble if the > > > > browser isn't active? > > > > > > What does it mean for the browser not to be "active"? Does it mean that the > > > window is minimized, or doesn't have focus? > > > > > > https://codereview.chromium.org/2770423002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Hmm I see - yeah I guess I don't feel too strongly about that. It doesn't seem too problematic to me if a prompt shows up on a window that doesn't have focus, but I'm not a UX expert :) IMO, if a tab is visible to the user and shows a prompt it might not be that weird. However, coming back to a tab that was visible that will then ask for a permission might look a bit agressive. This said, you might want to check with a UX person :)
On 2017/04/12 15:39:19, mlamouri wrote: > On 2017/04/12 at 05:46:11, raymes wrote: > > On 2017/04/12 03:40:30, sky wrote: > > > Doesn't have focus. This could mean another chrome window, or possibly > > > another application has focus. > > > > > > -Scott > > > > > > On Tue, Apr 11, 2017 at 5:07 PM, <mailto:raymes@chromium.org> wrote: > > > > > > > On 2017/04/11 16:47:57, sky wrote: > > > > > On 2017/04/11 05:43:26, Qiang(Joe) Xu wrote: > > > > > > On 2017/04/11 03:56:55, Timothy Loh wrote: > > > > > > > Does this bug occur on platforms other than crOS? I wonder if it > > > > makes > > > > more > > > > > > > sense to push this down into the relevant PermissionPrompt subclass > > > > instead > > > > > of > > > > > > > adding more #if's into the PermissionRequestManager. > > > > > > > > > > > > Permission shown on desktop (reporter's bug, that means showing on > > > > invisible > > > > > > browser window) is Chrome OS only. > > > > > > But permission bubble shown on inactive browser window is common on > all > > > > > > platforms. I started thinking that maybe > > > > > > permission bubble shown on inactive window is not a bad thing. It is > > > > just > > > > > > silently showing there, which doesn't cause > > > > > > any UI bad look. sky@, what do you think? > > > > > > > > > > IMO that results in a bad UI. Why would we want to show the bubble if > the > > > > > browser isn't active? > > > > > > > > What does it mean for the browser not to be "active"? Does it mean that > the > > > > window is minimized, or doesn't have focus? > > > > > > > > https://codereview.chromium.org/2770423002/ > > > > > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > Hmm I see - yeah I guess I don't feel too strongly about that. It doesn't seem > too problematic to me if a prompt shows up on a window that doesn't have focus, > but I'm not a UX expert :) > > IMO, if a tab is visible to the user and shows a prompt it might not be that > weird. However, coming back to a tab that was visible that will then ask for a > permission might look a bit agressive. This said, you might want to check with a > UX person :) I do agree. We should run this by UX for their thoughts. Warx@, can you do that? kuscher is likely a good one to ask.
Message was sent while issue was closed.
On 2017/04/12 15:49:46, sky wrote: > On 2017/04/12 15:39:19, mlamouri wrote: > > On 2017/04/12 at 05:46:11, raymes wrote: > > > On 2017/04/12 03:40:30, sky wrote: > > > > Doesn't have focus. This could mean another chrome window, or possibly > > > > another application has focus. > > > > > > > > -Scott > > > > > > > > On Tue, Apr 11, 2017 at 5:07 PM, <mailto:raymes@chromium.org> wrote: > > > > > > > > > On 2017/04/11 16:47:57, sky wrote: > > > > > > On 2017/04/11 05:43:26, Qiang(Joe) Xu wrote: > > > > > > > On 2017/04/11 03:56:55, Timothy Loh wrote: > > > > > > > > Does this bug occur on platforms other than crOS? I wonder if it > > > > > makes > > > > > more > > > > > > > > sense to push this down into the relevant PermissionPrompt > subclass > > > > > instead > > > > > > of > > > > > > > > adding more #if's into the PermissionRequestManager. > > > > > > > > > > > > > > Permission shown on desktop (reporter's bug, that means showing on > > > > > invisible > > > > > > > browser window) is Chrome OS only. > > > > > > > But permission bubble shown on inactive browser window is common on > > all > > > > > > > platforms. I started thinking that maybe > > > > > > > permission bubble shown on inactive window is not a bad thing. It is > > > > > just > > > > > > > silently showing there, which doesn't cause > > > > > > > any UI bad look. sky@, what do you think? > > > > > > > > > > > > IMO that results in a bad UI. Why would we want to show the bubble if > > the > > > > > > browser isn't active? > > > > > > > > > > What does it mean for the browser not to be "active"? Does it mean that > > the > > > > > window is minimized, or doesn't have focus? > > > > > > > > > > https://codereview.chromium.org/2770423002/ > > > > > > > > > > > > > -- > > > > You received this message because you are subscribed to the Google Groups > > > > "Chromium-reviews" group. > > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > Hmm I see - yeah I guess I don't feel too strongly about that. It doesn't > seem > > too problematic to me if a prompt shows up on a window that doesn't have > focus, > > but I'm not a UX expert :) > > > > IMO, if a tab is visible to the user and shows a prompt it might not be that > > weird. However, coming back to a tab that was visible that will then ask for a > > permission might look a bit agressive. This said, you might want to check with > a > > UX person :) > > I do agree. We should run this by UX for their thoughts. Warx@, can you do that? > kuscher is likely a good one to ask. I am closing this issue. The root cause on Chrome OS platform is found and tracked in a separate CL: https://codereview.chromium.org/2859333003/. Thanks for everyone's input on this thread, appreciate it!
Message was sent while issue was closed.
Good job for getting to the root of the problem! On Fri, May 5, 2017 at 10:34 AM, <warx@chromium.org> wrote: > On 2017/04/12 15:49:46, sky wrote: > > On 2017/04/12 15:39:19, mlamouri wrote: > > > On 2017/04/12 at 05:46:11, raymes wrote: > > > > On 2017/04/12 03:40:30, sky wrote: > > > > > Doesn't have focus. This could mean another chrome window, or > possibly > > > > > another application has focus. > > > > > > > > > > -Scott > > > > > > > > > > On Tue, Apr 11, 2017 at 5:07 PM, <mailto:raymes@chromium.org> > wrote: > > > > > > > > > > > On 2017/04/11 16:47:57, sky wrote: > > > > > > > On 2017/04/11 05:43:26, Qiang(Joe) Xu wrote: > > > > > > > > On 2017/04/11 03:56:55, Timothy Loh wrote: > > > > > > > > > Does this bug occur on platforms other than crOS? I wonder > if it > > > > > > makes > > > > > > more > > > > > > > > > sense to push this down into the relevant PermissionPrompt > > subclass > > > > > > instead > > > > > > > of > > > > > > > > > adding more #if's into the PermissionRequestManager. > > > > > > > > > > > > > > > > Permission shown on desktop (reporter's bug, that means > showing on > > > > > > invisible > > > > > > > > browser window) is Chrome OS only. > > > > > > > > But permission bubble shown on inactive browser window is > common > on > > > all > > > > > > > > platforms. I started thinking that maybe > > > > > > > > permission bubble shown on inactive window is not a bad > thing. It > is > > > > > > just > > > > > > > > silently showing there, which doesn't cause > > > > > > > > any UI bad look. sky@, what do you think? > > > > > > > > > > > > > > IMO that results in a bad UI. Why would we want to show the > bubble > if > > > the > > > > > > > browser isn't active? > > > > > > > > > > > > What does it mean for the browser not to be "active"? Does it > mean > that > > > the > > > > > > window is minimized, or doesn't have focus? > > > > > > > > > > > > https://codereview.chromium.org/2770423002/ > > > > > > > > > > > > > > > > -- > > > > > You received this message because you are subscribed to the Google > Groups > > > > > "Chromium-reviews" group. > > > > > To unsubscribe from this group and stop receiving emails from it, > send > an > > > email > > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > Hmm I see - yeah I guess I don't feel too strongly about that. It > doesn't > > seem > > > too problematic to me if a prompt shows up on a window that doesn't > have > > focus, > > > but I'm not a UX expert :) > > > > > > IMO, if a tab is visible to the user and shows a prompt it might not > be that > > > weird. However, coming back to a tab that was visible that will then > ask for > a > > > permission might look a bit agressive. This said, you might want to > check > with > > a > > > UX person :) > > > > I do agree. We should run this by UX for their thoughts. Warx@, can you > do > that? > > kuscher is likely a good one to ask. > > I am closing this issue. The root cause on Chrome OS platform is found and > tracked in a > separate CL: https://codereview.chromium.org/2859333003/. > > Thanks for everyone's input on this thread, appreciate it! > > https://codereview.chromium.org/2770423002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
