Settings Overides -- warn user when extensions override their NTP.

Settings Overides -- warn user when extensions override their NTP.

BUG=366103
R=sky@chromium.org, yoz@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266878

[Finnur, 2014-04-22 18:16:33]

Yoyo, do you think you'll have a chance to look at this today (before your time
off)?

Peter, can you review browser_tabstrip.cc and DEPS (including the comment
below)?

https://codereview.chromium.org/246653003/diff/20001/chrome/browser/ui/DEPS
File chrome/browser/ui/DEPS (right):

https://codereview.chromium.org/246653003/diff/20001/chrome/browser/ui/DEPS#n...
chrome/browser/ui/DEPS:4:
"+chrome/browser/ui/views/settings_api_bubble_helper_views.h"
I'll admit, this is a bit unfortunate. Is there a way to accomplish the same
without a wholesale refactor of how NewTab works? I'm drawing a blank... :/

[Peter Kasting, 2014-04-22 20:28:39]

I agree that the browser_tabstrip.cc/DEPS change looks bad.  I'm switching the
review for those to sky, who is more likely to have a concrete suggestion on how
to address this.  Perhaps the tabstrip code here needs to call some abstract
function which the views code implements to do what you want.

[sky, 2014-04-22 22:13:26]

https://codereview.chromium.org/246653003/diff/20001/chrome/browser/ui/DEPS
File chrome/browser/ui/DEPS (right):

https://codereview.chromium.org/246653003/diff/20001/chrome/browser/ui/DEPS#n...
chrome/browser/ui/DEPS:4:
"+chrome/browser/ui/views/settings_api_bubble_helper_views.h"
On 2014/04/22 18:16:33, Finnur wrote:
> I'll admit, this is a bit unfortunate. Is there a way to accomplish the same
> without a wholesale refactor of how NewTab works? I'm drawing a blank... :/

Yes, we definitely don't want this dependency here. Probably best to call your
function
from BrowserView::TabInsertedAt. You'll need a check of the URL, and you'll have
to be careful you don't accidentally show when dragging a tab from one window to
another.

[Yoyo Zhou, 2014-04-23 00:04:23]

LGTM. Will this bubble show for existing users of NTP overriding extensions?

https://chromiumcodereview.appspot.com/246653003/diff/20001/chrome/browser/ex...
File chrome/browser/extensions/ntp_controlled_bubble_controller.h (right):

https://chromiumcodereview.appspot.com/246653003/diff/20001/chrome/browser/ex...
chrome/browser/extensions/ntp_controlled_bubble_controller.h:15: class
NtpControlledBubbleController : public ExtensionMessageBubbleController {
How about NTPOverriddenBubbleController (since we call these override pages)? I
prefer "NTP" vs. "Ntp" but I don't feel strongly.

https://chromiumcodereview.appspot.com/246653003/diff/20001/chrome/browser/ui...
File chrome/browser/ui/views/settings_api_bubble_helper_views.cc (right):

https://chromiumcodereview.appspot.com/246653003/diff/20001/chrome/browser/ui...
chrome/browser/ui/views/settings_api_bubble_helper_views.cc:102:
scoped_ptr<extensions::NtpControlledBubbleController> ntp_controlled_bubble(
nit: don't need extensions::

https://chromiumcodereview.appspot.com/246653003/diff/20001/extensions/browse...
File extensions/browser/extension_prefs.h (right):

https://chromiumcodereview.appspot.com/246653003/diff/20001/extensions/browse...
extensions/browser/extension_prefs.h:328: bool
HasNtpControlledBubbleBeenAcknowledged(const std::string& extension_id);
likewise, Overridden instead of Controlled?

[Yoyo Zhou, 2014-04-23 00:08:34]

https://chromiumcodereview.appspot.com/246653003/diff/20001/chrome/browser/ex...
File chrome/browser/extensions/ntp_controlled_bubble_controller.cc (right):

https://chromiumcodereview.appspot.com/246653003/diff/20001/chrome/browser/ex...
chrome/browser/extensions/ntp_controlled_bubble_controller.cc:157: void
NtpControlledBubbleDelegate::RestrictToSingleExtension(
Considering the lifetime of NtpControlledBubbleController, this seems like it
may be unnecessary.

[Finnur, 2014-04-23 11:57:56]

So that I don't forget to answer, I'll tackle this question right away:

> Will this bubble show for existing users of NTP overriding extensions?

Yes. I believe that's the intent.

[Finnur, 2014-04-23 13:51:35]

https://codereview.chromium.org/246653003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/ntp_controlled_bubble_controller.h (right):

https://codereview.chromium.org/246653003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/ntp_controlled_bubble_controller.h:15: class
NtpControlledBubbleController : public ExtensionMessageBubbleController {
Made the change Controlled -> Overridden, but Ntp seems to be favored in the
codebase over NTP.

On 2014/04/23 00:04:24, Yoyo Zhou wrote:
> How about NTPOverriddenBubbleController (since we call these override pages)?
I
> prefer "NTP" vs. "Ntp" but I don't feel strongly.

https://codereview.chromium.org/246653003/diff/20001/chrome/browser/ui/DEPS
File chrome/browser/ui/DEPS (right):

https://codereview.chromium.org/246653003/diff/20001/chrome/browser/ui/DEPS#n...
chrome/browser/ui/DEPS:4:
"+chrome/browser/ui/views/settings_api_bubble_helper_views.h"
Thanks for the suggestion!

It turns out I don't need to worry about dragging. You can't drag a tab that
hasn't been created and this one-time bubble only shows up the first time the
new tab page is created. I did check, though, to see if the tab being created is
in the foreground, because that's consistent with when we want to show the
bubble (no bubble for background tabs, ever) -- and that flag, incidentally, is
set to false when dragging, so that should cover dragging as well (if it were to
come to that).

https://codereview.chromium.org/246653003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/settings_api_bubble_helper_views.cc (right):

https://codereview.chromium.org/246653003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/settings_api_bubble_helper_views.cc:102:
scoped_ptr<extensions::NtpControlledBubbleController> ntp_controlled_bubble(
On 2014/04/23 00:04:24, Yoyo Zhou wrote:
> nit: don't need extensions::

Good point (this function moved around a bit during previous unsuccessful
attempts at re-factoring to get rid of the nasty DEPS change). Done now.

https://codereview.chromium.org/246653003/diff/20001/extensions/browser/exten...
File extensions/browser/extension_prefs.h (right):

https://codereview.chromium.org/246653003/diff/20001/extensions/browser/exten...
extensions/browser/extension_prefs.h:328: bool
HasNtpControlledBubbleBeenAcknowledged(const std::string& extension_id);
On 2014/04/23 00:04:24, Yoyo Zhou wrote:
> likewise, Overridden instead of Controlled?

Done.

[Finnur, 2014-04-25 11:21:43]

Scott: ping?

[sky, 2014-04-25 14:17:10]

LGTM

[Finnur, 2014-04-28 14:05:51]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2014-04-28 14:06:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/finnur@chromium.org/246653003/120001

[commit-bot: I haz the power, 2014-04-28 14:51:10]

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

[commit-bot: I haz the power, 2014-04-28 14:51:11]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[Finnur, 2014-04-29 11:15:44]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2014-04-29 11:16:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/finnur@chromium.org/246653003/140001

[commit-bot: I haz the power, 2014-04-29 12:13:33]

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

[commit-bot: I haz the power, 2014-04-29 12:13:34]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Finnur, 2014-04-29 14:28:36]

Committed patchset #8 manually as r266878 (presubmit successful).