Set LayoutListBox's height as 'size' attribute * (height of the tallest option).

Set LayoutListBox's height as 'size' attribute * (height of the tallest option).

This will change the current behaviour where we set the size of HTML select element based on the height of first option.
The new behaviour is consistent with other browsers, e.g. Firefox sizes the box as (size * (height of the tallest option)).

BUG=616187
Committed: https://crrev.com/e676eabb8a1e897bbc31a228fe5e49dab06d53e3
Cr-Commit-Position: refs/heads/master@{#401166}

[Gleb Lanbin, 2016-06-10 19:53:31]

Patchset #1 (id:1) has been deleted

[Gleb Lanbin, 2016-06-10 19:53:49]

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

[commit-bot: I haz the power, 2016-06-10 19:54:35]

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

[Gleb Lanbin, 2016-06-10 20:03:11]

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

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

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

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

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-10 22:22:00]

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

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

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

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

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

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

Dry run: Try jobs failed on following builders:
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Gleb Lanbin, 2016-06-10 22:55:47]

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

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

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

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

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

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

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_...)

[szager1, 2016-06-13 19:46:37]

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

[szager1, 2016-06-13 19:46:38]

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

https://codereview.chromium.org/2060493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListBox.cpp:87: LayoutObject*
layoutObject = toLayoutBox(element->layoutObject());
Remove toLayoutBox:

LayoutObject* layoutObject = element->layoutObject();

https://codereview.chromium.org/2060493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListBox.cpp:89: maxHeight =
std::max(maxHeight, toLayoutBox(element->layoutObject())->size().height());
Use layoutObject here, rather than calling element->layoutObject() again.

https://codereview.chromium.org/2060493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListBox.cpp:91: return
std::max(maxHeight, defaultItemHeight());
I don't think this is quite right; you shouldn't expand to defaultItemHeight()
if all of the items have computed sizes.  I think the loop should be:

for (Element* element ...) {
  ...
  LayoutUnit itemHeight;
  if (layoutObject && layoutObject->isBox())
    itemHeight = toLayoutBox(layoutObject)->size().height();
  else
    itemHeight = defaultItemHeight();
  maxHeight = std::max(maxHeight, itemHeight);
}
return maxHeight;

[Gleb Lanbin, 2016-06-13 22:12:55]

Patchset #4 (id:80001) has been deleted

[Gleb Lanbin, 2016-06-13 22:13:38]

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

https://codereview.chromium.org/2060493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListBox.cpp:87: LayoutObject*
layoutObject = toLayoutBox(element->layoutObject());
On 2016/06/13 19:46:38, szager1 wrote:
> Remove toLayoutBox:
> 
> LayoutObject* layoutObject = element->layoutObject();

Done.

https://codereview.chromium.org/2060493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListBox.cpp:89: maxHeight =
std::max(maxHeight, toLayoutBox(element->layoutObject())->size().height());
On 2016/06/13 19:46:38, szager1 wrote:
> Use layoutObject here, rather than calling element->layoutObject() again.

Done.

https://codereview.chromium.org/2060493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListBox.cpp:91: return
std::max(maxHeight, defaultItemHeight());
On 2016/06/13 19:46:38, szager1 wrote:
> I don't think this is quite right; you shouldn't expand to defaultItemHeight()
> if all of the items have computed sizes.  I think the loop should be:
> 
> for (Element* element ...) {
>   ...
>   LayoutUnit itemHeight;
>   if (layoutObject && layoutObject->isBox())
>     itemHeight = toLayoutBox(layoutObject)->size().height();
>   else
>     itemHeight = defaultItemHeight();
>   maxHeight = std::max(maxHeight, itemHeight);
> }
> return maxHeight;

Done. thanks.

[szager1, 2016-06-13 22:46:00]

lgtm, but please wait for eae.

[eae, 2016-06-21 09:09:11]

LGTM

Thank you!

[eae, 2016-06-21 09:09:19]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2016-06-21 09:09:51]

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

[commit-bot: I haz the power, 2016-06-21 09:11:56]

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

[commit-bot: I haz the power, 2016-06-21 09:11:58]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[Gleb Lanbin, 2016-06-21 17:53:09]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2016-06-21 17:53:09]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
szager@chromium.org
Link to the patchset: https://codereview.chromium.org/2060493002/#ps120001
(title: "Resolve a merge conflict")

[commit-bot: I haz the power, 2016-06-21 17:53:48]

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

[commit-bot: I haz the power, 2016-06-21 19:00:18]

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

[commit-bot: I haz the power, 2016-06-21 19:00:20]

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-21 21:18:49]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2016-06-21 21:18:49]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
szager@chromium.org
Link to the patchset: https://codereview.chromium.org/2060493002/#ps140001
(title: "2nd merge conflict")

[commit-bot: I haz the power, 2016-06-21 21:19:30]

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

[commit-bot: I haz the power, 2016-06-22 00:35:23]

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

[commit-bot: I haz the power, 2016-06-22 00:35:24]

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-22 00:41:32]

The CQ bit was checked by glebl@chromium.org

[commit-bot: I haz the power, 2016-06-22 00:42:05]

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

[commit-bot: I haz the power, 2016-06-22 02:18:07]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-06-22 02:20:18]

Description was changed from

==========
Set LayoutListBox's height as 'size' attribute * (height of the tallest option).

This will change the current behaviour where we set the size of HTML select
element based on the height of first option.
The new behaviour is consistent with other browsers, e.g. Firefox sizes the box
as (size * (height of the tallest option)).

BUG=616187
==========

to

==========
Set LayoutListBox's height as 'size' attribute * (height of the tallest option).

This will change the current behaviour where we set the size of HTML select
element based on the height of first option.
The new behaviour is consistent with other browsers, e.g. Firefox sizes the box
as (size * (height of the tallest option)).

BUG=616187

Committed: https://crrev.com/e676eabb8a1e897bbc31a228fe5e49dab06d53e3
Cr-Commit-Position: refs/heads/master@{#401166}
==========

[commit-bot: I haz the power, 2016-06-22 02:20:19]

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