Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
972123003:
Fixed behavior in case of disabled devtools (Closed)
|
Created:
5 years, 9 months ago by peletskyi Modified:
5 years, 9 months ago CC:
chromium-reviews, aandrey+blink_chromium.org, yurys, pfeldman, devtools-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixed behavior in case of disabled devtools
BUG=460543
Committed: https://crrev.com/5ec60bf78732b36a771459d4c3a8fbab6cab6260
Cr-Commit-Position: refs/heads/master@{#320257}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added test and removed reurn of pointer from OpenDevToolsWindow(). #
Total comments: 8
Patch Set 3 : Added one test + changed function ToggleDevToolsWindow() #Patch Set 4 : #Patch Set 5 : Fixed conflict #
Total comments: 5
Patch Set 6 : #Patch Set 7 : #
Total comments: 10
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Total comments: 3
Patch Set 11 : #Patch Set 12 : #
Messages
Total messages: 42 (15 generated)
peletskyi@chromium.org changed reviewers: + atwilson@chromium.org, vsevik@chromium.org
Hi all, please review my solution of the bug. Best wishes, Oleksandr
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
lgtm
The CQ bit was checked by peletskyi@chromium.org
The CQ bit was unchecked by peletskyi@chromium.org
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
lgtm https://codereview.chromium.org/972123003/diff/1/chrome/browser/devtools/devt... File chrome/browser/devtools/devtools_window.cc (right): https://codereview.chromium.org/972123003/diff/1/chrome/browser/devtools/devt... chrome/browser/devtools/devtools_window.cc:424: // If development tools disabled by policy return NULL nit: "null" please https://codereview.chromium.org/972123003/diff/1/chrome/browser/devtools/devt... chrome/browser/devtools/devtools_window.cc:426: return NULL; nullptr
Just to document our offline discussion: 1) We should change OpenDevToolsWindow to not have a return value, so we don't accidentally add code later which crashes with an NPE when policy is enforced. 2) Let's add a test to ensure that the window doesn't open when disabled by policy.
lgtm with one comment, but good to get another lgtm from devtools owners. https://codereview.chromium.org/972123003/diff/20001/chrome/browser/devtools/... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/972123003/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/devtools_sanity_browsertest.cc:894: IN_PROC_BROWSER_TEST_F(DevToolsSanityTest, PolicyTrue) { Maybe also test the case where you set the DevToolsDisabled policy to false, and make sure that WindowExists() is true.
There is a test for DevTools policy: There is already a test for DevTools policy: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pol.... Now I'm interested in why the test works, but your usecase does not. Please do not submit before finding out the cause. https://codereview.chromium.org/972123003/diff/20001/chrome/browser/ui/apps/c... File chrome/browser/ui/apps/chrome_app_window_client.cc (right): https://codereview.chromium.org/972123003/diff/20001/chrome/browser/ui/apps/c... chrome/browser/ui/apps/chrome_app_window_client.cc:66: devtools_window->SetLoadCompletedCallback(callback); What if there is no window? Perhaps something is waiting for the |callback| to be called?
On 2015/03/03 19:31:24, dgozman wrote: > There is a test for DevTools policy: There is already a test for DevTools > policy: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pol.... > > Now I'm interested in why the test works, but your usecase does not. Please do > not submit before finding out the cause. The cause is that this test only tests a single way of bringing up dev tools windows (i.e. a browser command) but there are many other ways to bring up dev tools (navigating to chrome://inspect, for example). This CL disables dev tools at a lower level so will prevent users from accessing dev tools through other means. > > https://codereview.chromium.org/972123003/diff/20001/chrome/browser/ui/apps/c... > File chrome/browser/ui/apps/chrome_app_window_client.cc (right): > > https://codereview.chromium.org/972123003/diff/20001/chrome/browser/ui/apps/c... > chrome/browser/ui/apps/chrome_app_window_client.cc:66: > devtools_window->SetLoadCompletedCallback(callback); > What if there is no window? Perhaps something is waiting for the |callback| to > be called? What would you like here - should we invoke the callback in this case, or not?
> The cause is that this test only tests a single way of bringing up dev tools > windows (i.e. a browser command) but there are many other ways to bring up dev > tools (navigating to chrome://inspect, for example). This CL disables dev tools > at a lower level so will prevent users from accessing dev tools through other > means. Thank you for explanation. Should we remove the previous check to avoid duplication? > https://codereview.chromium.org/972123003/diff/20001/chrome/browser/ui/apps/c... > > chrome/browser/ui/apps/chrome_app_window_client.cc:66: > > devtools_window->SetLoadCompletedCallback(callback); > > What if there is no window? Perhaps something is waiting for the |callback| to > > be called? > > What would you like here - should we invoke the callback in this case, or not? I think we should invoke the callback. For example, app window api waits for it before sending response: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser.... Someone from the apps team may know better though. https://codereview.chromium.org/972123003/diff/20001/chrome/browser/devtools/... File chrome/browser/devtools/devtools_window.h (right): https://codereview.chromium.org/972123003/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/devtools_window.h:66: static void OpenDevToolsWindow(content::WebContents* inspected_web_contents, Let's just mention in the comment to each method that returned window may be null, and modify all the callsites to check for that. It's not convenient to use DevToolsAgentHost and FindDevToolsWindow anytime I need the return value. https://codereview.chromium.org/972123003/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/devtools_window.h:74: static DevToolsWindow* ToggleDevToolsWindow( This method should be affected by the policy as well.
On 2015/03/03 20:04:54, dgozman wrote: > > The cause is that this test only tests a single way of bringing up dev tools > > windows (i.e. a browser command) but there are many other ways to bring up dev > > tools (navigating to chrome://inspect, for example). This CL disables dev > tools > > at a lower level so will prevent users from accessing dev tools through other > > means. > Thank you for explanation. Should we remove the previous check to avoid > duplication? That test is also testing whether disabling dev tools by policy closes existing dev tools window, which I think is a good check. In any case, I don't think there's any redundant check to remove here. > > > > https://codereview.chromium.org/972123003/diff/20001/chrome/browser/ui/apps/c... > > > chrome/browser/ui/apps/chrome_app_window_client.cc:66: > > > devtools_window->SetLoadCompletedCallback(callback); > > > What if there is no window? Perhaps something is waiting for the |callback| > to > > > be called? > > > > What would you like here - should we invoke the callback in this case, or not? > I think we should invoke the callback. For example, app window api waits for it > before sending response: SG, we can invoke the callback.
https://codereview.chromium.org/972123003/diff/20001/chrome/browser/devtools/... File chrome/browser/devtools/devtools_window.h (right): https://codereview.chromium.org/972123003/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/devtools_window.h:66: static void OpenDevToolsWindow(content::WebContents* inspected_web_contents, On 2015/03/03 20:04:54, dgozman wrote: > Let's just mention in the comment to each method that returned window may be > null, and modify all the callsites to check for that. It's not convenient to use > DevToolsAgentHost and FindDevToolsWindow anytime I need the return value. In my experience, this will lead to crashes because people will misread the documentation or forget to do the check correctly, and won't test with dev tools disabled - this happens all the time, for example, when sync is disabled by command-line flags/policy. The current CL is a bit more foolproof. If you feel strongly that we should continue to return a value, then we can make this change, but I intentionally asked peletskyi@ to remove the return value since only two callsites use it currently, and one of them already had a DevToolsAgentHost. Anyhow, let me know how strongly you feel about this - we can change it back to returning a window if you still prefer, but I wanted to explain the motivation a bit more. https://codereview.chromium.org/972123003/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/devtools_window.h:74: static DevToolsWindow* ToggleDevToolsWindow( On 2015/03/03 20:04:54, dgozman wrote: > This method should be affected by the policy as well. This uses OpenDevToolsWindow() so it does the right thing, but agreed we should update the documentation for both routines to reflect policy behavior.
https://codereview.chromium.org/972123003/diff/20001/chrome/browser/devtools/... File chrome/browser/devtools/devtools_window.h (right): https://codereview.chromium.org/972123003/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/devtools_window.h:66: static void OpenDevToolsWindow(content::WebContents* inspected_web_contents, On 2015/03/04 13:32:42, Andrew T Wilson wrote: > On 2015/03/03 20:04:54, dgozman wrote: > > Let's just mention in the comment to each method that returned window may be > > null, and modify all the callsites to check for that. It's not convenient to > use > > DevToolsAgentHost and FindDevToolsWindow anytime I need the return value. > > In my experience, this will lead to crashes because people will misread the > documentation or forget to do the check correctly, and won't test with dev tools > disabled - this happens all the time, for example, when sync is disabled by > command-line flags/policy. The current CL is a bit more foolproof. > > If you feel strongly that we should continue to return a value, then we can make > this change, but I intentionally asked peletskyi@ to remove the return value > since only two callsites use it currently, and one of them already had a > DevToolsAgentHost. > > Anyhow, let me know how strongly you feel about this - we can change it back to > returning a window if you still prefer, but I wanted to explain the motivation a > bit more. I don't feel strongly. Let's go with void. https://codereview.chromium.org/972123003/diff/20001/chrome/browser/devtools/... chrome/browser/devtools/devtools_window.h:74: static DevToolsWindow* ToggleDevToolsWindow( On 2015/03/04 13:32:42, Andrew T Wilson wrote: > On 2015/03/03 20:04:54, dgozman wrote: > > This method should be affected by the policy as well. > > This uses OpenDevToolsWindow() so it does the right thing, but agreed we should > update the documentation for both routines to reflect policy behavior. The other way around: OpenDevToolsWindow uses ToggleDevToolsWindow, so the check should actually move there.
The CQ bit was checked by atwilson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org, pfeldman@chromium.org, atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/972123003/#ps60001 (title: " ")
The CQ bit was unchecked by atwilson@chromium.org
On 2015/03/04 13:44:48, dgozman wrote: > https://codereview.chromium.org/972123003/diff/20001/chrome/browser/devtools/... > File chrome/browser/devtools/devtools_window.h (right): > > https://codereview.chromium.org/972123003/diff/20001/chrome/browser/devtools/... > chrome/browser/devtools/devtools_window.h:66: static void > OpenDevToolsWindow(content::WebContents* inspected_web_contents, > On 2015/03/04 13:32:42, Andrew T Wilson wrote: > > On 2015/03/03 20:04:54, dgozman wrote: > > > Let's just mention in the comment to each method that returned window may be > > > null, and modify all the callsites to check for that. It's not convenient to > > use > > > DevToolsAgentHost and FindDevToolsWindow anytime I need the return value. > > > > In my experience, this will lead to crashes because people will misread the > > documentation or forget to do the check correctly, and won't test with dev > tools > > disabled - this happens all the time, for example, when sync is disabled by > > command-line flags/policy. The current CL is a bit more foolproof. > > > > If you feel strongly that we should continue to return a value, then we can > make > > this change, but I intentionally asked peletskyi@ to remove the return value > > since only two callsites use it currently, and one of them already had a > > DevToolsAgentHost. > > > > Anyhow, let me know how strongly you feel about this - we can change it back > to > > returning a window if you still prefer, but I wanted to explain the motivation > a > > bit more. > > I don't feel strongly. Let's go with void. > > https://codereview.chromium.org/972123003/diff/20001/chrome/browser/devtools/... > chrome/browser/devtools/devtools_window.h:74: static DevToolsWindow* > ToggleDevToolsWindow( > On 2015/03/04 13:32:42, Andrew T Wilson wrote: > > On 2015/03/03 20:04:54, dgozman wrote: > > > This method should be affected by the policy as well. > > > > This uses OpenDevToolsWindow() so it does the right thing, but agreed we > should > > update the documentation for both routines to reflect policy behavior. > > The other way around: OpenDevToolsWindow uses ToggleDevToolsWindow, so the check > should actually move there. PTAL,there are some changes from the first patch.
https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... chrome/browser/devtools/devtools_sanity_browsertest.cc:894: IN_PROC_BROWSER_TEST_F(DevToolsSanityTest, PolicyTrue) { Let's not reuse DevToolsSanityTest here, since you don't need any of it's functionality. https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... File chrome/browser/devtools/devtools_window.cc (right): https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... chrome/browser/devtools/devtools_window.cc:412: DevToolsWindow* DevToolsWindow::CreateDevToolsWindowForWorker( This one should check for policy as well. https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... chrome/browser/devtools/devtools_window.cc:456: window = Create(profile, GURL(), nullptr, isWorker, This one should check for policy as well. We should move the policy check to |Create| method. Sorry for not spotting that earlier. https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... File chrome/browser/devtools/devtools_window.h (right): https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... chrome/browser/devtools/devtools_window.h:82: // scoped_refptr<content::DevToolsAgentHost> agent( Please surround code snippet with blank lines and fix indentation there. https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... chrome/browser/devtools/devtools_window.h:193: static DevToolsWindow* FindDevToolsWindow(content::DevToolsAgentHost*); Please move this method near |AsDevToolsWindow| in header.
On 2015/03/10 05:43:24, dgozman wrote: > https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... > File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): > > https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... > chrome/browser/devtools/devtools_sanity_browsertest.cc:894: > IN_PROC_BROWSER_TEST_F(DevToolsSanityTest, PolicyTrue) { > Let's not reuse DevToolsSanityTest here, since you don't need any of it's > functionality. > > https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... > File chrome/browser/devtools/devtools_window.cc (right): > > https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... > chrome/browser/devtools/devtools_window.cc:412: DevToolsWindow* > DevToolsWindow::CreateDevToolsWindowForWorker( > This one should check for policy as well. > > https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... > chrome/browser/devtools/devtools_window.cc:456: window = Create(profile, GURL(), > nullptr, isWorker, > This one should check for policy as well. We should move the policy check to > |Create| method. Sorry for not spotting that earlier. > > https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... > File chrome/browser/devtools/devtools_window.h (right): > > https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... > chrome/browser/devtools/devtools_window.h:82: // > scoped_refptr<content::DevToolsAgentHost> agent( > Please surround code snippet with blank lines and fix indentation there. > > https://codereview.chromium.org/972123003/diff/80001/chrome/browser/devtools/... > chrome/browser/devtools/devtools_window.h:193: static DevToolsWindow* > FindDevToolsWindow(content::DevToolsAgentHost*); > Please move this method near |AsDevToolsWindow| in header. Please take another look :)
Almost there! https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:164: void OpenDevToolsWindowAndAgentHost(const std::string& test_page) { |test_page| not used. https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:165: ASSERT_TRUE(test_server()->Start()); Don't start test server. https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:166: GURL url("about:blank"); inline https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:912: IN_PROC_BROWSER_TEST_F(DevToolsPolicyTest, PolicyFalse) { I don't think this tests anything. I'd rather remove it and inline all the methods into previous test, removing the class. https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... File chrome/browser/devtools/devtools_window.cc (right): https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... chrome/browser/devtools/devtools_window.cc:406: DCHECK(window); Remove DCHECK. https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... chrome/browser/devtools/devtools_window.cc:425: DevToolsWindow::OpenDevToolsWindow(inspected_web_contents, nit: let's revert back to ToggleDevToolsWindow https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... chrome/browser/devtools/devtools_window.cc:445: DCHECK(window); remove DCHECK https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... chrome/browser/devtools/devtools_window.cc:508: DCHECK(window); remove DCHECK https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... chrome/browser/devtools/devtools_window.cc:747: if (AreDeveloperToolsDisabled(inspected_web_contents)) I'd just inline the function here. Note that |profile| is already passed as a parameter. https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... File chrome/browser/devtools/devtools_window.h (right): https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... chrome/browser/devtools/devtools_window.h:90: // content::DevToolsAgentHost::GetOrCreateFor(inspected_web_contents)); nit: indent by two spaces
On 2015/03/10 19:36:35, dgozman wrote: > Almost there! > > https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... > File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): > > https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... > chrome/browser/devtools/devtools_sanity_browsertest.cc:164: void > OpenDevToolsWindowAndAgentHost(const std::string& test_page) { > |test_page| not used. > > https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... > chrome/browser/devtools/devtools_sanity_browsertest.cc:165: > ASSERT_TRUE(test_server()->Start()); > Don't start test server. > > https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... > chrome/browser/devtools/devtools_sanity_browsertest.cc:166: GURL > url("about:blank"); > inline > > https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... > chrome/browser/devtools/devtools_sanity_browsertest.cc:912: > IN_PROC_BROWSER_TEST_F(DevToolsPolicyTest, PolicyFalse) { > I don't think this tests anything. I'd rather remove it and inline all the > methods into previous test, removing the class. > > https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... > File chrome/browser/devtools/devtools_window.cc (right): > > https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... > chrome/browser/devtools/devtools_window.cc:406: DCHECK(window); > Remove DCHECK. > > https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... > chrome/browser/devtools/devtools_window.cc:425: > DevToolsWindow::OpenDevToolsWindow(inspected_web_contents, > nit: let's revert back to ToggleDevToolsWindow > > https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... > chrome/browser/devtools/devtools_window.cc:445: DCHECK(window); > remove DCHECK > > https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... > chrome/browser/devtools/devtools_window.cc:508: DCHECK(window); > remove DCHECK > > https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... > chrome/browser/devtools/devtools_window.cc:747: if > (AreDeveloperToolsDisabled(inspected_web_contents)) > I'd just inline the function here. Note that |profile| is already passed as a > parameter. > > https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... > File chrome/browser/devtools/devtools_window.h (right): > > https://codereview.chromium.org/972123003/diff/120001/chrome/browser/devtools... > chrome/browser/devtools/devtools_window.h:90: // > content::DevToolsAgentHost::GetOrCreateFor(inspected_web_contents)); > nit: indent by two spaces PTAL
lgtm. Thank you! https://codereview.chromium.org/972123003/diff/180001/chrome/browser/devtools... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/972123003/diff/180001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:957: typedef InProcessBrowserTest DevToolsPolicyTest; typedef -> using https://codereview.chromium.org/972123003/diff/180001/chrome/browser/devtools... chrome/browser/devtools/devtools_sanity_browsertest.cc:960: true); nit: indentation https://codereview.chromium.org/972123003/diff/180001/chrome/browser/devtools... File chrome/browser/devtools/devtools_window.cc (right): https://codereview.chromium.org/972123003/diff/180001/chrome/browser/devtools... chrome/browser/devtools/devtools_window.cc:726: // If development tools disabled by policy don't open the window. nit: "development" -> "developer"
On 2015/03/11 10:32:32, dgozman wrote: > lgtm. Thank you! > > https://codereview.chromium.org/972123003/diff/180001/chrome/browser/devtools... > File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): > > https://codereview.chromium.org/972123003/diff/180001/chrome/browser/devtools... > chrome/browser/devtools/devtools_sanity_browsertest.cc:957: typedef > InProcessBrowserTest DevToolsPolicyTest; > typedef -> using > > https://codereview.chromium.org/972123003/diff/180001/chrome/browser/devtools... > chrome/browser/devtools/devtools_sanity_browsertest.cc:960: true); > nit: indentation > > https://codereview.chromium.org/972123003/diff/180001/chrome/browser/devtools... > File chrome/browser/devtools/devtools_window.cc (right): > > https://codereview.chromium.org/972123003/diff/180001/chrome/browser/devtools... > chrome/browser/devtools/devtools_window.cc:726: // If development tools disabled > by policy don't open the window. > nit: "development" -> "developer" Done :)
The CQ bit was checked by peletskyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from atwilson@chromium.org, pfeldman@chromium.org, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/972123003/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/972123003/200001
The CQ bit was checked by peletskyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from atwilson@chromium.org, dgozman@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/972123003/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/972123003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
peletskyi@chromium.org changed reviewers: + tapted@chromium.org
Hi, please take a look to my changes in chrome/browser/ui/apps/chrome_app_window_client.cc Thanks :)
The changes to chrome_app_window_client.cc lgtm. I have a wordy comment with potential follow-ups though :) The CL description needs some work - I was a bit lost. Usually it's good to see: Summary Description of current issue (why this change is needed). More detailed description of CL (how this change fixes the problem just described). e.g. Summary = "Respect DeveloperToolsDisabled enterprise policy when opening devtools via chrome://extensions" Currently clicking "background page" on chrome://extensions is able to open devtools for an extension, when ... even though etc.etc. This CL ensures this can't happen by instead doing foo and bar etc etc (please ensure my understanding is correct). I think this also fixes attempts to open devtools from a right-click -> inspect on a packaged app (was that intentional? and did you check that codepath to ensure it still behaves?). E.g. after enabling chrome://flags/#debug-packed-apps and opening, e.g. Google Keep -> right-click on background -> Inspect. I guess another question/follow-up would be whether we should just disable that flag and the "Developer mode" checkbox on chrome://extensions altogether when this enterprise settings is set, but I don't know much about enterprise policy stuff. That might offer a better UX (but this CL is more robust technically).
On 2015/03/11 18:45:05, tapted wrote: > The changes to chrome_app_window_client.cc lgtm. I have a wordy comment with > potential follow-ups though :) > > The CL description needs some work - I was a bit lost. Usually it's good to see: > > Summary > > Description of current issue (why this change is needed). > > More detailed description of CL (how this change fixes the problem just > described). > > > > e.g. > > Summary = "Respect DeveloperToolsDisabled enterprise policy when opening > devtools via chrome://extensions" > > Currently clicking "background page" on chrome://extensions is able to open > devtools for an extension, when ... even though etc.etc. > > This CL ensures this can't happen by instead doing foo and bar etc etc > > > > (please ensure my understanding is correct). > > I think this also fixes attempts to open devtools from a right-click -> inspect > on a packaged app (was that intentional? and did you check that codepath to > ensure it still behaves?). > > E.g. after enabling chrome://flags/#debug-packed-apps and opening, e.g. Google > Keep -> right-click on background -> Inspect. > > > I guess another question/follow-up would be whether we should just disable that > flag and the "Developer mode" checkbox on chrome://extensions altogether when > this enterprise settings is set, but I don't know much about enterprise policy > stuff. That might offer a better UX (but this CL is more robust technically). Summary: There are some use cases, when a user can open Developer Tools when the policy disallows him to do this. E.g. see bug description https://code.google.com/p/chromium/issues/detail?id=460543#c1 Description: When we try to create Developer Tools we check the policy. This gives us very robust solution (even if some UI element was not disabled when it should be disabled and tries to open DevTools, this attempt will not lead to opening of DevTools). Detailed description: Policy check was added to "Create" function, that is responsible for creation of DevTool window. In case of policy is set NULL is returned. Thus in already existed code we should check if pointer to the DevTools window is equal to NULL. Besides this it was found out that the pointer to DevTools window is used in just a few cases and most of them are in tests. Thus to prevent errors now one can get window pointer only from DevToolsAgentHost, but not directly from the functions ToggleDevToolsWindow, OpenDevToolsWindow etc (chrome/browser/ui/apps/chrome_app_window_client.cc is the case).
The CQ bit was checked by peletskyi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/972123003/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/5ec60bf78732b36a771459d4c3a8fbab6cab6260 Cr-Commit-Position: refs/heads/master@{#320257} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
