Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1310923003:
Devtools [LayoutEditor]: Patch values in the selected rule (Closed)
|
Created:
5 years, 3 months ago by sergeyv Modified:
5 years, 3 months ago CC:
blink-reviews, caseq+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@medias Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDevtools [LayoutEditor]: Patch values in the selected rule
BUG=501896
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201833
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 19
Patch Set 3 : Address comments #
Total comments: 10
Patch Set 4 : Address comments #
Total comments: 5
Patch Set 5 : Address comments #Patch Set 6 : Rebase #Patch Set 7 : Fix win compilation #Patch Set 8 : #
Messages
Total messages: 36 (15 generated)
sergeyv@chromium.org changed reviewers: + dgozman@chromium.org, pfeldman@chromium.org
dgozman@chromium.org changed reviewers: + lushnikov@chromium.org
https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... File Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1518: PassRefPtrWillBeRawPtr<CSSStyleDeclaration> InspectorCSSAgent::findEffectiveDeclaration(Element* element, CSSPropertyID propertyId) Can we kill this function? https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1521: return findEffectiveDeclaration(propertyId, ruleList.get(), element->pseudoId() ? nullptr : element->style()); Can we always call element->style()? https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1607: if (style->parentRule()) { Let's comment that this means inline style. https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1692: void InspectorCSSAgent::setCSSPropertyValueInRule(ErrorString* errorString, Element* element, CSSStyleRule* rule, CSSPropertyID propertyId, const String& value) This one can move to layout editor. https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... File Source/core/inspector/InspectorCSSAgent.h (right): https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.h:149: void setCSSPropertyValueInRule(ErrorString*, Element*, CSSStyleRule*, CSSPropertyID, const String&); const String& value https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.h:202: PassRefPtrWillBeRawPtr<CSSStyleDeclaration> findEffectiveDeclaration(CSSPropertyID, CSSRuleList*, CSSStyleDeclaration*); CSSRuleList* matchedRules, CSSStyleDeclaration* inlineStyle https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/L... File Source/core/inspector/LayoutEditor.cpp (right): https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/L... Source/core/inspector/LayoutEditor.cpp:296: m_cssAgent->setCSSPropertyValueInRule(&errorString, m_element.get(), m_currentRuleIndex == -1 ? nullptr : m_matchedRules[m_currentRuleIndex].get(), m_changingProperty, truncateZeroes(String::format("%.2f", newValue)) + CSSPrimitiveValue::unitTypeToString(m_valueUnitType)); I don't like nullptr as an indicator of inline style.
css lgtm https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... File Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1583: void InspectorCSSAgent::setCSSPropertyValue(ErrorString* errorString, Element* element, CSSPropertyID propertyId, const String& value) can we rename? there are 3 setCSSPropertyValue.. methods https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1698: if (effectiveDeclaration->parentRule() && effectiveDeclaration->parentRule()->type() == CSSRule::STYLE_RULE) effectiveDeclaration &&
https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... File Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1583: void InspectorCSSAgent::setCSSPropertyValue(ErrorString* errorString, Element* element, CSSPropertyID propertyId, const String& value) Inline this into it's single usage.
https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... File Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1518: PassRefPtrWillBeRawPtr<CSSStyleDeclaration> InspectorCSSAgent::findEffectiveDeclaration(Element* element, CSSPropertyID propertyId) On 2015/09/02 22:26:16, dgozman wrote: > Can we kill this function? Done. https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1521: return findEffectiveDeclaration(propertyId, ruleList.get(), element->pseudoId() ? nullptr : element->style()); On 2015/09/02 22:26:17, dgozman wrote: > Can we always call element->style()? Done. https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1583: void InspectorCSSAgent::setCSSPropertyValue(ErrorString* errorString, Element* element, CSSPropertyID propertyId, const String& value) On 2015/09/02 23:03:11, dgozman wrote: > Inline this into it's single usage. Done. https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1607: if (style->parentRule()) { On 2015/09/02 22:26:16, dgozman wrote: > Let's comment that this means inline style. Done. https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1692: void InspectorCSSAgent::setCSSPropertyValueInRule(ErrorString* errorString, Element* element, CSSStyleRule* rule, CSSPropertyID propertyId, const String& value) On 2015/09/02 22:26:16, dgozman wrote: > This one can move to layout editor. Done. https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1698: if (effectiveDeclaration->parentRule() && effectiveDeclaration->parentRule()->type() == CSSRule::STYLE_RULE) On 2015/09/02 23:00:43, lushnikov wrote: > effectiveDeclaration && Done. https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... File Source/core/inspector/InspectorCSSAgent.h (right): https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.h:149: void setCSSPropertyValueInRule(ErrorString*, Element*, CSSStyleRule*, CSSPropertyID, const String&); On 2015/09/02 22:26:17, dgozman wrote: > const String& value Done. https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.h:202: PassRefPtrWillBeRawPtr<CSSStyleDeclaration> findEffectiveDeclaration(CSSPropertyID, CSSRuleList*, CSSStyleDeclaration*); On 2015/09/02 22:26:17, dgozman wrote: > CSSRuleList* matchedRules, CSSStyleDeclaration* inlineStyle Done. https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/L... File Source/core/inspector/LayoutEditor.cpp (right): https://codereview.chromium.org/1310923003/diff/20001/Source/core/inspector/L... Source/core/inspector/LayoutEditor.cpp:296: m_cssAgent->setCSSPropertyValueInRule(&errorString, m_element.get(), m_currentRuleIndex == -1 ? nullptr : m_matchedRules[m_currentRuleIndex].get(), m_changingProperty, truncateZeroes(String::format("%.2f", newValue)) + CSSPrimitiveValue::unitTypeToString(m_valueUnitType)); On 2015/09/02 22:26:17, dgozman wrote: > I don't like nullptr as an indicator of inline style. Done.
https://codereview.chromium.org/1310923003/diff/40001/Source/core/inspector/I... File Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/1310923003/diff/40001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1577: void InspectorCSSAgent::setCSSPropertyValue(ErrorString* errorString, Element* element, CSSStyleDeclaration* style, CSSPropertyID propertyId, const String& value, bool forceImportant) RefPtrWillBeRawPtr<CSSStyleDeclaration> https://codereview.chromium.org/1310923003/diff/40001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1583: InspectorStyleSheet* styleSheet = bindStyleSheet(style->parentStyleSheet()); nit: two spaces https://codereview.chromium.org/1310923003/diff/40001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1655: Element* element = elementForId(errorString, nodeId); TODO: move testing from CSSAgent to layout editor. https://codereview.chromium.org/1310923003/diff/40001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1672: CSSStyleDeclaration* inlineStyle = element->style(); nit: two spaces https://codereview.chromium.org/1310923003/diff/40001/Source/core/inspector/L... File Source/core/inspector/LayoutEditor.cpp (right): https://codereview.chromium.org/1310923003/diff/40001/Source/core/inspector/L... Source/core/inspector/LayoutEditor.cpp:479: CSSStyleRule* rule = m_currentRuleIndex == -1 ? nullptr : m_matchedRules[m_currentRuleIndex].get(); RefPtrWil....<CSSStyleRule>
https://codereview.chromium.org/1310923003/diff/40001/Source/core/inspector/I... File Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/1310923003/diff/40001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1577: void InspectorCSSAgent::setCSSPropertyValue(ErrorString* errorString, Element* element, CSSStyleDeclaration* style, CSSPropertyID propertyId, const String& value, bool forceImportant) On 2015/09/03 20:42:14, dgozman wrote: > RefPtrWillBeRawPtr<CSSStyleDeclaration> Done. https://codereview.chromium.org/1310923003/diff/40001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1583: InspectorStyleSheet* styleSheet = bindStyleSheet(style->parentStyleSheet()); On 2015/09/03 20:42:14, dgozman wrote: > nit: two spaces Done. https://codereview.chromium.org/1310923003/diff/40001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1655: Element* element = elementForId(errorString, nodeId); On 2015/09/03 20:42:14, dgozman wrote: > TODO: move testing from CSSAgent to layout editor. Done. https://codereview.chromium.org/1310923003/diff/40001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:1672: CSSStyleDeclaration* inlineStyle = element->style(); On 2015/09/03 20:42:14, dgozman wrote: > nit: two spaces Done. https://codereview.chromium.org/1310923003/diff/40001/Source/core/inspector/L... File Source/core/inspector/LayoutEditor.cpp (right): https://codereview.chromium.org/1310923003/diff/40001/Source/core/inspector/L... Source/core/inspector/LayoutEditor.cpp:479: CSSStyleRule* rule = m_currentRuleIndex == -1 ? nullptr : m_matchedRules[m_currentRuleIndex].get(); On 2015/09/03 20:42:14, dgozman wrote: > RefPtrWil....<CSSStyleRule> Done.
https://codereview.chromium.org/1310923003/diff/60001/Source/core/inspector/I... File Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/1310923003/diff/60001/Source/core/inspector/I... Source/core/inspector/InspectorCSSAgent.cpp:113: uniqRules->rules().prepend(styleRule); Drop the n^2. https://codereview.chromium.org/1310923003/diff/60001/Source/core/inspector/L... File Source/core/inspector/LayoutEditor.cpp (right): https://codereview.chromium.org/1310923003/diff/60001/Source/core/inspector/L... Source/core/inspector/LayoutEditor.cpp:450: unsigned size = m_matchedRules ? m_matchedRules->length() : 0; Let's make it non-null. https://codereview.chromium.org/1310923003/diff/60001/Source/core/inspector/L... Source/core/inspector/LayoutEditor.cpp:452: if (effectiveDeclaration) { Let's have a single |if (effectiveDeclaration)|.
https://codereview.chromium.org/1310923003/diff/60001/Source/core/inspector/L... File Source/core/inspector/LayoutEditor.cpp (right): https://codereview.chromium.org/1310923003/diff/60001/Source/core/inspector/L... Source/core/inspector/LayoutEditor.cpp:450: unsigned size = m_matchedRules ? m_matchedRules->length() : 0; On 2015/09/03 23:04:20, dgozman wrote: > Let's make it non-null. Done. https://codereview.chromium.org/1310923003/diff/60001/Source/core/inspector/L... Source/core/inspector/LayoutEditor.cpp:452: if (effectiveDeclaration) { On 2015/09/03 23:04:20, dgozman wrote: > Let's have a single |if (effectiveDeclaration)|. Done.
lgtm
The CQ bit was checked by dgozman@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/1310923003/#ps80001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310923003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310923003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) 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_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sergeyv@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org, lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/1310923003/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310923003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310923003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sergeyv@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org, lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/1310923003/#ps120001 (title: "Fix win compilation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310923003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310923003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sergeyv@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310923003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310923003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sergeyv@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org, lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/1310923003/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310923003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310923003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201833
Message was sent while issue was closed.
On 2015/09/04 at 23:47:12, commit-bot wrote: > Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201833 Just fyi, this has caused failures on the oilpan bots: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... No need to roll out. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
