Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2971133002:
Add input element values to DOMSnapshot command (Closed)
|
Created:
3 years, 5 months ago by dvallet Modified:
3 years, 5 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd input element values to DOMSnapshot command
DomSnapshot is modified to populate NodeValue for textarea, input and option elements.
radio/checkbox and option elements are populated with a checked/selected string
BUG=546953
Review-Url: https://codereview.chromium.org/2971133002
Cr-Commit-Position: refs/heads/master@{#486541}
Committed: https://chromium.googlesource.com/chromium/src/+/537562c27b50ad655819d3648300027d1354d25b
Patch Set 1 #
Total comments: 8
Patch Set 2 : Modify tests to reflect new DomSnapshot input value functionality #Patch Set 3 : added additional fields to DomSnapshot::DOMNode #
Total comments: 6
Patch Set 4 : Always add input values #Patch Set 5 : Add input in separated div #Patch Set 6 : add width to input div #Patch Set 7 : Include Rebaseline #Patch Set 8 : add separate tests for input value #
Messages
Total messages: 56 (34 generated)
Description was changed from ========== First tentative version of adding input element values to DOMSnapshot BUG=546953 ========== to ========== Add input element values to DOMSnapshot command BUG=546953 ==========
dvallet@chromium.org changed reviewers: + alexclarke@chromium.org, eseckler@chromium.org, pfeldman@chromium.org
PTAL This is a first tentative version to populate NodeValue for textarea, input and option elements. radio/checkbox and option elements are populated with checked/selected, alternatively we could add extra fields. For more context, see http://g/ghost-rider/l14eiZEviMY Pending: modify layout tests
On 2017/07/06 07:21:08, dvallet wrote: > PTAL > > This is a first tentative version to populate NodeValue for textarea, input and > option elements. > > radio/checkbox and option elements are populated with checked/selected, > alternatively we could add extra fields. > > For more context, see http://g/ghost-rider/l14eiZEviMY > > Pending: modify layout tests Looks good. Yes, let's test this :)
https://codereview.chromium.org/2971133002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp (right): https://codereview.chromium.org/2971133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:237: value->setNodeValue("selected"); Nit: no need for {}.
https://codereview.chromium.org/2971133002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp (right): https://codereview.chromium.org/2971133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:215: HTMLTextAreaElement& text_area_element = toHTMLTextAreaElement(*element); Lets make all these references const. https://codereview.chromium.org/2971133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:222: String input_value = input_element.value(); You can probably use std::move here https://codereview.chromium.org/2971133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:226: input_value = "checked"; I believe the style guide wants us to use brackets for if then else statements.
Description was changed from ========== Add input element values to DOMSnapshot command BUG=546953 ========== to ========== Add input element values to DOMSnapshot command DomSnapshot is modified to populate NodeValue for textarea, input and option elements. radio/checkbox and option elements are populated with a checked/selected string BUG=546953 ==========
The CQ bit was checked by dvallet@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...
PTAL, tests added https://codereview.chromium.org/2971133002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp (right): https://codereview.chromium.org/2971133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:215: HTMLTextAreaElement& text_area_element = toHTMLTextAreaElement(*element); On 2017/07/06 at 07:48:18, alex clarke wrote: > Lets make all these references const. Done https://codereview.chromium.org/2971133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:222: String input_value = input_element.value(); On 2017/07/06 at 07:48:18, alex clarke wrote: > You can probably use std::move here I think it already does copy elision, If I add std::move I get: error: moving a temporary object prevents copy elision https://codereview.chromium.org/2971133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:226: input_value = "checked"; On 2017/07/06 at 07:48:18, alex clarke wrote: > I believe the style guide wants us to use brackets for if then else statements. Done https://codereview.chromium.org/2971133002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:237: value->setNodeValue("selected"); On 2017/07/06 at 07:41:39, Eric Seckler wrote: > Nit: no need for {}. Done
lgtm
nodeValue stands for https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeValue which is null by the DOM spec. We should have introduced auxProperties for the properties specific to nodeType, but ended up pasting them into the Node itself. So if we intend to follow the pattern, we should introduce inputValue, textValue, inputChecked and optionSelected in the node spec.
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 dvallet@chromium.org
Done. Thanks for the pointers!
The patchset sent to the CQ was uploaded after l-g-t-m from eseckler@chromium.org Link to the patchset: https://codereview.chromium.org/2971133002/#ps40001 (title: "added additional fields to DomSnapshot::DOMNode")
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 dvallet@chromium.org
The CQ bit was checked by dvallet@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_...)
lgtm % nits https://codereview.chromium.org/2971133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp (right): https://codereview.chromium.org/2971133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:217: if (!text_area_element.value().IsEmpty()) I think you should set it regardless, it can be important to know the empty input value. https://codereview.chromium.org/2971133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:224: value->setInputValue(input_element.value()); I would put it into the else branch and also not check for empty. https://codereview.chromium.org/2971133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2971133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:3380: { "name": "inputSelected", "type": "boolean", "optional": true, "description": "Only set for option elements, indicates if the element has been selected" }, I would name this optionSelected, because it is only valid for option element.
The CQ bit was checked by dvallet@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...
PTAL Layout tests seem to fail in Windows and Mac, e.g.: https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=db6a4d... Anyone has any pointers on how to solve this? https://codereview.chromium.org/2971133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp (right): https://codereview.chromium.org/2971133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:217: if (!text_area_element.value().IsEmpty()) On 2017/07/10 at 19:00:44, pfeldman wrote: > I think you should set it regardless, it can be important to know the empty input value. Done. https://codereview.chromium.org/2971133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:224: value->setInputValue(input_element.value()); On 2017/07/10 at 19:00:44, pfeldman wrote: > I would put it into the else branch and also not check for empty. Radio and checkbox elements can have also values associated, wouldn't we want to include this? E.g, <form> <input type="radio" name="gender" value="male" checked> Male<br> <input type="radio" name="gender" value="female"> Female<br> <input type="radio" name="gender" value="other"> Other </form> https://codereview.chromium.org/2971133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2971133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:3380: { "name": "inputSelected", "type": "boolean", "optional": true, "description": "Only set for option elements, indicates if the element has been selected" }, On 2017/07/10 at 19:00:44, pfeldman wrote: > I would name this optionSelected, because it is only valid for option element. Done
On 2017/07/11 00:26:07, dvallet wrote: > PTAL > > Layout tests seem to fail in Windows and Mac, e.g.: > https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=db6a4d... > > Anyone has any pointers on how to solve this? > Replace id with <number>. We have utilities that dump objects while replacing values for properties with certain names.
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 dvallet@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/07/11 at 01:43:43, pfeldman wrote: > On 2017/07/11 00:26:07, dvallet wrote: > > PTAL > > > > Layout tests seem to fail in Windows and Mac, e.g.: > > https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=db6a4d... > > > > Anyone has any pointers on how to solve this? > > > > Replace id with <number>. We have utilities that dump objects while replacing values for properties with certain names. Looks like it's due to boundingBox difference in the layoutTreeNodes response, I don't think we should be using <number> in that case, correct? https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=537ce3... @eseckler: Did you find this problem when submitting this test? Looks like by just adding an extra element we have layout disparity on Mac/Linux/Windows Perhaps should I separate this test from the current one?
On 2017/07/11 06:03:13, dvallet wrote: > On 2017/07/11 at 01:43:43, pfeldman wrote: > > On 2017/07/11 00:26:07, dvallet wrote: > > > PTAL > > > > > > Layout tests seem to fail in Windows and Mac, e.g.: > > > > https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=db6a4d... > > > > > > Anyone has any pointers on how to solve this? > > > > > > > Replace id with <number>. We have utilities that dump objects while replacing > values for properties with certain names. > > Looks like it's due to boundingBox difference in the layoutTreeNodes response, I > don't think we should be using <number> in that case, correct? > https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=537ce3... > > @eseckler: Did you find this problem when submitting this test? Looks like by > just adding an extra element we have layout disparity on Mac/Linux/Windows > > Perhaps should I separate this test from the current one? I didn't have this problem, no. Sounds like >=1 of your new input elements render differently depending on the OS. Maybe you can add some CSS styles to them to avoid the different rendering (e.g. set a border, width, height, ..)? Alternatively, you could also generate different expectations for different OSs, e.g. using try job rebaselining: https://chromium.googlesource.com/chromium/src/+/master/docs/testing/layout_t... I don't think you need to separate the test.
The CQ bit was checked by dvallet@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...
PTAL (rebaseline) I opted to do the rebaseline since I couldn't make it work cross platform by playing with the style, and all the examples I found seems to do the same.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Please don't create different expectations for platforms. That's not what you are testing. Replace coordinates with <number> in your test instead.
The CQ bit was checked by dvallet@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...
On 2017/07/12 at 21:02:29, pfeldman wrote: > Please don't create different expectations for platforms. That's not what you are testing. Replace coordinates with <number> in your test instead. It's not that easy, unfortunately, since also different computedStyles and some values are different, so adding those as unstableKeys would defeat the purpose of the original tests. I've uploaded what I think is the best solution: add a separate tests just for values that does not compare the layoutTreeNode nor the computedStyles. wdyt?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
If nobody has anything against it, I'll submit the patch
lgtm
The CQ bit was checked by dvallet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eseckler@chromium.org Link to the patchset: https://codereview.chromium.org/2971133002/#ps140001 (title: "add separate tests for input value")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1499988912391140,
"parent_rev": "c4d2973dd1dcaeaee85e6e48b9e949519268d115", "commit_rev":
"537562c27b50ad655819d3648300027d1354d25b"}
Message was sent while issue was closed.
Description was changed from ========== Add input element values to DOMSnapshot command DomSnapshot is modified to populate NodeValue for textarea, input and option elements. radio/checkbox and option elements are populated with a checked/selected string BUG=546953 ========== to ========== Add input element values to DOMSnapshot command DomSnapshot is modified to populate NodeValue for textarea, input and option elements. radio/checkbox and option elements are populated with a checked/selected string BUG=546953 Review-Url: https://codereview.chromium.org/2971133002 Cr-Commit-Position: refs/heads/master@{#486541} Committed: https://chromium.googlesource.com/chromium/src/+/537562c27b50ad655819d3648300... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/537562c27b50ad655819d3648300... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
