Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1774123006:
Implement link selection on alt+mouse drag. (Closed)
DescriptionImplement link selection on alt+mouse drag.
BUG=244738
TEST=webkit_unit_tests --gtest_filter=LinkSelectionTest.*
Committed: https://crrev.com/9e80a9e5b5b71a48119225a614b7c0b8369badf2
Cr-Commit-Position: refs/heads/master@{#383684}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address comments. #
Total comments: 7
Patch Set 3 : Improve |waitForDeferredTasks()|. #Patch Set 4 : Remove static and rename function. #
Total comments: 11
Patch Set 5 : Address comments. #
Total comments: 1
Patch Set 6 : Rename SelectionTestBase -> WebSelectionTestBase. #Patch Set 7 : Rename WebSelectionTestBase -> WebViewSelectionTestBase. #Patch Set 8 : Move SelectionTestBase to LinkSelectionTest.cpp. #
Total comments: 2
Patch Set 9 : Use String instead of std::string. #Messages
Total messages: 46 (9 generated)
kotenkov@yandex-team.ru changed reviewers: + yosin@chromium.org
Hi, yosin, here is the patch regarding link selection that I have promised you. PTAL. Would you please run try jobs for me? I have ran webkit_unittests and blink_tests locally and haven't noticed any problems. By the way, is there a way for me to trigger try jobs by myself? Asking someone to trigger them adds a bit of latency. https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:954: const bool isOverLink = !selectionController().mouseDownMayStartSelect() && result.isOverLink(); This is needed so that there is a beam cursor when we have started the link selection. https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1383: && !(selectionController().hasExtendedSelection() && mayBeLinkSelection(mev)); This is needed so that no click event is sent when we select a part of a link. https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/UnitTestHelpers.h (right): https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/UnitTestHelpers.h:39: void waitForDeferredTasks(double interval); This is needed for the tests regarding cursors -- they are updated on timer, so we need a means to wait for them. This is not inside a test file because it may be needed in some other test. In fact it is used in some our internal tests at Yandex. https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/SelectionTestBase.h (right): https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/SelectionTestBase.h:19: class SelectionTestBase : public ::testing::Test { This class is not inside the LinkSelectionTest, because we use it for other tests at Yandex, in particular for tests on table selection feature (https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/table$20s...). If this is not acceptable, I can move it to the test file.
The CQ bit was checked by yosin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1774123006/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1774123006/1
On 2016/03/09 at 17:27:59, kotenkov wrote: > Hi, yosin, > here is the patch regarding link selection that I have promised you. PTAL. > > Would you please run try jobs for me? I have ran webkit_unittests and blink_tests locally and haven't noticed any problems. Running now > By the way, is there a way for me to trigger try jobs by myself? Asking someone to trigger them adds a bit of latency. Here is protocol: http://dev.chromium.org/getting-involved/become-a-committer#TOC-Try-job-access
I asked UX-Review and send "Intent to ship" soon. https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/SelectionController.h (right): https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/SelectionController.h:92: inline bool mayBeLinkSelection(const MouseEventWithHitTestResults& event) nit: Since, this function isn't performance sensitive, could you implement in ".cpp" file. https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/UnitTestHelpers.h (right): https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/UnitTestHelpers.h:39: void waitForDeferredTasks(double interval); On 2016/03/09 at 17:27:59, kotenkov wrote: > This is needed for the tests regarding cursors -- they are updated on timer, so we need a means to wait for them. > This is not inside a test file because it may be needed in some other test. In fact it is used in some our internal tests at Yandex. Could you put an explanation as comment in .h file to improve code documentation? With unit of |interval|, is it seconds or milliseconds? https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp (right): https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:25: namespace { Note: We don't need to enclose test fixture class in anonymous namespace or should avoid to do so. Since, test fixture names in gTest aren't scoped by C++ namespace, all names introduced by TEST(), TEST_F() are put into one bag. https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:87: true /* sendDownEvent */, false /* sendUpEvent */); How about writing this as: const bool sendDownEvent = true; const bool sendUpEvent = false; emulateMouseDrag(m_rightPointInLink, m_leftPointInLink, 0, sendDownEvent, sendUpEvent); https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/SelectionTestBase.h (right): https://codereview.chromium.org/1774123006/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/SelectionTestBase.h:21: void emulateMouseDrag(const IntPoint& downPoint, const IntPoint& upPoint, int modifiers, Could you avoid to use |bool| parameters?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I've fixed the issues. https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/SelectionTestBase.h (right): https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/SelectionTestBase.h:28: DragFlags = SendDownEvent | SendUpEvent); I've replaced bool parameters by flags. If it's not okay, would you please suggest a better solution?
On 2016/03/10 01:39:15, Yosi_UTC9 wrote: > On 2016/03/09 at 17:27:59, kotenkov wrote: > > Hi, yosin, > > here is the patch regarding link selection that I have promised you. PTAL. > > > > Would you please run try jobs for me? I have ran webkit_unittests and > blink_tests locally and haven't noticed any problems. > Running now > > > > By the way, is there a way for me to trigger try jobs by myself? Asking > someone to trigger them adds a bit of latency. > Here is protocol: > http://dev.chromium.org/getting-involved/become-a-committer#TOC-Try-job-access May I ask you to nominate me for try job access? If the answer is yes, here is the info that I should provide, according to the page you linked: My name is Ivan Kotenkov, I work for Yandex. My email is kotenkov@yandex-team.ru. I'm mostly sending patches for Blink, they are mostly bugfixes and small improvements. Try job access would eliminate the latency that I incur when asking someone to trigger try jobs for me.
On 2016/03/10 at 12:01:56, kotenkov wrote: > On 2016/03/10 01:39:15, Yosi_UTC9 wrote: > > On 2016/03/09 at 17:27:59, kotenkov wrote: > > > Hi, yosin, > > > here is the patch regarding link selection that I have promised you. PTAL. > > > > > > Would you please run try jobs for me? I have ran webkit_unittests and > > blink_tests locally and haven't noticed any problems. > > Running now > > > > > > > By the way, is there a way for me to trigger try jobs by myself? Asking > > someone to trigger them adds a bit of latency. > > Here is protocol: > > http://dev.chromium.org/getting-involved/become-a-committer#TOC-Try-job-access > > May I ask you to nominate me for try job access? > > If the answer is yes, here is the info that I should provide, according to the page you linked: > > My name is Ivan Kotenkov, I work for Yandex. My email is kotenkov@yandex-team.ru. > I'm mostly sending patches for Blink, they are mostly bugfixes and small improvements. > Try job access would eliminate the latency that I incur when asking someone to trigger try jobs for me. I send nomination mail, please wait for approval.
core/editing changes are OK +tken@ for web/ changes. Please hold committing until we get approval from both UX and blink-dev@. UX answer will be appeared in crbug.com/244738. https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp (right): https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:36: static const char* kHTMLString = nit: we don't need to mark |static|. https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:167: static const char* kHTMLString = nit: we don't need to mark |static|. https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/SelectionTestBase.h (right): https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/SelectionTestBase.h:28: DragFlags = SendDownEvent | SendUpEvent); On 2016/03/10 at 12:00:30, kotenkov wrote: > I've replaced bool parameters by flags. If it's not okay, would you please suggest a better solution? Using flags seem to be OK. Better solution is follow to window.eventSender model, == state machine, holding mouse point. class MockMouse { public: void moveTo(const IntPoint&); void buttonUp(MouseButton); void buttonDown(MouseButton); private: IntPoint m_point; int m_buttonStates; // each bit hold mouse button press/release state, bit 0=left, bit1=middle, bit3=right, bit4=and more.. }; void MockMouse::buttonUp(MouseButton button) { ASSERT((m_buttonState & (1 << static_cast<int>(button>)) == 1); ... dispatch button up event }
https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp (right): https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:36: static const char* kHTMLString = On 2016/03/11 01:32:07, Yosi_UTC9 wrote: > nit: we don't need to mark |static|. Why not? There will be 6 test instantiations and they all will share this constant.
https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp (right): https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:36: static const char* kHTMLString = On 2016/03/11 at 21:22:51, kotenkov wrote: > On 2016/03/11 01:32:07, Yosi_UTC9 wrote: > > nit: we don't need to mark |static|. > > Why not? There will be 6 test instantiations and they all will share this constant. Since, it is literal and |kHTMLString| is never changed, compiler can recognize |kHTMLString| as constant literal string.
On 2016/03/14 at 04:04:57, Yosi_UTC9 wrote: > https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp (right): > > https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:36: static const char* kHTMLString = > On 2016/03/11 at 21:22:51, kotenkov wrote: > > On 2016/03/11 01:32:07, Yosi_UTC9 wrote: > > > nit: we don't need to mark |static|. > > > > Why not? There will be 6 test instantiations and they all will share this constant. > > Since, it is literal and |kHTMLString| is never changed, compiler can recognize |kHTMLString| as constant literal string. It doesn't affect execution speed but introduce bike shedding discussion. ;-) BTW, chrome-ui-review@ is reviewing this new UX feature now. Stay tuned!
Thanks for your updates, they are really helpful. https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp (right): https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:36: static const char* kHTMLString = On 2016/03/14 04:04:56, Yosi_UTC9 wrote: > On 2016/03/11 at 21:22:51, kotenkov wrote: > > On 2016/03/11 01:32:07, Yosi_UTC9 wrote: > > > nit: we don't need to mark |static|. > > > > Why not? There will be 6 test instantiations and they all will share this > constant. > > Since, it is literal and |kHTMLString| is never changed, compiler can recognize > |kHTMLString| as constant literal string. But why should we wait for the compiler to do such a simple optimization if we can do it ourselves, just by putting static here?
On 2016/03/17 at 06:36:44, kotenkov wrote: > Thanks for your updates, they are really helpful. > > https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp (right): > > https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:36: static const char* kHTMLString = > On 2016/03/14 04:04:56, Yosi_UTC9 wrote: > > On 2016/03/11 at 21:22:51, kotenkov wrote: > > > On 2016/03/11 01:32:07, Yosi_UTC9 wrote: > > > > nit: we don't need to mark |static|. > > > > > > Why not? There will be 6 test instantiations and they all will share this > > constant. > > > > Since, it is literal and |kHTMLString| is never changed, compiler can recognize > > |kHTMLString| as constant literal string. > > But why should we wait for the compiler to do such a simple optimization if we can do it ourselves, just by putting static here? I have an opposite opinion. Why do human need to do what compiler can do? We should spend time what computer can't do. ;-) Once, AlphaProgrammer is released, we don't need to write program.
On 2016/03/17 at 06:56:56, Yosi_UTC9 wrote: > On 2016/03/17 at 06:36:44, kotenkov wrote: > > Thanks for your updates, they are really helpful. > > > > https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp (right): > > > > https://codereview.chromium.org/1774123006/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:36: static const char* kHTMLString = > > On 2016/03/14 04:04:56, Yosi_UTC9 wrote: > > > On 2016/03/11 at 21:22:51, kotenkov wrote: > > > > On 2016/03/11 01:32:07, Yosi_UTC9 wrote: > > > > > nit: we don't need to mark |static|. > > > > > > > > Why not? There will be 6 test instantiations and they all will share this > > > constant. > > > > > > Since, it is literal and |kHTMLString| is never changed, compiler can recognize > > > |kHTMLString| as constant literal string. > > > > But why should we wait for the compiler to do such a simple optimization if we can do it ourselves, just by putting static here? > > I have an opposite opinion. Why do human need to do what compiler can do? > We should spend time what computer can't do. ;-) > > Once, AlphaProgrammer is released, we don't need to write program. BTW, if we really want to teach compiler that is constant, we should write |const char* const kFoo = "foo";|. If we write so, we don't need to have |static| at all. |static const char* kFoo = "foo";| tells compiler |kFoo| linkage is |static| and it can be modify. Compiler detects |kFoo| is never modified then compiler replaces all references to constant string reference.
I've removed the static and additionally renamed the test function. Guess it's time to add tkent@ as we can ship now according to https://bugs.chromium.org/p/chromium/issues/detail?id=244738#c14
I've removed the static and additionally renamed the test function. Guess it's time to add tkent@ as we can ship now according to https://bugs.chromium.org/p/chromium/issues/detail?id=244738#c14
kotenkov@yandex-team.ru changed reviewers: + tkent@chromium.org
https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionController.cpp:628: bool mayBeLinkSelection(const MouseEventWithHitTestResults& event) This looks "isLinkSelection" to me. Why "mayBe"? https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp (right): https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:19: #include <gmock/gmock.h> Please include it with "", not <>. Please specify path base on the top chromium src directory. #include "testing/gmock/include/gmock/gmock.h" https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:50: ASSERT(document); Blink's ASSERT() is deprecated. Use glog's DCHECK() or gtest's ASSERT_NE(nullptr, document) https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:52: ASSERT(linkToSelect); ASSERT() is deprecated. https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:131: ASSERT(textToSelect); ASSERT() is deprecated. https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:143: class LinkSelectionClickEvents : public SelectionTestBase { Please use "Test" suffix for test fixture class name. https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:178: ASSERT(document); ASSERT() is deprecated. https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:182: ASSERT_TRUE(emptyDiv); ASSERT_NE(nullptr, emptyDiv), or DCHECK_NE(emptyDiv, nullptr) https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:183: ASSERT_TRUE(textDiv); Ditto. https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/SelectionTestBase.cpp (right): https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/SelectionTestBase.cpp:25: static const int kMoveEventsNumber = 10; "static" for const local variable doesn't make sense. Please remove it. https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/SelectionTestBase.h (right): https://codereview.chromium.org/1774123006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/SelectionTestBase.h:11: #include <gtest/gtest.h> Don't use <> for gtest.h.
Renamed mayBeLinkSelection() -> isLinkSelection(). Removed statics. Used full paths. Renamed ASSERTs.
The CQ bit was checked by tkent@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1774123006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1774123006/80001
lgtm for core/editing https://codereview.chromium.org/1774123006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/SelectionTestBase.h (right): https://codereview.chromium.org/1774123006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/SelectionTestBase.h:19: class SelectionTestBase : public ::testing::Test { Could you name this class as |WebViewSelectionTestBase| or another? I'll introduce |Selection| class in near feature in "core/editing/" and I would like to use |SelectionTestBase| for |Selection| class.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Renamed SelectionTestBase -> WebSelectionTestBase.
On 2016/03/25 at 08:56:28, kotenkov wrote: > Renamed SelectionTestBase -> WebSelectionTestBase. Please follow yosin's advice. WebSelectionTestBase sounds like we have WebSelection class and WebSelectionTestBase is a test base class for it. Also, if WebSelectionTestBase is used only in LinkSelectionTest.cpp, please move it into LInkSelectionTest.cpp.
On 2016/03/28 01:09:14, tkent wrote: > Also, if WebSelectionTestBase is used only in LinkSelectionTest.cpp, please move > it into LInkSelectionTest.cpp. As I've said earlier: This class is not inside the LinkSelectionTest, because we use it for other tests at Yandex, in particular for tests on table selection feature (https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/table$20s...). If this is not acceptable, I can move it to the test file. Would it be okay to keep it separate? Of course, I will rename it to WebViewSelectionTestBase.
On 2016/03/28 at 05:20:54, kotenkov wrote: > On 2016/03/28 01:09:14, tkent wrote: > > Also, if WebSelectionTestBase is used only in LinkSelectionTest.cpp, please move > > it into LInkSelectionTest.cpp. > > As I've said earlier: > This class is not inside the LinkSelectionTest, because we use it for other > tests at Yandex, in particular for tests on table selection feature > (https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/table$20s...). > If this is not acceptable, I can move it to the test file. > Would it be okay to keep it separate? Of course, I will rename it to WebViewSelectionTestBase. Will we have other test classes use |WebViewSelectionTestBase|? If so, it is reasonable to keep in separate file. If not and LinkSelectionTest is only a class, it is better to unify LinkSelectionTest and WebViewSelectionBase to simplify Blink repository.
On 2016/03/28 07:19:45, Yosi_UTC9 wrote: > Will we have other test classes use |WebViewSelectionTestBase|? > If so, it is reasonable to keep in separate file. > If not and LinkSelectionTest is only a class, it is better to unify > LinkSelectionTest and WebViewSelectionBase to simplify Blink repository. Moved it to LinkSelectionTest.cpp.
On 2016/03/28 07:19:45, Yosi_UTC9 wrote: > Will we have other test classes use |WebViewSelectionTestBase|? > If so, it is reasonable to keep in separate file. > If not and LinkSelectionTest is only a class, it is better to unify > LinkSelectionTest and WebViewSelectionBase to simplify Blink repository. Moved it to LinkSelectionTest.cpp.
On 2016/03/28 at 07:32:54, kotenkov wrote: > On 2016/03/28 07:19:45, Yosi_UTC9 wrote: > > Will we have other test classes use |WebViewSelectionTestBase|? > > If so, it is reasonable to keep in separate file. > > If not and LinkSelectionTest is only a class, it is better to unify > > LinkSelectionTest and WebViewSelectionBase to simplify Blink repository. > > Moved it to LinkSelectionTest.cpp. Thanks! Please wait for tkent@
https://codereview.chromium.org/1774123006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp (right): https://codereview.chromium.org/1774123006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:45: std::string getSelectionText(); Please don't use std::string in Blink. Use WTF::String instead.
> Please don't use std::string in Blink. Use WTF::String instead. Done. https://codereview.chromium.org/1774123006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp (right): https://codereview.chromium.org/1774123006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/LinkSelectionTest.cpp:45: std::string getSelectionText(); On 2016/03/28 07:59:21, tkent wrote: > Please don't use std::string in Blink. Use WTF::String instead. FIY: there are 251 more mentions of std::string in web/tests. Should we convert them too?
lgtm On 2016/03/28 at 08:26:30, kotenkov wrote: > > Please don't use std::string in Blink. Use WTF::String instead. > > FIY: there are 251 more mentions of std::string in web/tests. Should we convert them too? IMO, it's ok to use std::string if there is a reason. Also, I don't think it's worth to convert existing std::string usage.
The CQ bit was checked by kotenkov@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/1774123006/#ps160001 (title: "Use String instead of std::string.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1774123006/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1774123006/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Implement link selection on alt+mouse drag. BUG=244738 TEST=webkit_unit_tests --gtest_filter=LinkSelectionTest.* ========== to ========== Implement link selection on alt+mouse drag. BUG=244738 TEST=webkit_unit_tests --gtest_filter=LinkSelectionTest.* Committed: https://crrev.com/9e80a9e5b5b71a48119225a614b7c0b8369badf2 Cr-Commit-Position: refs/heads/master@{#383684} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/9e80a9e5b5b71a48119225a614b7c0b8369badf2 Cr-Commit-Position: refs/heads/master@{#383684} |
