Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2894393002:
[DMC #10] Add type cast for TextMatchMarkerListImpl (Closed)
DescriptionAdd type cast for TextMatchMarkerListImpl
This will be used in DocumentMarkerController to cast a DocumentMarkerList* to
TextMatchMarkerListImpl* to call some rendered rect-related methods I'm going to
add to TextMatchMarkerListImpl.
BUG=707867
Review-Url: https://codereview.chromium.org/2894393002
Cr-Commit-Position: refs/heads/master@{#474230}
Committed: https://chromium.googlesource.com/chromium/src/+/a590e1184af37fc63ff311976de04dda8d48eb02
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Add MarkerType() test case #Patch Set 4 : Rebase #
Dependent Patchsets: Messages
Total messages: 30 (21 generated)
The CQ bit was checked by rlanday@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...
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
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 rlanday@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.
Could you add test cases, true and false, for type predicate?
On 2017/05/22 at 05:15:29, yosin wrote: > Could you add test cases, true and false, for type predicate? Sorry, I'm not sure exactly what you're asking for, can you please clarify? How can I write a true/false test case if MarkerType() doesn't return a bool?
On 2017/05/22 at 05:19:24, rlanday wrote: > On 2017/05/22 at 05:15:29, yosin wrote: > > Could you add test cases, true and false, for type predicate? > > Sorry, I'm not sure exactly what you're asking for, can you please clarify? How can I write a true/false test case if MarkerType() doesn't return a bool? EXPECT_TRUE(isTextMatchMarkerImpl(new TextMatchMarkerListImpl(...)); EXPECT_FALSE(isTextMatchMarkerImpl(new SpellCheckerMarkerListImpl(...)); It seems there are no MarkerType() test in TextMatchMarkerListImplTest.cpp. It is better to have: EXPECT_EQ(DM::TextMatchMarker, (new TextMathcMarkerListImpl())->MarkerType()); DEFINE_TYPE_CASTS() defines number of functions, isXX(), toXX(). When we miss use toXX(), compiler catches them. For isXX(), logic in DEFINE_TYPE_CASTS(), we want to verify that logic written correctly.
The CQ bit was checked by rlanday@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...
I couldn't actually find any isXX() methods defined by DEFINE_TYPE_CASTS: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/wtf/A... I added a test case for MarkerType() returning the correct value though. Is this OK? If this is what you want, I can put up a CL for the other marker list impls to match.
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 rlanday@chromium.org to run a CQ dry run
The CQ bit was checked by rlanday@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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/05/22 at 18:03:39, rlanday wrote: > I couldn't actually find any isXX() methods defined by DEFINE_TYPE_CASTS: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/wtf/A... > > I added a test case for MarkerType() returning the correct value though. Is this OK? If this is what you want, I can put up a CL for the other marker list impls to match. Oh sorry. I'm confused with DEFINE_TYPE_CAST.
The CQ bit was checked by yosin@chromium.org
lgtm
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": 1495616764966570,
"parent_rev": "1806913f25daa9da5ca69a0115fe74addaa35d32", "commit_rev":
"a590e1184af37fc63ff311976de04dda8d48eb02"}
Message was sent while issue was closed.
Description was changed from ========== Add type cast for TextMatchMarkerListImpl This will be used in DocumentMarkerController to cast a DocumentMarkerList* to TextMatchMarkerListImpl* to call some rendered rect-related methods I'm going to add to TextMatchMarkerListImpl. BUG=707867 ========== to ========== Add type cast for TextMatchMarkerListImpl This will be used in DocumentMarkerController to cast a DocumentMarkerList* to TextMatchMarkerListImpl* to call some rendered rect-related methods I'm going to add to TextMatchMarkerListImpl. BUG=707867 Review-Url: https://codereview.chromium.org/2894393002 Cr-Commit-Position: refs/heads/master@{#474230} Committed: https://chromium.googlesource.com/chromium/src/+/a590e1184af37fc63ff311976de0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a590e1184af37fc63ff311976de0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
