Replace Document::postTask with WebTaskRunner::postTask

Replace Document::postTask with WebTaskRunner::postTask

Document::postTask is deprecated in favor of WebTaskRunner::postTask.
This patch removes occurrences of postTask invocations from
Document type instances.

BUG=625927
Review-Url: https://codereview.chromium.org/2688473005
Cr-Commit-Position: refs/heads/master@{#450283}
Committed: https://chromium.googlesource.com/chromium/src/+/0c0fbcfa235ac535ef2b82690bb3619deee6b035

[yuryu, 2017-02-09 09:45:58]

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

[commit-bot: I haz the power, 2017-02-09 09:46:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 11:29:57]

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

[commit-bot: I haz the power, 2017-02-09 11:29:58]

Dry run: This issue passed the CQ dry run.

[yuryu, 2017-02-09 12:49:22]

yuryu@chromium.org changed reviewers:
+ nhiroki@chromium.org, tzik@chromium.org

[tzik, 2017-02-09 23:50:20]

https://codereview.chromium.org/2688473005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2688473005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:388: static void
runAutofocusTask(ExecutionContext* context) {
null check for |context| is needed here too.

https://codereview.chromium.org/2688473005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:5715: ExecutionContext* context)
{
null check for |context| is needed here, since postTask/crossThreadBind doesn't
have the task cancellation.

[yuryu, 2017-02-10 06:29:32]

tzik, I added null-checks.

There is an additional hunk that only appears in a delta from patch set 1
https://codereview.chromium.org/2688473005/diff2/1:40001/third_party/WebKit/S...
but it doesn't appear when you see the entire diff:
https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...

I'm not sure how I can fix this.

[yuryu, 2017-02-13 03:33:22]

https://codereview.chromium.org/2688473005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2688473005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:388: static void
runAutofocusTask(ExecutionContext* context) {
On 2017/02/09 23:50:20, tzik wrote:
> null check for |context| is needed here too.

Done.

https://codereview.chromium.org/2688473005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:5715: ExecutionContext* context)
{
On 2017/02/09 23:50:19, tzik wrote:
> null check for |context| is needed here, since postTask/crossThreadBind
doesn't
> have the task cancellation.

Done.

[tzik, 2017-02-13 06:20:38]

lgtm

[yuryu, 2017-02-13 06:37:31]

yuryu@chromium.org changed reviewers:
+ haraken@chromium.org

[yuryu, 2017-02-13 06:37:32]

haraken, can you take a look? thanks.

[nhiroki, 2017-02-13 06:42:07]

lgtm. Can you update/remove the prefix on the CL subject and description?

[haraken, 2017-02-13 06:43:52]

LGTM with comments.

https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:5734:
wrapCrossThreadWeakPersistent(this)));

I guess we should use a strong Persistent here because the console message
should be printed regardless of the document is gone or not.

https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6323:
WTF::bind(&runAutofocusTask, wrapWeakPersistent(this)));

Ditto.

[tzik, 2017-02-13 06:56:33]

https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6323:
WTF::bind(&runAutofocusTask, wrapWeakPersistent(this)));
On 2017/02/13 06:43:52, haraken wrote:
> 
> Ditto.

I think this should be weak, since focusing an element doesn't make sense on a
detached document.

[haraken, 2017-02-13 06:58:35]

https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6323:
WTF::bind(&runAutofocusTask, wrapWeakPersistent(this)));
On 2017/02/13 06:56:33, tzik wrote:
> On 2017/02/13 06:43:52, haraken wrote:
> > 
> > Ditto.
> 
> I think this should be weak, since focusing an element doesn't make sense on a
> detached document.

Then we should explicitly check if the document has been detached or not. Note
that a weak persistent handle is cleared when a GC is eventually triggered not
when the document gets detached.

[yuryu, 2017-02-13 08:05:40]

Description was changed from

==========
Worker: Replace Document::postTask with WebTaskRunner::postTask

Document::postTask is deprecated in favor of WebTaskRunner::postTask.
This patch removes occurrences of postTask invocations from
Document type instances.

BUG=625927
==========

to

==========
Replace Document::postTask with WebTaskRunner::postTask

Document::postTask is deprecated in favor of WebTaskRunner::postTask.
This patch removes occurrences of postTask invocations from
Document type instances.

BUG=625927
==========

[tzik, 2017-02-13 08:58:53]

https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6323:
WTF::bind(&runAutofocusTask, wrapWeakPersistent(this)));
On 2017/02/13 06:58:35, haraken wrote:
> On 2017/02/13 06:56:33, tzik wrote:
> > On 2017/02/13 06:43:52, haraken wrote:
> > > 
> > > Ditto.
> > 
> > I think this should be weak, since focusing an element doesn't make sense on
a
> > detached document.
> 
> Then we should explicitly check if the document has been detached or not. Note
> that a weak persistent handle is cleared when a GC is eventually triggered not
> when the document gets detached.

We don't need the check, since it's done in Element::focus().
The Persistent here extends the lifetime of Document slightly for an effective
nop.

[haraken, 2017-02-13 09:00:01]

On 2017/02/13 08:58:53, tzik wrote:
>
https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:6323:
> WTF::bind(&runAutofocusTask, wrapWeakPersistent(this)));
> On 2017/02/13 06:58:35, haraken wrote:
> > On 2017/02/13 06:56:33, tzik wrote:
> > > On 2017/02/13 06:43:52, haraken wrote:
> > > > 
> > > > Ditto.
> > > 
> > > I think this should be weak, since focusing an element doesn't make sense
on
> a
> > > detached document.
> > 
> > Then we should explicitly check if the document has been detached or not.
Note
> > that a weak persistent handle is cleared when a GC is eventually triggered
not
> > when the document gets detached.
> 
> We don't need the check, since it's done in Element::focus().
> The Persistent here extends the lifetime of Document slightly for an effective
> nop.

ah, makes sense.

[yuryu, 2017-02-13 09:14:04]

https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:5734:
wrapCrossThreadWeakPersistent(this)));
On 2017/02/13 06:43:51, haraken wrote:
> 
> I guess we should use a strong Persistent here because the console message
> should be printed regardless of the document is gone or not.
> 

Done.

https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6323:
WTF::bind(&runAutofocusTask, wrapWeakPersistent(this)));
Do you think it's good to add a comment about it? If it's confusing.

[haraken, 2017-02-13 09:15:23]

On 2017/02/13 09:14:04, yuryu wrote:
>
https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:5734:
> wrapCrossThreadWeakPersistent(this)));
> On 2017/02/13 06:43:51, haraken wrote:
> > 
> > I guess we should use a strong Persistent here because the console message
> > should be printed regardless of the document is gone or not.
> > 
> 
> Done.
> 
>
https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:6323:
> WTF::bind(&runAutofocusTask, wrapWeakPersistent(this)));
> Do you think it's good to add a comment about it? If it's confusing.

Yeah, sounds nice. LGTM.

[yuryu, 2017-02-13 09:21:15]

https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2688473005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:6323:
WTF::bind(&runAutofocusTask, wrapWeakPersistent(this)));
On 2017/02/13 09:14:04, yuryu wrote:
> Do you think it's good to add a comment about it? If it's confusing.

Done.

[yuryu, 2017-02-13 09:22:56]

The CQ bit was checked by yuryu@chromium.org

[yuryu, 2017-02-13 09:22:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
nhiroki@chromium.org, tzik@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2688473005/#ps80001
(title: "add a comment")

[commit-bot: I haz the power, 2017-02-13 09:23:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yuryu, 2017-02-13 10:27:14]

The CQ bit was unchecked by yuryu@chromium.org

[yuryu, 2017-02-13 10:35:32]

I forgot to remove a null-check in runAddConsoleMessageTask after making its
context persistent. Can you take a look? Thanks.

[yuryu, 2017-02-13 10:35:43]

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

[commit-bot: I haz the power, 2017-02-13 10:35:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 11:51:20]

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

[commit-bot: I haz the power, 2017-02-13 11:51:21]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-02-14 06:49:25]

lgtm

[yuryu, 2017-02-14 06:53:51]

The CQ bit was checked by yuryu@chromium.org

[yuryu, 2017-02-14 06:53:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2688473005/#ps100001
(title: "remove extra null-check: context on addConsoleMessage is now
persistent")

[commit-bot: I haz the power, 2017-02-14 06:54:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 06:57:44]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487055231531560,
"parent_rev": "2899a23c7bc25c04eb7c1130d404ddcfd2adff60", "commit_rev":
"0c0fbcfa235ac535ef2b82690bb3619deee6b035"}

[commit-bot: I haz the power, 2017-02-14 06:58:38]

Description was changed from

==========
Replace Document::postTask with WebTaskRunner::postTask

Document::postTask is deprecated in favor of WebTaskRunner::postTask.
This patch removes occurrences of postTask invocations from
Document type instances.

BUG=625927
==========

to

==========
Replace Document::postTask with WebTaskRunner::postTask

Document::postTask is deprecated in favor of WebTaskRunner::postTask.
This patch removes occurrences of postTask invocations from
Document type instances.

BUG=625927

Review-Url: https://codereview.chromium.org/2688473005
Cr-Commit-Position: refs/heads/master@{#450283}
Committed:
https://chromium.googlesource.com/chromium/src/+/0c0fbcfa235ac535ef2b82690bb3...
==========

[commit-bot: I haz the power, 2017-02-14 06:58:40]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0c0fbcfa235ac535ef2b82690bb3...