Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2413693002:
Rename DOM.getLayoutTreeNodes to CSS.getLayoutTreeAndStyles (Closed)
|
Created:
4 years, 2 months ago by alex clarke (OOO till 29th) Modified:
4 years, 2 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCSS.getLayoutTreeAndStyles will also return whitelisted computed styles.
This functionality is needed by headless chrome.
BUG=546953
Committed: https://crrev.com/00fb6e725b5a430daf992402acba94be93aa0412
Cr-Commit-Position: refs/heads/master@{#426187}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Changes for dgozman #Patch Set 3 : rebased #
Total comments: 13
Patch Set 4 : More changes for dgozman #Patch Set 5 : Fix iframe path and drop font-family from test since it's different on mac #Messages
Total messages: 40 (27 generated)
The CQ bit was checked by alexclarke@chromium.org 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...
alexclarke@chromium.org changed reviewers: + dgozman@chromium.org, pfeldman@chromium.org, skyostil@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:612: new protocol::Array<protocol::DOM::ComputedStyle>(std::move(styles))); Can just use addItem above, directly constructing the protocol::Array instead of using intermediate vector. https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:626: style.push_back( Since all the keys in all ComputedStyleArray instances are the same, I'd: - convert style_whitelist to array of CSSPropertyID beforehand; - have a map<vector<String>, int> as a cache; - append keys when constructing result. https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.h (right): https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.h:370: protocol::Array<protocol::DOM::LayoutTreeNode>& layout_tree_nodes, blink uses camelCaseNames for now. https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/browser_protocol.json:2211: "name": "getLayoutTreeNodes", I'd rename this to getLayoutTreeAndStyles and move it to CSS, and reuse types from CSS. https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/inspector_protocol/lib/Array_h.template (right): https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/inspector_protocol/lib/Array_h.template:45: Array() {} Why not Array::create() defined above? https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/inspector_protocol/lib/Array_h.template:46: explicit Array(std::vector<std::unique_ptr<T>> vector) : m_vector(std::move(vector)) { } Let's do static std::unique_ptr<Array<T>> fromVector(...) instead. https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/inspector_protocol/templates/TypeBuilder_h.template (right): https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/inspector_protocol/templates/TypeBuilder_h.template:94: const {{resolve_type(property).return_type}}& get{{property.name | to_title_case}}() const { return {{"m_" + property.name}}; } Could you please describe why this change is needed? Note this is now reused in v8's code, so similar .get() fixes will be needed there.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:612: new protocol::Array<protocol::DOM::ComputedStyle>(std::move(styles))); On 2016/10/12 21:00:40, dgozman wrote: > Can just use addItem above, directly constructing the protocol::Array instead of > using intermediate vector. The problem with that is: for (ComputedStylesMap::value_type& pair : style_to_index_map) { pair.second // this is not sorted so we can't use addItem } https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:626: style.push_back( On 2016/10/12 21:00:40, dgozman wrote: > Since all the keys in all ComputedStyleArray instances are the same, I'd: > - convert style_whitelist to array of CSSPropertyID beforehand; > - have a map<vector<String>, int> as a cache; > - append keys when constructing result. Right, in the spirit of that it seems simplest to pass around the whitelist as Vector<std::pair<String, CSSPropertyID>> https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.h (right): https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.h:370: protocol::Array<protocol::DOM::LayoutTreeNode>& layout_tree_nodes, On 2016/10/12 21:00:40, dgozman wrote: > blink uses camelCaseNames for now. OK :( https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/browser_protocol.json:2211: "name": "getLayoutTreeNodes", On 2016/10/12 21:00:40, dgozman wrote: > I'd rename this to getLayoutTreeAndStyles and move it to CSS, and reuse types > from CSS. Done. https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/inspector_protocol/lib/Array_h.template (right): https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/inspector_protocol/lib/Array_h.template:45: Array() {} On 2016/10/12 21:00:40, dgozman wrote: > Why not Array::create() defined above? I'm assuming you don't want this to be public? https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/inspector_protocol/lib/Array_h.template:46: explicit Array(std::vector<std::unique_ptr<T>> vector) : m_vector(std::move(vector)) { } On 2016/10/12 21:00:40, dgozman wrote: > Let's do > static std::unique_ptr<Array<T>> fromVector(...) > instead. Done. https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/inspector_protocol/templates/TypeBuilder_h.template (right): https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/inspector_protocol/templates/TypeBuilder_h.template:94: const {{resolve_type(property).return_type}}& get{{property.name | to_title_case}}() const { return {{"m_" + property.name}}; } On 2016/10/12 21:00:40, dgozman wrote: > Could you please describe why this change is needed? > > Note this is now reused in v8's code, so similar .get() fixes will be needed > there. Before my change it returned the object by value incurring a copy. That's fine when it's an int, not so nice when it's a string. I appreciate the complexity with the default value in the clause above. It's not nice that one returns a reference where the other doesn't. It may be possible to make both return a reference -- I didn't try very hard. Possible alternatives: 1. Add a new method which returns the reference 2. define bool operator<(...) (so you can put it in a std::map). Probably too complex
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by alexclarke@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/inspector_protocol/lib/Array_h.template (right): https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/inspector_protocol/lib/Array_h.template:45: Array() {} On 2016/10/13 20:25:10, alex clarke wrote: > On 2016/10/12 21:00:40, dgozman wrote: > > Why not Array::create() defined above? > > I'm assuming you don't want this to be public? Yes. https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/inspector_protocol/templates/TypeBuilder_h.template (right): https://codereview.chromium.org/2413693002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/inspector_protocol/templates/TypeBuilder_h.template:94: const {{resolve_type(property).return_type}}& get{{property.name | to_title_case}}() const { return {{"m_" + property.name}}; } On 2016/10/13 20:25:10, alex clarke wrote: > On 2016/10/12 21:00:40, dgozman wrote: > > Could you please describe why this change is needed? > > > > Note this is now reused in v8's code, so similar .get() fixes will be needed > > there. > > Before my change it returned the object by value incurring a copy. That's fine > when it's an int, not so nice when it's a string. > > I appreciate the complexity with the default value in the clause above. It's > not nice that one returns a reference where the other doesn't. It may be > possible to make both return a reference -- I didn't try very hard. > > Possible alternatives: > > 1. Add a new method which returns the reference > 2. define bool operator<(...) (so you can put it in a std::map). Probably too > complex - Note that Strings are cheap in blink - they are shallow wrappers around refcounted buffer. - I don't like two different return types depending on the optionality of the field. https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2380: for (ComputedStylesMap::value_type& pair : styleToIndexMap) { nit: I'd suggest to pass ComputedStylesArray along the ComputedStylesMap and insert into it in getStyleIndexForNode, so that we have them ordered already. https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2403: style.push_back( We can push_back just a value (String), and convert them to protocol:: structure before sending response. I believe this allows to remove hash and equal functions entirely. https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2405: .setName(pair.first) This also allows cssPrpoertyWhitelist to only have CSSPropertyID instead of a pair. https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h (right): https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:324: using ComputedStyleArray = std::vector< Sorry for pointing this late, but I think we should use WTF::Vector in blink. https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:327: using ComputedStylesMap = std::unordered_map<ComputedStyleArray, int>; Same goes for WTF::HashMap. https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:2904: "experimental": true nit: this whole domain is experimental, no need to mark entities as such. https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/inspector_protocol/lib/Array_h.template (right): https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/inspector_protocol/lib/Array_h.template:50: nit: extra blank line
PTAL https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2380: for (ComputedStylesMap::value_type& pair : styleToIndexMap) { On 2016/10/14 01:32:33, dgozman wrote: > nit: I'd suggest to pass ComputedStylesArray along the ComputedStylesMap and > insert into it in getStyleIndexForNode, so that we have them ordered already. Done. https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2403: style.push_back( On 2016/10/14 01:32:33, dgozman wrote: > We can push_back just a value (String), and convert them to protocol:: structure > before sending response. Done. I believe this allows to remove hash and equal > functions entirely. Well I had to implement the WTF equivalent since we need to de-dupe the computed style. https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h (right): https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:324: using ComputedStyleArray = std::vector< On 2016/10/14 01:32:33, dgozman wrote: > Sorry for pointing this late, but I think we should use WTF::Vector in blink. Done. https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:327: using ComputedStylesMap = std::unordered_map<ComputedStyleArray, int>; On 2016/10/14 01:32:33, dgozman wrote: > Same goes for WTF::HashMap. That was surprisingly hard. Done. https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/inspector_protocol/lib/Array_h.template (right): https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/inspector_protocol/lib/Array_h.template:50: On 2016/10/14 01:32:34, dgozman wrote: > nit: extra blank line Acknowledged.
The CQ bit was checked by alexclarke@chromium.org 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 ========== DOM.getLayoutTreeNodes to also return whitelisted computed styles This functionality is needed by headless chrome. BUG=546953 ========== to ========== CSS.getLayoutTreeAndStyles will also return whitelisted computed styles. This functionality is needed by headless chrome. BUG=546953 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by alexclarke@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL :)
Ping :)
Sorry for delay! lgtm https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/2413693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2403: style.push_back( > I believe this allows to remove hash and equal > > functions entirely. > Well I had to implement the WTF equivalent since we need to de-dupe the computed > style. That's unfortunate... I thought WTF would have a default hash for vector.
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== CSS.getLayoutTreeAndStyles will also return whitelisted computed styles. This functionality is needed by headless chrome. BUG=546953 ========== to ========== CSS.getLayoutTreeAndStyles will also return whitelisted computed styles. This functionality is needed by headless chrome. BUG=546953 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== CSS.getLayoutTreeAndStyles will also return whitelisted computed styles. This functionality is needed by headless chrome. BUG=546953 ========== to ========== CSS.getLayoutTreeAndStyles will also return whitelisted computed styles. This functionality is needed by headless chrome. BUG=546953 Committed: https://crrev.com/00fb6e725b5a430daf992402acba94be93aa0412 Cr-Commit-Position: refs/heads/master@{#426187} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/00fb6e725b5a430daf992402acba94be93aa0412 Cr-Commit-Position: refs/heads/master@{#426187} |
