screenshot disabling policy

Screenshot disabling policy (work in progress)

Add group policy to disable taking screenshots:
* by keyboard shortcut (using local state)
* when reporting feedback (using local state)
* by extension API (using user prefs)

How to test:
- Press Ctrl+F5 (or the corresponding key combination on chromebook) to take a screenshot. This should flash a screen and create a screenshot in your files, if the policy is not enabled.
- Install http://code.google.com/chrome/extensions/examples/api/tabs/screenshot.zip extension to test extension APIs. This allows to take tab screenshots of code.google.com.
- In Chrome menu, use Tools -> Report an Issue... to go to issue screen, which should contain a screenshot (if the policy is not enabled)

Implementation:
- Changed browser::GrabWindowSnapshot and ScreenshotTaker::GrabWindowSnapshot to block screenshots triggered by accelerators and feedback form
- CaptureVisibleTabFunction::RunImpl to disable tab screenshots made by extension API

BUG=chromium-os:24747
TEST= Disable screenshots through policy, check I screenshots can created/viewed
TBR= zmo@chromium.org, jbates@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148963

[Mattias Nissler (ping if slow), 2012-07-06 15:15:51]

http://codereview.chromium.org/10692110/diff/1/chrome/app/policy/policy_templ...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10692110/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:134: 'supported_on':
['chrome_os:0.11-'],  # TODO: chose correct version
chrome_os.22-

http://codereview.chromium.org/10692110/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:141: 'desc': '''Disables taking
screenshots with keyboard accelerators shortcuts'''
More verbose description, see other descriptions for examples.

http://codereview.chromium.org/10692110/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10692110/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/tabs/tabs.cc:82: #if defined(OS_CHROMEOS)
no need for preprocessor guards

http://codereview.chromium.org/10692110/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/tabs/tabs.cc:1607: PrefService* service =
g_browser_process->local_state();
Have you thought about registering this as a Profile pref as well and using
profile()->GetPrefs() here?

http://codereview.chromium.org/10692110/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/tabs/tabs.cc:1609:
EXTENSION_FUNCTION_ERROR("keys::kXXX");
add proper constants.

http://codereview.chromium.org/10692110/diff/1/chrome/browser/prefs/browser_p...
File chrome/browser/prefs/browser_prefs.cc (right):

http://codereview.chromium.org/10692110/diff/1/chrome/browser/prefs/browser_p...
chrome/browser/prefs/browser_prefs.cc:173:
local_state->RegisterBooleanPref(prefs::kDisableScreenshots, false);
Create static function for registration.

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
File chrome/browser/ui/views/ash/chrome_shell_delegate.cc (right):

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:343:
DCHECK(prefs::kDisableScreenshots ==
DCHECK_EQ

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:344:
*content::Details<std::string>(details).ptr());
4 spaces indentation after line breaks

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:359:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
I don't think we need this here, because the class lives on the UI thread.

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:361:
ash::Shell::GetInstance()->accelerator_controller();
indentation

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:364: if
(service->GetBoolean(prefs::kDisableScreenshots))
add curlies for multi-line block

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:369:
scoped_ptr<ash::ScreenshotDelegate>(new ScreenshotTaker).Pass());
Maybe this is shorter:

scoped_ptr<ash::ScreenshotDelegate> delegate;
if (/* enabled */)
    delegate.reset(new ScreenshotTaker);
accelerator_controller->SetScreenshotDelegate(delegate.Pass());

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
File chrome/browser/ui/views/ash/chrome_shell_delegate.h (right):

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
chrome/browser/ui/views/ash/chrome_shell_delegate.h:17: #if defined(OS_CHROMEOS)
no need for guards

http://codereview.chromium.org/10692110/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/10692110/diff/1/chrome/common/pref_names.cc#ne...
chrome/common/pref_names.cc:491: // Disables screenshots by communicating with
ash
Missing period.
Update the reflect new implementation.

[qfel, 2012-07-06 17:00:39]

http://codereview.chromium.org/10692110/diff/1/chrome/app/policy/policy_templ...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10692110/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:134: 'supported_on':
['chrome_os:0.11-'],  # TODO: chose correct version
Done

http://codereview.chromium.org/10692110/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:141: 'desc': '''Disables taking
screenshots with keyboard accelerators shortcuts'''
Done (more verbose, not sure if verbose enough)

http://codereview.chromium.org/10692110/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10692110/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/tabs/tabs.cc:82: #if defined(OS_CHROMEOS)
Done

http://codereview.chromium.org/10692110/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/tabs/tabs.cc:1607: PrefService* service =
g_browser_process->local_state();
On 2012/07/06 15:15:52, Mattias Nissler wrote:
> Have you thought about registering this as a Profile pref as well and using
> profile()->GetPrefs() here?

Done.

http://codereview.chromium.org/10692110/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/tabs/tabs.cc:1609:
EXTENSION_FUNCTION_ERROR("keys::kXXX");
On 2012/07/06 15:15:52, Mattias Nissler wrote:
> add proper constants.

Done.

http://codereview.chromium.org/10692110/diff/1/chrome/browser/prefs/browser_p...
File chrome/browser/prefs/browser_prefs.cc (right):

http://codereview.chromium.org/10692110/diff/1/chrome/browser/prefs/browser_p...
chrome/browser/prefs/browser_prefs.cc:173:
local_state->RegisterBooleanPref(prefs::kDisableScreenshots, false);
Done

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
File chrome/browser/ui/views/ash/chrome_shell_delegate.cc (right):

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:343:
DCHECK(prefs::kDisableScreenshots ==
Done

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:344:
*content::Details<std::string>(details).ptr());
Done

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:359:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
Done

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:361:
ash::Shell::GetInstance()->accelerator_controller();
Done

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:364: if
(service->GetBoolean(prefs::kDisableScreenshots))
On 2012/07/06 15:15:52, Mattias Nissler wrote:
> add curlies for multi-line block

Done.

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:369:
scoped_ptr<ash::ScreenshotDelegate>(new ScreenshotTaker).Pass());
On 2012/07/06 15:15:52, Mattias Nissler wrote:
> Maybe this is shorter:
> 
> scoped_ptr<ash::ScreenshotDelegate> delegate;
> if (/* enabled */)
>     delegate.reset(new ScreenshotTaker);
> accelerator_controller->SetScreenshotDelegate(delegate.Pass());

Done.

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
File chrome/browser/ui/views/ash/chrome_shell_delegate.h (right):

http://codereview.chromium.org/10692110/diff/1/chrome/browser/ui/views/ash/ch...
chrome/browser/ui/views/ash/chrome_shell_delegate.h:17: #if defined(OS_CHROMEOS)
Done

http://codereview.chromium.org/10692110/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/10692110/diff/1/chrome/common/pref_names.cc#ne...
chrome/common/pref_names.cc:491: // Disables screenshots by communicating with
ash
On 2012/07/06 15:15:52, Mattias Nissler wrote:
> Missing period.
> Update the reflect new implementation.

Done.

[Mattias Nissler (ping if slow), 2012-07-09 09:30:12]

A couple more comments. Can you also fill in the CL description please?

Adding sky for the ash OWNERS review.

http://codereview.chromium.org/10692110/diff/2002/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10692110/diff/2002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:2765: 'desc': '''Disables taking
screenshots
All other description strings end the first sentence in a period, please do so
as well.

http://codereview.chromium.org/10692110/diff/2002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:2767: If enabled screenshots cannot be
taken using keyboard shortcuts or extension APIs'''
Missing period.
Please also add a sentence explaining what happens if the policy is set to false
or not set.

http://codereview.chromium.org/10692110/diff/2002/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

http://codereview.chromium.org/10692110/diff/2002/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:251:
ChromeShellDelegate::RegisterUserPrefs(user_prefs);
I think making this OS_CHROMEOS only will break, because the extension API code
will try to read the pref also for non-OS_CHROMEOS configurations. Have you
tested on a desktop build?

That brings up the question of how to handle desktop builds :) Either ifdef
OS_CHROMEOS in the extension API (but that seems weird) or move the pref
registration somewhere else...

http://codereview.chromium.org/10692110/diff/2002/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/chrome_shell_delegate.cc (right):

http://codereview.chromium.org/10692110/diff/2002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:343:
*content::Details<std::string>(details).ptr());
align parameters at (

http://codereview.chromium.org/10692110/diff/2002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:362:
PrefService::UNSYNCABLE_PREF);
align at (

http://codereview.chromium.org/10692110/diff/2002/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/chrome_shell_delegate.h (right):

http://codereview.chromium.org/10692110/diff/2002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/chrome_shell_delegate.h:36: // This is used for
registering both local state and user prefs.
That comment doesn't seem very helpful as the function names tell the whole
story already.

http://codereview.chromium.org/10692110/diff/2002/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/10692110/diff/2002/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:491: // Disables screenshot accelerators and
extension APIs.
Would be good to put a sentence saying that the extension API is controlled by
the profile pref, while the accelerator code reads local state.

[qfel, 2012-07-11 12:34:49]

Issue reporting screen still needs some tweaks when the policy is enabled,
otherwise it is hopefully fine.

http://codereview.chromium.org/10692110/diff/2002/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10692110/diff/2002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:2765: 'desc': '''Disables taking
screenshots
On 2012/07/09 09:30:12, Mattias Nissler wrote:
> All other description strings end the first sentence in a period, please do so
> as well.

Done.

http://codereview.chromium.org/10692110/diff/2002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:2767: If enabled screenshots cannot be
taken using keyboard shortcuts or extension APIs'''
On 2012/07/09 09:30:12, Mattias Nissler wrote:
> Missing period.
> Please also add a sentence explaining what happens if the policy is set to
false
> or not set.

Done.

http://codereview.chromium.org/10692110/diff/2002/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

http://codereview.chromium.org/10692110/diff/2002/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:251:
ChromeShellDelegate::RegisterUserPrefs(user_prefs);
Updated to make profile pref always present, and local state  OS_CHROMEOS
guarded. Not sure it's the best approach though.

http://codereview.chromium.org/10692110/diff/2002/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/chrome_shell_delegate.cc (right):

http://codereview.chromium.org/10692110/diff/2002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:343:
*content::Details<std::string>(details).ptr());
On 2012/07/09 09:30:12, Mattias Nissler wrote:
> align parameters at (

Done.

http://codereview.chromium.org/10692110/diff/2002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/chrome_shell_delegate.cc:362:
PrefService::UNSYNCABLE_PREF);
On 2012/07/09 09:30:12, Mattias Nissler wrote:
> align at (

Done.

http://codereview.chromium.org/10692110/diff/2002/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/chrome_shell_delegate.h (right):

http://codereview.chromium.org/10692110/diff/2002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/chrome_shell_delegate.h:36: // This is used for
registering both local state and user prefs.
On 2012/07/09 09:30:12, Mattias Nissler wrote:
> That comment doesn't seem very helpful as the function names tell the whole
> story already.
At the beginning there was one method that was supposed to act like those two,
and I must admit I forgot to remove the comment..
Done.

http://codereview.chromium.org/10692110/diff/2002/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/10692110/diff/2002/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:491: // Disables screenshot accelerators and
extension APIs.
On 2012/07/09 09:30:12, Mattias Nissler wrote:
> Would be good to put a sentence saying that the extension API is controlled by
> the profile pref, while the accelerator code reads local state.

Done.

[Mattias Nissler (ping if slow), 2012-07-12 08:59:31]

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:22: #include
"chrome/browser/browser_process.h"
still required?

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/screenshot_taker.cc (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/screenshot_taker.cc:121: return false;
add blank line before comment

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot.cc (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot.cc:17: #include
"ui/gfx/rect.h"
Please clean up the list of includes to contain only what is used in this file.

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot.cc:23: bool
GrabWindowSnapshotOS(gfx::NativeWindow window,
I don't really like this name. Maybe keep the existing name here and change the
other name to CheckAndGrabWindowSnapshot, or GrabWindowSnapshotIfPermitted?

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot.cc:24: std::vector<unsigned
char>* png_representation,
indentation.

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot.cc:30: #if
defined(OS_CHROMEOS)
should this really be OS_CHROMEOS only? What are the callers of this code?

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot.h (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot.h:30: void
RegisterPrefs(PrefService* service);
This should have a better name. browser::RegisterPrefs is pretty unhelpful, so I
suggest something like browser::RegisterScreenshotPrefs.

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot_aura.cc (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot_aura.cc:19:
std::vector<unsigned char>* png_representation,
indentation

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot_gtk.cc (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot_gtk.cc:28:
std::vector<unsigned char>* png_representation,
indentation

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot_mac.mm (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot_mac.mm:17:
std::vector<unsigned char>* png_representation,
indentation

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot_win.cc (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot_win.cc:40:
std::vector<unsigned char>* png_representation,
indentation

[Joao da Silva, 2012-07-12 09:10:34]

http://codereview.chromium.org/10692110/diff/11015/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:4180:
'browser/ui/window_snapshot/window_snapshot.cc',
.cc before .h

[qfel, 2012-07-12 14:48:06]

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:22: #include
"chrome/browser/browser_process.h"
On 2012/07/12 08:59:32, Mattias Nissler wrote:
> still required?

Done.

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/screenshot_taker.cc (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/screenshot_taker.cc:121: return false;
On 2012/07/12 08:59:32, Mattias Nissler wrote:
> add blank line before comment

Done.

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot.cc (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot.cc:17: #include
"ui/gfx/rect.h"
On 2012/07/12 08:59:32, Mattias Nissler wrote:
> Please clean up the list of includes to contain only what is used in this
file.

Done.

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot.cc:23: bool
GrabWindowSnapshotOS(gfx::NativeWindow window,
On 2012/07/12 08:59:32, Mattias Nissler wrote:
> I don't really like this name. Maybe keep the existing name here and change
the
> other name to CheckAndGrabWindowSnapshot, or GrabWindowSnapshotIfPermitted?

Done.

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot.cc:24: std::vector<unsigned
char>* png_representation,
On 2012/07/12 08:59:32, Mattias Nissler wrote:
> indentation.

Done.

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot.cc:30: #if
defined(OS_CHROMEOS)
On 2012/07/12 08:59:32, Mattias Nissler wrote:
> should this really be OS_CHROMEOS only? What are the callers of this code?
Now the policy fully works on every platform.
Done.

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot.h (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot.h:30: void
RegisterPrefs(PrefService* service);
On 2012/07/12 08:59:32, Mattias Nissler wrote:
> This should have a better name. browser::RegisterPrefs is pretty unhelpful, so
I
> suggest something like browser::RegisterScreenshotPrefs.

Most of the stuff called in RegisterLocalState (browser_prefs.cc) is called
RegisterPrefs, but it seems there are already few exceptions.
Done.

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot_aura.cc (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot_aura.cc:19:
std::vector<unsigned char>* png_representation,
On 2012/07/12 08:59:32, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot_gtk.cc (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot_gtk.cc:28:
std::vector<unsigned char>* png_representation,
On 2012/07/12 08:59:32, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot_mac.mm (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot_mac.mm:17:
std::vector<unsigned char>* png_representation,
On 2012/07/12 08:59:32, Mattias Nissler wrote:
> indentation
That's how it was in the original.
Changed to the usual way - done.

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot_win.cc (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot_win.cc:40:
std::vector<unsigned char>* png_representation,
On 2012/07/12 08:59:32, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/10692110/diff/11015/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10692110/diff/11015/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:4180:
'browser/ui/window_snapshot/window_snapshot.cc',
On 2012/07/12 09:10:34, Joao da Silva wrote:
> .cc before .h

Done.

[qfel, 2012-07-13 13:28:24]

Rahul: please look over feedback* files. I made screenshot part of the report
hidden by default - that way it doesn't strangely collapse on load when
screenshots are disabled.

[Mattias Nissler (ping if slow), 2012-07-13 15:23:42]

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
File chrome/browser/resources/feedback.html (right):

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
chrome/browser/resources/feedback.html:70: <div id="saved-screenshots"
class="thumbnail-list" hidden></div>
hide?

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
chrome/browser/resources/feedback.html:72: <div id="current-screenshots"
class="thumbnail-list"></div>
hide?

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
File chrome/browser/resources/feedback.js (left):

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
chrome/browser/resources/feedback.js:81: $('screenshot-checkbox').checked =
false;
Why have you removed the updates to screenshot-checkbox, current-screenshots and
saved-screenshots? It seems they would have to be updated as well.

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/ui/window_sn...
File chrome/browser/ui/window_snapshot/window_snapshot.cc (right):

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/ui/window_sn...
chrome/browser/ui/window_snapshot/window_snapshot.cc:15: // specific, so there's
only a declaration here.
This comment feels a bit chatty. Just saying "Takes a screenshot, implemented in
a platform-specific way." might be good enough.

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/ui/window_sn...
File chrome/browser/ui/window_snapshot/window_snapshot.h (right):

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/ui/window_sn...
chrome/browser/ui/window_snapshot/window_snapshot.h:25: bool
GrabWindowSnapshotIfPermitted(gfx::NativeWindow window,
parameters need to be aligned, break line after (

http://codereview.chromium.org/10692110/diff/2005/chrome/test/base/ui_test_ut...
File chrome/test/base/ui_test_utils_win.cc (right):

http://codereview.chromium.org/10692110/diff/2005/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils_win.cc:129: rect.bottom - rect.top)) &&
align parameters

[rkc, 2012-07-13 19:49:00]

Other than Mattias's comments, feedback LGTM.

On 2012/07/13 15:23:42, Mattias Nissler wrote:
>
http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
> File chrome/browser/resources/feedback.html (right):
> 
>
http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
> chrome/browser/resources/feedback.html:70: <div id="saved-screenshots"
> class="thumbnail-list" hidden></div>
> hide?
> 
>
http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
> chrome/browser/resources/feedback.html:72: <div id="current-screenshots"
> class="thumbnail-list"></div>
> hide?
> 
>
http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
> File chrome/browser/resources/feedback.js (left):
> 
>
http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
> chrome/browser/resources/feedback.js:81: $('screenshot-checkbox').checked =
> false;
> Why have you removed the updates to screenshot-checkbox, current-screenshots
and
> saved-screenshots? It seems they would have to be updated as well.
> 
>
http://codereview.chromium.org/10692110/diff/2005/chrome/browser/ui/window_sn...
> File chrome/browser/ui/window_snapshot/window_snapshot.cc (right):
> 
>
http://codereview.chromium.org/10692110/diff/2005/chrome/browser/ui/window_sn...
> chrome/browser/ui/window_snapshot/window_snapshot.cc:15: // specific, so
there's
> only a declaration here.
> This comment feels a bit chatty. Just saying "Takes a screenshot, implemented
in
> a platform-specific way." might be good enough.
> 
>
http://codereview.chromium.org/10692110/diff/2005/chrome/browser/ui/window_sn...
> File chrome/browser/ui/window_snapshot/window_snapshot.h (right):
> 
>
http://codereview.chromium.org/10692110/diff/2005/chrome/browser/ui/window_sn...
> chrome/browser/ui/window_snapshot/window_snapshot.h:25: bool
> GrabWindowSnapshotIfPermitted(gfx::NativeWindow window,
> parameters need to be aligned, break line after (
> 
>
http://codereview.chromium.org/10692110/diff/2005/chrome/test/base/ui_test_ut...
> File chrome/test/base/ui_test_utils_win.cc (right):
> 
>
http://codereview.chromium.org/10692110/diff/2005/chrome/test/base/ui_test_ut...
> chrome/test/base/ui_test_utils_win.cc:129: rect.bottom - rect.top)) &&
> align parameters

[qfel, 2012-07-16 09:11:12]

Hopefully I got the formatting right..
I think there was no need to modify additional hidden flags in
feedback.(html|js) - see comments.

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
File chrome/browser/resources/feedback.html (right):

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
chrome/browser/resources/feedback.html:70: <div id="saved-screenshots"
class="thumbnail-list" hidden></div>
On 2012/07/13 15:23:42, Mattias Nissler wrote:
> hide?
It should stay hidden even if you display screenshot-row (clicking
screenshot-link-tosaved makes it visible)

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
chrome/browser/resources/feedback.html:72: <div id="current-screenshots"
class="thumbnail-list"></div>
On 2012/07/13 15:23:42, Mattias Nissler wrote:
> hide?
That's inside screenshot-row, which is initially hidden

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
File chrome/browser/resources/feedback.js (left):

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/resources/fe...
chrome/browser/resources/feedback.js:81: $('screenshot-checkbox').checked =
false;
On 2012/07/13 15:23:42, Mattias Nissler wrote:
> Why have you removed the updates to screenshot-checkbox, current-screenshots
and
> saved-screenshots? It seems they would have to be updated as well.

saved-screenshots and current-screenshots are inside screenshot-row, so that's
redundant. screenshot-checkbox was unchecked by default and so is now.

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/ui/window_sn...
File chrome/browser/ui/window_snapshot/window_snapshot.cc (right):

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/ui/window_sn...
chrome/browser/ui/window_snapshot/window_snapshot.cc:15: // specific, so there's
only a declaration here.
On 2012/07/13 15:23:42, Mattias Nissler wrote:
> This comment feels a bit chatty. Just saying "Takes a screenshot, implemented
in
> a platform-specific way." might be good enough.

Done.

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/ui/window_sn...
File chrome/browser/ui/window_snapshot/window_snapshot.h (right):

http://codereview.chromium.org/10692110/diff/2005/chrome/browser/ui/window_sn...
chrome/browser/ui/window_snapshot/window_snapshot.h:25: bool
GrabWindowSnapshotIfPermitted(gfx::NativeWindow window,
On 2012/07/13 15:23:42, Mattias Nissler wrote:
> parameters need to be aligned, break line after (

Done.

http://codereview.chromium.org/10692110/diff/2005/chrome/test/base/ui_test_ut...
File chrome/test/base/ui_test_utils_win.cc (right):

http://codereview.chromium.org/10692110/diff/2005/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils_win.cc:129: rect.bottom - rect.top)) &&
On 2012/07/13 15:23:42, Mattias Nissler wrote:
> align parameters
Ah! It's line no. 34's fault.
Done.

[Mattias Nissler (ping if slow), 2012-07-16 10:47:38]

LGTM, now you'll have to get full OWNERS coverage, in particular for ui and
extensions

https://chromiumcodereview.appspot.com/10692110/diff/16001/chrome/test/base/u...
File chrome/test/base/ui_test_utils_win.cc (right):

https://chromiumcodereview.appspot.com/10692110/diff/16001/chrome/test/base/u...
chrome/test/base/ui_test_utils_win.cc:127: if
(browser::GrabWindowSnapshotIfPermitted(NULL, &png_data,
nit: I think this would become a little more readable if you were to break the
line before between ( and NULL.

[qfel, 2012-07-16 11:56:01]

Kindly asking owners to review:

skerner - chrome/browser/extensions (CaptureVisibleTabFunction altered to check
if capturing screenshots is enabled)

estade - chrome/browser/resources, chrome/browser/ui/webui (altered feedback
form not to display screenshot if not permitted)

sky - chrome/browser/ui/window_snapshot,
chrome/browser/ui/views/ash/screenshot_taker.cc (mostly renames, made
GrabWindowSnapshot internal, use GrabWindowSnapshotIfPermitted instead)


Small things:

nirnimesh - boilerplate in chrome/test/functional/policy_test_cases.py

zmo - Renames in 	 chrome/test/gpu/gpu_pixel_browsertest.cc
jbates - Renames in chrome/test/base and chrome/test/perf

[sky, 2012-07-16 14:01:53]

http://codereview.chromium.org/10692110/diff/16001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot.h (right):

http://codereview.chromium.org/10692110/diff/16001/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot.h:25: bool
GrabWindowSnapshotIfPermitted(
Lead this named GrabWindowSnapshot and name the implementation something like
GrabWindowSnapshotImpl

[Sam Kerner (Chrome), 2012-07-16 14:50:02]

Added aa to review the change to the API of chrome.tabs.captureVisibleTab().
I have been away from this code too long to meaningfully review it.

[Evan Stade, 2012-07-16 18:39:45]

http://codereview.chromium.org/10692110/diff/16001/chrome/browser/ui/webui/fe...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/10692110/diff/16001/chrome/browser/ui/webui/fe...
chrome/browser/ui/webui/feedback_ui.cc:445: ListValue dialog_defaults;
this would be a lot better as a dictionary.

[Nirnimesh, 2012-07-16 18:43:13]

chrome/test/functional LGTM

[qfel, 2012-07-17 13:33:43]

http://codereview.chromium.org/10692110/diff/16001/chrome/browser/ui/webui/fe...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/10692110/diff/16001/chrome/browser/ui/webui/fe...
chrome/browser/ui/webui/feedback_ui.cc:445: ListValue dialog_defaults;
On 2012/07/16 18:39:50, Evan Stade wrote:
> this would be a lot better as a dictionary.

Done.

http://codereview.chromium.org/10692110/diff/16001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot.h (right):

http://codereview.chromium.org/10692110/diff/16001/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot.h:25: bool
GrabWindowSnapshotIfPermitted(
On 2012/07/16 14:01:53, sky wrote:
> Lead this named GrabWindowSnapshot and name the implementation something like
> GrabWindowSnapshotImpl

Done.

http://codereview.chromium.org/10692110/diff/16001/chrome/test/base/ui_test_u...
File chrome/test/base/ui_test_utils_win.cc (right):

http://codereview.chromium.org/10692110/diff/16001/chrome/test/base/ui_test_u...
chrome/test/base/ui_test_utils_win.cc:127: if
(browser::GrabWindowSnapshotIfPermitted(NULL, &png_data,
On 2012/07/16 10:47:38, Mattias Nissler wrote:
> nit: I think this would become a little more readable if you were to break the
> line before between ( and NULL.

No longer applicable.

[Evan Stade, 2012-07-17 18:27:30]

webui and resources LGTM with nits

http://codereview.chromium.org/10692110/diff/22006/chrome/browser/resources/f...
File chrome/browser/resources/feedback.js (right):

http://codereview.chromium.org/10692110/diff/22006/chrome/browser/resources/f...
chrome/browser/resources/feedback.js:282: $('page-url-text').value =
defaults['current_url'];
nit: defaults.name rather than defaults['name']

http://codereview.chromium.org/10692110/diff/22006/chrome/browser/ui/webui/fe...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/10692110/diff/22006/chrome/browser/ui/webui/fe...
chrome/browser/ui/webui/feedback_ui.cc:451: "disable_screenshots",
nit: camel case

[sky, 2012-07-17 22:05:57]

http://codereview.chromium.org/10692110/diff/22006/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/screenshot_taker.cc (right):

http://codereview.chromium.org/10692110/diff/22006/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/screenshot_taker.cc:121: return false;
This is going to result in a LOG(ERROR) on line 180ish, which I don't think
makes sense here. Maybe you should put this check in HandleTakePartialScreenshot
so that we early out there.

[qfel, 2012-07-18 09:24:26]

http://codereview.chromium.org/10692110/diff/22006/chrome/browser/resources/f...
File chrome/browser/resources/feedback.js (right):

http://codereview.chromium.org/10692110/diff/22006/chrome/browser/resources/f...
chrome/browser/resources/feedback.js:282: $('page-url-text').value =
defaults['current_url'];
On 2012/07/17 18:27:30, Evan Stade wrote:
> nit: defaults.name rather than defaults['name']

Done.

http://codereview.chromium.org/10692110/diff/22006/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/screenshot_taker.cc (right):

http://codereview.chromium.org/10692110/diff/22006/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/screenshot_taker.cc:121: return false;
On 2012/07/17 22:05:57, sky wrote:
> This is going to result in a LOG(ERROR) on line 180ish, which I don't think
> makes sense here. Maybe you should put this check in
HandleTakePartialScreenshot
> so that we early out there.

In any case, I think we should leave GrabWindowSnapshot as is (safer, policy
checked in one place), and eventually do redundant checks elsewhere
(HandleTakePartialScreenshot) for nicer log output.
Added check in HandleTakePartialScreenshot.

http://codereview.chromium.org/10692110/diff/22006/chrome/browser/ui/webui/fe...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/10692110/diff/22006/chrome/browser/ui/webui/fe...
chrome/browser/ui/webui/feedback_ui.cc:451: "disable_screenshots",
On 2012/07/17 18:27:30, Evan Stade wrote:
> nit: camel case

Done.

[sky, 2012-07-18 15:17:27]

LGTM

[qfel, 2012-07-19 10:17:40]

On 2012/07/16 14:50:02, Sam Kerner (Chrome) wrote:
> Added aa to review the change to the API of chrome.tabs.captureVisibleTab().
> I have been away from this code too long to meaningfully review it.

Since Aaron seems to be away, you sure about it? The modifications I made were
rather small, and the majority of the touched function's code was last modified
by you.

[skerner, 2012-07-19 15:30:37]

On Thu, Jul 19, 2012 at 6:17 AM,  <qfel@google.com> wrote:
> On 2012/07/16 14:50:02, Sam Kerner (Chrome) wrote:
>>
>> Added aa to review the change to the API of
>> chrome.tabs.captureVisibleTab().
>> I have been away from this code too long to meaningfully review it.
>
>
> Since Aaron seems to be away, you sure about it?

I am sure.

> The modifications I made
> were
> rather small,
> and the majority of the touched function's code was last
> modified
> by you.

I have not been on the extensions team for over a year.  The fact that
I fixed a few bugs doesn't qualify me to approve a change that will
change a published extension API's behavior.  At least one person on
the extensions team should be aware of it.

I will send a request to chrome-extensions-team asking that someone take a
look.

>
> http://codereview.chromium.org/10692110/

[Matt Perry, 2012-07-19 23:05:49]

This seems OK to me. LGTM

[commit-bot: I haz the power, 2012-07-30 15:09:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qfel@google.com/10692110/41003

[commit-bot: I haz the power, 2012-07-30 17:09:35]

Change committed as 148963

[grt (UTC plus 2), 2012-07-31 09:54:57]

On 2012/07/30 17:09:35, I haz the power (commit-bot) wrote:
> Change committed as 148963

This change has introduced a regression in the Chrome Frame tests.  Please see
http://crbug.com/139694#c11 and fix accordingly.  Thanks.