|
|
Created:
4 years ago by sashab Modified:
4 years ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-layout_chromium.org, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rwlbuis, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange 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}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Changed to early return #
Total comments: 4
Patch Set 3 : Review feedback #Patch Set 4 : Rebase #
Messages
Total messages: 25 (14 generated)
The CQ bit was checked by sashab@chromium.org to run a CQ dry run
sashab@chromium.org changed reviewers: + napper@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sashab@chromium.org to run a CQ dry run
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? ^_^
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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@chromium.org changed reviewers: + alancutter@chromium.org
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.
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.
Cool, agreed.
The CQ bit was checked by sashab@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by sashab@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481691430235260, "parent_rev": "c836dfbc44c6a33003e57171ad37aaba2a8336d9", "commit_rev": "f11aed415beed4a26e2f7c804e484ae71a4683e8"}
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5fb8c41aea7e2a5a7ec8f33a3c6bee46eb86d95d Cr-Commit-Position: refs/heads/master@{#438448} |