Remove dump_line_box_trees and debug_render_tree from LayoutDumpFlags.

Remove dump_line_box_trees and debug_render_tree from LayoutDumpFlags.

LayoutDumpFlags::debug_render_tree was always set to false.

LayoutDumpFlags::dump_line_box_trees was controlled by
--dump-line-box-trees switch.  I have not found any usage of this switch
(searching chromium repo via web/codesearch [1] and manually searching
the build repo [2] that contains trybot and fyibot scripts).

Removing these flags is desirable because:
- Replication of LayoutDumpFlags across OOPIFs [3] gets sligthly and
  unnecessarily more complicated when it has to take into account values
  coming from command-line switches.
- Code removal is desirable in general (reduces maintenance burden).

BUG=587175
[1] https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/common/shell_switches.cc&q=%22dump-line-box-trees%22&sq=package:chromium&type=cs&l=33

[2] https://chromium.googlesource.com/chromium/tools/build.git

[3] work-in-progress, 2 approaches being considered at crrev.com/1715573002 and crrev.com/1736353002.

Committed: https://crrev.com/c76d857e4749bb13f8eeaf0a77d57e69338dc7f5
Cr-Commit-Position: refs/heads/master@{#379322}

[Łukasz Anforowicz, 2016-03-03 17:33:26]

lukasza@chromium.org changed reviewers:
+ jochen@chromium.org

[Łukasz Anforowicz, 2016-03-03 17:33:27]

jochen@, could you please take a look?

I can live with these 2 flags, but I thought that removing them will make things
easier / cleaner.  These dump modes are still available for debugging (i.e. I am
not removing the related code from Blink) and can be turned on by editing
layout_dump.cc (i.e. same effort as before for dump_line_box_trees, slightly
bigger but IMO acceptable effort compared to dump-line-box-trees switch).

WDYT?

[Łukasz Anforowicz, 2016-03-03 17:33:41]

Description was changed from

==========
Remove dump_line_box_trees and debug_render_tree from LayoutDumpFlags.

LayoutDumpFlags::debug_render_tree was always set to false.

LayoutDumpFlags::dump_line_box_trees was controlled by
dump-line-box-trees switch.  I have not found any usage of this switch
(searching chromium repo via web/codesearch [1] and manually searching
the build repo [2] that contains trybot and fyibot scripts).

Removing these flags is desirable because:
- Replication of LayoutDumpFlags across OOPIFs [3] gets sligthly and
  unnecessarily more complicated when it has to take into account values
  coming from command-line switches.
- Code removal is desirable in general (reduces maintenance burden).

BUG=587175

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/comm...

[2] https://chromium.googlesource.com/chromium/tools/build.git

[3] work-in-progress, 2 approaches being considered at crrev.com/1715573002 and
crrev.com/1736353002.
==========

to

==========
Remove dump_line_box_trees and debug_render_tree from LayoutDumpFlags.

LayoutDumpFlags::debug_render_tree was always set to false.

LayoutDumpFlags::dump_line_box_trees was controlled by
--dump-line-box-trees switch.  I have not found any usage of this switch
(searching chromium repo via web/codesearch [1] and manually searching
the build repo [2] that contains trybot and fyibot scripts).

Removing these flags is desirable because:
- Replication of LayoutDumpFlags across OOPIFs [3] gets sligthly and
  unnecessarily more complicated when it has to take into account values
  coming from command-line switches.
- Code removal is desirable in general (reduces maintenance burden).

BUG=587175

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/comm...

[2] https://chromium.googlesource.com/chromium/tools/build.git

[3] work-in-progress, 2 approaches being considered at crrev.com/1715573002 and
crrev.com/1736353002.
==========

[jochen (gone - plz use gerrit), 2016-03-04 12:33:36]

lgtm

https://codereview.chromium.org/1755123004/diff/20001/content/shell/browser/s...
File content/shell/browser/shell_content_browser_client.cc (left):

https://codereview.chromium.org/1755123004/diff/20001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.cc:237:
switches::kDumpLineBoxTrees)) {
ugh, this should have been handled in
content/shell/browser/layout_test/layout_test_content_browser_client.cc

[jochen (gone - plz use gerrit), 2016-03-04 12:34:14]

https://codereview.chromium.org/1755123004/diff/20001/content/shell/common/sh...
File content/shell/common/shell_switches.cc (left):

https://codereview.chromium.org/1755123004/diff/20001/content/shell/common/sh...
content/shell/common/shell_switches.cc:33: const char kDumpLineBoxTrees[] =
"dump-line-box-trees";
maybe we should factor out layout_test switches into
c/s/common/layout_test/layout_test_switches.*

[Łukasz Anforowicz, 2016-03-04 17:40:08]

Thanks for reviewing.

On 2016/03/04 12:34:14, jochen wrote:
>
https://codereview.chromium.org/1755123004/diff/20001/content/shell/common/sh...
> File content/shell/common/shell_switches.cc (left):
> 
>
https://codereview.chromium.org/1755123004/diff/20001/content/shell/common/sh...
> content/shell/common/shell_switches.cc:33: const char kDumpLineBoxTrees[] =
> "dump-line-box-trees";
> maybe we should factor out layout_test switches into
> c/s/common/layout_test/layout_test_switches.*

Ok - I am trying to do this in https://codereview.chromium.org/1763113002/.

Also - we probably want to also move c/s/common/shell_messages.h into
c/s/common/layout_test/layout_test_messages.h (AFAICT all these messages are
specific to layout tests).  OTOH, I'd rather postpone this for a while for 2
reasons:

1. I want to selfishly avoid merge conflicts with the pending CLs under review.
:-/

2. There are a few open questions around such move (i.e. all the messages are
highly-inaccurately named - the names include "Shell" and "View" when in reality
some messages target Process or Frame, not View.

https://codereview.chromium.org/1755123004/diff/20001/content/shell/browser/s...
File content/shell/browser/shell_content_browser_client.cc (left):

https://codereview.chromium.org/1755123004/diff/20001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.cc:237:
switches::kDumpLineBoxTrees)) {
On 2016/03/04 12:33:36, jochen wrote:
> ugh, this should have been handled in
> content/shell/browser/layout_test/layout_test_content_browser_client.cc

Acknowledged - I try to fix the remaining ones in
https://codereview.chromium.org/1763113002/.

[Łukasz Anforowicz, 2016-03-04 17:51:28]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-03-04 17:51:48]

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

[commit-bot: I haz the power, 2016-03-04 18:59:14]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-04 19:00:43]

Description was changed from

==========
Remove dump_line_box_trees and debug_render_tree from LayoutDumpFlags.

LayoutDumpFlags::debug_render_tree was always set to false.

LayoutDumpFlags::dump_line_box_trees was controlled by
--dump-line-box-trees switch.  I have not found any usage of this switch
(searching chromium repo via web/codesearch [1] and manually searching
the build repo [2] that contains trybot and fyibot scripts).

Removing these flags is desirable because:
- Replication of LayoutDumpFlags across OOPIFs [3] gets sligthly and
  unnecessarily more complicated when it has to take into account values
  coming from command-line switches.
- Code removal is desirable in general (reduces maintenance burden).

BUG=587175

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/comm...

[2] https://chromium.googlesource.com/chromium/tools/build.git

[3] work-in-progress, 2 approaches being considered at crrev.com/1715573002 and
crrev.com/1736353002.
==========

to

==========
Remove dump_line_box_trees and debug_render_tree from LayoutDumpFlags.

LayoutDumpFlags::debug_render_tree was always set to false.

LayoutDumpFlags::dump_line_box_trees was controlled by
--dump-line-box-trees switch.  I have not found any usage of this switch
(searching chromium repo via web/codesearch [1] and manually searching
the build repo [2] that contains trybot and fyibot scripts).

Removing these flags is desirable because:
- Replication of LayoutDumpFlags across OOPIFs [3] gets sligthly and
  unnecessarily more complicated when it has to take into account values
  coming from command-line switches.
- Code removal is desirable in general (reduces maintenance burden).

BUG=587175

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/comm...

[2] https://chromium.googlesource.com/chromium/tools/build.git

[3] work-in-progress, 2 approaches being considered at crrev.com/1715573002 and
crrev.com/1736353002.

Committed: https://crrev.com/c76d857e4749bb13f8eeaf0a77d57e69338dc7f5
Cr-Commit-Position: refs/heads/master@{#379322}
==========

[commit-bot: I haz the power, 2016-03-04 19:00:45]

Patchset 2 (id:??) landed as
https://crrev.com/c76d857e4749bb13f8eeaf0a77d57e69338dc7f5
Cr-Commit-Position: refs/heads/master@{#379322}

[rhogan, 2016-04-20 19:54:15]

On 2016/03/04 at 19:00:45, commit-bot wrote:
> Patchset 2 (id:??) landed as
https://crrev.com/c76d857e4749bb13f8eeaf0a77d57e69338dc7f5
> Cr-Commit-Position: refs/heads/master@{#379322}

Am disappointed to see the removal of --dump-line-box-trees - it was very useful
when debugging line layout. Any chance of getting this back? Is there an
alternative?

[Łukasz Anforowicz, 2016-04-20 20:03:11]

On 2016/04/20 19:54:15, rhogan wrote:
> On 2016/03/04 at 19:00:45, commit-bot wrote:
> > Patchset 2 (id:??) landed as
> https://crrev.com/c76d857e4749bb13f8eeaf0a77d57e69338dc7f5
> > Cr-Commit-Position: refs/heads/master@{#379322}
> 
> Am disappointed to see the removal of --dump-line-box-trees - it was very
useful
> when debugging line layout. Any chance of getting this back? Is there an
> alternative?

Can you manually (temporarily, while debugging) add 
    layout_text_behavior |=
        WebFrameContentDumper::LayoutAsTextWithLineTrees;
to DumpLayout function in your local components/test_runner/layout_dump.cc?

[rhogan, 2016-04-27 20:22:31]

On 2016/04/20 at 20:03:11, lukasza wrote:
> On 2016/04/20 19:54:15, rhogan wrote:
> > On 2016/03/04 at 19:00:45, commit-bot wrote:
> > > Patchset 2 (id:??) landed as
> > https://crrev.com/c76d857e4749bb13f8eeaf0a77d57e69338dc7f5
> > > Cr-Commit-Position: refs/heads/master@{#379322}
> > 
> > Am disappointed to see the removal of --dump-line-box-trees - it was very
useful
> > when debugging line layout. Any chance of getting this back? Is there an
> > alternative?
> 
> Can you manually (temporarily, while debugging) add 
>     layout_text_behavior |=
>         WebFrameContentDumper::LayoutAsTextWithLineTrees;
> to DumpLayout function in your local components/test_runner/layout_dump.cc?

Thanks for the tip lukasza!