Reserve enough vertical space for the menuList layout element setting its logical height to the max…

Reserve enough vertical space for the menuList layout element setting its logical height to the max option height.

The option's height is calculated from option's text FloatRect generated by CachingWordShaper.

BUG=484632
TEST=fast/forms/select/menulist-height-change.html

Committed: https://crrev.com/4681903723a0df535a60069ec9f7d6ea21fe69d0
Cr-Commit-Position: refs/heads/master@{#399293}

[Gleb Lanbin, 2016-06-08 23:00:39]

Patchset #1 (id:1) has been deleted

[Gleb Lanbin, 2016-06-08 23:00:54]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-08 23:02:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055593002/20001

[commit-bot: I haz the power, 2016-06-08 23:52:38]

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

[commit-bot: I haz the power, 2016-06-08 23:52:39]

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

[Gleb Lanbin, 2016-06-08 23:58:47]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-08 23:59:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055593002/40001

[Gleb Lanbin, 2016-06-09 00:03:35]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-06-09 01:28:35]

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

[commit-bot: I haz the power, 2016-06-09 01:28:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Gleb Lanbin, 2016-06-09 03:18:42]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-09 03:18:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055593002/60001

[Gleb Lanbin, 2016-06-09 03:21:14]

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

[commit-bot: I haz the power, 2016-06-09 05:28:38]

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

[commit-bot: I haz the power, 2016-06-09 05:28:39]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[eae, 2016-06-09 06:50:59]

This looks great.

Please include a TEST= line in the CL description, like this:

TEST=fast/forms/select/menulist-height-change.html

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

https://codereview.chromium.org/2055593002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutMenuList.cpp:173: const
ComputedStyle* itemStyle = element->computedStyle() ? element->computedStyle() :
style();
Is element->style() not guaranteed to be set?

[Gleb Lanbin, 2016-06-09 20:53:53]

Description was changed from

==========
Reserve enough vertical space for the menuList layout element setting its
logical height to the max option height.

The option's height is calculated from option's text FloatRect generated by
CachingWordShaper.

BUG=484632
==========

to

==========
Reserve enough vertical space for the menuList layout element setting its
logical height to the max option height.

The option's height is calculated from option's text FloatRect generated by
CachingWordShaper.

BUG=484632

TEST=fast/forms/select/menulist-height-change.html
==========

[Gleb Lanbin, 2016-06-09 21:08:49]

Thanks for the review.

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

https://codereview.chromium.org/2055593002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutMenuList.cpp:173: const
ComputedStyle* itemStyle = element->computedStyle() ? element->computedStyle() :
style();
On 2016/06/09 06:50:58, eae wrote:
> Is element->style() not guaranteed to be set?  

element->style() will return nullptr, most likely because "option" HTML doesn't
have layout object associated with them at this moment. 

According to this comment
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/NodeC...
<option> can be styled even if they don't get layout objects. Non layout
computed style can be accessed through computedStyle()->
nonLayoutObjectComputedStyle()

[eae, 2016-06-10 06:15:18]

LGTM

Thanks for the clarification.

[Gleb Lanbin, 2016-06-10 16:39:42]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2016-06-10 16:39:42]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2055593002/#ps100001
(title: "NeedsManualRebaseline")

[commit-bot: I haz the power, 2016-06-10 16:40:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055593002/100001

[commit-bot: I haz the power, 2016-06-10 20:52:17]

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

[commit-bot: I haz the power, 2016-06-10 20:52:19]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Gleb Lanbin, 2016-06-10 21:07:53]

The CQ bit was checked by glebl@chromium.org

[commit-bot: I haz the power, 2016-06-10 21:08:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055593002/100001

[commit-bot: I haz the power, 2016-06-10 22:15:29]

Description was changed from

==========
Reserve enough vertical space for the menuList layout element setting its
logical height to the max option height.

The option's height is calculated from option's text FloatRect generated by
CachingWordShaper.

BUG=484632

TEST=fast/forms/select/menulist-height-change.html
==========

to

==========
Reserve enough vertical space for the menuList layout element setting its
logical height to the max option height.

The option's height is calculated from option's text FloatRect generated by
CachingWordShaper.

BUG=484632

TEST=fast/forms/select/menulist-height-change.html
==========

[commit-bot: I haz the power, 2016-06-10 22:15:30]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-06-10 22:17:10]

Description was changed from

==========
Reserve enough vertical space for the menuList layout element setting its
logical height to the max option height.

The option's height is calculated from option's text FloatRect generated by
CachingWordShaper.

BUG=484632

TEST=fast/forms/select/menulist-height-change.html
==========

to

==========
Reserve enough vertical space for the menuList layout element setting its
logical height to the max option height.

The option's height is calculated from option's text FloatRect generated by
CachingWordShaper.

BUG=484632

TEST=fast/forms/select/menulist-height-change.html

Committed: https://crrev.com/4681903723a0df535a60069ec9f7d6ea21fe69d0
Cr-Commit-Position: refs/heads/master@{#399293}
==========

[commit-bot: I haz the power, 2016-06-10 22:17:12]

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

[drott, 2016-06-13 07:29:12]

Nice! Thank you, great to see this fixed.

[tkent, 2016-06-20 23:57:18]

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

[tkent, 2016-06-20 23:57:19]

https://codereview.chromium.org/2055593002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutMenuList.cpp (right):

https://codereview.chromium.org/2055593002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutMenuList.cpp:341: +
LayoutUnit(cInnerBlockVerticalPaddingEm * style()->computedFontSize());
Do we really need to add this?
I'm afraid this caused a regression.  crbug.com/620613