Bug 141634

Summary: Web Inspector: ES6: Improved Console Support for Promise Objects
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, burg, graouts, joepeck, jonowells, mattbaker, nvasilyev, ossy, rniwa, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 141746    
Bug Blocks:    
Attachments:
Description Flags
[PATCH] Work In Progress
none
[PATCH] Proposed Fix
timothy: review+, buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-mavericks
none
Archive of layout-test-results from ews105 for mac-mavericks-wk2 none

Description Joseph Pecoraro 2015-02-16 00:01:49 PST
* SUMMARY
Improved Console Support for Promise Objects.

Promises have internal state that would be useful to show in Inspector: (Table 59 — Internal Slots of Promise Instances)

[[PromiseState]]
  - A string value that governs how a promise will react to incoming calls to its then method.
  - The possible values are: undefined, "pending", "fulfilled", and "rejected".

[[PromiseResult]]
  - The value with which the promise has been fulfilled or rejected, if any.
  - Only meaningful if [[PromiseState]] is not "pending".

[[PromiseFulfillReactions]]
  - A List of PromiseReaction records to be processed when/if the promise transitions from the "pending" state to the "fulfilled" state.

[[PromiseRejectReactions]]
  - A List of PromiseReaction records to be processed when/if the promise transitions from the "pending" state to the "rejected" state.
Comment 1 Radar WebKit Bug Importer 2015-02-16 00:02:10 PST
<rdar://problem/19843138>
Comment 2 Joseph Pecoraro 2015-02-16 00:06:47 PST
Created attachment 246637 [details]
[PATCH] Work In Progress

Work in progress patch:
- First time we make sure of InjectedScriptHost.getInternalProperties
- Add a way to distinguish Internal property previews from regular properties
- Trivially supports [[PromiseState]] and [[PromiseResult]].

Whats next:
- show the chain of reactions next (the .thens, and .catches)
- discuss UI for Internal Properties in ObjectPreviews / ObjectTrees
Comment 3 Joseph Pecoraro 2015-02-16 13:47:01 PST
> [[PromiseFulfillReactions]]
>   - A List of PromiseReaction records to be processed when/if the promise
> transitions from the "pending" state to the "fulfilled" state.
> 
> [[PromiseRejectReactions]]
>   - A List of PromiseReaction records to be processed when/if the promise
> transitions from the "pending" state to the "rejected" state.

I will handle these states in:
<https://webkit.org/b/141664> Web Inspector: ES6: Improved Support for Promises - Promise Reactions

For now, lets just do state / value.
Comment 4 Joseph Pecoraro 2015-02-16 13:51:02 PST
Created attachment 246675 [details]
[PATCH] Proposed Fix
Comment 5 Build Bot 2015-02-16 14:19:00 PST
Comment on attachment 246675 [details]
[PATCH] Proposed Fix

Attachment 246675 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5867389241524224

New failing tests:
inspector-protocol/runtime/getProperties.html
Comment 6 Build Bot 2015-02-16 14:19:03 PST
Created attachment 246682 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 7 Build Bot 2015-02-16 14:27:54 PST
Comment on attachment 246675 [details]
[PATCH] Proposed Fix

Attachment 246675 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4929187995451392

New failing tests:
inspector-protocol/runtime/getProperties.html
Comment 8 Build Bot 2015-02-16 14:27:57 PST
Created attachment 246683 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 9 Joseph Pecoraro 2015-02-16 14:28:45 PST
Comment on attachment 246675 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/inspector/InjectedScript.cpp:139
> -    auto array = BindingTraits<Array<Inspector::Protocol::Runtime::InternalPropertyDescriptor>>::runtimeCast(WTF::move(result));
> -    *properties = array->length() > 0 ? array : nullptr;
> +    *properties = BindingTraits<Array<Inspector::Protocol::Runtime::InternalPropertyDescriptor>>::runtimeCast(WTF::move(result));

I should revert this part for:
spector-protocol/runtime/getProperties.html

I wanted to always include the array, but not including it if the array is empty will simplify the response.
Comment 10 Timothy Hatcher 2015-02-16 14:41:03 PST
Comment on attachment 246675 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:200
> +    result->putDirect(exec->vm(), Identifier(exec, "name"), jsString(exec, name));

jsNontrivialString and ASCIILiteral for name?
Comment 11 Joseph Pecoraro 2015-02-16 14:47:33 PST
Comment on attachment 246675 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:200
>> +    result->putDirect(exec->vm(), Identifier(exec, "name"), jsString(exec, name));
> 
> jsNontrivialString and ASCIILiteral for name?

jsNontrivialString - I debated it, and said no in case we ever have a single character string. (it asserts length > 1, not >= 1).
ASCIILiteral - I thought there was a jsString for char* but there isn't. I'll change the param to const String& and use ASCIILiteral above.
Comment 12 Joseph Pecoraro 2015-02-17 12:18:13 PST
http://trac.webkit.org/changeset/180235
Comment 13 Csaba Osztrogonác 2015-02-17 23:59:24 PST
(In reply to comment #12)
> http://trac.webkit.org/changeset/180235

It broke the !ENABLE(PROMISES) build, see bug141746 for details.