third_party/WebKit/Source/core/layout/LayoutObject.cpp - Issue 2905733003: Remove WTFLogAlways() usages from core/layout/

Side by Side Diff: third_party/WebKit/Source/core/layout/LayoutObject.cpp

Issue 2905733003: Remove WTFLogAlways() usages from core/layout/

Patch Set: Created 3 years, 7 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

OLD	NEW
1 /*	1 /*

2 * Copyright (C) 1999 Lars Knoll (knoll@kde.org)	2 * Copyright (C) 1999 Lars Knoll (knoll@kde.org)

3 * (C) 1999 Antti Koivisto (koivisto@kde.org)	3 * (C) 1999 Antti Koivisto (koivisto@kde.org)

4 * (C) 2000 Dirk Mueller (mueller@kde.org)	4 * (C) 2000 Dirk Mueller (mueller@kde.org)

5 * (C) 2004 Allan Sandfeld Jensen (kde@carewolf.com)	5 * (C) 2004 Allan Sandfeld Jensen (kde@carewolf.com)

6 * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2011 Apple Inc.	6 * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2011 Apple Inc.

7 * All rights reserved.	7 * All rights reserved.

8 * Copyright (C) 2009 Google Inc. All rights reserved.	8 * Copyright (C) 2009 Google Inc. All rights reserved.

9 * Copyright (C) 2009 Torch Mobile Inc. All rights reserved.	9 * Copyright (C) 2009 Torch Mobile Inc. All rights reserved.

10 * (http://www.torchmobile.com/)	10 * (http://www.torchmobile.com/)

(...skipping 1337 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
1348 " \"%s\" ", ToLayoutText(this)->GetText().Ascii().data()));	1348 " \"%s\" ", ToLayoutText(this)->GetText().Ascii().data()));

1349	1349

1350 if (VirtualContinuation())	1350 if (VirtualContinuation())

1351 string_builder.Append(	1351 string_builder.Append(

1352 String::Format(" continuation=%p", VirtualContinuation()));	1352 String::Format(" continuation=%p", VirtualContinuation()));

1353	1353

1354 if (GetNode()) {	1354 if (GetNode()) {

1355 while (string_builder.length() < kShowTreeCharacterOffset)	1355 while (string_builder.length() < kShowTreeCharacterOffset)

1356 string_builder.Append(' ');	1356 string_builder.Append(' ');

1357 string_builder.Append('\t');	1357 string_builder.Append('\t');

1358 WTFLogAlways("%s%s", string_builder.ToString().Utf8().data(),	1358 LOG(INFO) << "\n"
	fs 2017/06/14 20:14:19 Why an initial \n (WTFLogAlways always made sure t Why an initial \n (WTFLogAlways always made sure that there was one at the end, but not a the beginning?) tkent 2017/06/14 23:41:39 ShowLayoutObject() is one-line summary of this Lay ShowLayoutObject() is one-line summary of this LayoutObject, and is called multiple times in ShowLayoutTree(). So this function should not use LOG* macros. I think this function should be operator<<() for LayoutObject. mrunal 2017/06/19 22:10:57 @fs, No idea, I was just trying to follow other pa Show quoted text On 2017/06/14 20:14:19, fs wrote: > Why an initial \n (WTFLogAlways always made sure that there was one at the end, > but not a the beginning?) @fs, No idea, I was just trying to follow other patches which have replaced WTFLogAlways in the past. For example, https://codereview.chromium.org/2292843002. mrunal 2017/06/19 22:10:57 @tkent, so just overload operator<<() instead of S Show quoted text On 2017/06/14 23:41:39, tkent wrote: > ShowLayoutObject() is one-line summary of this LayoutObject, and is called > multiple times in ShowLayoutTree(). So this function should not use LOG* > macros. > > I think this function should be operator<<() for LayoutObject. > @tkent, so just overload operator<<() instead of ShowLayoutObject() then? tkent 2017/06/20 02:36:23 Yes, I meant it. However, I realized it was unnece Show quoted text On 2017/06/19 at 22:10:57, mrunal wrote: > On 2017/06/14 23:41:39, tkent wrote: > > ShowLayoutObject() is one-line summary of this LayoutObject, and is called > > multiple times in ShowLayoutTree(). So this function should not use LOG* > > macros. > > > > I think this function should be operator<<() for LayoutObject. > > > > @tkent, so just overload operator<<() instead of ShowLayoutObject() then? Yes, I meant it. However, I realized it was unnecessary to eliminate WTFLogAlways(). The minimum change to eliminate WTFLogAlways() would be: * LayoutObject::ShowLayoutObject() with no argument DLOG(INFO) with the result of ShowLayoutObject(StringBuilder&) * LayoutObject::ShowLayoutObject(StringBuilder&) Just remove WTFLogAlways(). Callsites of this function are responsible to print. * LayoutObject::ShowLayoutTreeAndMark() Add StringBuilder& argument, and pour resultant strings to it. * ::showTree(), ::showLineTree() Replace WTFLogAlways() with DLOG(INFO). * ::showLayoutTree(object1, object2) If object1 is not null, make a StringBuilder, pass it to ShowLayoutTreeAndMark(), pour "\n" + resultant StringBuilder to DLOG(INFO). Replace WTFLogAlways() with DLOG(INFO).
1359 GetNode()->ToString().Utf8().data());	1359 << string_builder.ToString().Utf8().data()
	fs 2017/06/14 20:14:19 Don't we have an operator<< that would work here? Don't we have an operator<< that would work here? (So just string_builder.ToString() and similarly for the Node.) mrunal 2017/06/19 22:10:57 Again, I was just trying to follow other patches w Show quoted text On 2017/06/14 20:14:19, fs wrote: > Don't we have an operator<< that would work here? (So just > string_builder.ToString() and similarly for the Node.) Again, I was just trying to follow other patches which have replaced WTFLogAlways in the past. Your suggestion does make sense so will try to apply that change. tkent 2017/06/20 02:36:24 \|<< string_builder.ToString()\| and \|<< string_buil Show quoted text On 2017/06/19 at 22:10:57, mrunal wrote: > On 2017/06/14 20:14:19, fs wrote: > > Don't we have an operator<< that would work here? (So just > > string_builder.ToString() and similarly for the Node.) > > Again, I was just trying to follow other patches which have replaced > WTFLogAlways in the past. Your suggestion does make sense so will try to apply that change. \|<< string_builder.ToString()\| and \|<< string_builder.ToString().Utf8().data()\| have different results. In this case, .Utf8().data() has a backward-compatible output. fs 2017/06/20 08:24:35 I don't particularly care either way, but I don't Show quoted text On 2017/06/20 at 02:36:24, tkent wrote: > On 2017/06/19 at 22:10:57, mrunal wrote: > > On 2017/06/14 20:14:19, fs wrote: > > > Don't we have an operator<< that would work here? (So just > > > string_builder.ToString() and similarly for the Node.) > > > > Again, I was just trying to follow other patches which have replaced > > WTFLogAlways in the past. Your suggestion does make sense so will try to apply that change. > > \|<< string_builder.ToString()\| and \|<< string_builder.ToString().Utf8().data()\| have different results. In this case, .Utf8().data() has a backward-compatible output. I don't particularly care either way, but I don't really see the need for "backwards compatibility" here - it's not like we use this for DRT, or in general care about any sort of fidelity of the output here (since for instance non-ASCII codepoints will just be mangled in the beginning of this method.)
	1360 << GetNode()->ToString().Utf8().data();

1360 } else {	1361 } else {

1361 WTFLogAlways("%s", string_builder.ToString().Utf8().data());	1362 LOG(INFO) << "\n" << string_builder.ToString().Utf8().data();
	mstensho (USE GERRIT) 2017/06/20 08:49:40 Changing from WTFLogAlways() to LOG() introduces a Changing from WTFLogAlways() to LOG() introduces a lot of noise! For example, I often dump the layout tree from gdb, by calling showLayoutTree(). <!DOCTYPE html> <div>Hello world</div> Used to look like this: LayoutView 0x26c7bd004010 #document LayoutBlockFlow 0x26c7bd010010 HTML LayoutBlockFlow 0x26c7bd010138 BODY LayoutBlockFlow 0x26c7bd010260 DIV LayoutText 0x26c7bd01c0d8 #text "Hello world" Now it looks like this: [26238:26267:0620/104440.297233:4051463759306:INFO:LayoutObject.cpp(1359)] LayoutView 0x2ffe3a804010 #document [26238:26267:0620/104440.297341:4051463759411:INFO:LayoutObject.cpp(1359)] LayoutBlockFlow 0x2ffe3a810010 HTML [26238:26267:0620/104440.297401:4051463759470:INFO:LayoutObject.cpp(1359)] LayoutBlockFlow 0x2ffe3a810138 BODY [26238:26267:0620/104440.297456:4051463759524:INFO:LayoutObject.cpp(1359)] LayoutBlockFlow 0x2ffe3a8104b0 DIV [26238:26267:0620/104440.297508:4051463759577:INFO:LayoutObject.cpp(1359)] LayoutText 0x2ffe3a81c588 #text "Hello world" I.e. unreadable. I don't like this at all! Can we get rid of the noise somehow?
1362 }	1363 }

1363 }	1364 }

1364	1365

1365 void LayoutObject::ShowLayoutTreeAndMark(const LayoutObject* marked_object1,	1366 void LayoutObject::ShowLayoutTreeAndMark(const LayoutObject* marked_object1,

1366 const char* marked_label1,	1367 const char* marked_label1,

1367 const LayoutObject* marked_object2,	1368 const LayoutObject* marked_object2,

1368 const char* marked_label2,	1369 const char* marked_label2,

1369 unsigned depth) const {	1370 unsigned depth) const {

1370 StringBuilder string_builder;	1371 StringBuilder string_builder;

1371 if (marked_object1 == this && marked_label1)	1372 if (marked_object1 == this && marked_label1)

(...skipping 2243 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
3615 }	3616 }

3616	3617

3617 } // namespace blink	3618 } // namespace blink

3618	3619

3619 #ifndef NDEBUG	3620 #ifndef NDEBUG

3620	3621

3621 void showTree(const blink::LayoutObject* object) {	3622 void showTree(const blink::LayoutObject* object) {

3622 if (object)	3623 if (object)

3623 object->ShowTreeForThis();	3624 object->ShowTreeForThis();

3624 else	3625 else

3625 WTFLogAlways("%s", "Cannot showTree. Root is (nil)");	3626 LOG(INFO) << "\n Cannot showTree. Root is (nil)";
	tkent 2017/06/14 23:41:39 The leading "\n" is unnecessary. The leading "\n" is unnecessary. tkent 2017/06/14 23:41:40 The leading "\n" is unnecessary. The leading "\n" is unnecessary. mrunal 2017/06/19 22:10:57 Acknowledged. Show quoted text On 2017/06/14 23:41:39, tkent wrote: > The leading "\n" is unnecessary. Acknowledged.
3626 }	3627 }

3627	3628

3628 void showLineTree(const blink::LayoutObject* object) {	3629 void showLineTree(const blink::LayoutObject* object) {

3629 if (object)	3630 if (object)

3630 object->ShowLineTreeForThis();	3631 object->ShowLineTreeForThis();

3631 else	3632 else

3632 WTFLogAlways("%s", "Cannot showLineTree. Root is (nil)");	3633 LOG(INFO) << "\n Cannot showLineTree. Root is (nil)";
	tkent 2017/06/14 23:41:39 The leading "\n" is unnecessary. The leading "\n" is unnecessary. mrunal 2017/06/19 22:10:57 Acknowledged. Show quoted text On 2017/06/14 23:41:39, tkent wrote: > The leading "\n" is unnecessary. Acknowledged.
3633 }	3634 }

3634	3635

3635 void showLayoutTree(const blink::LayoutObject* object1) {	3636 void showLayoutTree(const blink::LayoutObject* object1) {

3636 showLayoutTree(object1, 0);	3637 showLayoutTree(object1, 0);

3637 }	3638 }

3638	3639

3639 void showLayoutTree(const blink::LayoutObject* object1,	3640 void showLayoutTree(const blink::LayoutObject* object1,

3640 const blink::LayoutObject* object2) {	3641 const blink::LayoutObject* object2) {

3641 if (object1) {	3642 if (object1) {

3642 const blink::LayoutObject* root = object1;	3643 const blink::LayoutObject* root = object1;

3643 while (root->Parent())	3644 while (root->Parent())

3644 root = root->Parent();	3645 root = root->Parent();

3645 root->ShowLayoutTreeAndMark(object1, "*", object2, "-", 0);	3646 root->ShowLayoutTreeAndMark(object1, "*", object2, "-", 0);

3646 } else {	3647 } else {

3647 WTFLogAlways("%s", "Cannot showLayoutTree. Root is (nil)");	3648 LOG(INFO) << "\n Cannot showLayoutTree. Root is (nil)";
	tkent 2017/06/14 23:41:39 The leading "\n" is unnecessary. The leading "\n" is unnecessary. mrunal 2017/06/19 22:10:57 Acknowledged. Show quoted text On 2017/06/14 23:41:39, tkent wrote: > The leading "\n" is unnecessary. Acknowledged.
3648 }	3649 }

3649 }	3650 }

3650	3651

3651 #endif	3652 #endif

OLD	NEW

« third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp ('K') | « third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp ('k') | no next file » | no next file with comments »