Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2083113002:
Disable Panels on OSes other than ChromeOS, in preparation to removing the code. (Closed)
|
Created:
4 years, 6 months ago by Dmitry Titov Modified:
4 years, 6 months ago CC:
chromium-reviews, dcheng, jennb, jianli, Dmitry Titov, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable Panels on OSes other than ChromeOS, in preparation to removing the code.
BUG=571511
Committed: https://crrev.com/29d043dfa8b9029637fc783d06a0a36677a65795
Cr-Commit-Position: refs/heads/master@{#401390}
Patch Set 1 #
Total comments: 4
Patch Set 2 : leave flags in place #
Total comments: 2
Messages
Total messages: 15 (4 generated)
dimich@chromium.org changed reviewers: + estade@chromium.org
Evan, I'm making the flag you've introduced (--disable_panels) permanent on OSes other than ChromeOS. Once this is in and is settled (so there is no possibility to revert), I'll remove various Panel implementations as well. PTAL
https://codereview.chromium.org/2083113002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2083113002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5554: + Allow extensions to create panel windows that open outside of the browser frame. Attempts to open a panel will open a popup instead if not enabled. Default behavior is to allow only for whitelisted extensions. Enabled behavior is to allow for all extensions. This functionality will be deprecated soon. nit: technically, it's already deprecated (i.e. use is discouraged). It will be removed soon. https://codereview.chromium.org/2083113002/diff/1/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.cc (left): https://codereview.chromium.org/2083113002/diff/1/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:112: switches::kDisablePanels)) Did you consider leaving the flags alone and just inverting this check, i.e. !HasSwitch(enable)
https://codereview.chromium.org/2083113002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2083113002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5554: + Allow extensions to create panel windows that open outside of the browser frame. Attempts to open a panel will open a popup instead if not enabled. Default behavior is to allow only for whitelisted extensions. Enabled behavior is to allow for all extensions. This functionality will be deprecated soon. On 2016/06/21 21:18:23, Evan Stade wrote: > nit: technically, it's already deprecated (i.e. use is discouraged). It will be > removed soon. Updated wording to mention removal and new default. https://codereview.chromium.org/2083113002/diff/1/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.cc (left): https://codereview.chromium.org/2083113002/diff/1/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.cc:112: switches::kDisablePanels)) On 2016/06/21 21:18:23, Evan Stade wrote: > Did you consider leaving the flags alone and just inverting this check, i.e. > !HasSwitch(enable) Done, keep switches working for now, changing default. Since it'll remain enabled for ChromeOS, the check is a bit different...
I couldn't find in the email thread or in the bug a discussion of why this would remain in place on cros. (Ironically, the comment says "See bug for details" but the bug has few to no details :) It doesn't make much sense to me that it continues to exist on cros solely for the hangouts extension, as they have to change the extension anyway to continue supporting other OSes.
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org
https://codereview.chromium.org/2083113002/diff/20001/chrome/browser/ui/panel... File chrome/browser/ui/panels/panel_manager.cc (right): https://codereview.chromium.org/2083113002/diff/20001/chrome/browser/ui/panel... chrome/browser/ui/panels/panel_manager.cc:138: // Without --enable-panels, only support Hangouts on ChromeOS. This comment is a bit misleading - Chrome OS does not use this code at all. Maybe we should refer to this feature as "Non Chrome OS Desktop panels" or something equally awkward? :)
PTAL please. I think this patch is minimal (massive cleanup comes later) and it achieves the goal of changing default behavior of chrome.windows.create on non-ChromeOS platforms to fall on popups. On ChromeOS, it'll be still using panels for Hangouts extension - for a while, subject to be removed as well at some point. https://codereview.chromium.org/2083113002/diff/20001/chrome/browser/ui/panel... File chrome/browser/ui/panels/panel_manager.cc (right): https://codereview.chromium.org/2083113002/diff/20001/chrome/browser/ui/panel... chrome/browser/ui/panels/panel_manager.cc:138: // Without --enable-panels, only support Hangouts on ChromeOS. On 2016/06/22 17:08:33, stevenjb wrote: > This comment is a bit misleading - Chrome OS does not use this code at all. > Maybe we should refer to this feature as "Non Chrome OS Desktop panels" or > something equally awkward? :) It is in fact correct, although I agree not very clear. ChromeOS path in chrome.windows.create is still using static PanelManager::ShouldUsePanels() to activate. The effect of the change is that this hack will continue to work on ChromeOS but not on other desktop OSes. Once the disable is stable and Hangouts team have time to react, I'm planning to clean the code up leaving only ChromeOS parts, and removing chrome.windows.create hack as soon as we can.
On 2016/06/22 17:53:58, Dmitry Titov wrote: > PTAL please. > > I think this patch is minimal (massive cleanup comes later) and it achieves the > goal of changing default behavior of chrome.windows.create on non-ChromeOS > platforms to fall on popups. On ChromeOS, it'll be still using panels for > Hangouts extension - for a while, subject to be removed as well at some point. > > https://codereview.chromium.org/2083113002/diff/20001/chrome/browser/ui/panel... > File chrome/browser/ui/panels/panel_manager.cc (right): > > https://codereview.chromium.org/2083113002/diff/20001/chrome/browser/ui/panel... > chrome/browser/ui/panels/panel_manager.cc:138: // Without --enable-panels, only > support Hangouts on ChromeOS. > On 2016/06/22 17:08:33, stevenjb wrote: > > This comment is a bit misleading - Chrome OS does not use this code at all. > > Maybe we should refer to this feature as "Non Chrome OS Desktop panels" or > > something equally awkward? :) > > It is in fact correct, although I agree not very clear. ChromeOS path in > chrome.windows.create is still using static PanelManager::ShouldUsePanels() to > activate. The effect of the change is that this hack will continue to work on > ChromeOS but not on other desktop OSes. Once the disable is stable and Hangouts > team have time to react, I'm planning to clean the code up leaving only ChromeOS > parts, and removing chrome.windows.create hack as soon as we can. I'll defer to Steven here.
I really don't care about a comment in code we are planning to erase, so lgtm.
The CQ bit was checked by dimich@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2083113002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Disable Panels on OSes other than ChromeOS, in preparation to removing the code. BUG=571511 ========== to ========== Disable Panels on OSes other than ChromeOS, in preparation to removing the code. BUG=571511 Committed: https://crrev.com/29d043dfa8b9029637fc783d06a0a36677a65795 Cr-Commit-Position: refs/heads/master@{#401390} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/29d043dfa8b9029637fc783d06a0a36677a65795 Cr-Commit-Position: refs/heads/master@{#401390} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
