CanvasRenderingContext2D should update the render tree to avoid stale computed style.

CanvasRenderingContext2D should update the render tree to avoid stale computed style.

Before accessing the computed style for populating 'font' property of CanvasRenderingContext2D
we have to make sure we have a up-to-date render tree to avoid any stale reference.

BUG=411058
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182137

[vivekg, 2014-09-10 13:16:24]

vivekg@chromium.org changed reviewers:
+ danakj@chromium.org, esprehn@chromium.org, vivekg@chromium.org

[vivekg, 2014-09-10 13:16:25]

PTAL, thanks!

[esprehn, 2014-09-10 19:50:31]

On 2014/09/10 13:16:25, vivekg_ wrote:
> PTAL, thanks!

You need a test, should be easy to write one today that sets the font color on
the parent of the canvas, the sets the color to inherit in the context, then
draw to the canvas and you'll get the wrong color today.

[vivekg, 2014-09-11 00:30:03]

On 2014/09/10 at 19:50:31, esprehn wrote:
> On 2014/09/10 13:16:25, vivekg_ wrote:
> > PTAL, thanks!
> 
> You need a test, should be easy to write one today that sets the font color on
the parent of the canvas, the sets the color to inherit in the context, then
draw to the canvas and you'll get the wrong color today.

Created this test http://jsfiddle.net/2e2sya36/ but the color is ignored in the
context. Is this what you were referring to?
The spec, [1], says the default color is black which is applied.

[1]
http://www.whatwg.org/specs/web-apps/current-work/multipage/scripting.html#ca...

[esprehn, 2014-09-11 00:52:20]

On 2014/09/11 at 00:30:03, vivekg wrote:
> On 2014/09/10 at 19:50:31, esprehn wrote:
> > On 2014/09/10 13:16:25, vivekg_ wrote:
> > > PTAL, thanks!
> > 
> > You need a test, should be easy to write one today that sets the font color
on the parent of the canvas, the sets the color to inherit in the context, then
draw to the canvas and you'll get the wrong color today.
> 
> Created this test http://jsfiddle.net/2e2sya36/ but the color is ignored in
the context. Is this what you were referring to?
> The spec, [1], says the default color is black which is applied.
> 
> [1]
http://www.whatwg.org/specs/web-apps/current-work/multipage/scripting.html#ca...

It seems color is not a good example, it's easy to trigger the bug with size.

http://jsfiddle.net/2e2sya36/2/

[vivekg, 2014-09-11 07:31:19]

Uploaded a new patch, PTAL. Thanks!

[vivekg, 2014-09-11 23:07:26]

On 2014/09/11 07:31:19, vivekg_ wrote:
> Uploaded a new patch, PTAL. Thanks!

Friendly ping.

[vivekg, 2014-09-15 23:02:07]

@esprehn any feedback? PTAL.

[Justin Novosad, 2014-09-16 20:58:57]

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

[Justin Novosad, 2014-09-16 20:58:58]

https://codereview.chromium.org/552053003/diff/20001/LayoutTests/fast/canvas/...
File LayoutTests/fast/canvas/canvas-set-font-with-updated-style.html (right):

https://codereview.chromium.org/552053003/diff/20001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/canvas-set-font-with-updated-style.html:45: var
expectedFontSize = drawCanvasText('expected', 'Some Text', true);
This test does not need a reference canvas. I feel it would make more sense to
just have the 'actual' canvas and output the context.font

That way, test failures will produce a more informative diff, and it will be
possible to baseline the expected result, which can be useful in some
situations.

[vivekg, 2014-09-17 00:28:15]

On 2014/09/16 at 20:58:58, junov wrote:
>
https://codereview.chromium.org/552053003/diff/20001/LayoutTests/fast/canvas/...
> File LayoutTests/fast/canvas/canvas-set-font-with-updated-style.html (right):
> 
>
https://codereview.chromium.org/552053003/diff/20001/LayoutTests/fast/canvas/...
> LayoutTests/fast/canvas/canvas-set-font-with-updated-style.html:45: var
expectedFontSize = drawCanvasText('expected', 'Some Text', true);
> This test does not need a reference canvas. I feel it would make more sense to
just have the 'actual' canvas and output the context.font
> 
> That way, test failures will produce a more informative diff, and it will be
possible to baseline the expected result, which can be useful in some
situations.

Done, PTAL. Thank you.

[Justin Novosad, 2014-09-17 00:46:21]

On 2014/09/17 00:28:15, vivekg_ wrote:
> On 2014/09/16 at 20:58:58, junov wrote:
> >
>
https://codereview.chromium.org/552053003/diff/20001/LayoutTests/fast/canvas/...
> > File LayoutTests/fast/canvas/canvas-set-font-with-updated-style.html
(right):
> > 
> >
>
https://codereview.chromium.org/552053003/diff/20001/LayoutTests/fast/canvas/...
> > LayoutTests/fast/canvas/canvas-set-font-with-updated-style.html:45: var
> expectedFontSize = drawCanvasText('expected', 'Some Text', true);
> > This test does not need a reference canvas. I feel it would make more sense
to
> just have the 'actual' canvas and output the context.font
> > 
> > That way, test failures will produce a more informative diff, and it will be
> possible to baseline the expected result, which can be useful in some
> situations.
> 
> Done, PTAL. Thank you.

lgtm

[vivekg, 2014-09-17 01:26:27]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-09-17 01:27:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/552053003/60001

[commit-bot: I haz the power, 2014-09-17 03:14:09]

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

[commit-bot: I haz the power, 2014-09-17 03:14:10]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/27373)

[vivekg, 2014-09-17 03:15:03]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-09-17 03:16:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/552053003/60001

[commit-bot: I haz the power, 2014-09-17 05:00:33]

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

[commit-bot: I haz the power, 2014-09-17 05:00:35]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/27388)

[vivekg, 2014-09-17 05:08:42]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-09-17 05:08:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/552053003/60001

[commit-bot: I haz the power, 2014-09-17 06:46:56]

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

[commit-bot: I haz the power, 2014-09-17 06:46:57]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/27396)

[vivekg, 2014-09-17 06:49:16]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-09-17 06:50:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/552053003/60001

[commit-bot: I haz the power, 2014-09-17 07:24:44]

Committed patchset #4 (id:60001) as 182137