toBlob: If idle image encoding is postponed for too long, switch to main thread

toBlob: If idle image encoding is postponed for too long, switch to main thread

BUG=583070
Committed: https://crrev.com/dc5c70a312e24ca03d94696f16c5be318ce198d6
Cr-Commit-Position: refs/heads/master@{#377054}

[xlai (Olivia), 2016-02-05 17:00:37]

xlai@chromium.org changed reviewers:
+ junov@chromium.org

[xlai (Olivia), 2016-02-05 17:00:38]

According to this research study
9https://www.nngroup.com/articles/response-times-3-important-limits/), 0.1
second is about the limit for having the user feel that the system is reacting
instantaneously. So I set it to be the threshold.

I think we should still switch it to Blink thread pool, instead of posting it to
main thread (and making it synchronous). If there's a way to do it better than
directly posting to main thread and postponing it forever, why not just use it?

[Justin Novosad, 2016-02-09 16:32:21]

https://codereview.chromium.org/1665413004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/1665413004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:28: const
double IdleTaskDelayThreshold = 100.0; // 100ms is the limit for the user to
feel that the system is reacting instantaneously
Name suggestion: IdleTaskTimeoutDelay

https://codereview.chromium.org/1665413004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:74:
Platform::current()->mainThread()->taskRunner()->postDelayedTask(BLINK_FROM_HERE,
WTF::bind(&CanvasAsyncBlobCreator::inspectIdleTaskStatus, this, quality),
IdleTaskDelayThreshold);
I like this approach. But it is non trivial. You should add a coment to explain
how the race between the two posted tasks works.

https://codereview.chromium.org/1665413004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:166: void
CanvasAsyncBlobCreator::inspectIdleTaskStatus(double quality)
Suggested name for func: idleTaskTiemout event.

https://codereview.chromium.org/1665413004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:175: }
There is the case where we have an Idle taks that has started, but may take
forever because there is very little available idle time.  We should do
something for that case to ensure timely completion.  Is it possible to switch
encoding work that is already in progress over to another thread? If not perhaps
we should just finish encoding immediately on the main thread?

[xlai (Olivia), 2016-02-11 02:07:06]

Summary of changes:
1. Renaming based on feedback
2. Adding a timeout limit threshold for completion time of idle task
3. Extract out the initialization of png because it is used by both situations
4. Clean out some schedule functions that are only invoked once
5. IdleTaskStatus enum
6. Adding m_timeoutSelfRef which is only cleared by the two timeout event
functions.

[xlai (Olivia), 2016-02-11 02:08:05]

Description was changed from

==========
toBlob: If idle image encoding is postponed for too long, switch to thread pool.

BUG=583070
==========

to

==========
toBlob: If idle image encoding is postponed for too long, switch to main thread

BUG=583070
==========

[Justin Novosad, 2016-02-11 20:36:45]

https://codereview.chromium.org/1665413004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/1665413004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:62:
m_timeoutSelfRef = nullptr;
this is not necessary, RefPtr default constructor does this.

https://codereview.chromium.org/1665413004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:88: //
There's no risk of race condition as both tasks are on main thread.
"race condition" -> "concurrency"

https://codereview.chromium.org/1665413004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:140: void
CanvasAsyncBlobCreator::encodeRowsPng()
encodeRowsPngOnMainThread

https://codereview.chromium.org/1665413004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:195: if
(m_idleTaskStatus == IdleTaskStatus::IdleTaskStarted) {
This is good, but you need test coverage. The layout tests provide no guarantee
that both of these "if" statements are covered in testing. Writing the test can
be a bit tricky. One thing you could do is make this class use a scheduler and a
taskRunner that are specified to the constructor. That way the unit test could
initialize it with a custom scheduler instead of using
Platform::current()->mainThread()->scheduler() directly. Then you can use a mock
in the unit test and manually control the order in which events are scheduled in
order to test the different races.

https://codereview.chromium.org/1665413004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:201: if
(m_idleTaskStatus == IdleTaskStatus::IdleTaskNotStarted) {
this could be "else if"

https://codereview.chromium.org/1665413004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:219: if
(m_idleTaskStatus == IdleTaskStatus::IdleTaskStarted) {
This too needs unit test coverage.

https://codereview.chromium.org/1665413004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
(right):

https://codereview.chromium.org/1665413004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:52:
RefPtr<CanvasAsyncBlobCreator> m_selfRef;
Could you group the two self ref pointers so they are next to each other, and
rename this one to add clarity. And add a comment two explain why two self
references (because we don't know in advance what will be the last event idle or
timeout)

https://codereview.chromium.org/1665413004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:53: void
clearSelfReference();
Function name should indicate which self ref it is referring to.

[xlai (Olivia), 2016-02-11 21:13:53]

Patchset #4 (id:60001) has been deleted

[xlai (Olivia), 2016-02-22 18:48:26]

Patchset #5 (id:100001) has been deleted

[xlai (Olivia), 2016-02-22 18:58:06]

Added a unit test. Many existing functions in CanvasAsyncBlobCreator become
protected functions so that the Mock creator can call them.

Because there're lots of change, I've added many in-line comments to help code
reviewing.

https://codereview.chromium.org/1665413004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/1665413004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:201: if
(m_idleTaskStatus == IdleTaskStatus::IdleTaskNotStarted) {
On 2016/02/11 20:36:44, Justin Novosad wrote:
> this could be "else if"

Getting a coding style check error if changing it to "else if"...

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:236:
signalTaskSwitchInStartTimeoutEvent();
Note this empty function here.

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:257:
signalTaskSwitchInCompleteTimeoutEvent();
Note this empty function here.

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
(right):

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:34: virtual
void signalTaskSwitchInStartTimeoutEvent() { }
These two signal functions are empty functions that are inserted to a particular
point in the execution. Then in the unit test, we can use EXPECT_CALL to check
whether the execution goes through an expected code path.

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:76: void
idleTaskCompleteTimeoutEvent();
These two TimeoutEvent functions are posted in specific delay time after idle
task has been fired. They are used to inspect whether the idle task has started,
or completed, within reasonable time frame.

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreatorTest.cpp
(right):

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreatorTest.cpp:23:
class MockCanvasAsyncBlobCreator : public CanvasAsyncBlobCreator {
The inheritance hierarchy of all these classes are:

CanvasAsyncBlobCreator <-- MockCanvasAsyncBlobCreator
MockCanvasAsyncBlobCreator <-- MockCanvasAsyncBlobCreatorWithoutStart
MockCanvasAsyncBlobCreator <-- MockCanvasAsyncBlobCreatorWithoutComplete

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreatorTest.cpp:47:
testing::exitRunLoop();
Whenever clearAlternativeSelfReference, we wake up the corresponding
"testing::enterRunLoop()" in the test, so that the test knows that the main
execution finishes and now it can start checking values.

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreatorTest.cpp:52:
Platform::current()->mainThread()->taskRunner()->postTask(location, task);
We override the postDelayedTask so that the delayMs is ignored in unit test
(i.e. no need to wait in unit test).

[Justin Novosad, 2016-02-22 20:26:34]

Great job with the tests.

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
(right):

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:34: virtual
void signalTaskSwitchInStartTimeoutEvent() { }
On 2016/02/22 18:58:05, Olivia wrote:
> These two signal functions are empty functions that are inserted to a
particular
> point in the execution. Then in the unit test, we can use EXPECT_CALL to check
> whether the execution goes through an expected code path.

You should add "ForTesting" to the end of the names of both functions.

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreatorTest.cpp
(right):

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreatorTest.cpp:156:
this->prepareMockCanvasAsyncBlobCreatorWithoutStart();
Add a comment to explain the flow of this test, and exactly what gets verified.

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreatorTest.cpp:168:
this->prepareMockCanvasAsyncBlobCreatorWithoutComplete();
Ditto

https://codereview.chromium.org/1665413004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreatorTest.cpp:180:
this->prepareMockCanvasAsyncBlobCreatorFail();
Ditto

[xlai (Olivia), 2016-02-22 22:21:28]

Patchset #7 (id:160001) has been deleted

[xlai (Olivia), 2016-02-22 22:28:57]

Rebased with master to fix merge conflicts with another recently landed patch.
Added comments and renames based on feedbacks.

[Justin Novosad, 2016-02-22 22:41:21]

lgtm

[xlai (Olivia), 2016-02-23 16:45:17]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2016-02-23 16:45:17]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1665413004/#ps200001
(title: "Removed unnecessary include in unit test")

[commit-bot: I haz the power, 2016-02-23 16:46:34]

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

[xlai (Olivia), 2016-02-23 19:02:10]

The CQ bit was unchecked by xlai@chromium.org

[xlai (Olivia), 2016-02-23 19:04:57]

The CQ bit was checked by xlai@chromium.org

[commit-bot: I haz the power, 2016-02-23 19:16:56]

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

[commit-bot: I haz the power, 2016-02-23 19:29:57]

Description was changed from

==========
toBlob: If idle image encoding is postponed for too long, switch to main thread

BUG=583070
==========

to

==========
toBlob: If idle image encoding is postponed for too long, switch to main thread

BUG=583070
==========

[commit-bot: I haz the power, 2016-02-23 19:29:58]

Committed patchset #8 (id:200001)

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

Description was changed from

==========
toBlob: If idle image encoding is postponed for too long, switch to main thread

BUG=583070
==========

to

==========
toBlob: If idle image encoding is postponed for too long, switch to main thread

BUG=583070

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

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

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

[xlai (Olivia), 2016-02-24 15:57:26]

A revert of this CL (patchset #8 id:200001) has been created in
https://codereview.chromium.org/1732913002/ by xlai@chromium.org.

The reason for reverting is: I am suspecting that this CL is causing a leak in
WebKit Leak bot. Reverting it first to inspect the source of problems..