Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1218653006:
Add support code to test the cast system tray item. (Closed)
DescriptionAdd support code to test the cast system tray item.
This supporting code is used in https://codereview.chromium.org/1231593002/.
BUG=497343
Committed: https://crrev.com/b90450d5cec8b5658ff098e923dff03b0668c0fe
Cr-Commit-Position: refs/heads/master@{#340790}
Patch Set 1 #
Total comments: 12
Patch Set 2 : #
Total comments: 16
Patch Set 3 : #
Total comments: 1
Patch Set 4 : Move testing API into ash/test #
Total comments: 8
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Total comments: 4
Patch Set 7 : #
Total comments: 13
Patch Set 8 : #Patch Set 9 : #
Total comments: 2
Patch Set 10 : #Patch Set 11 : Remove ASH_EXPORT #
Messages
Total messages: 65 (21 generated)
jdufault@chromium.org changed reviewers: + achuith@chromium.org
On 2015/07/08 18:03:08, jdufault wrote: > mailto:jdufault@chromium.org changed reviewers: > + mailto:achuith@chromium.org Achuith, can you take a look? Thanks!
https://codereview.chromium.org/1218653006/diff/1/ash/system/cast/tray_cast.cc File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1218653006/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:397: void SimulateViewClickedForTest(std::string id); const std::string& receiver_id https://codereview.chromium.org/1218653006/diff/1/ash/system/cast/tray_cast.h File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/1/ash/system/cast/tray_cast.h... ash/system/cast/tray_cast.h:27: // The following are functions used for testing. It would be great if they Have you looked at FRIEND_TEST_ALL_PREFIXES? https://codereview.chromium.org/1218653006/diff/1/ash/system/cast/tray_cast.h... ash/system/cast/tray_cast.h:30: bool IsTrayInitializedForTest(); const https://codereview.chromium.org/1218653006/diff/1/ash/system/cast/tray_cast.h... ash/system/cast/tray_cast.h:31: bool IsTrayVisibleForTest(); const https://codereview.chromium.org/1218653006/diff/1/ash/system/cast/tray_cast.h... ash/system/cast/tray_cast.h:33: void StartCastForTest(std::string id); const std::string& receiver_id https://codereview.chromium.org/1218653006/diff/1/ash/system/tray/system_tray.cc File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/1218653006/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:209: tray_cast_ = new TrayCast(this); Could this be a problem on linux chrome?
https://codereview.chromium.org/1218653006/diff/1/ash/system/cast/tray_cast.cc File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1218653006/diff/1/ash/system/cast/tray_cast.c... ash/system/cast/tray_cast.cc:397: void SimulateViewClickedForTest(std::string id); On 2015/07/08 21:45:56, achuithb wrote: > const std::string& receiver_id Done. https://codereview.chromium.org/1218653006/diff/1/ash/system/cast/tray_cast.h File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/1/ash/system/cast/tray_cast.h... ash/system/cast/tray_cast.h:27: // The following are functions used for testing. It would be great if they On 2015/07/08 21:45:57, achuithb wrote: > Have you looked at FRIEND_TEST_ALL_PREFIXES? Doesn't seem to work, since the test and this class are in different compilation modules. https://codereview.chromium.org/1218653006/diff/1/ash/system/cast/tray_cast.h... ash/system/cast/tray_cast.h:30: bool IsTrayInitializedForTest(); On 2015/07/08 21:45:57, achuithb wrote: > const Done. https://codereview.chromium.org/1218653006/diff/1/ash/system/cast/tray_cast.h... ash/system/cast/tray_cast.h:31: bool IsTrayVisibleForTest(); On 2015/07/08 21:45:57, achuithb wrote: > const Done. https://codereview.chromium.org/1218653006/diff/1/ash/system/cast/tray_cast.h... ash/system/cast/tray_cast.h:33: void StartCastForTest(std::string id); On 2015/07/08 21:45:57, achuithb wrote: > const std::string& receiver_id Done. https://codereview.chromium.org/1218653006/diff/1/ash/system/tray/system_tray.cc File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/1218653006/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:209: tray_cast_ = new TrayCast(this); On 2015/07/08 21:45:57, achuithb wrote: > Could this be a problem on linux chrome? What kind of problem are you thinking of?
PTAL, a few things in here have changed to support the multiple receivers test. Thanks!
https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:397: void SimulateViewClickedForTest(const std::string& id); I think it's worthwhile to add a function comment here. id -> activity_id https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:22: class ASH_EXPORT TrayCastTestMethods { TrayCastTestingInterface? Does it compile if you move this definition to after the definition of TrayCast? It's kind of unfortunate that the testing interface is the first thing you see in this file. https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:26: virtual bool IsTrayCastViewVisibleForTest() const = 0; Maybe add a comment explaining the different between this and the next function https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:46: // Overridden from TrayCastTestMethods: use a period to be consistent with the rest of this file https://codereview.chromium.org/1218653006/diff/40001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/1218653006/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray.cc:210: AddTrayItem(tray_cast_); Tell me again why we have this on linux?
https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:397: void SimulateViewClickedForTest(const std::string& id); On 2015/07/14 18:06:39, achuithb wrote: > I think it's worthwhile to add a function comment here. > > id -> activity_id Done. https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:22: class ASH_EXPORT TrayCastTestMethods { On 2015/07/14 18:06:39, achuithb wrote: > TrayCastTestingInterface? > > Does it compile if you move this definition to after the definition of TrayCast? > It's kind of unfortunate that the testing interface is the first thing you see > in this file. Done. I am unable to get it to compile moving it after TrayCast (I also tried using an incomplete type). https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:26: virtual bool IsTrayCastViewVisibleForTest() const = 0; On 2015/07/14 18:06:39, achuithb wrote: > Maybe add a comment explaining the different between this and the next function Done. https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:46: // Overridden from TrayCastTestMethods: On 2015/07/14 18:06:39, achuithb wrote: > use a period to be consistent with the rest of this file Done. https://codereview.chromium.org/1218653006/diff/40001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/1218653006/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray.cc:210: AddTrayItem(tray_cast_); On 2015/07/14 18:06:39, achuithb wrote: > Tell me again why we have this on linux? I don't know - this was in the initial code you handed over to me.
https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:22: class ASH_EXPORT TrayCastTestMethods { On 2015/07/15 17:35:01, jdufault wrote: > On 2015/07/14 18:06:39, achuithb wrote: > > TrayCastTestingInterface? > > > > Does it compile if you move this definition to after the definition of > TrayCast? > > It's kind of unfortunate that the testing interface is the first thing you see > > in this file. > > Done. > > I am unable to get it to compile moving it after TrayCast (I also tried using an > incomplete type). That's what I would expect if the compiler refuses to look ahead in the file. How do you feel about putting it in a separate .h? https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:26: virtual bool IsTrayCastViewVisibleForTest() const = 0; On 2015/07/15 17:35:01, jdufault wrote: > On 2015/07/14 18:06:39, achuithb wrote: > > Maybe add a comment explaining the different between this and the next > function > > Done. I meant the difference between IsTrayCastViewVisibleForTest and IsTraySelectViewVisibleForTest. Also, the comments should go in the interface definition, not in the implementation. https://codereview.chromium.org/1218653006/diff/40001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/1218653006/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray.cc:210: AddTrayItem(tray_cast_); On 2015/07/15 17:35:01, jdufault wrote: > On 2015/07/14 18:06:39, achuithb wrote: > > Tell me again why we have this on linux? > > I don't know - this was in the initial code you handed over to me. Let get rid of it and see if anything breaks. I believe this should still be available on chromeos-chrome for linux
https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:22: class ASH_EXPORT TrayCastTestMethods { On 2015/07/15 18:56:47, achuithb wrote: > On 2015/07/15 17:35:01, jdufault wrote: > > On 2015/07/14 18:06:39, achuithb wrote: > > > TrayCastTestingInterface? > > > > > > Does it compile if you move this definition to after the definition of > > TrayCast? > > > It's kind of unfortunate that the testing interface is the first thing you > see > > > in this file. > > > > Done. > > > > I am unable to get it to compile moving it after TrayCast (I also tried using > an > > incomplete type). > > That's what I would expect if the compiler refuses to look ahead in the file. > > How do you feel about putting it in a separate .h? Yes; I would have been surprised if it worked as well. I've moved the testing interface into its own file. https://codereview.chromium.org/1218653006/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:26: virtual bool IsTrayCastViewVisibleForTest() const = 0; On 2015/07/15 18:56:47, achuithb wrote: > On 2015/07/15 17:35:01, jdufault wrote: > > On 2015/07/14 18:06:39, achuithb wrote: > > > Maybe add a comment explaining the different between this and the next > > function > > > > Done. > > I meant the difference between IsTrayCastViewVisibleForTest and > IsTraySelectViewVisibleForTest. > > Also, the comments should go in the interface definition, not in the > implementation. Oops; done. https://codereview.chromium.org/1218653006/diff/40001/ash/system/tray/system_... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/1218653006/diff/40001/ash/system/tray/system_... ash/system/tray/system_tray.cc:210: AddTrayItem(tray_cast_); On 2015/07/15 18:56:47, achuithb wrote: > On 2015/07/15 17:35:01, jdufault wrote: > > On 2015/07/14 18:06:39, achuithb wrote: > > > Tell me again why we have this on linux? > > > > I don't know - this was in the initial code you handed over to me. > > Let get rid of it and see if anything breaks. I believe this should still be > available on chromeos-chrome for linux Done.
jdufault@chromium.org changed reviewers: + oshima@chromium.org
Oshima, would you mind taking a look? Thanks!
https://codereview.chromium.org/1218653006/diff/80001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast_testing_interface.h (right): https://codereview.chromium.org/1218653006/diff/80001/ash/system/cast/tray_ca... ash/system/cast/tray_cast_testing_interface.h:15: class ASH_EXPORT TrayCastTestingInterface { We usually put the test API in ash/test, and make it friend so that the test API class can access the internal state of the target class. This way, you can avoid having test code / dependency in production code. Please see ash/test/*test_api.h
On 2015/07/17 20:20:59, oshima wrote: > https://codereview.chromium.org/1218653006/diff/80001/ash/system/cast/tray_ca... > File ash/system/cast/tray_cast_testing_interface.h (right): > > https://codereview.chromium.org/1218653006/diff/80001/ash/system/cast/tray_ca... > ash/system/cast/tray_cast_testing_interface.h:15: class ASH_EXPORT > TrayCastTestingInterface { > We usually put the test API in ash/test, and make it friend so that the test API > class can access the internal state of the > target class. This way, you can avoid having test code / dependency in > production code. Please see > > ash/test/*test_api.h Done, but this required moving a bunch of classes originally defined in tray_cast.cc into tray_cast.h. I could also move them into a tray_cast_support.h header instead, wdyt?
https://codereview.chromium.org/1218653006/diff/100001/ash/test/tray_cast_tes... File ash/test/tray_cast_test_api.cc (right): https://codereview.chromium.org/1218653006/diff/100001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.cc:26: tray_cast_->default_->cast_view(); easy way to test this is to define IDs for view in TrayCast, assign them to views (View::set_id), then find the view using GetViewByID. This way, you don't have to expose internal classes. For IsTrayVisibleForTest. you can just get view by id and check IsDrawn() https://codereview.chromium.org/1218653006/diff/100001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.cc:41: tray_cast_->default_->cast_view()->StopCasting(); and you can have these two methods on TrayCast. https://codereview.chromium.org/1218653006/diff/100001/ash/test/tray_cast_tes... File ash/test/tray_cast_test_api.h (right): https://codereview.chromium.org/1218653006/diff/100001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.h:20: bool IsTrayInitializedForTest() const; You can simply call it IsTrayInitialized() because all methods here should be for tests. https://codereview.chromium.org/1218653006/diff/100001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.h:41: }; DISALLOW_COPY_AND_ASSIGN
Okay, PTAL. Thanks https://codereview.chromium.org/1218653006/diff/100001/ash/test/tray_cast_tes... File ash/test/tray_cast_test_api.cc (right): https://codereview.chromium.org/1218653006/diff/100001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.cc:26: tray_cast_->default_->cast_view(); On 2015/07/17 22:59:28, oshima wrote: > easy way to test this is to define IDs for view in TrayCast, assign them to > views (View::set_id), then find the view using GetViewByID. This way, you don't > have to expose internal classes. > > For IsTrayVisibleForTest. you can just get view by id and check IsDrawn() Done. https://codereview.chromium.org/1218653006/diff/100001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.cc:41: tray_cast_->default_->cast_view()->StopCasting(); On 2015/07/17 22:59:28, oshima wrote: > and you can have these two methods on TrayCast. Done. https://codereview.chromium.org/1218653006/diff/100001/ash/test/tray_cast_tes... File ash/test/tray_cast_test_api.h (right): https://codereview.chromium.org/1218653006/diff/100001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.h:20: bool IsTrayInitializedForTest() const; On 2015/07/17 22:59:29, oshima wrote: > You can simply call it IsTrayInitialized() because all methods here should be > for tests. Done. https://codereview.chromium.org/1218653006/diff/100001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.h:41: }; On 2015/07/17 22:59:28, oshima wrote: > DISALLOW_COPY_AND_ASSIGN Done.
https://codereview.chromium.org/1218653006/diff/160001/ash/system/cast/tray_c... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/160001/ash/system/cast/tray_c... ash/system/cast/tray_cast.h:32: views::View* FindChildViewForTest(ChildViewIds view_id) const; can't you access view hierarchy from system_tray like tray_cast_->system_tray()->GetViewById() ?
https://codereview.chromium.org/1218653006/diff/160001/ash/system/cast/tray_c... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/160001/ash/system/cast/tray_c... ash/system/cast/tray_cast.h:32: views::View* FindChildViewForTest(ChildViewIds view_id) const; On 2015/07/18 01:09:39, oshima wrote: > can't you access view hierarchy from system_tray like > > tray_cast_->system_tray()->GetViewById() > > ? Done.
lgtm
https://codereview.chromium.org/1218653006/diff/180001/ash/test/tray_cast_tes... File ash/test/tray_cast_test_api.cc (right): https://codereview.chromium.org/1218653006/diff/180001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.cc:23: views::View* tray_view = nit: const should work here? https://codereview.chromium.org/1218653006/diff/180001/ash/test/tray_cast_tes... File ash/test/tray_cast_test_api.h (right): https://codereview.chromium.org/1218653006/diff/180001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.h:40: TrayCast* tray_cast_; nit: add // not owned.
https://codereview.chromium.org/1218653006/diff/180001/ash/test/tray_cast_tes... File ash/test/tray_cast_test_api.cc (right): https://codereview.chromium.org/1218653006/diff/180001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.cc:23: views::View* tray_view = On 2015/07/20 22:28:26, achuithb wrote: > nit: const should work here? Done. https://codereview.chromium.org/1218653006/diff/180001/ash/test/tray_cast_tes... File ash/test/tray_cast_test_api.h (right): https://codereview.chromium.org/1218653006/diff/180001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.h:40: TrayCast* tray_cast_; On 2015/07/20 22:28:26, achuithb wrote: > nit: add // not owned. Done.
lgtm
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/1218653006/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218653006/200001
The CQ bit was unchecked by jdufault@chromium.org
On 2015/07/20 22:49:01, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1218653006/200001 I messed something up when I was testing patch #10, as it does not work. Please take a look at patch #11 - the only difference is we expose a default_view() on TrayCast and use it instead of the system_tray() for finding child elements.
https://codereview.chromium.org/1218653006/diff/220001/ash/system/cast/tray_c... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/220001/ash/system/cast/tray_c... ash/system/cast/tray_cast.h:31: views::View* default_view(); You can inline the function here https://codereview.chromium.org/1218653006/diff/220001/ash/system/cast/tray_c... ash/system/cast/tray_cast.h:32: enum ChildViewIds { TRAY_VIEW = 1, SELECT_VIEW, CAST_VIEW }; ChildViewId? https://codereview.chromium.org/1218653006/diff/220001/ash/test/tray_cast_tes... File ash/test/tray_cast_test_api.cc (right): https://codereview.chromium.org/1218653006/diff/220001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.cc:22: bool TrayCastTestAPI::IsTrayVisible() const { Create a helper like: bool IsTrayViewVisible(ChildViewId view_id) const { const views::View* view = tray_cast_->default_view()->GetViewByID(view_id); return view != nullptr && view->IsDrawn(); } https://codereview.chromium.org/1218653006/diff/220001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.cc:29: views::View* view = const https://codereview.chromium.org/1218653006/diff/220001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.cc:35: views::View* view = const
https://codereview.chromium.org/1218653006/diff/220001/ash/system/cast/tray_c... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/220001/ash/system/cast/tray_c... ash/system/cast/tray_cast.h:31: views::View* default_view(); On 2015/07/21 19:52:29, achuithb wrote: > You can inline the function here default_ is an incomplete type, so it cannot be converted to view::View*. https://codereview.chromium.org/1218653006/diff/220001/ash/system/cast/tray_c... ash/system/cast/tray_cast.h:32: enum ChildViewIds { TRAY_VIEW = 1, SELECT_VIEW, CAST_VIEW }; On 2015/07/21 19:52:29, achuithb wrote: > ChildViewId? Done. https://codereview.chromium.org/1218653006/diff/220001/ash/test/tray_cast_tes... File ash/test/tray_cast_test_api.cc (right): https://codereview.chromium.org/1218653006/diff/220001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.cc:22: bool TrayCastTestAPI::IsTrayVisible() const { On 2015/07/21 19:52:29, achuithb wrote: > Create a helper like: > > bool IsTrayViewVisible(ChildViewId view_id) const { > const views::View* view = tray_cast_->default_view()->GetViewByID(view_id); > return view != nullptr && view->IsDrawn(); > } Done. https://codereview.chromium.org/1218653006/diff/220001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.cc:29: views::View* view = On 2015/07/21 19:52:29, achuithb wrote: > const Done. https://codereview.chromium.org/1218653006/diff/220001/ash/test/tray_cast_tes... ash/test/tray_cast_test_api.cc:35: views::View* view = On 2015/07/21 19:52:29, achuithb wrote: > const Done.
https://codereview.chromium.org/1218653006/diff/220001/ash/system/cast/tray_c... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/220001/ash/system/cast/tray_c... ash/system/cast/tray_cast.h:31: views::View* default_view(); On 2015/07/21 20:21:31, jdufault wrote: > On 2015/07/21 19:52:29, achuithb wrote: > > You can inline the function here > > default_ is an incomplete type, so it cannot be converted to view::View*. Oops, old comment. I converted the type using a cast.
https://codereview.chromium.org/1218653006/diff/220001/ash/system/cast/tray_c... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/220001/ash/system/cast/tray_c... ash/system/cast/tray_cast.h:31: views::View* default_view(); On 2015/07/21 20:22:24, jdufault wrote: > On 2015/07/21 20:21:31, jdufault wrote: > > On 2015/07/21 19:52:29, achuithb wrote: > > > You can inline the function here > > > > default_ is an incomplete type, so it cannot be converted to view::View*. > > Oops, old comment. I converted the type using a cast. I'm really sorry, but I didn't expect there would be a cast. I thought you would just need to return the variable, in which case this would be ok in the .h. Also, I think we would like to avoid reinterpret cast. Does static_cast not work? Let's move this back to the cc. So sorry for the back and forth.
To keep the patch history manageable, you can delete intermediate patches that have no comments and were not sent out for review.
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
Patchset #7 (id:200001) has been deleted
https://codereview.chromium.org/1218653006/diff/220001/ash/system/cast/tray_c... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/220001/ash/system/cast/tray_c... ash/system/cast/tray_cast.h:31: views::View* default_view(); On 2015/07/21 20:30:17, achuithb wrote: > On 2015/07/21 20:22:24, jdufault wrote: > > On 2015/07/21 20:21:31, jdufault wrote: > > > On 2015/07/21 19:52:29, achuithb wrote: > > > > You can inline the function here > > > > > > default_ is an incomplete type, so it cannot be converted to view::View*. > > > > Oops, old comment. I converted the type using a cast. > > I'm really sorry, but I didn't expect there would be a cast. I thought you would > just need to return the variable, in which case this would be ok in the .h. > > Also, I think we would like to avoid reinterpret cast. Does static_cast not > work? > > Let's move this back to the cc. So sorry for the back and forth. No worries, I've moved it back to the cc file. Nope, static_cast didn't work because the types were unrelated via inheritance.
On 2015/07/21 20:48:53, jdufault wrote: > https://codereview.chromium.org/1218653006/diff/220001/ash/system/cast/tray_c... > File ash/system/cast/tray_cast.h (right): > > https://codereview.chromium.org/1218653006/diff/220001/ash/system/cast/tray_c... > ash/system/cast/tray_cast.h:31: views::View* default_view(); > On 2015/07/21 20:30:17, achuithb wrote: > > On 2015/07/21 20:22:24, jdufault wrote: > > > On 2015/07/21 20:21:31, jdufault wrote: > > > > On 2015/07/21 19:52:29, achuithb wrote: > > > > > You can inline the function here > > > > > > > > default_ is an incomplete type, so it cannot be converted to view::View*. > > > > > > Oops, old comment. I converted the type using a cast. > > > > I'm really sorry, but I didn't expect there would be a cast. I thought you > would > > just need to return the variable, in which case this would be ok in the .h. > > > > Also, I think we would like to avoid reinterpret cast. Does static_cast not > > work? > > > > Let's move this back to the cc. So sorry for the back and forth. > > No worries, I've moved it back to the cc file. > > Nope, static_cast didn't work because the types were unrelated via inheritance. lgtm. Please wait for Oshima-san
lgtm with a nit https://codereview.chromium.org/1218653006/diff/260001/ash/system/cast/tray_c... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/260001/ash/system/cast/tray_c... ash/system/cast/tray_cast.h:31: views::View* default_view(); GetDefaultView() make it const / const if possible. (disregard if not)
https://codereview.chromium.org/1218653006/diff/260001/ash/system/cast/tray_c... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1218653006/diff/260001/ash/system/cast/tray_c... ash/system/cast/tray_cast.h:31: views::View* default_view(); On 2015/07/21 21:04:08, oshima wrote: > GetDefaultView() > > make it const / const if possible. (disregard if not) Done.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/1218653006/#ps280001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218653006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1218653006/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/07/23 17:18:54, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Looks like there are some unreleated failures with chromevox. I'll try to commit again later today.
The CQ bit was checked by jdufault@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218653006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1218653006/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by jdufault@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218653006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1218653006/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by jdufault@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218653006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1218653006/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The windows build is failing with linker errors: > e:\b\build\slave\win\build\src\ash\test\tray_cast_test_api.cc(18) : warning C4273: 'ash::TrayCastTestAPI::IsTrayInitialized' : inconsistent dll linkage > e:\b\build\slave\win\build\src\ash\test\tray_cast_test_api.h(20) : see previous definition of 'IsTrayInitialized' It looks like ASH_IMPLEMENTATION is not defined for the gyp file group 'ash_test_support_sources', which is causing ASH_EXPORT to resolve to __declspec(dllimport) instead of __declspec(dllexport). I can think of a couple of ways to fix this: 1. Move tray_cast_test_api.[h,cc] into 'ash_sources' (the only gyp file group that defines ASH_IMPLEMENTATION), potentially also moving its location outside of the ash/test folder. 2. Create a new gyp file group that defines ASH_IMPLEMENTATION that only contains tray_cast_test_api.[h,cc]. 3. Define ASH_IMPLEMENATION for the build target corresponding to 'ash_test_support_sources'. Thoughts? I expect #1 will be the fastest turn-around, but #2 (if it works) seems like the cleanest solution.
On 2015/07/28 18:31:48, jdufault wrote: > The windows build is failing with linker errors: > > > e:\b\build\slave\win\build\src\ash\test\tray_cast_test_api.cc(18) : warning > C4273: 'ash::TrayCastTestAPI::IsTrayInitialized' : inconsistent dll linkage > > e:\b\build\slave\win\build\src\ash\test\tray_cast_test_api.h(20) : see > previous definition of 'IsTrayInitialized' > > It looks like ASH_IMPLEMENTATION is not defined for the gyp file group > 'ash_test_support_sources', which is causing ASH_EXPORT to resolve to > __declspec(dllimport) instead of __declspec(dllexport). > > I can think of a couple of ways to fix this: > 1. Move tray_cast_test_api.[h,cc] into 'ash_sources' (the only gyp file group > that defines ASH_IMPLEMENTATION), potentially also moving its location outside > of the ash/test folder. > 2. Create a new gyp file group that defines ASH_IMPLEMENTATION that only > contains tray_cast_test_api.[h,cc]. > 3. Define ASH_IMPLEMENATION for the build target corresponding to > 'ash_test_support_sources'. > > Thoughts? I expect #1 will be the fastest turn-around, but #2 (if it works) > seems like the cleanest solution. You shouldn't have to export the test api. It's static library.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/1218653006/#ps290001 (title: "Remove ASH_EXPORT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218653006/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1218653006/290001
Message was sent while issue was closed.
Committed patchset #11 (id:290001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/b90450d5cec8b5658ff098e923dff03b0668c0fe Cr-Commit-Position: refs/heads/master@{#340790} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
