Change ComputedStyle::setWritingMode() to return void instead of bool

Change ComputedStyle::setWritingMode() to return void instead of bool

Change ComputedStyle::setWritingMode() to return void instead of bool,
and updated the 2 callsites that use this functionality to match.

There is overhead from checking whether writingMode has changed and
returning instead of setting the value, and this patch removes that
overhead for callers that don't need this functionality.

Also, this is pre-work to generating writing-mode in ComputedStyleBase,
which aims to make all properties regular and share the same getter and
setter patterns where possible.

BUG=628043
Committed: https://crrev.com/5fb8c41aea7e2a5a7ec8f33a3c6bee46eb86d95d
Cr-Commit-Position: refs/heads/master@{#438448}

[sashab, 2016-12-12 02:24:06]

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

[sashab, 2016-12-12 02:24:06]

sashab@chromium.org changed reviewers:
+ napper@chromium.org

[commit-bot: I haz the power, 2016-12-12 02:24:21]

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

[sashab, 2016-12-12 02:26:08]

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

[sashab, 2016-12-12 02:26:17]

Small change that can be un-done haha

https://codereview.chromium.org/2567963002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/2567963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleResolverState.h:187: if
(oldWritingMode != newWritingMode) {
For no reason whatsoever, changed this to be an early return. It's a little
prettier maybe? ^_^

[commit-bot: I haz the power, 2016-12-12 02:26:26]

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

[napper, 2016-12-12 03:53:33]

lgtm

https://codereview.chromium.org/2567963002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/2567963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolverState.h:186: const
WritingMode oldWritingMode = m_style->getWritingMode();
Not sure you need the temporary here.

https://codereview.chromium.org/2567963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolverState.h:187: if
(oldWritingMode == newWritingMode)
FWIW I have a style preference for either:

if (oldWritingMode == newWritingMode) return;

- or -

if (oldWritingMode == newWritingMode) {
  return;
}

since I have seen many examples of the following bug:

if (oldWritingMode == newWritingMode)
  <added line here>
  return;

[sashab, 2016-12-12 03:58:38]

sashab@chromium.org changed reviewers:
+ alancutter@chromium.org

[sashab, 2016-12-12 03:58:39]

Thanks napper@!

Alancutter@ please review, mainly your thoughts on the early return braces... I
thought it said somewhere in our style guide that single-line return statements
must not use braces but now I can't find it, and it's not enforced by any
presubmit checker...

https://codereview.chromium.org/2567963002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/2567963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolverState.h:186: const
WritingMode oldWritingMode = m_style->getWritingMode();
On 2016/12/12 at 03:53:33, napper wrote:
> Not sure you need the temporary here.

Sure, thought it helped with readability. Kept the other one called
'newWritingMode' to make it clear.

https://codereview.chromium.org/2567963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolverState.h:187: if
(oldWritingMode == newWritingMode)
On 2016/12/12 at 03:53:33, napper wrote:
> FWIW I have a style preference for either:
> 
> if (oldWritingMode == newWritingMode) return;
> 
> - or -
> 
> if (oldWritingMode == newWritingMode) {
>   return;
> }
> 
> since I have seen many examples of the following bug:
> 
> if (oldWritingMode == newWritingMode)
>   <added line here>
>   return;

Ack, that's a really good point!! I actually thought our style checker presubmit
would reject these brackets since the statement is only one line, but it didn't
:) Will get alancutter@s opinion too.

[alancutter (OOO until 2018), 2016-12-13 00:01:44]

lgtm

On 2016/12/12 at 03:58:39, sashab wrote:
> Thanks napper@!
> 
> Alancutter@ please review, mainly your thoughts on the early return braces...
I thought it said somewhere in our style guide that single-line return
statements must not use braces but now I can't find it, and it's not enforced by
any presubmit checker...

That was when we were using WebKit's style guide, now it's optional.
Personally I'd prefer to always use braces when using 2 space indents because
it's not as obvious as it used to be with 4 space indents.

[sashab, 2016-12-13 00:07:11]

Cool, agreed.

[sashab, 2016-12-13 00:07:13]

The CQ bit was checked by sashab@chromium.org

[sashab, 2016-12-13 00:07:14]

The patchset sent to the CQ was uploaded after l-g-t-m from napper@chromium.org
Link to the patchset: https://codereview.chromium.org/2567963002/#ps40001
(title: "Review feedback")

[commit-bot: I haz the power, 2016-12-13 00:08:13]

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-12-13 02:07:44]

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

[commit-bot: I haz the power, 2016-12-13 02:07:45]

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

[sashab, 2016-12-14 04:57:10]

The CQ bit was checked by sashab@chromium.org

[sashab, 2016-12-14 04:57:10]

The patchset sent to the CQ was uploaded after l-g-t-m from napper@chromium.org,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2567963002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-12-14 04:57:37]

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-12-14 06:23:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481691430235260,
"parent_rev": "c836dfbc44c6a33003e57171ad37aaba2a8336d9", "commit_rev":
"f11aed415beed4a26e2f7c804e484ae71a4683e8"}

[commit-bot: I haz the power, 2016-12-14 06:24:16]

Description was changed from

==========
Change ComputedStyle::setWritingMode() to return void instead of bool

Change ComputedStyle::setWritingMode() to return void instead of bool,
and updated the 2 callsites that use this functionality to match.

There is overhead from checking whether writingMode has changed and
returning instead of setting the value, and this patch removes that
overhead for callers that don't need this functionality.

Also, this is pre-work to generating writing-mode in ComputedStyleBase,
which aims to make all properties regular and share the same getter and
setter patterns where possible.

BUG=628043
==========

to

==========
Change ComputedStyle::setWritingMode() to return void instead of bool

Change ComputedStyle::setWritingMode() to return void instead of bool,
and updated the 2 callsites that use this functionality to match.

There is overhead from checking whether writingMode has changed and
returning instead of setting the value, and this patch removes that
overhead for callers that don't need this functionality.

Also, this is pre-work to generating writing-mode in ComputedStyleBase,
which aims to make all properties regular and share the same getter and
setter patterns where possible.

BUG=628043

Review-Url: https://codereview.chromium.org/2567963002
==========

[commit-bot: I haz the power, 2016-12-14 06:24:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-14 06:28:05]

Description was changed from

==========
Change ComputedStyle::setWritingMode() to return void instead of bool

Change ComputedStyle::setWritingMode() to return void instead of bool,
and updated the 2 callsites that use this functionality to match.

There is overhead from checking whether writingMode has changed and
returning instead of setting the value, and this patch removes that
overhead for callers that don't need this functionality.

Also, this is pre-work to generating writing-mode in ComputedStyleBase,
which aims to make all properties regular and share the same getter and
setter patterns where possible.

BUG=628043

Review-Url: https://codereview.chromium.org/2567963002
==========

to

==========
Change ComputedStyle::setWritingMode() to return void instead of bool

Change ComputedStyle::setWritingMode() to return void instead of bool,
and updated the 2 callsites that use this functionality to match.

There is overhead from checking whether writingMode has changed and
returning instead of setting the value, and this patch removes that
overhead for callers that don't need this functionality.

Also, this is pre-work to generating writing-mode in ComputedStyleBase,
which aims to make all properties regular and share the same getter and
setter patterns where possible.

BUG=628043

Committed: https://crrev.com/5fb8c41aea7e2a5a7ec8f33a3c6bee46eb86d95d
Cr-Commit-Position: refs/heads/master@{#438448}
==========

[commit-bot: I haz the power, 2016-12-14 06:28:07]

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