Fix a forgotten return statement in BrowserContentSettingBubbleModelDelegate.

Fix a forgotten return statement in BrowserContentSettingBubbleModelDelegate.

This meant that for bubbles which were invoked for both camera and microphone, we navigated correctly to the default settings page, but then immediately to mediastream exceptions page, which doesn't exist anymore.

NOTE: This test can fail if the settings UI opens in a separate window, but BrowserList does not recognize the window as active yet. Several rounds of testing on trybots suggest that this does not happen. If it in fact does happen, feel free to disable this test. msramek@ can then take another look and restrict it to the same-window case, or fix it to wait for the new window to be focused.

BUG=515376, 509962
Committed: https://crrev.com/04dfde123d21ac9cb872adc63bcaeba7d7532eba
Cr-Commit-Position: refs/heads/master@{#342997}

[msramek, 2015-08-05 10:56:43]

msramek@chromium.org changed reviewers:
+ msw@chromium.org

[msramek, 2015-08-05 10:56:44]

Hi Mike,

could you have a look at this tiny CL? It's a followup to
https://codereview.chromium.org/1266583003/ which you reviewed before. The
change worked well for microphone and camera, but not for both at the same time,
due to a forgotten "return;" statement :/

...this time I did two rounds of testing to be sure.

Thanks,
Martin

[msramek, 2015-08-05 15:33:36]

Actually, let me add a browsertest as well :)

[msw, 2015-08-05 17:05:38]

Nice test, just some nits to make it a bit cleaner.

https://codereview.chromium.org/1273683002/diff/20001/chrome/browser/ui/conte...
File
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
(right):

https://codereview.chromium.org/1273683002/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:104:
scoped_ptr<ContentSettingBubbleModel> GetNewBubbleInTheFirstTab() {
nit: rename this CreateBubble(), or better yet combine the helpers:
  scoped_ptr<ContentSettingBubbleModel> CreateBubbleForMediaStreamPermission(
      TabSpecificContentSettings::MicrophoneCameraState state) {
    <call OnMediaStreamPermissionSet>
    <return CreateContentSettingBubbleModel>
  }

Optionally, go even further to simplify the test fixture:
  void ManageMediaStreamPermission(
      TabSpecificContentSettings::MicrophoneCameraState state) {
    <call OnMediaStreamPermissionSet>
    <store local scoped_ptr for CreateContentSettingBubbleModel>
    <call OnManageLinkClicked>
    <call WaitForNavigation>
  }
  TEST_F {
    <ManageMediaStreamPermission(mic)>
    <EXPECT_EQ>
    <ManageMediaStreamPermission(cam)>
    <EXPECT_EQ>
    <ManageMediaStreamPermission(mic|cam)>
    <EXPECT_EQ>
  }

https://codereview.chromium.org/1273683002/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:123:
browser()->tab_strip_model()->GetActiveWebContents());
nit: use GetFirstTab() here?

https://codereview.chromium.org/1273683002/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:132:
std::string device_name_ = "is not important for this test";
nit: can you remove this and just pass std::string()?

[msramek, 2015-08-05 17:39:28]

https://codereview.chromium.org/1273683002/diff/20001/chrome/browser/ui/conte...
File
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
(right):

https://codereview.chromium.org/1273683002/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:104:
scoped_ptr<ContentSettingBubbleModel> GetNewBubbleInTheFirstTab() {
On 2015/08/05 17:05:38, msw wrote:
> nit: rename this CreateBubble(), or better yet combine the helpers:
>   scoped_ptr<ContentSettingBubbleModel> CreateBubbleForMediaStreamPermission(
>       TabSpecificContentSettings::MicrophoneCameraState state) {
>     <call OnMediaStreamPermissionSet>
>     <return CreateContentSettingBubbleModel>
>   }
> 
> Optionally, go even further to simplify the test fixture:
>   void ManageMediaStreamPermission(
>       TabSpecificContentSettings::MicrophoneCameraState state) {
>     <call OnMediaStreamPermissionSet>
>     <store local scoped_ptr for CreateContentSettingBubbleModel>
>     <call OnManageLinkClicked>
>     <call WaitForNavigation>
>   }
>   TEST_F {
>     <ManageMediaStreamPermission(mic)>
>     <EXPECT_EQ>
>     <ManageMediaStreamPermission(cam)>
>     <EXPECT_EQ>
>     <ManageMediaStreamPermission(mic|cam)>
>     <EXPECT_EQ>
>   }

Yep, that sounds good.

https://codereview.chromium.org/1273683002/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:123:
browser()->tab_strip_model()->GetActiveWebContents());
On 2015/08/05 17:05:38, msw wrote:
> nit: use GetFirstTab() here?

No actually. The bubble itself is invoked for the first tab, but clicking the
manage link will open chrome://settings/... in a separate tab (the index doesn't
matter, but we know that this tab will be active, hence GetActiveWebContents()).

Special case, if there is at least one chrome://settings/ tab already open, the
first of them becomes active. So if we invoked the bubble for a Tab that already
has the settings open in it, we would need just one tab. But I think it's better
to keep it more general.

https://codereview.chromium.org/1273683002/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:132:
std::string device_name_ = "is not important for this test";
On 2015/08/05 17:05:38, msw wrote:
> nit: can you remove this and just pass std::string()?

Done.

[msw, 2015-08-05 17:47:20]

lgtm, thanks

https://codereview.chromium.org/1273683002/diff/20001/chrome/browser/ui/conte...
File
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
(right):

https://codereview.chromium.org/1273683002/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:123:
browser()->tab_strip_model()->GetActiveWebContents());
On 2015/08/05 17:39:28, msramek wrote:
> On 2015/08/05 17:05:38, msw wrote:
> > nit: use GetFirstTab() here?
> 
> No actually. The bubble itself is invoked for the first tab, but clicking the
> manage link will open chrome://settings/... in a separate tab (the index
doesn't
> matter, but we know that this tab will be active, hence
GetActiveWebContents()).
> 
> Special case, if there is at least one chrome://settings/ tab already open,
the
> first of them becomes active. So if we invoked the bubble for a Tab that
already
> has the settings open in it, we would need just one tab. But I think it's
better
> to keep it more general.

Acknowledged.

[msramek, 2015-08-05 17:59:03]

Thank you!

[msramek, 2015-08-05 17:59:07]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2015-08-05 17:59:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273683002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273683002/40001

[commit-bot: I haz the power, 2015-08-05 18:56:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-05 18:56:48]

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...)

[msramek, 2015-08-05 22:46:49]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2015-08-05 22:47:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273683002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273683002/40001

[commit-bot: I haz the power, 2015-08-06 00:03:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 00:03:55]

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_...)

[msramek, 2015-08-06 15:12:50]

The ChromeOS test was consistently getting SIGTERM and I thought that the
navigation just took too long. I removed the check whether the navigation
finished, and looked for the visible url (which might or might not have finished
loading) instead of the committed url.

I left this change in, as it makes a bit more sense (we're not really interested
in whether the loading finishes), but it did not solve the problem.

Then I checked on ChromeOS - the settings are opened not just in a separate tab,
but also in a separate popup window. So I changed the test to look for the
active tab of the active window.

PTAL if you have any more comments :)

Thanks,
Martin

[msw, 2015-08-06 18:23:21]

lgtm with minor nits.

https://codereview.chromium.org/1273683002/diff/80001/chrome/browser/ui/conte...
File
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
(right):

https://codereview.chromium.org/1273683002/diff/80001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:18:
#include "content/public/test/test_navigation_observer.h"
nit: remove if not needed.

https://codereview.chromium.org/1273683002/diff/80001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:119:
// Click the management link.
nit "Click the management link, which opens in a new tab or window."

[msramek, 2015-08-06 18:32:11]

Thanks!

https://codereview.chromium.org/1273683002/diff/80001/chrome/browser/ui/conte...
File
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
(right):

https://codereview.chromium.org/1273683002/diff/80001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:18:
#include "content/public/test/test_navigation_observer.h"
On 2015/08/06 18:23:20, msw wrote:
> nit: remove if not needed.

It's still used by ContentSettingBubbleModelMixedScriptTest.

https://codereview.chromium.org/1273683002/diff/80001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:119:
// Click the management link.
On 2015/08/06 18:23:20, msw wrote:
> nit "Click the management link, which opens in a new tab or window."

Done.

[msramek, 2015-08-06 18:32:28]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-08-06 18:32:29]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1273683002/#ps100001
(title: "Improved a comment.")

[commit-bot: I haz the power, 2015-08-06 18:33:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273683002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273683002/100001

[msw, 2015-08-06 18:41:30]

https://codereview.chromium.org/1273683002/diff/100001/chrome/browser/ui/cont...
File
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
(right):

https://codereview.chromium.org/1273683002/diff/100001/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:120:
bubble->OnManageLinkClicked();
One more thought (sorry). You might actually want to still wait for this tab to
load (or at least open), if you synchronously click a link and then try to get
the active window/tab, the window manager might not have even processed the
focus change and this test might be flaky as-is. It probably wouldn't hurt to
leave the nav observer wait here.

[msramek, 2015-08-06 18:42:28]

The CQ bit was unchecked by msramek@chromium.org

[msramek, 2015-08-06 19:11:25]

https://codereview.chromium.org/1273683002/diff/100001/chrome/browser/ui/cont...
File
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
(right):

https://codereview.chromium.org/1273683002/diff/100001/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:120:
bubble->OnManageLinkClicked();
On 2015/08/06 18:41:30, msw wrote:
> One more thought (sorry). You might actually want to still wait for this tab
to
> load (or at least open), if you synchronously click a link and then try to get
> the active window/tab, the window manager might not have even processed the
> focus change and this test might be flaky as-is. It probably wouldn't hurt to
> leave the nav observer wait here.

Well, but the nav observer waits for a specific tab to finish navigation. If we
can't rely on the newly opened tab being focused, then we don't know on which
tab to wait for the navigation. So that wouldn't solve the problem. (see e.g.
line 96 above - we're waiting for GetActiveWebContents()).

Waiting for a navigation would only help us if GetVisibleUrl() didn't return the
currently loaded URL immediately, but only a while after the new tab was opened
and focused.

I readded the observer in case there is something else I missed (I mean, a lot
of other tests do it that way), but it would probably have been ok anyway.

[msw, 2015-08-06 19:24:11]

https://codereview.chromium.org/1273683002/diff/100001/chrome/browser/ui/cont...
File
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
(right):

https://codereview.chromium.org/1273683002/diff/100001/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:120:
bubble->OnManageLinkClicked();
On 2015/08/06 19:11:25, msramek wrote:
> On 2015/08/06 18:41:30, msw wrote:
> > One more thought (sorry). You might actually want to still wait for this tab
> to
> > load (or at least open), if you synchronously click a link and then try to
get
> > the active window/tab, the window manager might not have even processed the
> > focus change and this test might be flaky as-is. It probably wouldn't hurt
to
> > leave the nav observer wait here.
> 
> Well, but the nav observer waits for a specific tab to finish navigation. If
we
> can't rely on the newly opened tab being focused, then we don't know on which
> tab to wait for the navigation. So that wouldn't solve the problem. (see e.g.
> line 96 above - we're waiting for GetActiveWebContents()).
> 
> Waiting for a navigation would only help us if GetVisibleUrl() didn't return
the
> currently loaded URL immediately, but only a while after the new tab was
opened
> and focused.
> 
> I readded the observer in case there is something else I missed (I mean, a lot
> of other tests do it that way), but it would probably have been ok anyway.

Hmm, good point. I hope GetActiveTab() doesn't return the old tab used to launch
the manage link, otherwise this might wait indefinitely for a navigation that's
happening in another window/tab that wasn't yet opened/active, and then the test
would time out. Maybe you could add a fatal ASSERT_NE(original_tab,
GetActiveTab()) before trying to wait (doing original_tab = GetFirstTab()
earlier, potentially in-lining GetFirstTab(), since then it'd just be called
once). Otherwise, I don't know how make this bullet-proof; you might just have
to watch this test to see if it's flaky, or ping someone else for better advice.

[msramek, 2015-08-11 13:07:41]

Sorry for the late reply. I was OOO and didn't want to land a possibly flaky
test right before that :)

https://codereview.chromium.org/1273683002/diff/100001/chrome/browser/ui/cont...
File
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
(right):

https://codereview.chromium.org/1273683002/diff/100001/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:120:
bubble->OnManageLinkClicked();
On 2015/08/06 19:24:11, msw wrote:
> On 2015/08/06 19:11:25, msramek wrote:
> > On 2015/08/06 18:41:30, msw wrote:
> > > One more thought (sorry). You might actually want to still wait for this
tab
> > to
> > > load (or at least open), if you synchronously click a link and then try to
> get
> > > the active window/tab, the window manager might not have even processed
the
> > > focus change and this test might be flaky as-is. It probably wouldn't hurt
> to
> > > leave the nav observer wait here.
> > 
> > Well, but the nav observer waits for a specific tab to finish navigation. If
> we
> > can't rely on the newly opened tab being focused, then we don't know on
which
> > tab to wait for the navigation. So that wouldn't solve the problem. (see
e.g.
> > line 96 above - we're waiting for GetActiveWebContents()).
> > 
> > Waiting for a navigation would only help us if GetVisibleUrl() didn't return
> the
> > currently loaded URL immediately, but only a while after the new tab was
> opened
> > and focused.
> > 
> > I readded the observer in case there is something else I missed (I mean, a
lot
> > of other tests do it that way), but it would probably have been ok anyway.
> 
> Hmm, good point. I hope GetActiveTab() doesn't return the old tab used to
launch
> the manage link, otherwise this might wait indefinitely for a navigation
that's
> happening in another window/tab that wasn't yet opened/active, and then the
test
> would time out. Maybe you could add a fatal ASSERT_NE(original_tab,
> GetActiveTab()) before trying to wait (doing original_tab = GetFirstTab()
> earlier, potentially in-lining GetFirstTab(), since then it'd just be called
> once). Otherwise, I don't know how make this bullet-proof; you might just have
> to watch this test to see if it's flaky, or ping someone else for better
advice.

Done. I inlined the original tab and added an assert. I made sure that the
original tab points to a non-internal URL, otherwise the assert would not even
be true - if there already is a chrome://something tab open, it gets reused by
|chrome::ShowContentSettings|.

My trybots are still green after several tries, so I feel comfortable landing
this and watching the status. I also left a comment so that sheriffs feel free
to disable the test if necessary.

[msw, 2015-08-11 17:03:14]

lgtm with nits.

https://codereview.chromium.org/1273683002/diff/140001/chrome/browser/ui/cont...
File
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
(right):

https://codereview.chromium.org/1273683002/diff/140001/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:108:
ui_test_utils::NavigateToURL(
nit: follow the pattern of the other tests:
GURL url(https_server_->GetURL(
    "files/content_setting_bubble/mixed_script.html"));
ui_test_utils::NavigateToURL(browser(), url);

https://codereview.chromium.org/1273683002/diff/140001/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:146:
// TODO(msramek): This test can fail if the settings UI opens in a separate
nit: put this in the CL description, not the code.

[msramek, 2015-08-12 08:40:29]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-08-12 08:40:30]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1273683002/#ps160001
(title: "Nits.")

[commit-bot: I haz the power, 2015-08-12 08:40:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273683002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273683002/160001

[msramek, 2015-08-12 08:41:58]

https://codereview.chromium.org/1273683002/diff/140001/chrome/browser/ui/cont...
File
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
(right):

https://codereview.chromium.org/1273683002/diff/140001/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:108:
ui_test_utils::NavigateToURL(
On 2015/08/11 17:03:14, msw wrote:
> nit: follow the pattern of the other tests:
> GURL url(https_server_->GetURL(
>     "files/content_setting_bubble/mixed_script.html"));
> ui_test_utils::NavigateToURL(browser(), url);

Well, I can separate the construction of the URL from the navigation itself.

Other than that, I don't have a server instance here. To test mixed content, we
need an HTTPS server, but I don't think it's worth to add the same boilerplate
code here just for the sake of consistency. I just need the tab to navigate to
an existing non-internal URL, for which ui_test_utils::GetTestUrl() is suitable.

https://codereview.chromium.org/1273683002/diff/140001/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:146:
// TODO(msramek): This test can fail if the settings UI opens in a separate
On 2015/08/11 17:03:14, msw wrote:
> nit: put this in the CL description, not the code.

Done.

[commit-bot: I haz the power, 2015-08-12 09:37:34]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-08-12 09:38:06]

Patchset 9 (id:??) landed as
https://crrev.com/04dfde123d21ac9cb872adc63bcaeba7d7532eba
Cr-Commit-Position: refs/heads/master@{#342997}