Bug 113357 - Web Inspector: gather accessor property getter and setter under a single tree node
Summary: Web Inspector: gather accessor property getter and setter under a single tree...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 113585
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-26 17:12 PDT by Peter Rybin
Modified: 2014-12-13 13:11 PST (History)
15 users (show)

See Also:


Attachments
Patch (6.68 KB, patch)
2013-03-26 17:19 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (619.99 KB, application/zip)
2013-03-26 17:52 PDT, Build Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-01 for chromium-linux-x86_64 (1.33 MB, application/zip)
2013-03-26 18:10 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-future (659.99 KB, application/zip)
2013-03-27 07:41 PDT, Build Bot
no flags Details
Patch (16.33 KB, patch)
2013-03-27 17:55 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (511.29 KB, application/zip)
2013-03-27 19:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-03 for chromium-linux-x86_64 (792.59 KB, application/zip)
2013-03-27 19:11 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (869.28 KB, application/zip)
2013-03-27 19:48 PDT, Build Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-06 for chromium-linux-x86_64 (1.30 MB, application/zip)
2013-03-27 19:52 PDT, WebKit Review Bot
no flags Details
Patch (17.03 KB, patch)
2013-03-28 08:33 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff
Screenshot (14.03 KB, image/png)
2013-03-28 08:38 PDT, Peter Rybin
no flags Details
Screenshot 2 (97.16 KB, image/png)
2013-03-28 08:39 PDT, Peter Rybin
no flags Details
Patch (17.16 KB, patch)
2013-03-28 16:14 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Rybin 2013-03-26 17:12:54 PDT
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.
Comment 1 Peter Rybin 2013-03-26 17:19:28 PDT
Created attachment 195194 [details]
Patch
Comment 2 Build Bot 2013-03-26 17:52:07 PDT
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
Comment 3 Build Bot 2013-03-26 17:52:08 PDT
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
Comment 4 WebKit Review Bot 2013-03-26 18:10:15 PDT
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
Comment 5 WebKit Review Bot 2013-03-26 18:10:19 PDT
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
Comment 6 Build Bot 2013-03-27 07:41:42 PDT
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
Comment 7 Build Bot 2013-03-27 07:41:45 PDT
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
Comment 8 Peter Rybin 2013-03-27 17:55:18 PDT
Created attachment 195447 [details]
Patch
Comment 9 Build Bot 2013-03-27 19:00:39 PDT
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
Comment 10 Build Bot 2013-03-27 19:00:44 PDT
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
Comment 11 WebKit Review Bot 2013-03-27 19:11:08 PDT
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
Comment 12 WebKit Review Bot 2013-03-27 19:11:13 PDT
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
Comment 13 Build Bot 2013-03-27 19:48:24 PDT
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
Comment 14 Build Bot 2013-03-27 19:48:27 PDT
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
Comment 15 WebKit Review Bot 2013-03-27 19:52:52 PDT
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
Comment 16 WebKit Review Bot 2013-03-27 19:52:57 PDT
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
Comment 17 Yury Semikhatsky 2013-03-28 07:13:18 PDT
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
Comment 18 Peter Rybin 2013-03-28 08:33:18 PDT
Created attachment 195575 [details]
Patch
Comment 19 Peter Rybin 2013-03-28 08:38:31 PDT
Created attachment 195576 [details]
Screenshot
Comment 20 Peter Rybin 2013-03-28 08:39:35 PDT
Created attachment 195577 [details]
Screenshot 2
Comment 21 Timothy Hatcher 2013-03-28 09:56:30 PDT
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?
Comment 22 Peter Rybin 2013-03-28 12:13:26 PDT
> 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?
Comment 23 Timothy Hatcher 2013-03-28 16:03:40 PDT
(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.
Comment 24 Peter Rybin 2013-03-28 16:14:56 PDT
Created attachment 195669 [details]
Patch
Comment 25 Peter Rybin 2013-03-28 16:18:49 PDT
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
Comment 26 WebKit Review Bot 2013-03-29 08:00:34 PDT
Comment on attachment 195669 [details]
Patch

Clearing flags on attachment: 195669

Committed r147218: <http://trac.webkit.org/changeset/147218>
Comment 27 WebKit Review Bot 2013-03-29 08:00:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 WebKit Review Bot 2013-03-29 08:15:44 PDT
Re-opened since this is blocked by bug 113585
Comment 29 Vsevolod Vlasov 2013-03-29 08:48:35 PDT
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.