pfeldman@chromium.org changed reviewers: + lushnikov@chromium.org
PTAL or I add more :)
lgtm https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... File Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:380: m_cssRule = nullptr; FYI: this looks shaggy as this might kill the "result" CSSStyleRule; though I understand that this doesn't happen due to a particular way you call this method from the outside. https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... File Source/core/inspector/InspectorStyleSheet.cpp (right): https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorStyleSheet.cpp:1106: stray line https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorStyleSheet.cpp:1120: stray line https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorStyleSheet.cpp:1250: CSSRule* rule = nullptr; RefPtr<CSSRule> rule; why do you want this to be a raw pointer? https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorStyleSheet.cpp:1251: unsigned containingRuleLength = 0; .. = UCHAR_MAX; to make it clear that you minimize over this parameter. https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorStyleSheet.cpp:1253: String styleSheetText = m_parsedStyleSheet->text(); unused? https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorStyleSheet.cpp:1261: if (startBelongs != endBelongs) The two if-statements could be replaced with if (!startBelongs || !endBelongs) continue;
https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... File Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:380: m_cssRule = nullptr; On 2015/06/17 09:56:22, lushnikov wrote: > FYI: this looks shaggy as this might kill the "result" CSSStyleRule; though I > understand that > this doesn't happen due to a particular way you call this method from the > outside. Done. https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... File Source/core/inspector/InspectorStyleSheet.cpp (right): https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorStyleSheet.cpp:1106: On 2015/06/17 09:56:22, lushnikov wrote: > stray line Done. https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorStyleSheet.cpp:1120: On 2015/06/17 09:56:22, lushnikov wrote: > stray line Acknowledged. https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorStyleSheet.cpp:1250: CSSRule* rule = nullptr; On 2015/06/17 09:56:23, lushnikov wrote: > RefPtr<CSSRule> rule; > > why do you want this to be a raw pointer? Done. https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorStyleSheet.cpp:1251: unsigned containingRuleLength = 0; On 2015/06/17 09:56:22, lushnikov wrote: > .. = UCHAR_MAX; > > to make it clear that you minimize over this parameter. I was consistent with your code below. https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorStyleSheet.cpp:1253: String styleSheetText = m_parsedStyleSheet->text(); On 2015/06/17 09:56:22, lushnikov wrote: > unused? Done. https://codereview.chromium.org/1179323003/diff/80001/Source/core/inspector/I... Source/core/inspector/InspectorStyleSheet.cpp:1261: if (startBelongs != endBelongs) On 2015/06/17 09:56:22, lushnikov wrote: > The two if-statements could be replaced with > > > if (!startBelongs || !endBelongs) > continue; Can't do.
The CQ bit was checked by pfeldman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/1179323003/#ps100001 (title: "review comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1179323003/100001
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197261
Chromium Code Reviews
