[CrOS Tether] Open the Tether settings page when a tether notification body is clicked.

[CrOS Tether] Open the Tether settings page when a tether notification body is clicked.

BUG=672263
Review-Url: https://codereview.chromium.org/2861583002
Cr-Commit-Position: refs/heads/master@{#469214}
Committed: https://chromium.googlesource.com/chromium/src/+/98255c447048dbbcd7e30dccff90b812f98adb5a

[Kyle Horimoto, 2017-05-03 00:21:03]

khorimoto@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Kyle Horimoto, 2017-05-03 00:21:04]

Hey Steven - in the tests which call ShowSettingsSubPageForProfile(), I get the
below error, which is due to the check here:
https://cs.chromium.org/chromium/src/ash/shell.cc?l=268.

[32255:32255:0502/171854.822001:99058744084:FATAL:shell.cc(268)] Check failed:
HasInstance(). 
#0 0x000004c03a7c base::debug::StackTrace::StackTrace()
#1 0x000004c1bad1 logging::LogMessage::~LogMessage()
#2 0x0000068c62e2 ash::Shell::GetPrimaryRootWindow()
#3 0x000006c1f832 BrowserFrameAsh::GetWidgetParams()
#4 0x000006bd432a BrowserFrame::InitBrowserFrame()
#5 0x000006bdc8a5 BrowserWindow::CreateBrowserWindow()
#6 0x000006a8162f Browser::Browser()
#7 0x000006a9257f chrome::Navigate()
#8 0x000006ab2b72 chrome::SettingsWindowManager::ShowChromePageForProfile()
#9 0x000006a97f1b chrome::ShowSettingsSubPageForProfile()
#10 0x0000033d6ba4
chromeos::tether::TetherNotificationPresenter::OnNotificationClicked()
#11 0x00000231e8b4 chromeos::tether::(anonymous
namespace)::TestMessageCenter::NotifyNotificationTapped()
#12 0x000002320e31
chromeos::tether::TetherNotificationPresenterTest_TestMultiplePotentialHotspotNotification_TapNotification_Test::TestBody()
#13 0x000004985046 testing::Test::Run()
#14 0x000004985ac0 testing::TestInfo::Run()
#15 0x000004985ff7 testing::TestCase::Run()
#16 0x00000498d047 testing::internal::UnitTestImpl::RunAllTests()
#17 0x00000498cccb testing::UnitTest::Run()
#18 0x00000418c836 base::TestSuite::Run()
#19 0x00000418e30b base::(anonymous namespace)::LaunchUnitTestsInternal()
#20 0x00000418e196 base::LaunchUnitTests()
#21 0x000004186349 main
#22 0x7f4d27ed7f45 __libc_start_main
#23 0x0000006be71e <unknown>

What can I do to stub this out for tests?

[stevenjb, 2017-05-03 15:54:03]

On 2017/05/03 00:21:04, Kyle Horimoto wrote:
> Hey Steven - in the tests which call ShowSettingsSubPageForProfile(), I get
the
> below error, which is due to the check here:
> https://cs.chromium.org/chromium/src/ash/shell.cc?l=268.
> 
> [32255:32255:0502/171854.822001:99058744084:FATAL:shell.cc(268)] Check failed:
> HasInstance(). 
> #0 0x000004c03a7c base::debug::StackTrace::StackTrace()
> #1 0x000004c1bad1 logging::LogMessage::~LogMessage()
> #2 0x0000068c62e2 ash::Shell::GetPrimaryRootWindow()
> #3 0x000006c1f832 BrowserFrameAsh::GetWidgetParams()
> #4 0x000006bd432a BrowserFrame::InitBrowserFrame()
> #5 0x000006bdc8a5 BrowserWindow::CreateBrowserWindow()
> #6 0x000006a8162f Browser::Browser()
> #7 0x000006a9257f chrome::Navigate()
> #8 0x000006ab2b72 chrome::SettingsWindowManager::ShowChromePageForProfile()
> #9 0x000006a97f1b chrome::ShowSettingsSubPageForProfile()
> #10 0x0000033d6ba4
> chromeos::tether::TetherNotificationPresenter::OnNotificationClicked()
> #11 0x00000231e8b4 chromeos::tether::(anonymous
> namespace)::TestMessageCenter::NotifyNotificationTapped()
> #12 0x000002320e31
>
chromeos::tether::TetherNotificationPresenterTest_TestMultiplePotentialHotspotNotification_TapNotification_Test::TestBody()
> #13 0x000004985046 testing::Test::Run()
> #14 0x000004985ac0 testing::TestInfo::Run()
> #15 0x000004985ff7 testing::TestCase::Run()
> #16 0x00000498d047 testing::internal::UnitTestImpl::RunAllTests()
> #17 0x00000498cccb testing::UnitTest::Run()
> #18 0x00000418c836 base::TestSuite::Run()
> #19 0x00000418e30b base::(anonymous namespace)::LaunchUnitTestsInternal()
> #20 0x00000418e196 base::LaunchUnitTests()
> #21 0x000004186349 main
> #22 0x7f4d27ed7f45 __libc_start_main
> #23 0x0000006be71e <unknown>
> 
> What can I do to stub this out for tests?

So, if you wanted to test actually showing Settings, you would want to write a
browser test (and ensure that code path wasn't called in a unit test), but
Settings is pretty well tested so I wouldn't bother.

Instead you can create a simple UI Delegate in TetherNotificationPresenter for
showing settings and override it for the test (with a SetDelegateForTest method
like we did before).

[Kyle Horimoto, 2017-05-03 19:55:35]

Description was changed from

==========
[CrOS Tether] Open the Tether settings page when a tether notification body is
clicked.

Note: This CL is a work-in-progress and is not yet ready for review.

BUG=672263
==========

to

==========
[CrOS Tether] Open the Tether settings page when a tether notification body is
clicked.

BUG=672263
==========

[Kyle Horimoto, 2017-05-03 19:55:50]

On 2017/05/03 15:54:03, stevenjb wrote:
> On 2017/05/03 00:21:04, Kyle Horimoto wrote:
> > Hey Steven - in the tests which call ShowSettingsSubPageForProfile(), I get
> the
> > below error, which is due to the check here:
> > https://cs.chromium.org/chromium/src/ash/shell.cc?l=268.
> > 
> > [32255:32255:0502/171854.822001:99058744084:FATAL:shell.cc(268)] Check
failed:
> > HasInstance(). 
> > #0 0x000004c03a7c base::debug::StackTrace::StackTrace()
> > #1 0x000004c1bad1 logging::LogMessage::~LogMessage()
> > #2 0x0000068c62e2 ash::Shell::GetPrimaryRootWindow()
> > #3 0x000006c1f832 BrowserFrameAsh::GetWidgetParams()
> > #4 0x000006bd432a BrowserFrame::InitBrowserFrame()
> > #5 0x000006bdc8a5 BrowserWindow::CreateBrowserWindow()
> > #6 0x000006a8162f Browser::Browser()
> > #7 0x000006a9257f chrome::Navigate()
> > #8 0x000006ab2b72 chrome::SettingsWindowManager::ShowChromePageForProfile()
> > #9 0x000006a97f1b chrome::ShowSettingsSubPageForProfile()
> > #10 0x0000033d6ba4
> > chromeos::tether::TetherNotificationPresenter::OnNotificationClicked()
> > #11 0x00000231e8b4 chromeos::tether::(anonymous
> > namespace)::TestMessageCenter::NotifyNotificationTapped()
> > #12 0x000002320e31
> >
>
chromeos::tether::TetherNotificationPresenterTest_TestMultiplePotentialHotspotNotification_TapNotification_Test::TestBody()
> > #13 0x000004985046 testing::Test::Run()
> > #14 0x000004985ac0 testing::TestInfo::Run()
> > #15 0x000004985ff7 testing::TestCase::Run()
> > #16 0x00000498d047 testing::internal::UnitTestImpl::RunAllTests()
> > #17 0x00000498cccb testing::UnitTest::Run()
> > #18 0x00000418c836 base::TestSuite::Run()
> > #19 0x00000418e30b base::(anonymous namespace)::LaunchUnitTestsInternal()
> > #20 0x00000418e196 base::LaunchUnitTests()
> > #21 0x000004186349 main
> > #22 0x7f4d27ed7f45 __libc_start_main
> > #23 0x0000006be71e <unknown>
> > 
> > What can I do to stub this out for tests?
> 
> So, if you wanted to test actually showing Settings, you would want to write a
> browser test (and ensure that code path wasn't called in a unit test), but
> Settings is pretty well tested so I wouldn't bother.
> 
> Instead you can create a simple UI Delegate in TetherNotificationPresenter for
> showing settings and override it for the test (with a SetDelegateForTest
method
> like we did before).

Done.

This CL is now ready for review - PTAL. Thanks!

[Kyle Horimoto, 2017-05-03 19:55:51]

On 2017/05/03 15:54:03, stevenjb wrote:
> On 2017/05/03 00:21:04, Kyle Horimoto wrote:
> > Hey Steven - in the tests which call ShowSettingsSubPageForProfile(), I get
> the
> > below error, which is due to the check here:
> > https://cs.chromium.org/chromium/src/ash/shell.cc?l=268.
> > 
> > [32255:32255:0502/171854.822001:99058744084:FATAL:shell.cc(268)] Check
failed:
> > HasInstance(). 
> > #0 0x000004c03a7c base::debug::StackTrace::StackTrace()
> > #1 0x000004c1bad1 logging::LogMessage::~LogMessage()
> > #2 0x0000068c62e2 ash::Shell::GetPrimaryRootWindow()
> > #3 0x000006c1f832 BrowserFrameAsh::GetWidgetParams()
> > #4 0x000006bd432a BrowserFrame::InitBrowserFrame()
> > #5 0x000006bdc8a5 BrowserWindow::CreateBrowserWindow()
> > #6 0x000006a8162f Browser::Browser()
> > #7 0x000006a9257f chrome::Navigate()
> > #8 0x000006ab2b72 chrome::SettingsWindowManager::ShowChromePageForProfile()
> > #9 0x000006a97f1b chrome::ShowSettingsSubPageForProfile()
> > #10 0x0000033d6ba4
> > chromeos::tether::TetherNotificationPresenter::OnNotificationClicked()
> > #11 0x00000231e8b4 chromeos::tether::(anonymous
> > namespace)::TestMessageCenter::NotifyNotificationTapped()
> > #12 0x000002320e31
> >
>
chromeos::tether::TetherNotificationPresenterTest_TestMultiplePotentialHotspotNotification_TapNotification_Test::TestBody()
> > #13 0x000004985046 testing::Test::Run()
> > #14 0x000004985ac0 testing::TestInfo::Run()
> > #15 0x000004985ff7 testing::TestCase::Run()
> > #16 0x00000498d047 testing::internal::UnitTestImpl::RunAllTests()
> > #17 0x00000498cccb testing::UnitTest::Run()
> > #18 0x00000418c836 base::TestSuite::Run()
> > #19 0x00000418e30b base::(anonymous namespace)::LaunchUnitTestsInternal()
> > #20 0x00000418e196 base::LaunchUnitTests()
> > #21 0x000004186349 main
> > #22 0x7f4d27ed7f45 __libc_start_main
> > #23 0x0000006be71e <unknown>
> > 
> > What can I do to stub this out for tests?
> 
> So, if you wanted to test actually showing Settings, you would want to write a
> browser test (and ensure that code path wasn't called in a unit test), but
> Settings is pretty well tested so I wouldn't bother.
> 
> Instead you can create a simple UI Delegate in TetherNotificationPresenter for
> showing settings and override it for the test (with a SetDelegateForTest
method
> like we did before).

Done.

This CL is now ready for review - PTAL. Thanks!

[stevenjb, 2017-05-03 23:07:44]

https://codereview.chromium.org/2861583002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/tether_notification_presenter.h (right):

https://codereview.chromium.org/2861583002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/net/tether_notification_presenter.h:78: virtual
~SettingsUiDelegate();
blank line and comment for ShowSettingsSubPageForProfile

https://codereview.chromium.org/2861583002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/net/tether_notification_presenter.h:80: const
std::string& sub_page);
This should be a pure virtual. Create a local Impl class in the .cc file. Also
make the destructor protected.

https://codereview.chromium.org/2861583002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/net/tether_notification_presenter.h:84:
std::unique_ptr<SettingsUiDelegate> settings_ui_delegate);
blank line

https://codereview.chromium.org/2861583002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/tether_notification_presenter_unittest.cc
(right):

https://codereview.chromium.org/2861583002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/net/tether_notification_presenter_unittest.cc:144:
TestSettingsUiDelegate() : last_profile_(nullptr) {}
nit: set to nullptr inline in declaration.

[Kyle Horimoto, 2017-05-03 23:25:48]

https://codereview.chromium.org/2861583002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/tether_notification_presenter.h (right):

https://codereview.chromium.org/2861583002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/net/tether_notification_presenter.h:78: virtual
~SettingsUiDelegate();
On 2017/05/03 23:07:44, stevenjb wrote:
> blank line and comment for ShowSettingsSubPageForProfile

Done.

https://codereview.chromium.org/2861583002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/net/tether_notification_presenter.h:80: const
std::string& sub_page);
On 2017/05/03 23:07:44, stevenjb wrote:
> This should be a pure virtual. Create a local Impl class in the .cc file. Also
> make the destructor protected.

Done, but I made the destructor public. Otherwise, I got a compiler error about
how unique_ptr can't delete it.

https://codereview.chromium.org/2861583002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/net/tether_notification_presenter.h:84:
std::unique_ptr<SettingsUiDelegate> settings_ui_delegate);
On 2017/05/03 23:07:44, stevenjb wrote:
> blank line

Done.

https://codereview.chromium.org/2861583002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/tether_notification_presenter_unittest.cc
(right):

https://codereview.chromium.org/2861583002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/net/tether_notification_presenter_unittest.cc:144:
TestSettingsUiDelegate() : last_profile_(nullptr) {}
On 2017/05/03 23:07:44, stevenjb wrote:
> nit: set to nullptr inline in declaration.

Done.

[stevenjb, 2017-05-03 23:42:37]

lgtm

https://codereview.chromium.org/2861583002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/tether_notification_presenter.h (right):

https://codereview.chromium.org/2861583002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/net/tether_notification_presenter.h:80: const
std::string& sub_page);
On 2017/05/03 23:25:48, Kyle Horimoto wrote:
> On 2017/05/03 23:07:44, stevenjb wrote:
> > This should be a pure virtual. Create a local Impl class in the .cc file.
Also
> > make the destructor protected.
> 
> Done, but I made the destructor public. Otherwise, I got a compiler error
about
> how unique_ptr can't delete it.

Acknowledged.

[Kyle Horimoto, 2017-05-03 23:47:11]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-05-03 23:47:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 00:22:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493855231964280,
"parent_rev": "7c97df67012325269eaf4a3f627e442f47266bbb", "commit_rev":
"98255c447048dbbcd7e30dccff90b812f98adb5a"}

[commit-bot: I haz the power, 2017-05-04 00:22:45]

Description was changed from

==========
[CrOS Tether] Open the Tether settings page when a tether notification body is
clicked.

BUG=672263
==========

to

==========
[CrOS Tether] Open the Tether settings page when a tether notification body is
clicked.

BUG=672263

Review-Url: https://codereview.chromium.org/2861583002
Cr-Commit-Position: refs/heads/master@{#469214}
Committed:
https://chromium.googlesource.com/chromium/src/+/98255c447048dbbcd7e30dccff90...
==========

[commit-bot: I haz the power, 2017-05-04 00:22:46]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/98255c447048dbbcd7e30dccff90...