Schedule cursor update on URL cursor image loaded

Schedule cursor update on URL cursor image loaded

When setting a custom cursor image through CSS URL, the cursor will reflect the change only when the image was already cached.

This is because we schedule cursor update on style/layout change, but at that time the resource image might not been downloaded yet.

This CL adds resource image listeners when the style changed, and will schedule update when the image was loaded.

Note:
The layout test will crash on Debug mode, this is due to https://crbug.com/596782 and not related to this CL.
(Release build is fine though)

BUG=532233
Committed: https://crrev.com/3f1e03998e7a4bc4a422e7eb4f6a25a17eebf508
Cr-Commit-Position: refs/heads/master@{#405019}

[chongz, 2016-07-08 18:04:36]

Description was changed from

==========
Update cursor on image resource changed

BUG=532233
==========

to

==========
Schedule cursor update on URL cursor image loaded

When setting a custom cursor image through CSS URL, the cursor will reflect the
change only when the image was already cached.

This is because we schedule cursor update on style/layout change, but at that
time the resource image might not been downloaded yet.

This CL adds resource image listeners when the style changed, and will schedule
update when the image was loaded.

Note:
The layout test will crash on Debug mode, this is due to
https://crbug.com/596782 and not related to this CL.
(Release build is fine though)

BUG=532233
==========

[chongz, 2016-07-08 18:04:54]

Patchset #1 (id:1) has been deleted

[chongz, 2016-07-08 18:06:08]

Patchset #1 (id:20001) has been deleted

[chongz, 2016-07-08 18:15:36]

chongz@chromium.org changed reviewers:
+ dtapuska@chromium.org

[chongz, 2016-07-08 18:15:36]

dtapuska@ PTAL at this cursor image CL, thanks!

PS: The layout test will crash on Debug mode (https://crbug.com/596782), do we
have any bots running layout tests on Debug mode?

[dtapuska, 2016-07-08 18:35:49]

On 2016/07/08 18:15:36, chongz wrote:
> dtapuska@ PTAL at this cursor image CL, thanks!
> 
> PS: The layout test will crash on Debug mode (https://crbug.com/596782), do we
> have any bots running layout tests on Debug mode?

I do imagine there are some builders running the layout tests in debug mode.
They may not be tree closer builders though.

The change seems fine to me but you'll need to get someone on the layout team
more familiar with the image loading path to review it.

[dtapuska, 2016-07-08 18:35:58]

https://codereview.chromium.org/2107043002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2107043002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2134: for (const
CursorData& cursorOld : *oldCursors) {
Can you just call removeCursorImageClient here?

[chongz, 2016-07-11 17:31:56]

chongz@chromium.org changed reviewers:
+ eae@chromium.org

[chongz, 2016-07-11 17:31:56]

eae@ can you take a look at this CL please? I'm trying to schedule cursor update
when the image finished loading, but I'm not sure if this is the correct way of
doing it... Thanks!

https://codereview.chromium.org/2107043002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2107043002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2134: for (const
CursorData& cursorOld : *oldCursors) {
On 2016/07/08 18:35:57, dtapuska wrote:
> Can you just call removeCursorImageClient here?

Done.

[eae, 2016-07-12 16:16:00]

https://codereview.chromium.org/2107043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2107043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3205: if (const
CursorList* cursors = style()->cursors()) {
I'm not entirely sure how we handle cursor images today but this feels like the
wrong place for this logic. Complicating the imageChanged method for the
LayoutObject base class for a _very_ rare edge case strikes me as undesirable.

How about adding this logic to CursorData instead?

[chongz, 2016-07-12 20:08:13]

eae@ Thanks for the review! However I'm not sure if I understood your
suggestions correctly, can you take a look again please? Thanks!

https://codereview.chromium.org/2107043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2107043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3205: if (const
CursorList* cursors = style()->cursors()) {
On 2016/07/12 16:16:00, eae wrote:
> I'm not entirely sure how we handle cursor images today but this feels like
the
> wrong place for this logic. Complicating the imageChanged method for the
> LayoutObject base class for a _very_ rare edge case strikes me as undesirable.

I've moved the logic to |LayoutFrame|, will it be a better place?

> 
> How about adding this logic to CursorData instead?

I'm not sure if I understood your suggestion, do you mean:
1. Only move |imageChanged| logic to |CursorData|, but leaves the listeners
here?
e.g. Create a method |CursorData::imageChanged(frame, image)| and call from
here.

Or, 

2. Move everything inside |CursorData|?
e.g. Register resource listeners in |CursorData::setImage()|.

[eae, 2016-07-12 20:26:21]

On 2016/07/12 20:08:13, chongz wrote:
> eae@ Thanks for the review! However I'm not sure if I understood your
> suggestions correctly, can you take a look again please? Thanks!
> 
>
https://codereview.chromium.org/2107043002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/2107043002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutObject.cpp:3205: if (const
> CursorList* cursors = style()->cursors()) {
> On 2016/07/12 16:16:00, eae wrote:
> > I'm not entirely sure how we handle cursor images today but this feels like
> the
> > wrong place for this logic. Complicating the imageChanged method for the
> > LayoutObject base class for a _very_ rare edge case strikes me as
undesirable.
> 
> I've moved the logic to |LayoutFrame|, will it be a better place?

Layout Frame is a lot better. Thanks.


> > How about adding this logic to CursorData instead?
> 
> I'm not sure if I understood your suggestion, do you mean:
> 1. Only move |imageChanged| logic to |CursorData|, but leaves the listeners
> here?
> e.g. Create a method |CursorData::imageChanged(frame, image)| and call from
> here.
> 
> Or, 
> 
> 2. Move everything inside |CursorData|?
> e.g. Register resource listeners in |CursorData::setImage()|.

Ideally it should all be local to the cursor but frame is an ok stopgap.

LGTM

[chongz, 2016-07-13 02:49:35]

The CQ bit was checked by chongz@chromium.org

[commit-bot: I haz the power, 2016-07-13 02:51:04]

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, 2016-07-13 04:31:10]

Description was changed from

==========
Schedule cursor update on URL cursor image loaded

When setting a custom cursor image through CSS URL, the cursor will reflect the
change only when the image was already cached.

This is because we schedule cursor update on style/layout change, but at that
time the resource image might not been downloaded yet.

This CL adds resource image listeners when the style changed, and will schedule
update when the image was loaded.

Note:
The layout test will crash on Debug mode, this is due to
https://crbug.com/596782 and not related to this CL.
(Release build is fine though)

BUG=532233
==========

to

==========
Schedule cursor update on URL cursor image loaded

When setting a custom cursor image through CSS URL, the cursor will reflect the
change only when the image was already cached.

This is because we schedule cursor update on style/layout change, but at that
time the resource image might not been downloaded yet.

This CL adds resource image listeners when the style changed, and will schedule
update when the image was loaded.

Note:
The layout test will crash on Debug mode, this is due to
https://crbug.com/596782 and not related to this CL.
(Release build is fine though)

BUG=532233
==========

[commit-bot: I haz the power, 2016-07-13 04:31:11]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-07-13 04:32:48]

Description was changed from

==========
Schedule cursor update on URL cursor image loaded

When setting a custom cursor image through CSS URL, the cursor will reflect the
change only when the image was already cached.

This is because we schedule cursor update on style/layout change, but at that
time the resource image might not been downloaded yet.

This CL adds resource image listeners when the style changed, and will schedule
update when the image was loaded.

Note:
The layout test will crash on Debug mode, this is due to
https://crbug.com/596782 and not related to this CL.
(Release build is fine though)

BUG=532233
==========

to

==========
Schedule cursor update on URL cursor image loaded

When setting a custom cursor image through CSS URL, the cursor will reflect the
change only when the image was already cached.

This is because we schedule cursor update on style/layout change, but at that
time the resource image might not been downloaded yet.

This CL adds resource image listeners when the style changed, and will schedule
update when the image was loaded.

Note:
The layout test will crash on Debug mode, this is due to
https://crbug.com/596782 and not related to this CL.
(Release build is fine though)

BUG=532233

Committed: https://crrev.com/3f1e03998e7a4bc4a422e7eb4f6a25a17eebf508
Cr-Commit-Position: refs/heads/master@{#405019}
==========

[commit-bot: I haz the power, 2016-07-13 04:32:49]

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