Bug 157307 – DOMPromise should only restrict the resolution type

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Restricting the error type is not efficient as some promises may reject with exception codes (int) or some dedicated errors.

[youenn fablet]

Created attachment 278488 [details]
Patch

[youenn fablet]

(In reply to comment #1)
> Created attachment 278488 [details]
> Patch

Patch is removing the DOMPromise rejection type restriction.

It also tries to remove some limitations of the resolve/reject methods.
Currently these methods take const references as parameter.
For DOM objects, it may be better to pass either plain references or Ref/RefPtr rvalues.

[WebKit Commit Bot]

Comment on attachment 278488 [details]
Patch

Rejecting attachment 278488 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
Volumes/Data/EWS/WebKit/Source/WebCore/loader/archive/ArchiveFactory.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/ArchiveFactory.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AccessibilityMediaControls.o accessibility/AccessibilityMediaControls.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/1302453

[youenn fablet]

Created attachment 278719 [details]
Patch for landing

[youenn fablet]

Comment on attachment 278719 [details]
Patch for landing

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

> Source/WebCore/bindings/js/JSDOMBinding.h:544
> +template<typename T> inline JSC::JSValue toJS(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, RefPtr<T>& ptr)

I hesitated on adding this one.
Ref<T> defines its cast to T&, but RefPtr<T> is not doing that for T*.
Is there a reason for that?

[WebKit Commit Bot]

Comment on attachment 278719 [details]
Patch for landing

Clearing flags on attachment: 278719

Committed r200766: <http://trac.webkit.org/changeset/200766>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 157623

[youenn fablet]

Created attachment 278824 [details]
Patch for landing - updating Ref/RefPtr detection

[youenn fablet]

Created attachment 278828 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 278828 [details]
Patch for landing

Rejecting attachment 278828 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 278828, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WTF/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/1314216

[youenn fablet]

Created attachment 278829 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 278829 [details]
Patch for landing

Clearing flags on attachment: 278829

Committed r200837: <http://trac.webkit.org/changeset/200837>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Last patch updated the way to detect Ref and RefPtr.
To make Yosemite build happy, I added Ref::isRef and RefPtr::isRefPtr constexpr.
The test is split in two tests (one for Ref and one for RefPtr) to make win bot happy.

I am not sure why Ref has a cast to T&, but RefPtr does not have for T*.

[Darin Adler]

(In reply to comment #15)
> I am not sure why Ref has a cast to T&, but RefPtr does not have for T*.

Both are dangerous. Silently converting a smart pointer or reference to a raw one can be dangerous because the lifetime of the object is not extended and can lead to use after free. This primary comes into play when assigning to a local variable.

Both are convenient. Silently converting without a call to get() or ptr() can be much more elegant when passing an argument to a function, and in that case the lifetime works out just fine.

[Chris Dumez]

I am still working on confirming but the bots showed a 3% regression on Speedometer / iOS when this patch landed.

[Chris Dumez]

Comment on attachment 278829 [details]
Patch for landing

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

> Source/WebCore/bindings/js/JSDOMBinding.h:-255
> -template<typename T> JSC::JSValue toJS(JSC::ExecState*, JSDOMGlobalObject*, const Ref<T>&);

To more overloads for Ref<>? Isn't it so now that if someone passes a Ref<>&& we're going to use the RefPtr<>&& overload and therefore do null checks?
This may have disabled my optimization from yesterday, which would explain why the benchmarks are back to their old scores.

[Chris Dumez]

Reverted r200837 for reason:

Seems to have regressed Speedometer and JetStream on iOS

Committed r200875: <http://trac.webkit.org/changeset/200875>

[youenn fablet]

(In reply to comment #17)
> I am still working on confirming but the bots showed a 3% regression on
> Speedometer / iOS when this patch landed.

Is it possible to have access to monitor these results?

> > Source/WebCore/bindings/js/JSDOMBinding.h:-255
> > -template<typename T> JSC::JSValue toJS(JSC::ExecState*, JSDOMGlobalObject*, const Ref<T>&);
> 
> To more overloads for Ref<>? Isn't it so now that if someone passes a
> Ref<>&& we're going to use the RefPtr<>&& overload and therefore do null
> checks?
> This may have disabled my optimization from yesterday, which would explain
> why the benchmarks are back to their old scores.

I guess we could add an additional toJS(... Ref<T>&&).

[Chris Dumez]

(In reply to comment #20)
> (In reply to comment #17)
> > I am still working on confirming but the bots showed a 3% regression on
> > Speedometer / iOS when this patch landed.
> 
> Is it possible to have access to monitor these results?

Not at the moment unfortunately. I am monitoring to make sure the scores go back up after the roll out.

[Chris Dumez]

(In reply to comment #19)
> Reverted r200837 for reason:
> 
> Seems to have regressed Speedometer and JetStream on iOS
> 
> Committed r200875: <http://trac.webkit.org/changeset/200875>

FYI, this patch was also a 2% regression on Kraken / iOS even though my original toJS() change did not progress Kraken at all. So it seems this did more than just disabling my optimization.

[youenn fablet]

(In reply to comment #22)
> (In reply to comment #19)
> > Reverted r200837 for reason:
> > 
> > Seems to have regressed Speedometer and JetStream on iOS
> > 
> > Committed r200875: <http://trac.webkit.org/changeset/200875>
> 
> FYI, this patch was also a 2% regression on Kraken / iOS even though my
> original toJS() change did not progress Kraken at all. So it seems this did
> more than just disabling my optimization.

That seems like a lot to me.
I was thinking Kraken was solely targetting JSC. 
I am surprised toJS() changes have such a big effect on Kraken results.

[youenn fablet]

Created attachment 279107 [details]
Patch for landing

[youenn fablet]

(In reply to comment #24)
> Created attachment 279107 [details]
> Patch for landing

I removed the toJS changes.

[WebKit Commit Bot]

Comment on attachment 279107 [details]
Patch for landing

Clearing flags on attachment: 279107

Committed r201013: <http://trac.webkit.org/changeset/201013>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.