Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 5 months ago by Jiang Jiang Modified:
6 years, 5 months ago Reviewers:
Peter Beverloo, stevenjb, Jun Mukai, dewittj CC:
chromium-reviews, Dmitry Titov Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFix component build linking with notifications=0
We will get the following errors if not:
2> Creating library ..\..\build\Debug\lib\message_center.lib and object ..\..\build\Debug\lib\message_center.exp
2>dummy_message_center.obj : error LNK2001: unresolved external symbol "public: virtual bool __thiscall message_center::NotificationDelegate::HasClickedListener(void)" (?HasClickedListener@NotificationDelegate@message_center@@UAE_NXZ)
2>dummy_message_center.obj : error LNK2001: unresolved external symbol "public: virtual void __thiscall message_center::NotificationDelegate::ButtonClick(int)" (?ButtonClick@NotificationDelegate@message_center@@UAEXH@Z)
Same fix for OS==android.
Done by Przemek Kudla <pkudla@opera.com>;
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281574
Patch Set 1 #
Total comments: 4
Patch Set 2 : Move notification_delegate match correctly. #
Created: 6 years, 5 months ago
Messages
Total messages: 16 (0 generated)
This looks good to me. I've launched a bunch of try-bots for you. https://codereview.chromium.org/369243002/diff/1/ui/message_center/message_ce... File ui/message_center/message_center.gyp (left): https://codereview.chromium.org/369243002/diff/1/ui/message_center/message_ce... ui/message_center/message_center.gyp:152: ['include', '^notifier_settings\\.cc$'], Are you sure we don't need the NotifierId stuff defined in this file for Android? https://codereview.chromium.org/369243002/diff/1/ui/message_center/message_ce... File ui/message_center/message_center.gyp (right): https://codereview.chromium.org/369243002/diff/1/ui/message_center/message_ce... ui/message_center/message_center.gyp:152: ['include', '^notification_delegate\\.cc$'], This line can be removed since we'll already include it for Android on line 143.
https://codereview.chromium.org/369243002/diff/1/ui/message_center/message_ce... File ui/message_center/message_center.gyp (right): https://codereview.chromium.org/369243002/diff/1/ui/message_center/message_ce... ui/message_center/message_center.gyp:152: ['include', '^notification_delegate\\.cc$'], On 2014/07/04 15:07:26, Peter Beverloo wrote: > This line can be removed since we'll already include it for Android on line 143. Ah sorry, I deleted the wrong line.
https://codereview.chromium.org/369243002/diff/1/ui/message_center/message_ce... File ui/message_center/message_center.gyp (left): https://codereview.chromium.org/369243002/diff/1/ui/message_center/message_ce... ui/message_center/message_center.gyp:152: ['include', '^notifier_settings\\.cc$'], On 2014/07/04 15:07:26, Peter Beverloo wrote: > Are you sure we don't need the NotifierId stuff defined in this file for > Android? It was accidentally deleted, restored.
On 2014/07/04 15:16:47, Jiang wrote: > https://codereview.chromium.org/369243002/diff/1/ui/message_center/message_ce... > File ui/message_center/message_center.gyp (left): > > https://codereview.chromium.org/369243002/diff/1/ui/message_center/message_ce... > ui/message_center/message_center.gyp:152: ['include', > '^notifier_settings\\.cc$'], > On 2014/07/04 15:07:26, Peter Beverloo wrote: > > Are you sure we don't need the NotifierId stuff defined in this file for > > Android? > > It was accidentally deleted, restored. Not quite sure why android try servers are failing.
On 2014/07/04 15:37:20, Jiang wrote: > On 2014/07/04 15:16:47, Jiang wrote: > > > https://codereview.chromium.org/369243002/diff/1/ui/message_center/message_ce... > > File ui/message_center/message_center.gyp (left): > > > > > https://codereview.chromium.org/369243002/diff/1/ui/message_center/message_ce... > > ui/message_center/message_center.gyp:152: ['include', > > '^notifier_settings\\.cc$'], > > On 2014/07/04 15:07:26, Peter Beverloo wrote: > > > Are you sure we don't need the NotifierId stuff defined in this file for > > > Android? > > > > It was accidentally deleted, restored. > > Not quite sure why android try servers are failing. Peter, can you please take another look at the patch?
lgtm, but you'll need a stamp from the OWNERS. I've launched a bunch of new try-bots too, both Android ones flaked on this build :/.
On 2014/07/07 09:18:27, Peter Beverloo wrote: > lgtm, but you'll need a stamp from the OWNERS. > > I've launched a bunch of new try-bots too, both Android ones flaked on this > build :/. Thanks Peter, so should I ping stevenjb or mukai for this review?
On 2014/07/07 09:21:16, Jiang wrote: > On 2014/07/07 09:18:27, Peter Beverloo wrote: > > lgtm, but you'll need a stamp from the OWNERS. > > > > I've launched a bunch of new try-bots too, both Android ones flaked on this > > build :/. > > Thanks Peter, so should I ping stevenjb or mukai for this review? Either will work :-). Basically anyone from the OWNERS file in either src/ui/message_center/, src/ui/ or src/ will be able to review your patch. Thanks for the patch!
dewittj, can you please take a look at this CL as well?
lgtm. I think dewittj is on leave
The CQ bit was checked by jiangj@opera.com
On 2014/07/07 17:02:39, Jun Mukai wrote: > lgtm. > > I think dewittj is on leave Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiangj@opera.com/369243002/20001
Message was sent while issue was closed.
Change committed as 281574 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
