Displaying "simplify page" on print preview by default

Displaying "simplify page" check box on print preview by default.

And also removing the "simplifiable page" detection heuristics from consideration when displaying the "simplify page" check box on print preview.

Also adding a cmdline and UI switch to disable it (--disable-print-preview-simplify and accompanying UI switch at chrome://flags)

R=thakis@chromium.org, nyquist@chromium.org, vitalybuka@chromium.org, thestig@chromium.org, mdjones@chromium.org, bengr@chromium.org,
isherman@chromium.org, mpearson@chromium.org
BUG=490809
Committed: https://crrev.com/95abe757ca491cf2521e8e30eb56104519d506c4
Cr-Commit-Position: refs/heads/master@{#357661}

[mvendramini_hp, 2015-10-23 12:38:33]

Description was changed from

==========
Adding a UI switch for enable-dom-distiller

On chrome://flags, this commit adds a UI selector for
the --enable-dom-distiller command line option

R=thakis@chromium.org, nyquist@chromium.org, vitalybuka@chromium.org
BUG=
==========

to

==========
Adding a UI switch for enable-dom-distiller

On chrome://flags, this commit adds a UI selector for
the --enable-dom-distiller command line option

R=thakis@chromium.org, nyquist@chromium.org, vitalybuka@chromium.org,
thestig@chromium.org 
BUG=
==========

[mvendramini_hp, 2015-10-23 12:38:33]

mvendramini@hp.com changed reviewers:
+ thestig@chromium.org

[Vitaly Buka (NO REVIEWS), 2015-10-23 20:05:38]

My assumption is that we are going to enable feature on Print Preview by
default. And flag is needed to be able to shutdown if needed

Please attach the BUG.

https://codereview.chromium.org/1419063002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1419063002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5433: +      <message
name="IDS_FLAGS_ENABLE_DOM_DISTILLER_NAME" desc="Name of the 'enable dom
distiller' lab">
this is flag about print preview only, so it should be called appropriatly
IDS_FLAGS_DISABLE_DISTILLER_IN_PRINT_PREVIEW

https://codereview.chromium.org/1419063002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5434: +        Enable simplify page on print
preview
We need flag to disable feature, as it's going to be enabled by default

https://codereview.chromium.org/1419063002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1419063002/diff/1/chrome/browser/about_flags....
chrome/browser/about_flags.cc:650:
ENABLE_DISABLE_VALUE_TYPE(switches::kEnableDomDistiller,
please take a look at "disable-touch-adjustment" for example

https://codereview.chromium.org/1419063002/diff/1/components/dom_distiller/co...
File components/dom_distiller/core/dom_distiller_switches.h (right):

https://codereview.chromium.org/1419063002/diff/1/components/dom_distiller/co...
components/dom_distiller/core/dom_distiller_switches.h:17: extern const char
kDisableDomDistiller[];
enable/disable distiller also all item into chrome menu and enables reader mode.
I guess goal to to change print preview and not that behavior.

So this switch should be next to other print preview switches

[Lei Zhang, 2015-10-23 21:14:11]

On 2015/10/23 20:05:38, Vitaly Buka wrote:
> My assumption is that we are going to enable feature on Print Preview by
> default. And flag is needed to be able to shutdown if needed

Is this feature actually ready to be on by default? Why is there no filled in
BUG= for this?

[Vitaly Buka (NO REVIEWS), 2015-10-23 22:02:54]

On 2015/10/23 21:14:11, Lei Zhang wrote:
> On 2015/10/23 20:05:38, Vitaly Buka wrote:
> > My assumption is that we are going to enable feature on Print Preview by
> > default. And flag is needed to be able to shutdown if needed
> 
> Is this feature actually ready to be on by default? Why is there no filled in
> BUG= for this?

You can try with current canary with "--enable-dom-distiller
--reader-mode-heuristics=alwaystrue"

I see only this: https://codereview.chromium.org/1294663003/

Yes, according my knowledge. Feature is on by default means just new check-box
in print preview UI.
User will need to turn this check box explicitly to apply this processing. So
there
should be no regression in existing functionality.

[Vitaly Buka (NO REVIEWS), 2015-10-23 22:03:06]

Description was changed from

==========
Adding a UI switch for enable-dom-distiller

On chrome://flags, this commit adds a UI selector for
the --enable-dom-distiller command line option

R=thakis@chromium.org, nyquist@chromium.org, vitalybuka@chromium.org,
thestig@chromium.org 
BUG=
==========

to

==========
Adding a UI switch for enable-dom-distiller

On chrome://flags, this commit adds a UI selector for
the --enable-dom-distiller command line option

R=thakis@chromium.org, nyquist@chromium.org, vitalybuka@chromium.org,
thestig@chromium.org 
BUG=490809
==========

[Vitaly Buka (NO REVIEWS), 2015-10-23 22:03:22]

I added bug

[Lei Zhang, 2015-10-23 22:06:25]

On 2015/10/23 22:02:54, Vitaly Buka wrote:
> On 2015/10/23 21:14:11, Lei Zhang wrote:
> > On 2015/10/23 20:05:38, Vitaly Buka wrote:
> > > My assumption is that we are going to enable feature on Print Preview by
> > > default. And flag is needed to be able to shutdown if needed
> > 
> > Is this feature actually ready to be on by default? Why is there no filled
in
> > BUG= for this?
> 
> You can try with current canary with "--enable-dom-distiller
> --reader-mode-heuristics=alwaystrue"
> 
> I see only this: https://codereview.chromium.org/1294663003/

Ye, I'll try to get back to that, but that's mostly just cleanup.

> Yes, according my knowledge. Feature is on by default means just new check-box
> in print preview UI.
> User will need to turn this check box explicitly to apply this processing. So
> there
> should be no regression in existing functionality.

Sure, but don't new user facing features usually go through some launch review
process internally?

[Vitaly Buka (NO REVIEWS), 2015-10-24 05:48:46]

Thanks Lei. I'll ask Andrew, I guess we have enough time for this.
Cl needs to be update in any case if it passes launch review.

[mvendramini_hp, 2015-10-26 15:51:38]

Some comments, for clarification purposes.

So there should be a new flag plus accompanying UI switch just for the Simplify
page check box on the print preview dialog?
And it should be unrelated to --enable-dom-distiller?

Please verify this assertion and comment, thanks.

https://codereview.chromium.org/1419063002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1419063002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5433: +      <message
name="IDS_FLAGS_ENABLE_DOM_DISTILLER_NAME" desc="Name of the 'enable dom
distiller' lab">
On 2015/10/23 20:05:37, Vitaly Buka wrote:
> this is flag about print preview only, so it should be called appropriatly
> IDS_FLAGS_DISABLE_DISTILLER_IN_PRINT_PREVIEW
> 

Please note that the --enable-dom-distiller flag also activates/deactivates the
Distill Page option at the main menu ("Customize and control Chromium", defaults
to the top-right of the browser). Are you sure you want this branded as print
preview only?

https://codereview.chromium.org/1419063002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5434: +        Enable simplify page on print
preview
On 2015/10/23 20:05:37, Vitaly Buka wrote:
> We need flag to disable feature, as it's going to be enabled by default

Acknowledged.

https://codereview.chromium.org/1419063002/diff/1/components/dom_distiller/co...
File components/dom_distiller/core/dom_distiller_switches.h (right):

https://codereview.chromium.org/1419063002/diff/1/components/dom_distiller/co...
components/dom_distiller/core/dom_distiller_switches.h:17: extern const char
kDisableDomDistiller[];
On 2015/10/23 20:05:38, Vitaly Buka wrote:
> enable/disable distiller also all item into chrome menu and enables reader
mode.
> I guess goal to to change print preview and not that behavior.
> 
> So this switch should be next to other print preview switches

Currently, --enable-dom-distiller activates/deactivates both the Simplify page
checkbox (print preview) and Distill Page (reader mode, chromium menu). Should
we create a new flag then, just for print preview and not consider
--enable-dom-distiller altogether?

[Vitaly Buka (NO REVIEWS), 2015-10-26 17:16:23]

https://codereview.chromium.org/1419063002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1419063002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5433: +      <message
name="IDS_FLAGS_ENABLE_DOM_DISTILLER_NAME" desc="Name of the 'enable dom
distiller' lab">
I assume we didn't no enable distiller in main menu.
So we need different flag and command line switch.
Is this possible?

On 2015/10/26 15:51:37, mvendramini wrote:
> On 2015/10/23 20:05:37, Vitaly Buka wrote:
> > this is flag about print preview only, so it should be called appropriatly
> > IDS_FLAGS_DISABLE_DISTILLER_IN_PRINT_PREVIEW
> > 
> 
> Please note that the --enable-dom-distiller flag also activates/deactivates
the
> Distill Page option at the main menu ("Customize and control Chromium",
defaults
> to the top-right of the browser). Are you sure you want this branded as print
> preview only?

[Vitaly Buka (NO REVIEWS), 2015-10-26 17:16:57]

I assume we do not enable distiller in the main menu.
So we need different flag and command line switch.
Is this possible?

On 2015/10/26 17:16:23, Vitaly Buka wrote:
>
https://codereview.chromium.org/1419063002/diff/1/chrome/app/generated_resour...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/1419063002/diff/1/chrome/app/generated_resour...
> chrome/app/generated_resources.grd:5433: +      <message
> name="IDS_FLAGS_ENABLE_DOM_DISTILLER_NAME" desc="Name of the 'enable dom
> distiller' lab">
> I assume we didn't no enable distiller in main menu.
> So we need different flag and command line switch.
> Is this possible?
> 
> On 2015/10/26 15:51:37, mvendramini wrote:
> > On 2015/10/23 20:05:37, Vitaly Buka wrote:
> > > this is flag about print preview only, so it should be called appropriatly
> > > IDS_FLAGS_DISABLE_DISTILLER_IN_PRINT_PREVIEW
> > > 
> > 
> > Please note that the --enable-dom-distiller flag also activates/deactivates
> the
> > Distill Page option at the main menu ("Customize and control Chromium",
> defaults
> > to the top-right of the browser). Are you sure you want this branded as
print
> > preview only?

[Vitaly Buka (NO REVIEWS), 2015-10-26 17:21:42]

Just to clarify, is case I am missing something.
Is this patch for https://code.google.com/p/chromium/issues/detail?id=490809

[mvendramini_hp, 2015-10-26 17:49:29]

On 2015/10/26 17:21:42, Vitaly Buka wrote:
> Just to clarify, is case I am missing something.
> Is this patch for https://code.google.com/p/chromium/issues/detail?id=490809

Yes it is, you filled the BUG correctly, we will read the rest of the comments
and follow up soon.

[mvendramini_hp, 2015-10-26 17:55:05]

On 2015/10/26 17:16:57, Vitaly Buka wrote:
> I assume we do not enable distiller in the main menu.
> So we need different flag and command line switch.
> Is this possible?
> 

Yes, it is possible, we will work on that now. Thanks.

[mvendramini_hp, 2015-10-26 17:58:53]

So we will start working on:

1) separating the --enable-dom-distiller activation semantics from the reader
mode feature and the simplify page feature on print preview

2) Refresh the UI switch accordingly

3) Set it as default

Would you suggest a name for the new flag? --enable-print-preview-simplify ?

https://codereview.chromium.org/1419063002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1419063002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5433: +      <message
name="IDS_FLAGS_ENABLE_DOM_DISTILLER_NAME" desc="Name of the 'enable dom
distiller' lab">
On 2015/10/26 17:16:23, Vitaly Buka wrote:
> I assume we didn't no enable distiller in main menu.
> So we need different flag and command line switch.
> Is this possible?
> 
> On 2015/10/26 15:51:37, mvendramini wrote:
> > On 2015/10/23 20:05:37, Vitaly Buka wrote:
> > > this is flag about print preview only, so it should be called appropriatly
> > > IDS_FLAGS_DISABLE_DISTILLER_IN_PRINT_PREVIEW
> > > 
> > 
> > Please note that the --enable-dom-distiller flag also activates/deactivates
> the
> > Distill Page option at the main menu ("Customize and control Chromium",
> defaults
> > to the top-right of the browser). Are you sure you want this branded as
print
> > preview only?
> 

Acknowledged.

[Vitaly Buka (NO REVIEWS), 2015-10-26 18:30:03]

On 2015/10/26 17:58:53, mvendramini wrote:
> So we will start working on:
> 
> 1) separating the --enable-dom-distiller activation semantics from the reader
> mode feature and the simplify page feature on print preview
> 
> 2) Refresh the UI switch accordingly
> 
> 3) Set it as default
> 
> Would you suggest a name for the new flag? --enable-print-preview-simplify ?

It must be "disable" if default is enabled.
So I am ok with, --disable-print-preview-simplify

[mvendramini_hp, 2015-10-26 18:47:06]

On 2015/10/26 18:30:03, Vitaly Buka wrote:
> On 2015/10/26 17:58:53, mvendramini wrote:
> > So we will start working on:
> > 
> > 1) separating the --enable-dom-distiller activation semantics from the
reader
> > mode feature and the simplify page feature on print preview
> > 
> > 2) Refresh the UI switch accordingly
> > 
> > 3) Set it as default
> > 
> > Would you suggest a name for the new flag? --enable-print-preview-simplify ?
> 
> It must be "disable" if default is enabled.
> So I am ok with, --disable-print-preview-simplify

Ok I understand it now. Acknowledged and agreed.

[mvendramini_hp, 2015-10-27 13:25:00]

On 2015/10/26 18:47:06, mvendramini wrote:
> On 2015/10/26 18:30:03, Vitaly Buka wrote:
> > On 2015/10/26 17:58:53, mvendramini wrote:
> > > So we will start working on:
> > > 
> > > 1) separating the --enable-dom-distiller activation semantics from the
> reader
> > > mode feature and the simplify page feature on print preview
> > > 
> > > 2) Refresh the UI switch accordingly
> > > 
> > > 3) Set it as default
> > > 
> > > Would you suggest a name for the new flag? --enable-print-preview-simplify
?
> > 
> > It must be "disable" if default is enabled.
> > So I am ok with, --disable-print-preview-simplify
> 
> Ok I understand it now. Acknowledged and agreed.

New patchset is in.

[Vitaly Buka (NO REVIEWS), 2015-10-29 00:30:19]

https://codereview.chromium.org/1419063002/diff/20001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1419063002/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1342: #if !defined(OS_ANDROID)
this should be
#if defined(ENABLE_PRINT_PREVIEW)

https://codereview.chromium.org/1419063002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/1419063002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1297:
dom_distiller::url_utils::IsUrlDistillable(
please run "git cl format"
Unfortunatly you'll have to undo "git cl format" results on about_flags.cc

https://codereview.chromium.org/1419063002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1300: initiator,
false,
you need to remove dom_distiller::IsDistillablePage() call

[mvendramini_hp, 2015-10-29 11:57:18]

Acknowledging the removal of the distillation detection heuristics call
(IsDistillablePage). Our original plan was to make a separate CL just for that,
sorry for the misunderstanding.

https://codereview.chromium.org/1419063002/diff/20001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1419063002/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1342: #if !defined(OS_ANDROID)
On 2015/10/29 00:30:19, Vitaly Buka wrote:
> this should be
> #if defined(ENABLE_PRINT_PREVIEW)

Done.

https://codereview.chromium.org/1419063002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/1419063002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1297:
dom_distiller::url_utils::IsUrlDistillable(
On 2015/10/29 00:30:19, Vitaly Buka wrote:
> please run "git cl format"
> Unfortunatly you'll have to undo "git cl format" results on about_flags.cc

Done.

https://codereview.chromium.org/1419063002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1300: initiator,
false,
On 2015/10/29 00:30:19, Vitaly Buka wrote:
> you need to remove dom_distiller::IsDistillablePage() call

Done.

[Vitaly Buka (NO REVIEWS), 2015-10-29 18:44:02]

lgtm

https://codereview.chromium.org/1419063002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/1419063002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1303: void
PrintPreviewHandler::HandleIsPageDistillableResult(bool distillable) {
Please remove HandleIsPageDistillableResult as unused

[mvendramini_hp, 2015-10-29 18:52:32]

Patchset 3 is in: removing leftover function.

https://codereview.chromium.org/1419063002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/1419063002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1303: void
PrintPreviewHandler::HandleIsPageDistillableResult(bool distillable) {
On 2015/10/29 18:44:01, Vitaly Buka wrote:
> Please remove HandleIsPageDistillableResult as unused 

Done.

[mvendramini_hp, 2015-10-29 18:52:56]

On 2015/10/29 18:52:32, mvendramini wrote:
> Patchset 4 is in: removing leftover function.
> 
>
https://codereview.chromium.org/1419063002/diff/40001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):
> 
>
https://codereview.chromium.org/1419063002/diff/40001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1303: void
> PrintPreviewHandler::HandleIsPageDistillableResult(bool distillable) {
> On 2015/10/29 18:44:01, Vitaly Buka wrote:
> > Please remove HandleIsPageDistillableResult as unused 
> 
> Done.

[Vitaly Buka (NO REVIEWS), 2015-10-29 18:59:28]

lgtm

[mvendramini_hp, 2015-10-29 19:07:53]

On 2015/10/29 18:59:28, Vitaly Buka wrote:
> lgtm

I forgot to remove a stale comment that made reference to the removed
HandleIsPageDistillableResult function. Patchset 5 is in, to fix that.

[Lei Zhang, 2015-10-29 19:14:57]

https://codereview.chromium.org/1419063002/diff/80001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1419063002/diff/80001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5432: +        Disables simplify page on
print preview
grammar: Disable

https://codereview.chromium.org/1419063002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/1419063002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1296:
!(cmdline->HasSwitch(switches::kDisablePrintPreviewSimplify)) &&
nit: Too many parenthesis. Just !cmdline->Foo() &&

You can also break this code up a bit more:

if (!cmdline->HasSwitch(switches::kDisablePrintPreviewSimplify))
  return;

WebContents* initiator = GetInitiator();
if (initiator &&
    dom_distiller::url_utils::IsUrlDistillable(
        initiator->GetLastCommittedURL())) {
  web_ui()->CallJavascriptFunction("allowDistillPage");
}

https://codereview.chromium.org/1419063002/diff/80001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_switches.h (right):

https://codereview.chromium.org/1419063002/diff/80001/components/dom_distille...
components/dom_distiller/core/dom_distiller_switches.h:16: // Switch to disable
simplify page on the print preview dialog.
nit: Alphabetical order please. Ditto in the .cc file.

[mvendramini_hp, 2015-10-30 11:05:36]

Obs: On the conditional refactor (the earlier return) we used git cl format to
decide which formatting should be used. Is that formatting acceptable?

https://codereview.chromium.org/1419063002/diff/80001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1419063002/diff/80001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5432: +        Disables simplify page on
print preview
On 2015/10/29 19:14:56, Lei Zhang wrote:
> grammar: Disable

Done.

https://codereview.chromium.org/1419063002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right):

https://codereview.chromium.org/1419063002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1296:
!(cmdline->HasSwitch(switches::kDisablePrintPreviewSimplify)) &&
On 2015/10/29 19:14:57, Lei Zhang wrote:
> nit: Too many parenthesis. Just !cmdline->Foo() &&
> 
> You can also break this code up a bit more:
> 
> if (!cmdline->HasSwitch(switches::kDisablePrintPreviewSimplify))
>   return;
> 
> WebContents* initiator = GetInitiator();
> if (initiator &&
>     dom_distiller::url_utils::IsUrlDistillable(
>         initiator->GetLastCommittedURL())) {
>   web_ui()->CallJavascriptFunction("allowDistillPage");
> }

Done.

https://codereview.chromium.org/1419063002/diff/80001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_switches.h (right):

https://codereview.chromium.org/1419063002/diff/80001/components/dom_distille...
components/dom_distiller/core/dom_distiller_switches.h:16: // Switch to disable
simplify page on the print preview dialog.
On 2015/10/29 19:14:57, Lei Zhang wrote:
> nit: Alphabetical order please. Ditto in the .cc file.

Done.

[Lei Zhang, 2015-10-30 16:36:47]

On 2015/10/30 11:05:36, mvendramini wrote:
> Obs: On the conditional refactor (the earlier return) we used git cl format to
> decide which formatting should be used. Is that formatting acceptable?

If that's what git cl format wants, then let's go with the flow. lgtm

[Vitaly Buka corp, 2015-10-30 17:06:41]

The CQ bit was checked by vitalybuka@google.com

[Vitaly Buka corp, 2015-10-30 17:06:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
vitalybuka@chromium.org
Link to the patchset: https://codereview.chromium.org/1419063002/#ps100001
(title: "Grammar corrections, alphabetical ordering of variables, small
conditional refactor.")

[commit-bot: I haz the power, 2015-10-30 17:07:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1419063002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1419063002/100001

[commit-bot: I haz the power, 2015-10-30 17:18:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-30 17:18:29]

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...)

[mvendramini_hp, 2015-10-30 18:26:02]

On 2015/10/30 17:18:29, commit-bot: I haz the power wrote:
> 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...)

Any action required to solve those bot issues?

[Lei Zhang, 2015-10-30 18:30:57]

On 2015/10/30 18:26:02, mvendramini wrote:
> On 2015/10/30 17:18:29, commit-bot: I haz the power wrote:
> > 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...)
> 
> Any action required to solve those bot issues?

Yes, more actions are required. Did you read the error message?

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    components/dom_distiller/core/dom_distiller_switches.cc
    components/dom_distiller/core/dom_distiller_switches.h

[mvendramini_hp, 2015-10-30 18:41:42]

mvendramini@hp.com changed reviewers:
+ bengr@chromium.org, mdjones@chromium.org

[mvendramini_hp, 2015-10-30 18:42:54]

On 2015/10/30 18:30:57, Lei Zhang wrote:
> On 2015/10/30 18:26:02, mvendramini wrote:
> > On 2015/10/30 17:18:29, commit-bot: I haz the power wrote:
> > > 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...)
> > 
> > Any action required to solve those bot issues?
> 
> Yes, more actions are required. Did you read the error message?
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>     components/dom_distiller/core/dom_distiller_switches.cc
>     components/dom_distiller/core/dom_distiller_switches.h

Had a little trouble combing through the logs - adding two more reviewers.

[Vitaly Buka (NO REVIEWS), 2015-10-30 19:40:48]

Flags needs to be added into that xml file. Example:
https://code.google.com/p/chromium/codesearch#search/&q=device-discovery-noti...

[Vitaly Buka (NO REVIEWS), 2015-10-30 19:44:04]

On 2015/10/30 19:40:48, Vitaly Buka wrote:
> Flags needs to be added into that xml file. Example:
>
https://code.google.com/p/chromium/codesearch#search/&q=device-discovery-noti...

BTW. Switches in dom_distiller_switches.cc are not used by distiller
So they probably more related to print preview so should be move into
chrome_switches.cc

Lei, WDYT?

[Lei Zhang, 2015-10-30 20:15:39]

On 2015/10/30 19:44:04, Vitaly Buka wrote:
> On 2015/10/30 19:40:48, Vitaly Buka wrote:
> > Flags needs to be added into that xml file. Example:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=device-discovery-noti...
> 
> BTW. Switches in dom_distiller_switches.cc are not used by distiller
> So they probably more related to print preview so should be move into
> chrome_switches.cc
> 
> Lei, WDYT?

SGTM

[mvendramini_hp, 2015-11-03 16:34:05]

On 2015/10/30 19:40:48, Vitaly Buka wrote:
> Flags needs to be added into that xml file. Example:
>
https://code.google.com/p/chromium/codesearch#search/&q=device-discovery-noti...

Done

[mvendramini_hp, 2015-11-03 16:35:17]

On 2015/10/30 19:44:04, Vitaly Buka wrote:
> On 2015/10/30 19:40:48, Vitaly Buka wrote:
> > Flags needs to be added into that xml file. Example:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=device-discovery-noti...
> 
> BTW. Switches in dom_distiller_switches.cc are not used by distiller
> So they probably more related to print preview so should be move into
> chrome_switches.cc
> 
> Lei, WDYT?

Should we create a new CL for this?

[Lei Zhang, 2015-11-03 16:59:28]

On 2015/11/03 16:35:17, mvendramini wrote:
> On 2015/10/30 19:44:04, Vitaly Buka wrote:
> > On 2015/10/30 19:40:48, Vitaly Buka wrote:
> > > Flags needs to be added into that xml file. Example:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=device-discovery-noti...
> > 
> > BTW. Switches in dom_distiller_switches.cc are not used by distiller
> > So they probably more related to print preview so should be move into
> > chrome_switches.cc
> > 
> > Lei, WDYT?
> 
> Should we create a new CL for this?

Why not just do it on this CL?

[mvendramini_hp, 2015-11-03 17:03:09]

On 2015/11/03 16:59:28, Lei Zhang wrote:
> On 2015/11/03 16:35:17, mvendramini wrote:
> > On 2015/10/30 19:44:04, Vitaly Buka wrote:
> > > On 2015/10/30 19:40:48, Vitaly Buka wrote:
> > > > Flags needs to be added into that xml file. Example:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=device-discovery-noti...
> > > 
> > > BTW. Switches in dom_distiller_switches.cc are not used by distiller
> > > So they probably more related to print preview so should be move into
> > > chrome_switches.cc
> > > 
> > > Lei, WDYT?
> > 
> > Should we create a new CL for this?
> 
> Why not just do it on this CL?


OK, so should we move *all* switches from dom_distiller_switches into
chrome_switches?
Should we also delete dom_distiller_switches?

[Lei Zhang, 2015-11-03 17:18:32]

On 2015/11/03 17:03:09, mvendramini wrote:
> On 2015/11/03 16:59:28, Lei Zhang wrote:
> > On 2015/11/03 16:35:17, mvendramini wrote:
> > > On 2015/10/30 19:44:04, Vitaly Buka wrote:
> > > > On 2015/10/30 19:40:48, Vitaly Buka wrote:
> > > > > Flags needs to be added into that xml file. Example:
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=device-discovery-noti...
> > > > 
> > > > BTW. Switches in dom_distiller_switches.cc are not used by distiller
> > > > So they probably more related to print preview so should be move into
> > > > chrome_switches.cc
> > > > 
> > > > Lei, WDYT?
> > > 
> > > Should we create a new CL for this?
> > 
> > Why not just do it on this CL?
> 
> 
> OK, so should we move *all* switches from dom_distiller_switches into
> chrome_switches?
> Should we also delete dom_distiller_switches?

Noooo, just kDisablePrintPreviewSimplify.

[mvendramini_hp, 2015-11-03 18:07:53]

On 2015/11/03 17:18:32, Lei Zhang wrote:
> On 2015/11/03 17:03:09, mvendramini wrote:
> > On 2015/11/03 16:59:28, Lei Zhang wrote:
> > > On 2015/11/03 16:35:17, mvendramini wrote:
> > > > On 2015/10/30 19:44:04, Vitaly Buka wrote:
> > > > > On 2015/10/30 19:40:48, Vitaly Buka wrote:
> > > > > > Flags needs to be added into that xml file. Example:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=device-discovery-noti...
> > > > > 
> > > > > BTW. Switches in dom_distiller_switches.cc are not used by distiller
> > > > > So they probably more related to print preview so should be move into
> > > > > chrome_switches.cc
> > > > > 
> > > > > Lei, WDYT?
> > > > 
> > > > Should we create a new CL for this?
> > > 
> > > Why not just do it on this CL?
> > 
> > 
> > OK, so should we move *all* switches from dom_distiller_switches into
> > chrome_switches?
> > Should we also delete dom_distiller_switches?
> 
> Noooo, just kDisablePrintPreviewSimplify.

Done.

[mvendramini_hp, 2015-11-03 18:10:47]

Description was changed from

==========
Adding a UI switch for enable-dom-distiller

On chrome://flags, this commit adds a UI selector for
the --enable-dom-distiller command line option

R=thakis@chromium.org, nyquist@chromium.org, vitalybuka@chromium.org,
thestig@chromium.org 
BUG=490809
==========

to

==========
Adding a UI switch for enable-dom-distiller

On chrome://flags, this commit adds a UI selector for
the --enable-dom-distiller command line option

R=thakis@chromium.org, nyquist@chromium.org, vitalybuka@chromium.org,
thestig@chromium.org,
mdjones@chromium.org, bengr@chromium.org,
isherman@chromium.org
BUG=490809
==========

[mvendramini_hp, 2015-11-03 18:10:47]

mvendramini@hp.com changed reviewers:
+ isherman@chromium.org

[bengr, 2015-11-03 18:15:12]

*dom_distiller* lgtm

[Vitaly Buka (NO REVIEWS), 2015-11-03 18:16:06]

Thanks, I suspect this change is conflicting with current master.
I can try to click CQ, but it will likely fail.
Please update your Chromium checkout and rebase the CL.

[mvendramini_hp, 2015-11-03 18:16:10]

We detected there's a parallel CL to show the "simplify print preview" checkbox
by default:
https://codereview.chromium.org/1419363004/

Please advise; should we wait, or rebase, or something else ?

[Vitaly Buka (NO REVIEWS), 2015-11-03 18:16:13]

The CQ bit was checked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2015-11-03 18:16:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, vitalybuka@chromium.org
Link to the patchset: https://codereview.chromium.org/1419063002/#ps140001
(title: "Factoring out the disable-print-preview-simplify switch from
dom_distiller_switches into chrome_swi…")

[Vitaly Buka (NO REVIEWS), 2015-11-03 18:17:39]

> Please advise; should we wait, or rebase, or something else ?

Yes, you need to rebase, fix conflicts if any and upload new version.

[commit-bot: I haz the power, 2015-11-03 18:18:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1419063002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1419063002/140001

[Vitaly Buka (NO REVIEWS), 2015-11-03 18:21:37]

The CQ bit was unchecked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2015-11-03 18:22:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-03 18:22:37]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mvendramini_hp, 2015-11-03 19:49:17]

On 2015/11/03 18:17:39, Vitaly Buka wrote:
> > Please advise; should we wait, or rebase, or something else ?
> 
> Yes, you need to rebase, fix conflicts if any and upload new version.

Done - rebased.

There was a conflict with the aforementioned CL
(https://codereview.chromium.org/1419363004/) - since that functionality was
already present on this CL, we basically kept our copy.

[mvendramini_hp, 2015-11-03 19:56:24]

The CQ bit was checked by mvendramini@hp.com

[mvendramini_hp, 2015-11-03 19:56:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, bengr@chromium.org, vitalybuka@chromium.org
Link to the patchset: https://codereview.chromium.org/1419063002/#ps160001
(title: "Latest rebase (nov 3/15)")

[commit-bot: I haz the power, 2015-11-03 19:58:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1419063002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1419063002/160001

[mvendramini_hp, 2015-11-03 20:11:32]

mvendramini@hp.com changed reviewers:
+ mpearson@chromium.org

[mvendramini_hp, 2015-11-03 20:11:55]

Description was changed from

==========
Adding a UI switch for enable-dom-distiller

On chrome://flags, this commit adds a UI selector for
the --enable-dom-distiller command line option

R=thakis@chromium.org, nyquist@chromium.org, vitalybuka@chromium.org,
thestig@chromium.org,
mdjones@chromium.org, bengr@chromium.org,
isherman@chromium.org
BUG=490809
==========

to

==========
Adding a UI switch for enable-dom-distiller

On chrome://flags, this commit adds a UI selector for
the --enable-dom-distiller command line option

R=thakis@chromium.org, nyquist@chromium.org, vitalybuka@chromium.org,
thestig@chromium.org,
mdjones@chromium.org, bengr@chromium.org,
isherman@chromium.org, mpearson@chromium.org
BUG=490809
==========

[commit-bot: I haz the power, 2015-11-03 20:16:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-03 20:16:57]

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...)

[mvendramini_hp, 2015-11-03 20:22:29]

Added more reviewers; still histograms.xml needs revision.

[Mark P, 2015-11-03 20:22:45]

histograms.xml lgtm

[Vitaly Buka (NO REVIEWS), 2015-11-03 20:24:31]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2015-11-03 20:26:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1419063002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1419063002/160001

[mvendramini_hp, 2015-11-03 20:36:35]

The CQ bit was unchecked by mvendramini@hp.com

[mvendramini_hp, 2015-11-03 20:45:32]

The CQ bit was checked by mvendramini@hp.com

[mvendramini_hp, 2015-11-03 20:45:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, mpearson@chromium.org, bengr@chromium.org,
vitalybuka@chromium.org
Link to the patchset: https://codereview.chromium.org/1419063002/#ps180001
(title: "Correcting histograms.xml")

[commit-bot: I haz the power, 2015-11-03 20:47:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1419063002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1419063002/180001

[mvendramini_hp, 2015-11-03 21:17:08]

vitaly@ and thestig@ (Lei@):

Do you think it would be a good call to edit the name of this CL at this point?
The original name no longer reflects the contents. Please advise?

[Vitaly Buka (NO REVIEWS), 2015-11-03 22:07:05]

The CQ bit was unchecked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2015-11-03 22:07:55]

> Do you think it would be a good call to edit the name of this CL at this
point?
> The original name no longer reflects the contents. Please advise?

Sure, please update using "Edit Issue" and CQ again.

[mvendramini_hp, 2015-11-03 22:24:55]

Description was changed from

==========
Adding a UI switch for enable-dom-distiller

On chrome://flags, this commit adds a UI selector for
the --enable-dom-distiller command line option

R=thakis@chromium.org, nyquist@chromium.org, vitalybuka@chromium.org,
thestig@chromium.org,
mdjones@chromium.org, bengr@chromium.org,
isherman@chromium.org, mpearson@chromium.org
BUG=490809
==========

to

==========
Displaying "simplify page" check box on print preview by default.

And also removing the "simplifiable page" detection heuristics from
consideration when displaying the "simplify page" check box on print preview.

Also adding a cmdline and UI switch to disable it
(--disable-print-preview-simplify and accompanying UI switch at chrome://flags)

R=thakis@chromium.org, nyquist@chromium.org, vitalybuka@chromium.org,
thestig@chromium.org, mdjones@chromium.org, bengr@chromium.org,
isherman@chromium.org, mpearson@chromium.org
BUG=490809
==========

[mvendramini_hp, 2015-11-03 22:26:24]

On 2015/11/03 22:07:55, Vitaly Buka wrote:
> > Do you think it would be a good call to edit the name of this CL at this
> point?
> > The original name no longer reflects the contents. Please advise?
> 
> Sure, please update using "Edit Issue" and CQ again.

Okay, I changed the title and description - doing CQ now.

[mvendramini_hp, 2015-11-03 22:26:38]

The CQ bit was checked by mvendramini@hp.com

[commit-bot: I haz the power, 2015-11-03 22:27:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1419063002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1419063002/180001

[commit-bot: I haz the power, 2015-11-03 22:43:29]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-11-03 22:44:26]

Patchset 10 (id:??) landed as
https://crrev.com/95abe757ca491cf2521e8e30eb56104519d506c4
Cr-Commit-Position: refs/heads/master@{#357661}