Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2905733003:
Remove WTFLogAlways() usages from core/layout/
|
Created:
3 years, 7 months ago by mrunal Modified:
3 years, 3 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove WTFLogAlways() usages from core/layout/
In LayoutObject.cpp replace with LOG(INFO) instead.
In LayoutGeomteryMap.cpp remove the conditional #define LAYOUT_GEOMETRY_MAP_LOGGING
altogether as the code is unmaintainable.
BUG=638849
Patch Set 1 #
Total comments: 22
Messages
Total messages: 15 (7 generated)
The CQ bit was checked by mrunal.kapade@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove WTFLogAlways() usages from core/layout/ In LayoutObject.cpp replace with LOG(INFO) instead. In LayoutGeomteryMap.cpp remove the conditional #define LAYOUT_GEOMETRY_MAP_LOGGING altogether as the code is unmaintainable. BUG=638849 ========== to ========== Remove WTFLogAlways() usages from core/layout/ In LayoutObject.cpp replace with LOG(INFO) instead. In LayoutGeomteryMap.cpp remove the conditional #define LAYOUT_GEOMETRY_MAP_LOGGING altogether as the code is unmaintainable. BUG=638849 ==========
mrunal.kapade@intel.com changed reviewers: + fs@opera.com, tkent@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tkent/fs, can you PTAL?
https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp (left): https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp:33: #define LAYOUT_GEOMETRY_MAP_LOGGING 0 "...as the code is unmaintainable." I'm not sure I understand this statement. I don't see the "unmaintainable" bit here. I don't particularly mind removing this debugging code, but I also didn't add it... So, could we have a better reason for removing it? (And, tangentially, there's a typo LayoutGeomteryMap in the same sentence in the description.) https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1358: LOG(INFO) << "\n" Why an initial \n (WTFLogAlways always made sure that there was one at the end, but not a the beginning?) https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1359: << string_builder.ToString().Utf8().data() Don't we have an operator<< that would work here? (So just string_builder.ToString() and similarly for the Node.)
Please refer to Node.cpp. It has very similar tree-aware debug logging code. https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp (left): https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp:36: #define LAYOUT_GEOMETRY_MAP_LOG(...) WTFLogAlways(__VA_ARGS__) Such conditional log macros is usually replaced with DVLOG(). https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1358: LOG(INFO) << "\n" 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. https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:3626: LOG(INFO) << "\n Cannot showTree. Root is (nil)"; The leading "\n" is unnecessary. https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:3626: LOG(INFO) << "\n Cannot showTree. Root is (nil)"; The leading "\n" is unnecessary. https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:3633: LOG(INFO) << "\n Cannot showLineTree. Root is (nil)"; The leading "\n" is unnecessary. https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:3648: LOG(INFO) << "\n Cannot showLayoutTree. Root is (nil)"; The leading "\n" is unnecessary.
https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp (left): https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp:33: #define LAYOUT_GEOMETRY_MAP_LOGGING 0 On 2017/06/14 20:14:19, fs wrote: > "...as the code is unmaintainable." > > I'm not sure I understand this statement. I don't see the "unmaintainable" bit The unmaintainable bit is below where LAYOUT_GEOMETRY_MAP_LOG is used. The variable references in LAYOUT_GEOMETRY_MAP_LOG are not compile checked so if their definitions change over time it's hard to keep that code up to date. > here. I don't particularly mind removing this debugging code, but I also didn't > add it... So, could we have a better reason for removing it? (And, tangentially, > there's a typo LayoutGeomteryMap in the same sentence in the description.) I will correct the LayoutGeomteryMap.cpp to LayoutGeometryMap.cpp in the description. https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp:275: layoutObject, offsetFromContainer.width().toInt(), @fs, For example there is no definition for offsetFromContainer now as it has changed. Things like these are unmaintainable if it cannot be checked compile time. https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1358: LOG(INFO) << "\n" 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. https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1358: LOG(INFO) << "\n" 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? https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1359: << string_builder.ToString().Utf8().data() 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. https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:3626: LOG(INFO) << "\n Cannot showTree. Root is (nil)"; On 2017/06/14 23:41:39, tkent wrote: > The leading "\n" is unnecessary. Acknowledged. https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:3633: LOG(INFO) << "\n Cannot showLineTree. Root is (nil)"; On 2017/06/14 23:41:39, tkent wrote: > The leading "\n" is unnecessary. Acknowledged. https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:3648: LOG(INFO) << "\n Cannot showLayoutTree. Root is (nil)"; On 2017/06/14 23:41:39, tkent wrote: > The leading "\n" is unnecessary. Acknowledged.
https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1358: LOG(INFO) << "\n" 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). https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1359: << string_builder.ToString().Utf8().data() 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.
https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp (left): https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp:33: #define LAYOUT_GEOMETRY_MAP_LOGGING 0 On 2017/06/19 at 22:10:57, mrunal wrote: > On 2017/06/14 20:14:19, fs wrote: > > "...as the code is unmaintainable." > > > > I'm not sure I understand this statement. I don't see the "unmaintainable" bit > The unmaintainable bit is below where LAYOUT_GEOMETRY_MAP_LOG is used. The variable references in LAYOUT_GEOMETRY_MAP_LOG are not compile checked so if their definitions change over time it's hard to keep that code up to date. Yes, sure, but that is pretty much the burden of having this code checked in in the first place... https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1359: << string_builder.ToString().Utf8().data() 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.)
mstensho@opera.com changed reviewers: + mstensho@opera.com
https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1362: LOG(INFO) << "\n" << string_builder.ToString().Utf8().data(); 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?
On 2017/06/20 08:49:40, mstensho (USE GERRIT) wrote: > https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): > > https://codereview.chromium.org/2905733003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutObject.cpp:1362: LOG(INFO) << "\n" > << string_builder.ToString().Utf8().data(); > 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? @mrunal, any update on it ? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
