Refactor screenshot directory source

Limit number of code paths for accessing screenshot sources to avoid potential future pitfalls.


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

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

[Harry McCleave, 2012-09-05 23:19:56]

Please take a look when you get a chance.

[Jun Mukai, 2012-09-06 09:30:06]

thanks for doing this.

Can you also take a look at chrome/browser/ui/webui/feedback_ui.cc ?  It also
has some functions, and it's good to put all of the file-related functions into
the same place; or at least moving some constants may work.

http://codereview.chromium.org/10908081/diff/3004/chrome/browser/ui/webui/scr...
File chrome/browser/ui/webui/screenshot_source.h (right):

http://codereview.chromium.org/10908081/diff/3004/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.h:20: #include
"chrome/browser/chromeos/login/user_manager.h"
These files seem not referred in the header file.  Could you move them to .cc?

[Harry McCleave, 2012-09-07 19:47:51]

On 2012/09/06 09:30:06, Jun Mukai wrote:
> thanks for doing this.
> 
> Can you also take a look at chrome/browser/ui/webui/feedback_ui.cc ?  It also
> has some functions, and it's good to put all of the file-related functions
into
> the same place; or at least moving some constants may work.
> 
>
http://codereview.chromium.org/10908081/diff/3004/chrome/browser/ui/webui/scr...
> File chrome/browser/ui/webui/screenshot_source.h (right):
> 
>
http://codereview.chromium.org/10908081/diff/3004/chrome/browser/ui/webui/scr...
> chrome/browser/ui/webui/screenshot_source.h:20: #include
> "chrome/browser/chromeos/login/user_manager.h"
> These files seem not referred in the header file.  Could you move them to .cc?

I think this change addresses those requests, unfortunately the move of the
functions from an ash only file requires some awkward looking #ifdef's.

[Jun Mukai, 2012-09-10 23:48:31]

nitpicking comments again...

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

http://codereview.chromium.org/10908081/diff/13001/chrome/browser/ui/webui/fe...
chrome/browser/ui/webui/feedback_ui.cc:83: const char* kCustomPageUrlParameter =
"customPageUrl=";
IIRC we prefer const char[] instead of char* for global string variables,
although I couldn't find it in our coding style guide explicitly.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...
just says "use const char[]" though.

FYI:
% git grep '^const char k[a-zA-Z]*\[\] =' | wc -l
5516
% git grep '^const char\* k[a-zA-Z]* =' | wc -l
178

http://codereview.chromium.org/10908081/diff/13001/chrome/browser/ui/webui/sc...
File chrome/browser/ui/webui/screenshot_source.cc (right):

http://codereview.chromium.org/10908081/diff/13001/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.cc:8: #include "ash/shell_delegate.h"
is it safe to exclude them from OS_CHROMEOS?  should we enclose them by USE_ASH
instead?

[Harry McCleave, 2012-09-11 01:21:00]

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

http://codereview.chromium.org/10908081/diff/13001/chrome/browser/ui/webui/fe...
chrome/browser/ui/webui/feedback_ui.cc:83: const char* kCustomPageUrlParameter =
"customPageUrl=";
On 2012/09/10 23:48:31, Jun Mukai wrote:
> IIRC we prefer const char[] instead of char* for global string variables,
> although I couldn't find it in our coding style guide explicitly.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...
> just says "use const char[]" though.
> 
> FYI:
> % git grep '^const char k[a-zA-Z]*\[\] =' | wc -l
> 5516
> % git grep '^const char\* k[a-zA-Z]* =' | wc -l
> 178

Done.

http://codereview.chromium.org/10908081/diff/13001/chrome/browser/ui/webui/sc...
File chrome/browser/ui/webui/screenshot_source.cc (right):

http://codereview.chromium.org/10908081/diff/13001/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.cc:8: #include "ash/shell_delegate.h"
On 2012/09/10 23:48:31, Jun Mukai wrote:
> is it safe to exclude them from OS_CHROMEOS?  should we enclose them by
USE_ASH
> instead?

Done.

[Jun Mukai, 2012-09-11 01:56:18]

lgtm with a nit

please get the approval from the real owner.

http://codereview.chromium.org/10908081/diff/6004/chrome/browser/ui/webui/scr...
File chrome/browser/ui/webui/screenshot_source.cc (right):

http://codereview.chromium.org/10908081/diff/6004/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.cc:10: #endif
nit: move #if defined... includes after the normal includes.

[Harry McCleave, 2012-09-12 02:18:34]

http://codereview.chromium.org/10908081/diff/6004/chrome/browser/ui/webui/scr...
File chrome/browser/ui/webui/screenshot_source.cc (right):

http://codereview.chromium.org/10908081/diff/6004/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.cc:10: #endif
On 2012/09/11 01:56:18, Jun Mukai wrote:
> nit: move #if defined... includes after the normal includes.

Done.

[sky, 2012-09-12 04:32:01]

http://codereview.chromium.org/10908081/diff/14005/chrome/browser/ui/ash/scre...
File chrome/browser/ui/ash/screenshot_taker.cc (right):

http://codereview.chromium.org/10908081/diff/14005/chrome/browser/ui/ash/scre...
chrome/browser/ui/ash/screenshot_taker.cc:130:
ScreenshotSource::GetScreenshotBaseFilename();
nit: indent this 4 spaces from previous line.

http://codereview.chromium.org/10908081/diff/14005/chrome/browser/ui/webui/sc...
File chrome/browser/ui/webui/screenshot_source.cc (right):

http://codereview.chromium.org/10908081/diff/14005/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.cc:43: const char
ScreenshotSource::kScreenshotUrlRoot[] = "chrome://screenshots/";
For statics like this we typically put:
// static
on the previous line.

http://codereview.chromium.org/10908081/diff/14005/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.cc:55: PrefService* pref_service =
profile->GetPrefs();
Do you really need the NULL check here?

http://codereview.chromium.org/10908081/diff/14005/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.cc:56: if (pref_service) {
remove {}

http://codereview.chromium.org/10908081/diff/14005/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.cc:78: std::string
ScreenshotSource::GetScreenshotBaseFilename() {
static methods also get:
// static
on the previous line

http://codereview.chromium.org/10908081/diff/14005/chrome/browser/ui/webui/sc...
File chrome/browser/ui/webui/screenshot_source.h (right):

http://codereview.chromium.org/10908081/diff/14005/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.h:35: #if defined(USE_ASH)
Why is this ash specific?

http://codereview.chromium.org/10908081/diff/14005/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.h:38: // Common access for the
screenshot directory, parameter is set to the
newline between 37/38

http://codereview.chromium.org/10908081/diff/14005/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.h:42: // Get the basefilename for
screenshots
newline between 41/42

http://codereview.chromium.org/10908081/diff/14005/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.h:58: static const char
kScreenshotUrlRoot[];
Document what all these are.

http://codereview.chromium.org/10908081/diff/14005/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.h:66: 
nit: remove one of these newlines.

[Harry McCleave, 2012-09-13 03:05:39]

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
File chrome/browser/ui/ash/screenshot_taker.cc (right):

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
chrome/browser/ui/ash/screenshot_taker.cc:130:
ScreenshotSource::GetScreenshotBaseFilename();
On 2012/09/12 04:32:01, sky wrote:
> nit: indent this 4 spaces from previous line.

Done.

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
File chrome/browser/ui/webui/screenshot_source.cc (right):

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
chrome/browser/ui/webui/screenshot_source.cc:43: const char
ScreenshotSource::kScreenshotUrlRoot[] = "chrome://screenshots/";
On 2012/09/12 04:32:01, sky wrote:
> For statics like this we typically put:
> // static
> on the previous line.

Done.

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
chrome/browser/ui/webui/screenshot_source.cc:55: PrefService* pref_service =
profile->GetPrefs();
On 2012/09/12 04:32:01, sky wrote:
> Do you really need the NULL check here?

I am not sure, the profile interface does not specify that the return must not
be null, though the only implementation of it does return a non-null value for
all code paths, the safe/conservative side of me says to make no assumptions
based on the implementation, what is the general practice for chromium on such
things?

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
chrome/browser/ui/webui/screenshot_source.cc:56: if (pref_service) {
On 2012/09/12 04:32:01, sky wrote:
> remove {}

Done.

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
chrome/browser/ui/webui/screenshot_source.cc:78: std::string
ScreenshotSource::GetScreenshotBaseFilename() {
On 2012/09/12 04:32:01, sky wrote:
> static methods also get:
> // static
> on the previous line

Done.

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
File chrome/browser/ui/webui/screenshot_source.h (right):

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
chrome/browser/ui/webui/screenshot_source.h:35: #if defined(USE_ASH)
On 2012/09/12 04:32:01, sky wrote:
> Why is this ash specific?
It has been moved from an ash specific file, and is only used in ash specific
code paths, I assumed no point in bringing it out of here to just be clutter for
the linker to ignore?

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
chrome/browser/ui/webui/screenshot_source.h:38: // Common access for the
screenshot directory, parameter is set to the
On 2012/09/12 04:32:01, sky wrote:
> newline between 37/38

Done.

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
chrome/browser/ui/webui/screenshot_source.h:42: // Get the basefilename for
screenshots
On 2012/09/12 04:32:01, sky wrote:
> newline between 41/42

Done.

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
chrome/browser/ui/webui/screenshot_source.h:58: static const char
kScreenshotUrlRoot[];
On 2012/09/12 04:32:01, sky wrote:
> Document what all these are.

Done.

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
chrome/browser/ui/webui/screenshot_source.h:66: 
On 2012/09/12 04:32:01, sky wrote:
> nit: remove one of these newlines.

Done.

[csilv, 2012-09-13 05:32:17]

lgtm

https://chromiumcodereview.appspot.com/10908081/diff/16001/chrome/browser/ui/...
File chrome/browser/ui/ash/screenshot_taker.cc (right):

https://chromiumcodereview.appspot.com/10908081/diff/16001/chrome/browser/ui/...
chrome/browser/ui/ash/screenshot_taker.cc:32: #include
"chrome/browser/chromeos/gdata/gdata_util.h"
nit: swap order of lines 31 & 32 for alphabetical order.

https://chromiumcodereview.appspot.com/10908081/diff/16001/chrome/browser/ui/...
File chrome/browser/ui/webui/screenshot_source.h (right):

https://chromiumcodereview.appspot.com/10908081/diff/16001/chrome/browser/ui/...
chrome/browser/ui/webui/screenshot_source.h:20: #include
"chrome/browser/chromeos/login/user_manager.h"
nit: alphabetical order lines 17-20

[sky, 2012-09-13 15:57:30]

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
File chrome/browser/ui/webui/screenshot_source.cc (right):

https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
chrome/browser/ui/webui/screenshot_source.cc:55: PrefService* pref_service =
profile->GetPrefs();
On 2012/09/13 03:05:39, Harry McCleave wrote:
> On 2012/09/12 04:32:01, sky wrote:
> > Do you really need the NULL check here?
> 
> I am not sure, the profile interface does not specify that the return must not
> be null, though the only implementation of it does return a non-null value for
> all code paths, the safe/conservative side of me says to make no assumptions
> based on the implementation, what is the general practice for chromium on such
> things?

Only have null checks if it can be null. Look at other code. I believe you
shouldn't have the null check here.

[Harry McCleave, 2012-09-13 20:37:42]

On 2012/09/13 15:57:30, sky wrote:
>
https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
> File chrome/browser/ui/webui/screenshot_source.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10908081/diff/14005/chrome/browser/ui/...
> chrome/browser/ui/webui/screenshot_source.cc:55: PrefService* pref_service =
> profile->GetPrefs();
> On 2012/09/13 03:05:39, Harry McCleave wrote:
> > On 2012/09/12 04:32:01, sky wrote:
> > > Do you really need the NULL check here?
> > 
> > I am not sure, the profile interface does not specify that the return must
not
> > be null, though the only implementation of it does return a non-null value
for
> > all code paths, the safe/conservative side of me says to make no assumptions
> > based on the implementation, what is the general practice for chromium on
such
> > things?
> 
> Only have null checks if it can be null. Look at other code. I believe you
> shouldn't have the null check here.

Done, (and include orderings corrected).

[sky, 2012-09-13 21:17:19]

LGTM

[commit-bot: I haz the power, 2012-09-13 21:18:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/10908081/3010

[commit-bot: I haz the power, 2012-09-13 23:01:57]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-09-17 17:36:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/10908081/3012

[commit-bot: I haz the power, 2012-09-17 20:01:48]

Change committed as 157174

[commit-bot: I haz the power, 2012-09-17 22:14:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/10908081/11006

[commit-bot: I haz the power, 2012-09-18 02:25:52]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-09-18 03:28:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/10908081/11006

[commit-bot: I haz the power, 2012-09-18 05:36:09]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-09-25 19:17:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/10908081/11006

[commit-bot: I haz the power, 2012-09-25 19:17:09]

Failed to apply patch for chrome/browser/ui/ash/screenshot_taker.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/ui/ash/screenshot_taker.cc
Hunk #1 FAILED at 13.
Hunk #2 succeeded at 47 with fuzz 2 (offset 1 line).
Hunk #3 succeeded at 117 (offset 15 lines).
Hunk #4 succeeded at 140 (offset 15 lines).
Hunk #5 succeeded at 169 (offset 15 lines).
Hunk #6 succeeded at 178 (offset 15 lines).
1 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/ui/ash/screenshot_taker.cc.rej

[commit-bot: I haz the power, 2012-09-25 22:09:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/10908081/36001

[commit-bot: I haz the power, 2012-09-25 23:56:17]

Change committed as 158695