reflow comments in core/layout/compositing,core/observer

reflow comments in core/layout/compositing,core/observer

BUG=563793
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/b236186c2c23bec4ca49a5ec3f20050c9271e22a
Cr-Commit-Position: refs/heads/master@{#423297}

[Nico, 2016-10-05 18:48:37]

Description was changed from

==========
reflow comments in core/layout/compositing,core/observer

BUG=563793
==========

to

==========
reflow comments in core/layout/compositing,core/observer

BUG=563793
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Nico, 2016-10-05 18:48:39]

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

[Nico, 2016-10-05 18:48:48]

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

[commit-bot: I haz the power, 2016-10-05 18:49:06]

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

[eae, 2016-10-05 19:53:01]

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
(right):

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:461:
// contains this squashed layer's clipping ancestor.  The clip rect is
Here and elsewhere: please don't add extra spaces between sentences. While we no
longer have a style rule explicitly prohibiting it that is not a good enough
reason to add it everywhere. Especially as it causes extra wrapping in some
cases.

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:618:
// enclosing composited layer that is not
Extra line break, "squashed)." fits on this line.

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
(right):

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:656:
// coordinates.  The following rect is at (-4000, 190, 4100, 100) in viewport
Breaking after coordinates. here and allowing the full following sentence on a
separate line is probably better.

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:247:
// already have it but need it.  Note that
Break before Note.

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h
(right):

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h:195: //
CompositingLayerAssigner once we've fixed
Rewrap at 80 col.

[Nico, 2016-10-05 19:58:15]

Thanks!

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
(right):

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:461:
// contains this squashed layer's clipping ancestor.  The clip rect is
On 2016/10/05 19:53:00, eae wrote:
> Here and elsewhere: please don't add extra spaces between sentences. While we
no
> longer have a style rule explicitly prohibiting it that is not a good enough
> reason to add it everywhere. Especially as it causes extra wrapping in some
> cases.

I use vim's gq to join lines manually, and it defaults to two spaces
after periods.

Looks like we deliberately decided that we don't care about this a while
ago: https://codereview.chromium.org/609633004

So this seems like the stylistic thing we shouldn't argue about on code reviews
(unless you feel extremely strongly).

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
(right):

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:656:
// coordinates.  The following rect is at (-4000, 190, 4100, 100) in viewport
On 2016/10/05 19:53:00, eae wrote:
> Breaking after coordinates. here and allowing the full following sentence on a
> separate line is probably better.

Done.

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:247:
// already have it but need it.  Note that
On 2016/10/05 19:53:01, eae wrote:
> Break before Note.

Done.

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h
(right):

https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h:195: //
CompositingLayerAssigner once we've fixed
On 2016/10/05 19:53:01, eae wrote:
> Rewrap at 80 col.

Whoops, thanks! Done.

[Nico, 2016-10-05 19:58:29]

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

[commit-bot: I haz the power, 2016-10-05 19:59:09]

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

[eae, 2016-10-05 20:10:19]

> I use vim's gq to join lines manually, and it defaults to two spaces
> after periods.
>
> Looks like we deliberately decided that we don't care about this a while
> ago: https://codereview.chromium.org/609633004
>
> So this seems like the stylistic thing we shouldn't argue about on code
> reviews (unless you feel extremely strongly).

The fact that we no longer explicitly prohibit it is not a good reason
to go back and reformat comments to match your personal preference.
You've had four different reviewers point this out, it's probably time
for you to change your vim configuration.


https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:656:
> // coordinates. The following rect is at (-4000, 190, 4100, 100) in
> viewport
> On 2016/10/05 19:53:00, eae wrote:
>> Breaking after coordinates. here and allowing the full following
> sentence on a
>> separate line is probably better.
>
> Done.

Thanks!

>
>
https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
> File
>
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
> (right):
>
>
https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
>
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:247:
> // already have it but need it. Note that
> On 2016/10/05 19:53:01, eae wrote:
>> Break before Note.
>
> Done.
>
>
https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
> File
> third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h
> (right):
>
>
https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h:195:
> // CompositingLayerAssigner once we've fixed
> On 2016/10/05 19:53:01, eae wrote:
>> Rewrap at 80 col.
>
> Whoops, thanks! Done.

Thanks. that was the only missing one I could find, good job!

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

[eae, 2016-10-05 20:10:20]

> I use vim's gq to join lines manually, and it defaults to two spaces
> after periods.
>
> Looks like we deliberately decided that we don't care about this a while
> ago: https://codereview.chromium.org/609633004
>
> So this seems like the stylistic thing we shouldn't argue about on code
> reviews (unless you feel extremely strongly).

The fact that we no longer explicitly prohibit it is not a good reason
to go back and reformat comments to match your personal preference.
You've had four different reviewers point this out, it's probably time
for you to change your vim configuration.


https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:656:
> // coordinates. The following rect is at (-4000, 190, 4100, 100) in
> viewport
> On 2016/10/05 19:53:00, eae wrote:
>> Breaking after coordinates. here and allowing the full following
> sentence on a
>> separate line is probably better.
>
> Done.

Thanks!

>
>
https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
> File
>
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
> (right):
>
>
https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
>
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:247:
> // already have it but need it. Note that
> On 2016/10/05 19:53:01, eae wrote:
>> Break before Note.
>
> Done.
>
>
https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
> File
> third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h
> (right):
>
>
https://codereview.chromium.org/2393673004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h:195:
> // CompositingLayerAssigner once we've fixed
> On 2016/10/05 19:53:01, eae wrote:
>> Rewrap at 80 col.
>
> Whoops, thanks! Done.

Thanks. that was the only missing one I could find, good job!

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

[Nico, 2016-10-05 20:19:52]

On Wed, Oct 5, 2016 at 4:10 PM, Emil A Eklund <eae@chromium.org> wrote:

> > I use vim's gq to join lines manually, and it defaults to two spaces
> > after periods.
> >
> > Looks like we deliberately decided that we don't care about this a while
> > ago: https://codereview.chromium.org/609633004
> >
> > So this seems like the stylistic thing we shouldn't argue about on code
> > reviews (unless you feel extremely strongly).
>
> The fact that we no longer explicitly prohibit it is not a good reason
> to go back and reformat comments to match your personal preference.
>

I try very hard to not have a personal preference about anything
style-related :-) This isn't my personal preference, it's just what the
tool I used happened to default to.


> You've had four different reviewers point this out, it's probably time
> for you to change your vim configuration.
>

So my view here is that I don't think arguing about the number of spaces
after a . is a good use of time for anyone. We should have tools that
enforce formatting (ideally, in a way where you can push a button and the
formatting is done for you, like with clang-format, not like
check-webkit-style, which only says "wrong"), so that this uninteresting
part of writing and reviewing patches is completely automated. Superficial
stylistic things not handled / enforced by tools should then not be brought
up in code review.

We're not there yet, so if you really want me to change this for this
patch, I'll invest the 10 minutes to do it, but I don't think it's a good
use of your time or mine, and the fact that we explicitly removed the check
for this from check-webkit-style over a year ago suggests to me that the
project agreed on that back then too. (I didn't try and find the thread
where this was discussed.)


>
>  https://codereview.chromium.org/2393673004/diff/1/third_
> party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.
> cpp#newcode656
> > third_party/WebKit/Source/core/layout/compositing/
> CompositedLayerMappingTest.cpp:656:
> > // coordinates. The following rect is at (-4000, 190, 4100, 100) in
> > viewport
> > On 2016/10/05 19:53:00, eae wrote:
> >> Breaking after coordinates. here and allowing the full following
> > sentence on a
> >> separate line is probably better.
> >
> > Done.
>
> Thanks!
>
> >
> > https://codereview.chromium.org/2393673004/diff/1/third_
> party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater
> .cpp
> > File
> > third_party/WebKit/Source/core/layout/compositing/
> CompositingRequirementsUpdater.cpp
> > (right):
> >
> > https://codereview.chromium.org/2393673004/diff/1/third_
> party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater
> .cpp#newcode247
> > third_party/WebKit/Source/core/layout/compositing/
> CompositingRequirementsUpdater.cpp:247:
> > // already have it but need it. Note that
> > On 2016/10/05 19:53:01, eae wrote:
> >> Break before Note.
> >
> > Done.
> >
> > https://codereview.chromium.org/2393673004/diff/1/third_
> party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h
> > File
> > third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h
> > (right):
> >
> > https://codereview.chromium.org/2393673004/diff/1/third_
> party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h#
> newcode195
> > third_party/WebKit/Source/core/layout/compositing/
> PaintLayerCompositor.h:195:
> > // CompositingLayerAssigner once we've fixed
> > On 2016/10/05 19:53:01, eae wrote:
> >> Rewrap at 80 col.
> >
> > Whoops, thanks! Done.
>
> Thanks. that was the only missing one I could find, good job!
>

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

[Nico, 2016-10-05 20:19:53]

On Wed, Oct 5, 2016 at 4:10 PM, Emil A Eklund <eae@chromium.org> wrote:

> > I use vim's gq to join lines manually, and it defaults to two spaces
> > after periods.
> >
> > Looks like we deliberately decided that we don't care about this a while
> > ago: https://codereview.chromium.org/609633004
> >
> > So this seems like the stylistic thing we shouldn't argue about on code
> > reviews (unless you feel extremely strongly).
>
> The fact that we no longer explicitly prohibit it is not a good reason
> to go back and reformat comments to match your personal preference.
>

I try very hard to not have a personal preference about anything
style-related :-) This isn't my personal preference, it's just what the
tool I used happened to default to.


> You've had four different reviewers point this out, it's probably time
> for you to change your vim configuration.
>

So my view here is that I don't think arguing about the number of spaces
after a . is a good use of time for anyone. We should have tools that
enforce formatting (ideally, in a way where you can push a button and the
formatting is done for you, like with clang-format, not like
check-webkit-style, which only says "wrong"), so that this uninteresting
part of writing and reviewing patches is completely automated. Superficial
stylistic things not handled / enforced by tools should then not be brought
up in code review.

We're not there yet, so if you really want me to change this for this
patch, I'll invest the 10 minutes to do it, but I don't think it's a good
use of your time or mine, and the fact that we explicitly removed the check
for this from check-webkit-style over a year ago suggests to me that the
project agreed on that back then too. (I didn't try and find the thread
where this was discussed.)


>
>  https://codereview.chromium.org/2393673004/diff/1/third_
> party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.
> cpp#newcode656
> > third_party/WebKit/Source/core/layout/compositing/
> CompositedLayerMappingTest.cpp:656:
> > // coordinates. The following rect is at (-4000, 190, 4100, 100) in
> > viewport
> > On 2016/10/05 19:53:00, eae wrote:
> >> Breaking after coordinates. here and allowing the full following
> > sentence on a
> >> separate line is probably better.
> >
> > Done.
>
> Thanks!
>
> >
> > https://codereview.chromium.org/2393673004/diff/1/third_
> party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater
> .cpp
> > File
> > third_party/WebKit/Source/core/layout/compositing/
> CompositingRequirementsUpdater.cpp
> > (right):
> >
> > https://codereview.chromium.org/2393673004/diff/1/third_
> party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater
> .cpp#newcode247
> > third_party/WebKit/Source/core/layout/compositing/
> CompositingRequirementsUpdater.cpp:247:
> > // already have it but need it. Note that
> > On 2016/10/05 19:53:01, eae wrote:
> >> Break before Note.
> >
> > Done.
> >
> > https://codereview.chromium.org/2393673004/diff/1/third_
> party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h
> > File
> > third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h
> > (right):
> >
> > https://codereview.chromium.org/2393673004/diff/1/third_
> party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h#
> newcode195
> > third_party/WebKit/Source/core/layout/compositing/
> PaintLayerCompositor.h:195:
> > // CompositingLayerAssigner once we've fixed
> > On 2016/10/05 19:53:01, eae wrote:
> >> Rewrap at 80 col.
> >
> > Whoops, thanks! Done.
>
> Thanks. that was the only missing one I could find, good job!
>

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

[eae, 2016-10-05 20:32:00]

On Wed, Oct 5, 2016 at 1:19 PM, Nico Weber <thakis@chromium.org> wrote:
> On Wed, Oct 5, 2016 at 4:10 PM, Emil A Eklund <eae@chromium.org> wrote:
>>
>> > I use vim's gq to join lines manually, and it defaults to two spaces
>> > after periods.
>> >
>> > Looks like we deliberately decided that we don't care about this a while
>> > ago: https://codereview.chromium.org/609633004
>> >
>> > So this seems like the stylistic thing we shouldn't argue about on code
>> > reviews (unless you feel extremely strongly).
>>
>> The fact that we no longer explicitly prohibit it is not a good reason
>> to go back and reformat comments to match your personal preference.
>
>
> I try very hard to not have a personal preference about anything
> style-related :-) This isn't my personal preference, it's just what the tool
> I used happened to default to.
>
>>
>> You've had four different reviewers point this out, it's probably time
>> for you to change your vim configuration.
>
>
> So my view here is that I don't think arguing about the number of spaces
> after a . is a good use of time for anyone. We should have tools that
> enforce formatting (ideally, in a way where you can push a button and the
> formatting is done for you, like with clang-format, not like
> check-webkit-style, which only says "wrong"), so that this uninteresting
> part of writing and reviewing patches is completely automated. Superficial
> stylistic things not handled / enforced by tools should then not be brought
> up in code review.
>
> We're not there yet, so if you really want me to change this for this patch,
> I'll invest the 10 minutes to do it, but I don't think it's a good use of
> your time or mine, and the fact that we explicitly removed the check for
> this from check-webkit-style over a year ago suggests to me that the project
> agreed on that back then too. (I didn't try and find the thread where this
> was discussed.)

Agreed, we should probably re-add the rule (one way or another). I
don't really care I just thought it was odd that you had gone through
all the comments to add the extra space.

I don't want you (or anyone really) to spend and extra time on this.
LGTM

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

[eae, 2016-10-05 20:32:00]

On Wed, Oct 5, 2016 at 1:19 PM, Nico Weber <thakis@chromium.org> wrote:
> On Wed, Oct 5, 2016 at 4:10 PM, Emil A Eklund <eae@chromium.org> wrote:
>>
>> > I use vim's gq to join lines manually, and it defaults to two spaces
>> > after periods.
>> >
>> > Looks like we deliberately decided that we don't care about this a while
>> > ago: https://codereview.chromium.org/609633004
>> >
>> > So this seems like the stylistic thing we shouldn't argue about on code
>> > reviews (unless you feel extremely strongly).
>>
>> The fact that we no longer explicitly prohibit it is not a good reason
>> to go back and reformat comments to match your personal preference.
>
>
> I try very hard to not have a personal preference about anything
> style-related :-) This isn't my personal preference, it's just what the tool
> I used happened to default to.
>
>>
>> You've had four different reviewers point this out, it's probably time
>> for you to change your vim configuration.
>
>
> So my view here is that I don't think arguing about the number of spaces
> after a . is a good use of time for anyone. We should have tools that
> enforce formatting (ideally, in a way where you can push a button and the
> formatting is done for you, like with clang-format, not like
> check-webkit-style, which only says "wrong"), so that this uninteresting
> part of writing and reviewing patches is completely automated. Superficial
> stylistic things not handled / enforced by tools should then not be brought
> up in code review.
>
> We're not there yet, so if you really want me to change this for this patch,
> I'll invest the 10 minutes to do it, but I don't think it's a good use of
> your time or mine, and the fact that we explicitly removed the check for
> this from check-webkit-style over a year ago suggests to me that the project
> agreed on that back then too. (I didn't try and find the thread where this
> was discussed.)

Agreed, we should probably re-add the rule (one way or another). I
don't really care I just thought it was odd that you had gone through
all the comments to add the extra space.

I don't want you (or anyone really) to spend and extra time on this.
LGTM

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

[eae, 2016-10-05 20:34:30]

LGTM

[Nico, 2016-10-05 20:35:00]

On Wed, Oct 5, 2016 at 4:31 PM, Emil A Eklund <eae@chromium.org> wrote:

> On Wed, Oct 5, 2016 at 1:19 PM, Nico Weber <thakis@chromium.org> wrote:
> > On Wed, Oct 5, 2016 at 4:10 PM, Emil A Eklund <eae@chromium.org> wrote:
> >>
> >> > I use vim's gq to join lines manually, and it defaults to two spaces
> >> > after periods.
> >> >
> >> > Looks like we deliberately decided that we don't care about this a
> while
> >> > ago: https://codereview.chromium.org/609633004
> >> >
> >> > So this seems like the stylistic thing we shouldn't argue about on
> code
> >> > reviews (unless you feel extremely strongly).
> >>
> >> The fact that we no longer explicitly prohibit it is not a good reason
> >> to go back and reformat comments to match your personal preference.
> >
> >
> > I try very hard to not have a personal preference about anything
> > style-related :-) This isn't my personal preference, it's just what the
> tool
> > I used happened to default to.
> >
> >>
> >> You've had four different reviewers point this out, it's probably time
> >> for you to change your vim configuration.
> >
> >
> > So my view here is that I don't think arguing about the number of spaces
> > after a . is a good use of time for anyone. We should have tools that
> > enforce formatting (ideally, in a way where you can push a button and the
> > formatting is done for you, like with clang-format, not like
> > check-webkit-style, which only says "wrong"), so that this uninteresting
> > part of writing and reviewing patches is completely automated.
> Superficial
> > stylistic things not handled / enforced by tools should then not be
> brought
> > up in code review.
> >
> > We're not there yet, so if you really want me to change this for this
> patch,
> > I'll invest the 10 minutes to do it, but I don't think it's a good use of
> > your time or mine, and the fact that we explicitly removed the check for
> > this from check-webkit-style over a year ago suggests to me that the
> project
> > agreed on that back then too. (I didn't try and find the thread where
> this
> > was discussed.)
>
> Agreed, we should probably re-add the rule (one way or another). I
> don't really care I just thought it was odd that you had gone through
> all the comments to add the extra space.
>
> I don't want you (or anyone really) to spend and extra time on this.
> LGTM
>

Thanks! To be clear, I don't have any problems with a consistent "one space
after periods" rule in blink. I don't want to argue about it, though. If
you do the arguing part (driving consensus on blink-dev), I'd happily
re-add the check and fix up all the comments. Or all reviewers could just
be happy with either one or two spaces -- any outcome where this stops
being raised on reviews would be great. (I honestly don't know which of the
two approaches is easier.)

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

[Nico, 2016-10-05 20:35:03]

On Wed, Oct 5, 2016 at 4:31 PM, Emil A Eklund <eae@chromium.org> wrote:

> On Wed, Oct 5, 2016 at 1:19 PM, Nico Weber <thakis@chromium.org> wrote:
> > On Wed, Oct 5, 2016 at 4:10 PM, Emil A Eklund <eae@chromium.org> wrote:
> >>
> >> > I use vim's gq to join lines manually, and it defaults to two spaces
> >> > after periods.
> >> >
> >> > Looks like we deliberately decided that we don't care about this a
> while
> >> > ago: https://codereview.chromium.org/609633004
> >> >
> >> > So this seems like the stylistic thing we shouldn't argue about on
> code
> >> > reviews (unless you feel extremely strongly).
> >>
> >> The fact that we no longer explicitly prohibit it is not a good reason
> >> to go back and reformat comments to match your personal preference.
> >
> >
> > I try very hard to not have a personal preference about anything
> > style-related :-) This isn't my personal preference, it's just what the
> tool
> > I used happened to default to.
> >
> >>
> >> You've had four different reviewers point this out, it's probably time
> >> for you to change your vim configuration.
> >
> >
> > So my view here is that I don't think arguing about the number of spaces
> > after a . is a good use of time for anyone. We should have tools that
> > enforce formatting (ideally, in a way where you can push a button and the
> > formatting is done for you, like with clang-format, not like
> > check-webkit-style, which only says "wrong"), so that this uninteresting
> > part of writing and reviewing patches is completely automated.
> Superficial
> > stylistic things not handled / enforced by tools should then not be
> brought
> > up in code review.
> >
> > We're not there yet, so if you really want me to change this for this
> patch,
> > I'll invest the 10 minutes to do it, but I don't think it's a good use of
> > your time or mine, and the fact that we explicitly removed the check for
> > this from check-webkit-style over a year ago suggests to me that the
> project
> > agreed on that back then too. (I didn't try and find the thread where
> this
> > was discussed.)
>
> Agreed, we should probably re-add the rule (one way or another). I
> don't really care I just thought it was odd that you had gone through
> all the comments to add the extra space.
>
> I don't want you (or anyone really) to spend and extra time on this.
> LGTM
>

Thanks! To be clear, I don't have any problems with a consistent "one space
after periods" rule in blink. I don't want to argue about it, though. If
you do the arguing part (driving consensus on blink-dev), I'd happily
re-add the check and fix up all the comments. Or all reviewers could just
be happy with either one or two spaces -- any outcome where this stops
being raised on reviews would be great. (I honestly don't know which of the
two approaches is easier.)

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

[Nico, 2016-10-05 20:35:17]

The CQ bit was unchecked by thakis@chromium.org

[Nico, 2016-10-05 20:35:24]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2016-10-05 20:36: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-10-05 21:23:00]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-05 21:25:28]

Description was changed from

==========
reflow comments in core/layout/compositing,core/observer

BUG=563793
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
reflow comments in core/layout/compositing,core/observer

BUG=563793
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2016-10-05 21:25:29]

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