New SELECT Popup: Recalc style when a web font is loaded (issue 1159833003 by keishi@chromium.org)

1 view
Skip to first unread message

kei...@chromium.org

unread,
May 29, 2015, 12:39:55 AM5/29/15
to tk...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com
Reviewers: tkent,

Message:
PTAL

Description:
New SELECT Popup: Recalc style when a web font is loaded

PopupMenuCSSFontSelector should relay the invalidation callback from
m_ownerFontSelector so the style willbe recalculated when a font loads on
the
owner page.

BUG=481515

Please review this at https://codereview.chromium.org/1159833003/

Base URL: svn://svn.chromium.org/blink/trunk

Affected files (+82, -24 lines):
M LayoutTests/TestExpectations
M LayoutTests/fast/forms/resources/picker-common.js
A LayoutTests/http/tests/webfont/popup-menu-load-webfont-after-open.html
A +
LayoutTests/http/tests/webfont/popup-menu-load-webfont-after-open-expected.txt
A
LayoutTests/platform/linux/http/tests/webfont/popup-menu-load-webfont-after-open-expected.png
M Source/core/css/CSSFontSelector.h
M Source/core/css/parser/CSSPropertyParser.cpp
M Source/core/dom/StyleEngine.cpp
M Source/web/PopupMenuImpl.cpp
M Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py


tk...@chromium.org

unread,
May 29, 2015, 12:48:51 AM5/29/15
to kei...@chromium.org, ksak...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com

tk...@chromium.org

unread,
May 29, 2015, 1:41:03 AM5/29/15
to kei...@chromium.org, ksak...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com

https://codereview.chromium.org/1159833003/diff/1/LayoutTests/http/tests/webfont/popup-menu-load-webfont-after-open.html
File
LayoutTests/http/tests/webfont/popup-menu-load-webfont-after-open.html
(right):

https://codereview.chromium.org/1159833003/diff/1/LayoutTests/http/tests/webfont/popup-menu-load-webfont-after-open.html#newcode9
LayoutTests/http/tests/webfont/popup-menu-load-webfont-after-open.html:9:
@font-face {
Indentation in this <style> is inconsistent.

https://codereview.chromium.org/1159833003/diff/1/LayoutTests/http/tests/webfont/popup-menu-load-webfont-after-open.html#newcode22
LayoutTests/http/tests/webfont/popup-menu-load-webfont-after-open.html:22:
description('Test for FontFaceSet.status attribute');
Update the description

https://codereview.chromium.org/1159833003/diff/1/Source/core/css/parser/CSSPropertyParser.cpp
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1159833003/diff/1/Source/core/css/parser/CSSPropertyParser.cpp#newcode109
Source/core/css/parser/CSSPropertyParser.cpp:109: if (ruleType ==
StyleRule::Viewport) {
Why do you need to move the code from parseValue(2 args) to parseValue(6
args)?

https://codereview.chromium.org/1159833003/diff/1/Source/web/PopupMenuImpl.cpp
File Source/web/PopupMenuImpl.cpp (right):

https://codereview.chromium.org/1159833003/diff/1/Source/web/PopupMenuImpl.cpp#newcode30
Source/web/PopupMenuImpl.cpp:30: class PopupMenuCSSFontSelector : public
CSSFontSelector, public CSSFontSelectorClient {
public CSSFontSelectorClient -> private CSSFontSelectorClient

https://codereview.chromium.org/1159833003/

ksak...@chromium.org

unread,
May 29, 2015, 1:57:35 AM5/29/15
to kei...@chromium.org, tk...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com

kei...@chromium.org

unread,
May 29, 2015, 3:30:52 AM5/29/15
to tk...@chromium.org, ksak...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com

https://codereview.chromium.org/1159833003/diff/1/LayoutTests/http/tests/webfont/popup-menu-load-webfont-after-open.html
File
LayoutTests/http/tests/webfont/popup-menu-load-webfont-after-open.html
(right):

https://codereview.chromium.org/1159833003/diff/1/LayoutTests/http/tests/webfont/popup-menu-load-webfont-after-open.html#newcode9
LayoutTests/http/tests/webfont/popup-menu-load-webfont-after-open.html:9:
@font-face {
On 2015/05/29 05:41:02, tkent wrote:
> Indentation in this <style> is inconsistent.

Done.

https://codereview.chromium.org/1159833003/diff/1/LayoutTests/http/tests/webfont/popup-menu-load-webfont-after-open.html#newcode22
LayoutTests/http/tests/webfont/popup-menu-load-webfont-after-open.html:22:
description('Test for FontFaceSet.status attribute');
On 2015/05/29 05:41:02, tkent wrote:
> Update the description

Done.

https://codereview.chromium.org/1159833003/diff/1/Source/core/css/parser/CSSPropertyParser.cpp
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1159833003/diff/1/Source/core/css/parser/CSSPropertyParser.cpp#newcode109
Source/core/css/parser/CSSPropertyParser.cpp:109: if (ruleType ==
StyleRule::Viewport) {
On 2015/05/29 05:41:03, tkent wrote:
> Why do you need to move the code from parseValue(2 args) to
parseValue(6 args)?

Sorry, changes to this file got mixed in by mistake.
On 2015/05/29 05:57:35, Kunihiko Sakamoto wrote:
> Don't we need to unregister from m_ownerFontSelector?

Done.

https://codereview.chromium.org/1159833003/

ksak...@chromium.org

unread,
May 29, 2015, 3:40:07 AM5/29/15
to kei...@chromium.org, tk...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com

https://codereview.chromium.org/1159833003/diff/20001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1159833003/diff/20001/LayoutTests/TestExpectations#newcode29
LayoutTests/TestExpectations:29: #crbug.com/481515
http/tests/webfont/popup-menu-load-webfont-after-open.html [
NeedsRebaseline ]
Commented out intentionally?

https://codereview.chromium.org/1159833003/diff/20001/Source/web/PopupMenuImpl.cpp
File Source/web/PopupMenuImpl.cpp (right):

https://codereview.chromium.org/1159833003/diff/20001/Source/web/PopupMenuImpl.cpp#newcode62
Source/web/PopupMenuImpl.cpp:62: #if ENABLE(OILPAN)
Should be #if !ENABLE(OILPAN).

https://codereview.chromium.org/1159833003/

kei...@chromium.org

unread,
Jun 2, 2015, 1:33:00 AM6/2/15
to tk...@chromium.org, ksak...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com

https://codereview.chromium.org/1159833003/diff/20001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1159833003/diff/20001/LayoutTests/TestExpectations#newcode29
LayoutTests/TestExpectations:29: #crbug.com/481515
http/tests/webfont/popup-menu-load-webfont-after-open.html [
NeedsRebaseline ]
On 2015/05/29 07:40:06, Kunihiko Sakamoto wrote:
> Commented out intentionally?

Sorry, uncommented.
On 2015/05/29 07:40:06, Kunihiko Sakamoto wrote:
> Should be #if !ENABLE(OILPAN).

Done.

https://codereview.chromium.org/1159833003/

tk...@chromium.org

unread,
Jun 2, 2015, 1:37:05 AM6/2/15
to kei...@chromium.org, ksak...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com

ksak...@chromium.org

unread,
Jun 2, 2015, 1:41:58 AM6/2/15
to kei...@chromium.org, tk...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com

'commit-bot: I haz the power' via codereview.chromium.org

unread,
Jun 3, 2015, 1:18:05 AM6/3/15
to kei...@chromium.org, tk...@chromium.org, ksak...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com

'commit-bot: I haz the power' via codereview.chromium.org

unread,
Jun 3, 2015, 1:21:52 AM6/3/15
to kei...@chromium.org, tk...@chromium.org, ksak...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com
Try jobs failed on following builders:
mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/builds/46293)

https://codereview.chromium.org/1159833003/

'commit-bot: I haz the power' via codereview.chromium.org

unread,
Jun 12, 2015, 9:47:02 AM6/12/15
to kei...@chromium.org, tk...@chromium.org, ksak...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com

'commit-bot: I haz the power' via codereview.chromium.org

unread,
Jun 12, 2015, 9:53:34 AM6/12/15
to kei...@chromium.org, tk...@chromium.org, ksak...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com
Try jobs failed on following builders:
blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/35270)

https://codereview.chromium.org/1159833003/

'commit-bot: I haz the power' via codereview.chromium.org

unread,
Jun 12, 2015, 10:01:40 AM6/12/15
to kei...@chromium.org, tk...@chromium.org, ksak...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com

'commit-bot: I haz the power' via codereview.chromium.org

unread,
Jun 12, 2015, 11:07:14 AM6/12/15
to kei...@chromium.org, tk...@chromium.org, ksak...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, apavlo...@chromium.org, alexis...@intel.com, rob....@samsung.com
Reply all
Reply to author
Forward
0 new messages