Fix incorrectly positioned block element inside list-style-position:inside item

Fix incorrectly positioned block element inside list-style-position:inside item

This fixes the interop issue with incorrectly positioned block element inside of LayoutListItem(list-style-position: inline).
CSS WG discussion: http://lists.w3.org/Archives/Public/www-style/2015Mar/0163.html

Details:
list-style-position:inside makes the ::marker pseudo an ordinary position:static element
that needs to be wrapped in an anonymous block before the block element.
This patch changes the current behaviour where we try to position the marker as the first inline child of the block.

Accessibility changes:
this patch changes src/content/test/data/accessibility/aria/aria-owns.html accessibility test. See the link below for screenshots
https://bugs.chromium.org/p/chromium/issues/detail?id=468092#c9

BUG=468092
TEST=third_party/WebKit/LayoutTests/css2.1/20110323/list-style-position-005.htm

Committed: https://crrev.com/36d85fd5ae96048efcef076a209655ddf2bd02fe
Cr-Commit-Position: refs/heads/master@{#417688}

[Gleb Lanbin, 2016-09-09 01:21:22]

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

[commit-bot: I haz the power, 2016-09-09 01:21:49]

Dry run: 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-09 03:02:16]

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

[commit-bot: I haz the power, 2016-09-09 03:02:17]

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

[Gleb Lanbin, 2016-09-09 03:47:21]

Description was changed from

==========
Fix incorrectly positioned block element inside list-style-position:inside item

This fixes the interop issue with incorrectly positioned block element inside of
LayoutListItem(list-style-position: inline).
CSS WG discussion:
http://lists.w3.org/Archives/Public/www-style/2015Mar/0163.html

Details:
list-style-position:inside makes the ::marker pseudo an ordinary position:static
element
that needs to be wrapped in an anonymous block before the block element.
This patch changes the current behaviour where we try to position the marker as
the first inline child of the block.

BUG=468092
TEST=third_party/WebKit/LayoutTests/css2.1/20110323/list-style-position-005.htm
==========

to

==========
Fix incorrectly positioned block element inside list-style-position:inside item

This fixes the interop issue with incorrectly positioned block element inside of
LayoutListItem(list-style-position: inline).
CSS WG discussion:
http://lists.w3.org/Archives/Public/www-style/2015Mar/0163.html

Details:
list-style-position:inside makes the ::marker pseudo an ordinary position:static
element
that needs to be wrapped in an anonymous block before the block element.
This patch changes the current behaviour where we try to position the marker as
the first inline child of the block.

BUG=468092
TEST=third_party/WebKit/LayoutTests/css2.1/20110323/list-style-position-005.htm
==========

[Gleb Lanbin, 2016-09-09 03:47:22]

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

[Gleb Lanbin, 2016-09-09 05:50:19]

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

[commit-bot: I haz the power, 2016-09-09 05:50:29]

Dry run: 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-09 08:21:27]

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

[commit-bot: I haz the power, 2016-09-09 08:21:28]

Dry run: 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_...)

[eae, 2016-09-09 08:55:56]

LGTM w/nit

https://codereview.chromium.org/2328623003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutListItem.cpp (right):

https://codereview.chromium.org/2328623003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListItem.cpp:36: LayoutObject*
getParentOfFirstLineBox(LayoutBlockFlow* curr, LayoutListMarker* marker)
Why not make it a static inline instead?

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

https://codereview.chromium.org/2328623003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutListItem.cpp (right):

https://codereview.chromium.org/2328623003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListItem.cpp:36: LayoutObject*
getParentOfFirstLineBox(LayoutBlockFlow* curr, LayoutListMarker* marker)
On 2016/09/09 08:55:56, eae wrote:
> Why not make it a static inline instead?

'inline' is recommended for function that are small(<10 lines of code)
the link below briefly describes why inline functions may make programs slower
in some cases slower

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[eae, 2016-09-09 16:18:40]

On 2016/09/09 16:06:53, glebl wrote:
>
https://codereview.chromium.org/2328623003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutListItem.cpp (right):
> 
>
https://codereview.chromium.org/2328623003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutListItem.cpp:36: LayoutObject*
> getParentOfFirstLineBox(LayoutBlockFlow* curr, LayoutListMarker* marker)
> On 2016/09/09 08:55:56, eae wrote:
> > Why not make it a static inline instead?
> 
> 'inline' is recommended for function that are small(<10 lines of code)
> the link below briefly describes why inline functions may make programs slower
> in some cases slower
> 
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

It's only used in a single place so the code size argument doesn't really apply.
Traditionally in WebKit we've used static inlines for file-local functions. If
you prefer to make it a non-inline though that's fine :)

Please keep it in the same place as before though, or move it to right above the
function it's called from.

[Gleb Lanbin, 2016-09-09 16:24:23]

On 2016/09/09 16:18:40, eae wrote:
> On 2016/09/09 16:06:53, glebl wrote:
> >
>
https://codereview.chromium.org/2328623003/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutListItem.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2328623003/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutListItem.cpp:36: LayoutObject*
> > getParentOfFirstLineBox(LayoutBlockFlow* curr, LayoutListMarker* marker)
> > On 2016/09/09 08:55:56, eae wrote:
> > > Why not make it a static inline instead?
> > 
> > 'inline' is recommended for function that are small(<10 lines of code)
> > the link below briefly describes why inline functions may make programs
slower
> > in some cases slower
> > 
> >
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
> 
> It's only used in a single place so the code size argument doesn't really
apply.
> Traditionally in WebKit we've used static inlines for file-local functions. If
> you prefer to make it a non-inline though that's fine :)
> 
> Please keep it in the same place as before though, or move it to right above
the
> function it's called from.

done

[eae, 2016-09-09 16:31:04]

The CQ bit was checked by eae@chromium.org

[eae, 2016-09-09 16:31:05]

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/2328623003/#ps40001
(title: "move getParentOfFirstLineBox back")

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

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

[eae, 2016-09-09 16:32:19]

Thanks (you can still keep it in a separate namespace, I just prefer the
function to be defined closer to where it is used).

[Gleb Lanbin, 2016-09-09 16:32:54]

Description was changed from

==========
Fix incorrectly positioned block element inside list-style-position:inside item

This fixes the interop issue with incorrectly positioned block element inside of
LayoutListItem(list-style-position: inline).
CSS WG discussion:
http://lists.w3.org/Archives/Public/www-style/2015Mar/0163.html

Details:
list-style-position:inside makes the ::marker pseudo an ordinary position:static
element
that needs to be wrapped in an anonymous block before the block element.
This patch changes the current behaviour where we try to position the marker as
the first inline child of the block.

BUG=468092
TEST=third_party/WebKit/LayoutTests/css2.1/20110323/list-style-position-005.htm
==========

to

==========
Fix incorrectly positioned block element inside list-style-position:inside item

This fixes the interop issue with incorrectly positioned block element inside of
LayoutListItem(list-style-position: inline).
CSS WG discussion:
http://lists.w3.org/Archives/Public/www-style/2015Mar/0163.html

Details:
list-style-position:inside makes the ::marker pseudo an ordinary position:static
element
that needs to be wrapped in an anonymous block before the block element.
This patch changes the current behaviour where we try to position the marker as
the first inline child of the block.

Accessibility changes:
this patch changes src/content/test/data/accessibility/aria/aria-owns.html
accessibility test. See the link below for screenshots
https://bugs.chromium.org/p/chromium/issues/detail?id=468092#c9 

BUG=468092
TEST=third_party/WebKit/LayoutTests/css2.1/20110323/list-style-position-005.htm
==========

[Gleb Lanbin, 2016-09-09 16:35:58]

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

[Gleb Lanbin, 2016-09-09 16:37:56]

Alice, can you please take a look if you OK with the change from the
accessibility side. Please see
https://bugs.chromium.org/p/chromium/issues/detail?id=468092#c9 for screenshots.

[Gleb Lanbin, 2016-09-09 16:48:27]

On 2016/09/09 16:32:19, eae wrote:
> Thanks (you can still keep it in a separate namespace, I just prefer the
> function to be defined closer to where it is used).

as the one who came from the Java world I like the file organization proposed by
Oracle 
https://web.archive.org/web/20130516014426/http://www.oracle.com/technetwork/...
i.e.
static methods first, then public, protected and private. google3 c++ partially
follows this convention as well, i.e. unnamed namespace, static, constructors,
public, protected, private etc. I'm fine with either way.

since the final version of this patch doesn't require changes in
getParentOfFirstLineBox, I think it makes sense to put it back. Hope it's OK.
Thank you.

[Gleb Lanbin, 2016-09-09 16:48:57]

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

[commit-bot: I haz the power, 2016-09-09 16:49:24]

Dry run: 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-09 19:18:29]

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

[commit-bot: I haz the power, 2016-09-09 19:18:30]

Dry run: This issue passed the CQ dry run.

[aboxhall, 2016-09-09 20:10:24]

lgtm

I'm happy with this a11y-wise - it's weird, but it looks weird too, so I think
the weirdness is consistent.

[Gleb Lanbin, 2016-09-09 20:11:51]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2016-09-09 20:11:52]

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/2328623003/#ps60001
(title: "update aria-owns-expected-mac.txt")

[commit-bot: I haz the power, 2016-09-09 20:12:28]

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-09 20:17:46]

Description was changed from

==========
Fix incorrectly positioned block element inside list-style-position:inside item

This fixes the interop issue with incorrectly positioned block element inside of
LayoutListItem(list-style-position: inline).
CSS WG discussion:
http://lists.w3.org/Archives/Public/www-style/2015Mar/0163.html

Details:
list-style-position:inside makes the ::marker pseudo an ordinary position:static
element
that needs to be wrapped in an anonymous block before the block element.
This patch changes the current behaviour where we try to position the marker as
the first inline child of the block.

Accessibility changes:
this patch changes src/content/test/data/accessibility/aria/aria-owns.html
accessibility test. See the link below for screenshots
https://bugs.chromium.org/p/chromium/issues/detail?id=468092#c9 

BUG=468092
TEST=third_party/WebKit/LayoutTests/css2.1/20110323/list-style-position-005.htm
==========

to

==========
Fix incorrectly positioned block element inside list-style-position:inside item

This fixes the interop issue with incorrectly positioned block element inside of
LayoutListItem(list-style-position: inline).
CSS WG discussion:
http://lists.w3.org/Archives/Public/www-style/2015Mar/0163.html

Details:
list-style-position:inside makes the ::marker pseudo an ordinary position:static
element
that needs to be wrapped in an anonymous block before the block element.
This patch changes the current behaviour where we try to position the marker as
the first inline child of the block.

Accessibility changes:
this patch changes src/content/test/data/accessibility/aria/aria-owns.html
accessibility test. See the link below for screenshots
https://bugs.chromium.org/p/chromium/issues/detail?id=468092#c9 

BUG=468092
TEST=third_party/WebKit/LayoutTests/css2.1/20110323/list-style-position-005.htm
==========

[commit-bot: I haz the power, 2016-09-09 20:17:47]

Committed patchset #4 (id:60001)

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

Description was changed from

==========
Fix incorrectly positioned block element inside list-style-position:inside item

This fixes the interop issue with incorrectly positioned block element inside of
LayoutListItem(list-style-position: inline).
CSS WG discussion:
http://lists.w3.org/Archives/Public/www-style/2015Mar/0163.html

Details:
list-style-position:inside makes the ::marker pseudo an ordinary position:static
element
that needs to be wrapped in an anonymous block before the block element.
This patch changes the current behaviour where we try to position the marker as
the first inline child of the block.

Accessibility changes:
this patch changes src/content/test/data/accessibility/aria/aria-owns.html
accessibility test. See the link below for screenshots
https://bugs.chromium.org/p/chromium/issues/detail?id=468092#c9 

BUG=468092
TEST=third_party/WebKit/LayoutTests/css2.1/20110323/list-style-position-005.htm
==========

to

==========
Fix incorrectly positioned block element inside list-style-position:inside item

This fixes the interop issue with incorrectly positioned block element inside of
LayoutListItem(list-style-position: inline).
CSS WG discussion:
http://lists.w3.org/Archives/Public/www-style/2015Mar/0163.html

Details:
list-style-position:inside makes the ::marker pseudo an ordinary position:static
element
that needs to be wrapped in an anonymous block before the block element.
This patch changes the current behaviour where we try to position the marker as
the first inline child of the block.

Accessibility changes:
this patch changes src/content/test/data/accessibility/aria/aria-owns.html
accessibility test. See the link below for screenshots
https://bugs.chromium.org/p/chromium/issues/detail?id=468092#c9

BUG=468092
TEST=third_party/WebKit/LayoutTests/css2.1/20110323/list-style-position-005.htm

Committed: https://crrev.com/36d85fd5ae96048efcef076a209655ddf2bd02fe
Cr-Commit-Position: refs/heads/master@{#417688}
==========

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

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