Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740953003:
Add TODO to deprecate ComputedStyle::operator==. (Closed)
DescriptionAdd TODO to deprecate ComputedStyle::operator==.
ComputedStyle::operator== excludes some data members in the comparison,
and it's not clear what members are compared and what are not. Ideally,
the equality operator would be replaced with a named method that better
describes which data members are compared.
BUG=628043
Review-Url: https://codereview.chromium.org/2740953003
Cr-Commit-Position: refs/heads/master@{#456177}
Committed: https://chromium.googlesource.com/chromium/src/+/db0b85bc46d4602f7b275001f26cbf6052267d35
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase #
Total comments: 1
Patch Set 3 : Address comments #Messages
Total messages: 18 (11 generated)
shend@chromium.org changed reviewers: + sashab@chromium.org
Hi Sasha, PTAL
lgtm https://codereview.chromium.org/2740953003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2740953003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.h:2341: // TODO(shend): deprecate operator== in favour of a named method nit, start with capital letter and end with full stop (see style guide). Also word it in a more actionable way, eg: "Replace users of operator== with a named method instead, e.g. inheritedEqual()." https://codereview.chromium.org/2740953003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.h:2349: inline bool independentInheritedEqual(const ComputedStyle&) const; Unrelated to this patch... Check with dcheng/esprehn but I think the naming convention we are aiming for is "isEqual", "isEmpty", "isInheritedEqual" etc for boolean comparison methods. So we might wanna rename these at some point, maybe add a todo as well (you can just do it in this patch, or make a new patch and get them to review it).
Re: naming convention for "isEqual", yeah I'll do it a separate patch. https://codereview.chromium.org/2740953003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2740953003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.h:2341: // TODO(shend): deprecate operator== in favour of a named method On 2017/03/09 at 22:57:45, sashab wrote: > nit, start with capital letter and end with full stop (see style guide). Also word it in a more actionable way, eg: > > "Replace users of operator== with a named method instead, e.g. inheritedEqual()." Got it. Done.
Still LGTM but... :) https://codereview.chromium.org/2740953003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2740953003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:2341: // TODO(shend): Replace callers of operator== wth a named method instead, e.g. Nit - end with full stop ;)
On 2017/03/09 at 23:38:38, sashab wrote: > Still LGTM but... :) > > https://codereview.chromium.org/2740953003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/style/ComputedStyle.h (right): > > https://codereview.chromium.org/2740953003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/style/ComputedStyle.h:2341: // TODO(shend): Replace callers of operator== wth a named method instead, e.g. > Nit - end with full stop ;) LOL can't believe I missed that.
The CQ bit was checked by shend@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org Link to the patchset: https://codereview.chromium.org/2740953003/#ps40001 (title: "Address comments")
The CQ bit was unchecked by shend@chromium.org
The CQ bit was checked by shend@chromium.org to run a CQ dry run
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shend@chromium.org
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": 40001, "attempt_start_ts": 1489182386820910,
"parent_rev": "cb0a2dd7de80140a3dcf0bfef6e44fa8bf80432c", "commit_rev":
"db0b85bc46d4602f7b275001f26cbf6052267d35"}
Message was sent while issue was closed.
Description was changed from ========== Add TODO to deprecate ComputedStyle::operator==. ComputedStyle::operator== excludes some data members in the comparison, and it's not clear what members are compared and what are not. Ideally, the equality operator would be replaced with a named method that better describes which data members are compared. BUG=628043 ========== to ========== Add TODO to deprecate ComputedStyle::operator==. ComputedStyle::operator== excludes some data members in the comparison, and it's not clear what members are compared and what are not. Ideally, the equality operator would be replaced with a named method that better describes which data members are compared. BUG=628043 Review-Url: https://codereview.chromium.org/2740953003 Cr-Commit-Position: refs/heads/master@{#456177} Committed: https://chromium.googlesource.com/chromium/src/+/db0b85bc46d4602f7b275001f26c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/db0b85bc46d4602f7b275001f26c... |
