Print Preview: Add "print preview" back to about:flags; enable it by default just once.

Print Preview: Add "print preview" back to about:flags; enable it by default just once.

BUG=none
TEST=none

[vandebo (ex-Chrome), 2011-08-16 21:57:47]

http://codereview.chromium.org/7631008/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7631008/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:1429: #if defined(GOOGLE_CHROME_BUILD) &&
!defined(OS_MACOSX)
Not for CrOS as well?

http://codereview.chromium.org/7631008/diff/1/chrome/common/chrome_switches.h
File chrome/common/chrome_switches.h (left):

http://codereview.chromium.org/7631008/diff/1/chrome/common/chrome_switches.h...
chrome/common/chrome_switches.h:351: extern const char kDisablePrintPreview[];
Can we have both args - for those that have already disabled PP for their
atypical flow?

http://codereview.chromium.org/7631008/diff/1/content/browser/renderer_host/b...
File content/browser/renderer_host/browser_render_process_host.cc (left):

http://codereview.chromium.org/7631008/diff/1/content/browser/renderer_host/b...
content/browser/renderer_host/browser_render_process_host.cc:550: #if
defined(GOOGLE_CHROME_BUILD) && !defined(OS_CHROMEOS) && !defined(OS_MACOSX)
Do we still need this for CrOS?

[Lei Zhang, 2011-08-16 22:17:04]

http://codereview.chromium.org/7631008/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7631008/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:1429: #if defined(GOOGLE_CHROME_BUILD) &&
!defined(OS_MACOSX)
On 2011/08/16 21:57:48, vandebo wrote:
> Not for CrOS as well?

Done.

http://codereview.chromium.org/7631008/diff/1/chrome/common/chrome_switches.h
File chrome/common/chrome_switches.h (left):

http://codereview.chromium.org/7631008/diff/1/chrome/common/chrome_switches.h...
chrome/common/chrome_switches.h:351: extern const char kDisablePrintPreview[];
On 2011/08/16 21:57:48, vandebo wrote:
> Can we have both args - for those that have already disabled PP for their
> atypical flow?

Done.

http://codereview.chromium.org/7631008/diff/1/content/browser/renderer_host/b...
File content/browser/renderer_host/browser_render_process_host.cc (left):

http://codereview.chromium.org/7631008/diff/1/content/browser/renderer_host/b...
content/browser/renderer_host/browser_render_process_host.cc:550: #if
defined(GOOGLE_CHROME_BUILD) && !defined(OS_CHROMEOS) && !defined(OS_MACOSX)
On 2011/08/16 21:57:48, vandebo wrote:
> Do we still need this for CrOS?

CrOS has no way for the user to set command line flags. They can only set flags
through about:flags. However, the print preview flag in M13 is Win/Mac/Linux
only.

[vandebo (ex-Chrome), 2011-08-16 22:38:39]

http://codereview.chromium.org/7631008/diff/7001/chrome/common/chrome_switches.h
File chrome/common/chrome_switches.h (left):

http://codereview.chromium.org/7631008/diff/7001/chrome/common/chrome_switche...
chrome/common/chrome_switches.h:351: extern const char kDisablePrintPreview[];
Do we need this for disable to work?

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

http://codereview.chromium.org/7631008/diff/7001/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:533: // Enable print preview once for supported
platforms.
nit: this comment doesn't match the code.

http://codereview.chromium.org/7631008/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/browser_render_process_host.cc (left):

http://codereview.chromium.org/7631008/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/browser_render_process_host.cc:552:
switches::kDisablePrintPreview,
Do we want both flags here?

[Lei Zhang, 2011-08-16 23:36:30]

http://codereview.chromium.org/7631008/diff/7001/chrome/common/chrome_switches.h
File chrome/common/chrome_switches.h (left):

http://codereview.chromium.org/7631008/diff/7001/chrome/common/chrome_switche...
chrome/common/chrome_switches.h:351: extern const char kDisablePrintPreview[];
On 2011/08/16 22:38:39, vandebo wrote:
> Do we need this for disable to work?

We do.

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

http://codereview.chromium.org/7631008/diff/7001/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:533: // Enable print preview once for supported
platforms.
On 2011/08/16 22:38:39, vandebo wrote:
> nit: this comment doesn't match the code.

Renamed to enable_print_preview_once. How's that?

http://codereview.chromium.org/7631008/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/browser_render_process_host.cc (left):

http://codereview.chromium.org/7631008/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/browser_render_process_host.cc:552:
switches::kDisablePrintPreview,
On 2011/08/16 22:38:39, vandebo wrote:
> Do we want both flags here?

Why, yes.

[vandebo (ex-Chrome), 2011-08-16 23:57:48]

Do the following files need to be updated as well?
test/data/webui/print_preview.js
browser/chrome_content_browser_client.cc

http://codereview.chromium.org/7631008/diff/4003/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7631008/diff/4003/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1431: false,
nit: given the name, you probably want to reverse the meaning, i.e. default to
true and reset to false.

[Lei Zhang, 2011-08-17 00:23:41]

On 2011/08/16 23:57:48, vandebo wrote:
> Do the following files need to be updated as well?
> test/data/webui/print_preview.js
> browser/chrome_content_browser_client.cc
> 
>
http://codereview.chromium.org/7631008/diff/4003/chrome/browser/browser_main.cc
> File chrome/browser/browser_main.cc (right):
> 
>
http://codereview.chromium.org/7631008/diff/4003/chrome/browser/browser_main....
> chrome/browser/browser_main.cc:1431: false,
> nit: given the name, you probably want to reverse the meaning, i.e. default to
> true and reset to false.

I renamed the pref so true means we have enabled it, to be consistent with other
prefs that do the same thing.

[Lei Zhang, 2011-08-17 00:33:09]

On 2011/08/16 23:57:48, vandebo wrote:
> Do the following files need to be updated as well?
> test/data/webui/print_preview.js
> browser/chrome_content_browser_client.cc

They don't exist in M13.

[vandebo (ex-Chrome), 2011-08-17 03:27:49]

MIOn Aug 16, 2011 5:33 PM, <thestig@chromium.org> wrote:
>
> On 2011/08/16 23:57:48, vandebo wrote:
>>
>> Do the following files need to be updated as well?
>> test/data/webui/print_preview.js
>> browser/chrome_content_browser_client.cc
>
>
> They don't exist in M13.
>

Then lgtm

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

[Lei Zhang, 2011-08-22 18:21:59]

Closing this issue since the merge to M13 has been denied.