Bug 113357 – Web Inspector: gather accessor property getter and setter under a single tree node

• Status: RESOLVED
• Resolution: INVALID
• Component: Web Inspector (Deprecated)

[Peter Rybin]

Currently accessor property is visible as 2 separate get- and set- tree elements. Gather them under a single node. This node will probably host a calculated value subnode too.

[Peter Rybin]

Created attachment 195194 [details]
Patch

[Build Bot]

Comment on attachment 195194 [details]
Patch

Attachment 195194 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17327102

New failing tests:
inspector/runtime/runtime-getProperties.html
inspector/debugger/properties-special.html

[Build Bot]

Created attachment 195201 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[WebKit Review Bot]

Comment on attachment 195194 [details]
Patch

Attachment 195194 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17294291

New failing tests:
inspector/runtime/runtime-getProperties.html
inspector/debugger/properties-special.html

[WebKit Review Bot]

Created attachment 195203 [details]
Archive of layout-test-results from gce-cr-linux-01 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Build Bot]

Comment on attachment 195194 [details]
Patch

Attachment 195194 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17237594

New failing tests:
inspector/runtime/runtime-getProperties.html
inspector/debugger/properties-special.html

[Build Bot]

Created attachment 195321 [details]
Archive of layout-test-results from webkit-ews-04 for mac-future

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-future  Platform: Mac OS X 10.8.2

[Peter Rybin]

Created attachment 195447 [details]
Patch

[Build Bot]

Comment on attachment 195447 [details]
Patch

Attachment 195447 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17324225

New failing tests:
inspector/console/command-line-api.html

[Build Bot]

Created attachment 195457 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[WebKit Review Bot]

Comment on attachment 195447 [details]
Patch

Attachment 195447 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17257434

New failing tests:
inspector/console/command-line-api.html

[WebKit Review Bot]

Created attachment 195460 [details]
Archive of layout-test-results from gce-cr-linux-03 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Build Bot]

Comment on attachment 195447 [details]
Patch

Attachment 195447 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17216981

New failing tests:
inspector/console/command-line-api.html

[Build Bot]

Created attachment 195464 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[WebKit Review Bot]

Comment on attachment 195447 [details]
Patch

Attachment 195447 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17344034

New failing tests:
inspector/console/command-line-api.html

[WebKit Review Bot]

Created attachment 195466 [details]
Archive of layout-test-results from gce-cr-linux-06 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Yury Semikhatsky]

Comment on attachment 195447 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195447&action=review

Please attach screenshot.

> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:481
> +        if (properties[i].value == null)

== -> ===

> Source/WebCore/inspector/front-end/RemoteObject.js:218
>                  if (property.get || property.set) {

style: no {} around one liners
I'd rather move this get/set logic into constructor.

> Source/WebCore/inspector/front-end/RemoteObject.js:453
> +    if (descriptor && descriptor.get) {

style: no {} around one liners

> Source/WebCore/inspector/front-end/RemoteObject.js:456
> +    if (descriptor && descriptor.set) {

style: no {} around one liners

> LayoutTests/inspector/debugger/properties-special.html:71
> +                if (children[j].nameElement && children[j].nameElement.textContent === "__proto__") {

style: no {} around one liners

> LayoutTests/inspector/runtime/runtime-getProperties.html:63
> +    function convertPropertyValueForTest(propertyObj, fieldName)

propertyObj -> propertyObject

[Peter Rybin]

Created attachment 195575 [details]
Patch

[Peter Rybin]

Created attachment 195576 [details]
Screenshot

[Peter Rybin]

Created attachment 195577 [details]
Screenshot 2

[Timothy Hatcher]

Comment on attachment 195575 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195575&action=review

> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:621
> +        markerElement.textContent = "\u2388";

Why HELM SYMBOL?

[Peter Rybin]

> Why HELM SYMBOL?

I'm glad you someone asks this. I wanted a sing to distinguish accessor property from a normal one. The name is shown in italic, but I needed more explicit.

The symbol was chosen randomly. Maybe some kind of gear symbol would be more relevant.

Do you feel the helm is the wrong one? Do you think it would be wrong to commit (even temporary) such symbol into a repository?

[Timothy Hatcher]

(In reply to comment #22)
> > Why HELM SYMBOL?
> 
> I'm glad you someone asks this. I wanted a sing to distinguish accessor property from a normal one. The name is shown in italic, but I needed more explicit.
> 
> The symbol was chosen randomly. Maybe some kind of gear symbol would be more relevant.
> 
> Do you feel the helm is the wrong one? Do you think it would be wrong to commit (even temporary) such symbol into a repository?

I don't think a symbol is needed, and italics is enough. But I'm not driving the design of the WebKit Web Inspector anymore either. It is up to you guys.

[Peter Rybin]

Created attachment 195669 [details]
Patch

[Peter Rybin]

Comment on attachment 195447 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195447&action=review

>> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:481
>> +        if (properties[i].value == null)
> 
> == -> ===

Done

>> Source/WebCore/inspector/front-end/RemoteObject.js:218
>>                  if (property.get || property.set) {
> 
> style: no {} around one liners
> I'd rather move this get/set logic into constructor.

Done

>> Source/WebCore/inspector/front-end/RemoteObject.js:453
>> +    if (descriptor && descriptor.get) {
> 
> style: no {} around one liners

Done

>> Source/WebCore/inspector/front-end/RemoteObject.js:456
>> +    if (descriptor && descriptor.set) {
> 
> style: no {} around one liners

Done

>> LayoutTests/inspector/debugger/properties-special.html:71
>> +                if (children[j].nameElement && children[j].nameElement.textContent === "__proto__") {
> 
> style: no {} around one liners

Done

>> LayoutTests/inspector/runtime/runtime-getProperties.html:63
>> +    function convertPropertyValueForTest(propertyObj, fieldName)
> 
> propertyObj -> propertyObject

Done

[WebKit Review Bot]

Comment on attachment 195669 [details]
Patch

Clearing flags on attachment: 195669

Committed r147218: <http://trac.webkit.org/changeset/147218>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 113585

[Vsevolod Vlasov]

I have rolled this patch out.
I don't think we have an agreement on the way we want to implement properties inspection in UI.

I think property value should be accessible as a top level element.
It could also have "Show getter definition" and "Show setter definition" items in context menu.

In this case getter and setter could be kept inside prototype
Object
-- field: value
-- * property: propertyValue
-- __proto__
   -- someMethod: function() ...
   -- get property: function() ...
   -- set property: function() ...

If we move on with my suggestion we don't need this change at all, so we should decide first before landing this patch.

[Vsevolod Vlasov]

Also FYI:

<mlam> vsevik: http://build.webkit.org/builders/Apple%20Lion%20Release%20WK2%20%28Tests%29/builds/9398 may have caused http://build.webkit.org/results/Apple%20Lion%20Release%20WK2%20(Tests)/r147218%20(9398)/inspector/console/console-format-table-pretty-diff.html