Replace ErrorCallbackTask in RTCPeerConnection.cpp with WTF::bind()

Replace ErrorCallbackTask in RTCPeerConnection.cpp with WTF::bind()

BUG=478194
Committed: https://crrev.com/c04a33bce2761ef30c51a833781effc7162e68cd
Cr-Commit-Position: refs/heads/master@{#377901}

[hiroshige, 2016-02-19 20:10:38]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-19 20:11:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1712333002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1712333002/1

[hiroshige, 2016-02-19 20:21:12]

Patchset #2 (id:20001) has been deleted

[hiroshige, 2016-02-19 20:22:17]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-19 20:23:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1712333002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1712333002/40001

[hiroshige, 2016-02-19 20:38:31]

hiroshige@chromium.org changed reviewers:
+ guidou@chromium.org

[hiroshige, 2016-02-19 20:38:32]

Could you take a look?

[Guido Urdaneta, 2016-02-19 20:58:38]

On 2016/02/19 20:38:32, hiroshige wrote:
> Could you take a look?

During a code review this came up and it was mentioned that the RTCErrorCallback
object might be GC'ed before the microtask runs it because bind() does not keep
a persistent reference to it.
See
https://codereview.chromium.org/1661493002/diff/160001/third_party/WebKit/Sou...

However, if you are 100% sure this cannot happen, feel free to proceed.

[hiroshige, 2016-02-19 21:10:25]

hiroshige@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[hiroshige, 2016-02-19 21:10:25]

I think if we bind() a pointer to GarbageCollected object, WTF::Function holds
Persistent to that.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
(whether bind() hold a reference or not is complicated and I'd like to
cleanup/clarify in near future.)

oilpan-reviews@, could someone double check this?

[sof, 2016-02-19 21:18:55]

On 2016/02/19 21:10:25, hiroshige wrote:
> I think if we bind() a pointer to GarbageCollected object, WTF::Function holds
> Persistent to that.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> (whether bind() hold a reference or not is complicated and I'd like to
> cleanup/clarify in near future.)
> 
> oilpan-reviews@, could someone double check this?

That's correct. If you want a weaker alternative for microtasks, look at
CancellableTaskFactory::create() (or
CustomElementMicrotaskRunQueue::requestDispatchIfNeeded())

[commit-bot: I haz the power, 2016-02-19 21:36:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-19 21:36:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hiroshige, 2016-02-20 01:46:32]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-20 01:49:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1712333002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1712333002/40001

[commit-bot: I haz the power, 2016-02-20 02:51:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-20 02:51:30]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-02-23 18:48:39]

hiroshige@chromium.org changed reviewers:
+ philipj@opera.com

[hiroshige, 2016-02-23 18:48:39]

philipj@, could you take a look as an OWNER?

Using bind() with GarbageCollected pointers keeps Presistent references and
therefore safe here (Comments #8, #10, #11).

(bind() with non-GarbageCollected pointers doesn't keep reference though)

[philipj_slow, 2016-02-24 08:31:18]

LGTM, nice simplification!

It looks like it will be stored as CrossThreadPersistent<T>, which I guess is
more than necessary, but that seems fine.

I also assume that additional GarbageCollected pointer arguments would also be
treated the same way? guido@ is replacing the String argument with a
DOMException in https://codereview.chromium.org/1729563002/

[hiroshige, 2016-02-25 22:33:44]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-25 22:44:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1712333002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1712333002/60001

[hiroshige, 2016-02-26 01:05:21]

On 2016/02/24 08:31:18, philipj_UTC7 wrote:
> LGTM, nice simplification!
> 
> It looks like it will be stored as CrossThreadPersistent<T>, which I guess is
> more than necessary, but that seems fine.
Currently we use CrossThreadPersistent<T> because this code is also used for
cross-thread cases.
I might clean up this in the course of WTF::bind()/CrossThreadCopier refactoring
later.

> I also assume that additional GarbageCollected pointer arguments would also be
> treated the same way? guido@ is replacing the String argument with a
> DOMException in https://codereview.chromium.org/1729563002/
Yeah, I rebased after the CL is landed and replaced DOMException also in Patch
Set 3. PTAL again.

[commit-bot: I haz the power, 2016-02-26 01:45:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-26 01:45:46]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hiroshige, 2016-02-26 01:49:08]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-26 01:50:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1712333002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1712333002/60001

[commit-bot: I haz the power, 2016-02-26 03:36:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-26 03:36:55]

Dry run: This issue passed the CQ dry run.

[philipj_slow, 2016-02-26 07:50:38]

philipj@opera.com changed reviewers:
+ peter@chromium.org, tommi@chromium.org, tommyw@chromium.org

[philipj_slow, 2016-02-26 07:50:39]

Still LGTM, but also adding module owner for final say.

[haraken, 2016-02-26 08:41:15]

LGTM. I'd prefer using "WTF::"bind though.

[tommi (sloooow) - chröme, 2016-02-26 11:13:05]

lgtm

[hiroshige, 2016-02-26 16:19:06]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2016-02-26 16:21:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1712333002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1712333002/60001

[hiroshige, 2016-02-26 16:21:44]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2016-02-26 16:29:38]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2016-02-26 16:30:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1712333002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1712333002/60001

[hiroshige, 2016-02-26 16:36:32]

On 2016/02/26 08:41:15, haraken wrote:
> LGTM. I'd prefer using "WTF::"bind though.

Could you clarify the reason?
If WTF::bind() is better, how about remove "using WTF::bind;" in
wtf/Functional.h and replace all bind() usage without WTF:: prefix?

[commit-bot: I haz the power, 2016-02-26 16:45:45]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-02-26 16:46:35]

Description was changed from

==========
Replace ErrorCallbackTask in RTCPeerConnection.cpp with WTF::bind()

BUG=478194
==========

to

==========
Replace ErrorCallbackTask in RTCPeerConnection.cpp with WTF::bind()

BUG=478194

Committed: https://crrev.com/c04a33bce2761ef30c51a833781effc7162e68cd
Cr-Commit-Position: refs/heads/master@{#377901}
==========

[commit-bot: I haz the power, 2016-02-26 16:46:37]

Patchset 3 (id:??) landed as
https://crrev.com/c04a33bce2761ef30c51a833781effc7162e68cd
Cr-Commit-Position: refs/heads/master@{#377901}

[haraken, 2016-02-26 17:06:40]

On 2016/02/26 16:36:32, hiroshige wrote:
> On 2016/02/26 08:41:15, haraken wrote:
> > LGTM. I'd prefer using "WTF::"bind though.
> 
> Could you clarify the reason?
> If WTF::bind() is better, how about remove "using WTF::bind;" in
> wtf/Functional.h and replace all bind() usage without WTF:: prefix?

Sorry, you're right. I was just looking at Node.cpp and thought that we prefer
WTF::bind than bind.

[hiroshige, 2016-02-26 17:26:50]

On 2016/02/26 17:06:40, haraken wrote:
> On 2016/02/26 16:36:32, hiroshige wrote:
> > On 2016/02/26 08:41:15, haraken wrote:
> > > LGTM. I'd prefer using "WTF::"bind though.
> > 
> > Could you clarify the reason?
> > If WTF::bind() is better, how about remove "using WTF::bind;" in
> > wtf/Functional.h and replace all bind() usage without WTF:: prefix?
> 
> Sorry, you're right. I was just looking at Node.cpp and thought that we prefer
> WTF::bind than bind.

Er, I found a possible reason to enforce WTF::bind(): conflict with std::bind().
Usually this is not a problem because we don't use "using namespace std", but
sometimes we have to write "WTF::bind" explicitly to avoid to be resolved to
std::bind(), due to argument-dependent lookup.
Example: Line 123. Because |html| is |const std::string&|, so bind() here is
resolved to std::bind() if we don't write "WTF::".
https://codereview.chromium.org/1710403003/diff/80001/third_party/WebKit/Sour...