Rendering text to a canvas in a frame-less document.

Rendering text to a canvas in a frame-less document.

BUG=350091
Committed: https://crrev.com/93dd565e17f9ad8e630bfb668e5c83536af50a0f
Cr-Commit-Position: refs/heads/master@{#418400}

[zakerinasab, 2016-09-12 18:04:26]

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

[zakerinasab, 2016-09-12 18:04:26]

CL submitted.

[Justin Novosad, 2016-09-12 18:44:59]

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

https://codereview.chromium.org/2336693002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:475: ,
m_settings(Settings::create())
This should be lazily allocated in settings() instead of in the constructor
IMHO.

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

https://codereview.chromium.org/2336693002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.h:1439: std::unique_ptr<Settings>
m_settings;
You should call this m_defaultSettings

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

https://codereview.chromium.org/2336693002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/StyleEngine.cpp:64: ,
m_fontSelector(CSSFontSelector::create(&document))
This should be lazily allocated instead of in the constructor.

[zakerinasab, 2016-09-12 19:47:34]

New CL submitted.

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

https://codereview.chromium.org/2336693002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:475: ,
m_settings(Settings::create())
On 2016/09/12 18:44:59, Justin Novosad wrote:
> This should be lazily allocated in settings() instead of in the constructor
> IMHO.

Done.

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

https://codereview.chromium.org/2336693002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.h:1439: std::unique_ptr<Settings>
m_settings;
On 2016/09/12 18:44:59, Justin Novosad wrote:
> You should call this m_defaultSettings

Done.

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

https://codereview.chromium.org/2336693002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/StyleEngine.cpp:64: ,
m_fontSelector(CSSFontSelector::create(&document))
On 2016/09/12 18:44:59, Justin Novosad wrote:
> This should be lazily allocated instead of in the constructor.

Done.

[Justin Novosad, 2016-09-12 19:55:50]

lgtm

[zakerinasab, 2016-09-12 19:58:34]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2016-09-12 19:58:52]

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-09-12 22:14:44]

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

[commit-bot: I haz the power, 2016-09-12 22:14:45]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zakerinasab, 2016-09-13 13:38:18]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2016-09-13 13:38:33]

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

[Justin Novosad, 2016-09-13 14:45:43]

https://codereview.chromium.org/2336693002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/text-rendering-frameless-canvas-expected.html
(right):

https://codereview.chromium.org/2336693002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/text-rendering-frameless-canvas-expected.html:10:
ctx.font = "10px sans-serif";
On mac, it looks like the default font is a serif font.  Does the test pass if
you just remove this line?  I think verifying that the default font is the same
in a frame-less document vs regular document would be good enough.

[blink-reviews, 2016-09-13 14:46:57]

Oh. I get it. I'll check it now.

On Tue, Sep 13, 2016 at 10:45 AM, <junov@chromium.org> wrote:

>
> https://codereview.chromium.org/2336693002/diff/20001/third_party/WebKit/
> LayoutTests/fast/canvas/text-rendering-frameless-canvas-expected.html
> File
> third_party/WebKit/LayoutTests/fast/canvas/text-
> rendering-frameless-canvas-expected.html
> (right):
>
> https://codereview.chromium.org/2336693002/diff/20001/third_party/WebKit/
> LayoutTests/fast/canvas/text-rendering-frameless-canvas-
> expected.html#newcode10
> third_party/WebKit/LayoutTests/fast/canvas/text-
> rendering-frameless-canvas-expected.html:10:
> ctx.font = "10px sans-serif";
> On mac, it looks like the default font is a serif font. Does the test
> pass if you just remove this line? I think verifying that the default
> font is the same in a frame-less document vs regular document would be
> good enough.
>
> https://codereview.chromium.org/2336693002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-09-13 14:46:57]

Oh. I get it. I'll check it now.

On Tue, Sep 13, 2016 at 10:45 AM, <junov@chromium.org> wrote:

>
> https://codereview.chromium.org/2336693002/diff/20001/third_party/WebKit/
> LayoutTests/fast/canvas/text-rendering-frameless-canvas-expected.html
> File
> third_party/WebKit/LayoutTests/fast/canvas/text-
> rendering-frameless-canvas-expected.html
> (right):
>
> https://codereview.chromium.org/2336693002/diff/20001/third_party/WebKit/
> LayoutTests/fast/canvas/text-rendering-frameless-canvas-
> expected.html#newcode10
> third_party/WebKit/LayoutTests/fast/canvas/text-
> rendering-frameless-canvas-expected.html:10:
> ctx.font = "10px sans-serif";
> On mac, it looks like the default font is a serif font. Does the test
> pass if you just remove this line? I think verifying that the default
> font is the same in a frame-less document vs regular document would be
> good enough.
>
> https://codereview.chromium.org/2336693002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[zakerinasab, 2016-09-13 14:53:21]

The CQ bit was checked by zakerinasab@chromium.org

[zakerinasab, 2016-09-13 14:53:21]

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/2336693002/#ps40001
(title: "Addressing the issue with default fallback font on Mac.")

[commit-bot: I haz the power, 2016-09-13 14:53:35]

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

[zakerinasab, 2016-09-13 14:54:05]

New patch submitted.

https://codereview.chromium.org/2336693002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/text-rendering-frameless-canvas-expected.html
(right):

https://codereview.chromium.org/2336693002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/text-rendering-frameless-canvas-expected.html:10:
ctx.font = "10px sans-serif";
On 2016/09/13 14:45:42, Justin Novosad wrote:
> On mac, it looks like the default font is a serif font.  Does the test pass if
> you just remove this line?  I think verifying that the default font is the
same
> in a frame-less document vs regular document would be good enough.

Done.

[zakerinasab, 2016-09-13 14:54:07]

New patch submitted.

[Justin Novosad, 2016-09-13 15:38:54]

lgtm assuming tests pass.

[commit-bot: I haz the power, 2016-09-13 17:16:30]

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

[commit-bot: I haz the power, 2016-09-13 17:16:31]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zakerinasab, 2016-09-13 21:29:26]

The CQ bit was checked by zakerinasab@chromium.org

[zakerinasab, 2016-09-13 21:29:27]

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/2336693002/#ps60001
(title: "Adding the layout test to TestExpectations for Mac.")

[commit-bot: I haz the power, 2016-09-13 21:30:07]

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-09-13 23:07:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-13 23:09:57]

Description was changed from

==========
Rendering text to a canvas in a frame-less document.

BUG=350091
==========

to

==========
Rendering text to a canvas in a frame-less document.

BUG=350091

Committed: https://crrev.com/93dd565e17f9ad8e630bfb668e5c83536af50a0f
Cr-Commit-Position: refs/heads/master@{#418400}
==========

[commit-bot: I haz the power, 2016-09-13 23:09:58]

Patchset 4 (id:??) landed as
https://crrev.com/93dd565e17f9ad8e630bfb668e5c83536af50a0f
Cr-Commit-Position: refs/heads/master@{#418400}

[zakerinasab, 2016-09-14 14:15:00]

Patchset #4 (id:60001) has been deleted

[zakerinasab, 2016-09-14 14:18:00]

Description was changed from

==========
Rendering text to a canvas in a frame-less document.

BUG=350091

Committed: https://crrev.com/93dd565e17f9ad8e630bfb668e5c83536af50a0f
Cr-Commit-Position: refs/heads/master@{#418400}
==========

to

==========
Rendering text to a canvas in a frame-less document.

BUG=350091

Committed: https://crrev.com/93dd565e17f9ad8e630bfb668e5c83536af50a0f
Cr-Commit-Position: refs/heads/master@{#418400}
==========

[zakerinasab, 2016-09-14 14:18:00]

zakerinasab@chromium.org changed reviewers:
- junov@chromium.org

[zakerinasab, 2016-09-14 14:18:53]

The CQ bit was checked by zakerinasab@chromium.org

[zakerinasab, 2016-09-14 14:18:54]

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/2336693002/#ps80001
(title: "Adding the failed layout tests to TestExpctations for Mac.")

[commit-bot: I haz the power, 2016-09-14 14:19:08]

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-09-14 14:21:40]

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

[commit-bot: I haz the power, 2016-09-14 14:21:41]

Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zakerinasab, 2016-09-14 14:29:45]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2016-09-14 14:29:57]

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-09-14 14:32:40]

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

[commit-bot: I haz the power, 2016-09-14 14:32:42]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zakerinasab, 2016-09-14 14:34:46]

The CQ bit was unchecked by zakerinasab@chromium.org

[zakerinasab, 2016-09-14 14:46:42]

Patchset #4 (id:80001) has been deleted

[zakerinasab, 2016-09-14 14:48:07]

On 2016/09/14 14:32:42, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
>   chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
>   ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
>   mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
>   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

A previous patch has successfully landed. Corrections to TestExpectaions will be
done in another patch.

[zakerinasab, 2016-09-16 18:26:24]

Patchset #5 (id:120001) has been deleted

[esprehn, 2016-09-26 22:23:36]

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

[esprehn, 2016-09-26 22:23:37]

In the future please reach out to people on the style team when changing code in
the style system.

https://codereview.chromium.org/2336693002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2336693002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:2890: if (!inActiveDocument() &&
document().frame()) {
This is not correct, it means we don't return null here if your document had no
frame. Which makes us call ensureStyleResolver below on an frameless document
which will crash.

inActiveDocument() is guarding the code below. I'm also curious why you'd want
to run the style code for elements not in the document but inside a frameless
document? It seems strange we'd special case on frame here, in general this code
shouldn't care about that, it's about being in an active document (capable of
computing style) or not.

[esprehn, 2016-09-26 22:48:02]

Note that the change you made to Element::ensureComputedStyle is making this:

var doc = document.implementation.createHTMLDocument(...);
var div = doc.createElement("div");
getComputedStyle(div).color;

now run the whole style system inside the non-active document on <div>. We can
attempt to fix that ancient bug, but it's a much bigger project than just
canvas. You probably need to audit all of the various style machinery for
assumptions about being in an active document, ex. LayoutView, Frame, having a
loader, access to page(), various clients, etc.

[zakerinasab, 2016-09-27 14:31:07]

A revert of this CL (patchset #4 id:100001) has been created in
https://codereview.chromium.org/2370283002/ by zakerinasab@chromium.org.

The reason for reverting is: Causes some crashes on Canary..