Add an extension override bubble and warning box for proxy extensions.

Add an extension override bubble and warning box for proxy extensions.

Also use the browser action highlighting for extensions that have a browser action icon.

BUG=381291
R=dbeam@chromium.org, rdevlin.cronin@chromium.org, sky@chromium.org

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

[Finnur, 2014-05-20 16:03:34]

Devlin, mind taking this?

dbeam: OWNERS check for 
c/b/resources/options

[Dan Beam, 2014-05-20 16:46:41]

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:33: <h3 id="proxy-section"
i18n-content="sectionTitleProxy" hidden></h3>
can you hide the whole <section> instead?

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1507: * Set the enabled
state for the proxy settings button.
can you update this to say "proxy settings section" or something like this?

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1507: * Set the enabled
state for the proxy settings button.
nit: can you document these params? e.g.

  * @param {<type_goes_here>} disabled ....
  * @param {<type_goes_here>} extensionControlled ...

[Devlin, 2014-05-20 17:26:31]

https://codereview.chromium.org/288923004/diff/110001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5161: Your Internet connection is being
controlled
I assume "Internet" should not be capitalized - unless that's a UI decision?

https://codereview.chromium.org/288923004/diff/110001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5186: <message
name="IDS_EXTENSIONS_PROXY_CONTROLLED_FIRST_LINE" desc="Text displayed in the
Settings API bubble as first line when an extension has changed the proxy.">
s/in the Settings API bubble as first line/as the first line in the Settings*
API bubble

This isn't the Settings API bubble, though, right?  Or are we lumping them
together?

https://codereview.chromium.org/288923004/diff/110001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5187: An extension has taken control of your
proxy settings, which means it can change, break or eavesdrop on anything you do
online. If you aren't sure why this change happened, you probably don't want it.
nit: break, or eavesdrop

You'll get different answers on this, so feel free to ignore it.  But I prefer
", and" for all kinds of reasons.  (Classic example: if a last will leaves an
inheritance to 'Joe, Fred, and Bob' they all get 1/3 - if it's to 'Joe, Fred and
Bob', then Joe gets half and Fred and Bob split the other half.)

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
(right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:21:
#include "extensions/common/extension_builder.h"
nit: also include value_builder.h

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:173:
ProxyOverriddenBubbleController::OnBubbleAction();
So much of this code is duplicated, but I can't think of any better way off the
top of my head.  I'll think about it for a bit.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:320:
extensions::ExtensionBuilder builder;
no extensions:: prefix.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:330:
service_->AddExtension(builder.Build().get());
We'll get a ton of fun errors if these extensions just fail to install.  How
about:

testing::AssertionResult LoadExtensionOverridingProxy() {
  ...
  if (!ExtensionRegistry::Get(profile())->enabled_extensions().GetByID(id))
    return testing::AssertionFailure() << "Could not install extension: " << id;
  return testing::AssertionSuccess();
}

TEST_F(...) {
  ...
  ASSERT_TRUE(LoadExtensionOverridingProxy("1", kId1, Manifest::UNPACKED));
  ASSERT_TRUE(LoadExtensionOverridingProxy("2", kId2, Manifest::UNPACKED));
  ASSERT_TRUE(LoadExtensionOverridingProxy("3", kId3, Manifest::UNPACKED));
  ...
}

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:777:
extensions::ExtensionPrefs* prefs =
No extensions:: prefix.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:779:
// Load two extensions overriding proxy and one overriding something
It looks like all three are overriding proxy - am I missing something?

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:782:
LoadExtensionOverridingProxy("1", kId1, Manifest::UNPACKED);
nit: Why pass in location if it's always unpacked?

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:790:
EXPECT_TRUE(controller->ShouldShow(kId2));
To be safe, maybe an EXPECT_FALSE(controller->ShouldShow(<non-overriding
extension id>))

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:794:
EXPECT_TRUE(base::ASCIIToUTF16("Extension 2") ==
Why not
EXPECT_EQ(base::ASCIIToUTF16("Extension 2"), override_extensions[0]);
?

It seems like converting to a base::string16, then comparing against the c_str()
is very odd.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:804:
EXPECT_TRUE(controller->ShouldShow(kId2));
This was already checked on line 790, right?

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:810:
EXPECT_TRUE(service_->GetExtensionById(kId1, false) != NULL);
Please use ExtensionRegistry::enabled_extensions().GetByID() (we're making an
effort to deprecate ExtensionService::GetExtensionById()).  Also, I think
Registry makes it more clear that we're only checking enabled extensions.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:853:
EXPECT_TRUE(service_->GetExtensionById(kId2, false) == NULL);
ExtensionRegistry will help here, too, because it'd be good to check that it
_only_ got disabled (not uninstalled).

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/proxy_overridden_bubble_controller.cc (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:1: // Copyright
(c) 2014 The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:18: using
extensions::ProxyOverriddenBubbleController;
instead, why not just move the extensions namespace up here (and get rid of all
extensions:: prefixes)?

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:81:
ExtensionRegistry* registry = ExtensionRegistry::Get(profile_);
Only used once - why not inline?  ExtensionRegistry::Get(profile_)->...

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:83:
registry->GetExtensionById(extension_id, ExtensionRegistry::ENABLED);
nit: slightly more efficient:
registry->enabled_extensions().GetByID(extension_id);

Saves Registry from having to compare the mask.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:87:
extensions::ExtensionPrefs* prefs = extensions::ExtensionPrefs::Get(profile_);
Only used once - inline.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:100:
prefs->SetProxyOverriddenBubbleBeenAcknowledged(extension_id, true);
inline.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/proxy_overridden_bubble_controller.h (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.h:1: // Copyright
(c) 2014 The Chromium Authors. All rights reserved.
nit: no (c) anymore.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.h:8: #include
<string>
nit: I think there's supposed to be a newline between the system includes and
the chrome includes.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.h:14: : public
extensions::ExtensionMessageBubbleController {
nit: no extensions:: prefix.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/settings_api_bubble_controller.cc (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/settings_api_bubble_controller.cc:24: using
extensions::ExtensionToolbarModel;
Why not move extensions namespace up?

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/settings_api_helpers.cc (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/settings_api_helpers.cc:110: return
extensions::ExtensionRegistry::Get(browser_context)->GetExtensionById(
nit: no extensions:: prefix.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/settings_api_helpers.h (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/settings_api_helpers.h:47: const Extension*
OverridesProxy(
I think "OverridesProxy()" is a little nondescript (especially since all of
these are top-level in the extensions namespace).  Maybe something like
GetExtensionOverridingProxy()?

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/settings_api_helpers.h:48: content::BrowserContext*
browser_context);
nit: fits on one line.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_message_bubble_view.cc
(right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:369: return
false;
Hopefully no compilers complain that lines 371-386 are unreachable on
non-windows platforms.  You could also get rid of this, since we say Windows
only on line 288.

https://codereview.chromium.org/288923004/diff/110001/extensions/browser/exte...
File extensions/browser/extension_prefs.h (right):

https://codereview.chromium.org/288923004/diff/110001/extensions/browser/exte...
extensions/browser/extension_prefs.h:336: bool
HasProxyOverriddenBubbleBeenAcknowledged(
There was an effort awhile back to try and make ExtensionPrefs an abstract
storage area, rather than know about every individual thing.  Seeing all these
Has*BeenAcknowledged() and Set*Acknowledged() reminds me of why.  I think, at
some point, it'd be nicer to see the callers just use ReadPrefAsBoolean() and
UpdateExtensionPref(), since they're really not any longer and it reduces
clutter in ExtensionPrefs.  (There's also the concern of knowledge layering
violations - even though there are no illegal includes, I don't think top-level
/extensions should know about things like ProxyOverride bubbles, which are
inherently chrome-y.)

That said, that would be a larger change (since there are all the others, too),
so it shouldn't block this CL.  But, for the future. :)

[Finnur, 2014-05-21 16:30:32]

PTAL. 

Also adding Sky for OWNERS check on settings_api_bubble_helper_views.cc, which
I'll look at moving into extensions in a followup.

https://codereview.chromium.org/288923004/diff/110001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5161: Your Internet connection is being
controlled
If you search the grd file, you'll see we're not totally consistent on the
capitalization, not even in the same sentence.

You'll find a fair bit more cases where it is capitalized, though.

https://codereview.chromium.org/288923004/diff/110001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5186: <message
name="IDS_EXTENSIONS_PROXY_CONTROLLED_FIRST_LINE" desc="Text displayed in the
Settings API bubble as first line when an extension has changed the proxy.">
Copy paste error. Good catch.

https://codereview.chromium.org/288923004/diff/110001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5187: An extension has taken control of your
proxy settings, which means it can change, break or eavesdrop on anything you do
online. If you aren't sure why this change happened, you probably don't want it.
I agree. Got this text from the PM and I don't think this was intentional.
Fixed.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
(right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:21:
#include "extensions/common/extension_builder.h"
On 2014/05/20 17:26:31, D Cronin wrote:
> nit: also include value_builder.h

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:173:
ProxyOverriddenBubbleController::OnBubbleAction();
The only way I could think of was to move these counts into the parent class and
add accessors. But unfortunately, that would add code to Chrome that is only
useful for testing, so it seemed a bit wasteful...

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:320:
extensions::ExtensionBuilder builder;
Indeed.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:330:
service_->AddExtension(builder.Build().get());
On 2014/05/20 17:26:31, D Cronin wrote:
> We'll get a ton of fun errors if these extensions just fail to install.  How
> about:
> 
> testing::AssertionResult LoadExtensionOverridingProxy() {
>   ...
>   if (!ExtensionRegistry::Get(profile())->enabled_extensions().GetByID(id))
>     return testing::AssertionFailure() << "Could not install extension: " <<
id;
>   return testing::AssertionSuccess();
> }
> 
> TEST_F(...) {
>   ...
>   ASSERT_TRUE(LoadExtensionOverridingProxy("1", kId1, Manifest::UNPACKED));
>   ASSERT_TRUE(LoadExtensionOverridingProxy("2", kId2, Manifest::UNPACKED));
>   ASSERT_TRUE(LoadExtensionOverridingProxy("3", kId3, Manifest::UNPACKED));
>   ...
> }

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:777:
extensions::ExtensionPrefs* prefs =
On 2014/05/20 17:26:31, D Cronin wrote:
> No extensions:: prefix.

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:779:
// Load two extensions overriding proxy and one overriding something
Yes, the last one is overriding the StartPage.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:782:
LoadExtensionOverridingProxy("1", kId1, Manifest::UNPACKED);
Readability (location not buried inside the function itself) and consistency
with other LoadExtension* functions (for some the location matters a great
deal).

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:790:
EXPECT_TRUE(controller->ShouldShow(kId2));
On 2014/05/20 17:26:31, D Cronin wrote:
> To be safe, maybe an EXPECT_FALSE(controller->ShouldShow(<non-overriding
> extension id>))

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:794:
EXPECT_TRUE(base::ASCIIToUTF16("Extension 2") ==
I got failures on other platforms when I tried that initially, but I just tested
on Linux and it seems to work. Let me change this instance and if it works I'll
fix the others.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:804:
EXPECT_TRUE(controller->ShouldShow(kId2));
On 2014/05/20 17:26:31, D Cronin wrote:
> This was already checked on line 790, right?

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:810:
EXPECT_TRUE(service_->GetExtensionById(kId1, false) != NULL);
On 2014/05/20 17:26:31, D Cronin wrote:
> Please use ExtensionRegistry::enabled_extensions().GetByID() (we're making an
> effort to deprecate ExtensionService::GetExtensionById()).  Also, I think
> Registry makes it more clear that we're only checking enabled extensions.

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:853:
EXPECT_TRUE(service_->GetExtensionById(kId2, false) == NULL);
On 2014/05/20 17:26:31, D Cronin wrote:
> ExtensionRegistry will help here, too, because it'd be good to check that it
> _only_ got disabled (not uninstalled).

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/proxy_overridden_bubble_controller.cc (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:1: // Copyright
(c) 2014 The Chromium Authors. All rights reserved.
On 2014/05/20 17:26:31, D Cronin wrote:
> no (c)

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:18: using
extensions::ProxyOverriddenBubbleController;
See? You're not as dumb as I look! :)

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:81:
ExtensionRegistry* registry = ExtensionRegistry::Get(profile_);
On 2014/05/20 17:26:31, D Cronin wrote:
> Only used once - why not inline?  ExtensionRegistry::Get(profile_)->...

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:83:
registry->GetExtensionById(extension_id, ExtensionRegistry::ENABLED);
On 2014/05/20 17:26:31, D Cronin wrote:
> nit: slightly more efficient:
> registry->enabled_extensions().GetByID(extension_id);
> 
> Saves Registry from having to compare the mask.

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:87:
extensions::ExtensionPrefs* prefs = extensions::ExtensionPrefs::Get(profile_);
On 2014/05/20 17:26:31, D Cronin wrote:
> Only used once - inline.

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:100:
prefs->SetProxyOverriddenBubbleBeenAcknowledged(extension_id, true);
I find my way generally helps readability and avoids awkward line splits. The
compiler should optimize this out anyway, so the end result is the same. But
whatever... (shrug)

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/proxy_overridden_bubble_controller.h (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.h:1: // Copyright
(c) 2014 The Chromium Authors. All rights reserved.
On 2014/05/20 17:26:31, D Cronin wrote:
> nit: no (c) anymore.

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.h:8: #include
<string>
On 2014/05/20 17:26:31, D Cronin wrote:
> nit: I think there's supposed to be a newline between the system includes and
> the chrome includes.

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.h:14: : public
extensions::ExtensionMessageBubbleController {
On 2014/05/20 17:26:31, D Cronin wrote:
> nit: no extensions:: prefix.

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/settings_api_bubble_controller.cc (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/settings_api_bubble_controller.cc:24: using
extensions::ExtensionToolbarModel;
On 2014/05/20 17:26:31, D Cronin wrote:
> Why not move extensions namespace up?

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/settings_api_helpers.cc (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/settings_api_helpers.cc:110: return
extensions::ExtensionRegistry::Get(browser_context)->GetExtensionById(
On 2014/05/20 17:26:31, D Cronin wrote:
> nit: no extensions:: prefix.

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/settings_api_helpers.h (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/settings_api_helpers.h:47: const Extension*
OverridesProxy(
On 2014/05/20 17:26:31, D Cronin wrote:
> I think "OverridesProxy()" is a little nondescript (especially since all of
> these are top-level in the extensions namespace).  Maybe something like
> GetExtensionOverridingProxy()?

Done.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/settings_api_helpers.h:48: content::BrowserContext*
browser_context);
Not anymore.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:33: <h3 id="proxy-section"
i18n-content="sectionTitleProxy" hidden></h3>
Indeed.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1507: * Set the enabled
state for the proxy settings button.
So... there's a proxy button way down on the page (under the Network section),
which toggles enabled/disabled based on whether the user can configure the proxy
and also an associated message next to the button that is shown when the button
is disabled specifically because an extension is controlling the setting (as
opposed to policy).

This particular function is misnamed because there was no specific proxy section
(until now). So, I've changed the name to setupProxySettingsButton_ to alleviate
the ambiguity a bit.

If you are asking yourself why didn't we attach the message to that button
instead of adding a special section for it, the answer is: it is so far down,
below the fold, that people were concerned it was buried and wanted more
visibility that comes with adding a section on demand.

However, I've come to the realization that it is not right to remove the
message, so I've reverted this particular change.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_message_bubble_view.cc
(right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_message_bubble_view.cc:369: return
false;
Meh... less code to compile on other platforms...

https://codereview.chromium.org/288923004/diff/110001/extensions/browser/exte...
File extensions/browser/extension_prefs.h (right):

https://codereview.chromium.org/288923004/diff/110001/extensions/browser/exte...
extensions/browser/extension_prefs.h:336: bool
HasProxyOverriddenBubbleBeenAcknowledged(
Yeah, I've been pondering something along the same lines. Maybe I'll go back and
clean this up in a separate CL.

[Devlin, 2014-05-21 17:00:38]

LGTM with nits.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
(right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:779:
// Load two extensions overriding proxy and one overriding something
On 2014/05/21 16:30:32, Finnur wrote:
> Yes, the last one is overriding the StartPage.

D'oh.  Long function names + monospace font + same number of characters = hard
to see the difference.  My bad.

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:782:
LoadExtensionOverridingProxy("1", kId1, Manifest::UNPACKED);
On 2014/05/21 16:30:32, Finnur wrote:
> Readability (location not buried inside the function itself) and consistency
> with other LoadExtension* functions (for some the location matters a great
> deal).

Fair enough. :)

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:794:
EXPECT_TRUE(base::ASCIIToUTF16("Extension 2") ==
On 2014/05/21 16:30:32, Finnur wrote:
> I got failures on other platforms when I tried that initially, but I just
tested
> on Linux and it seems to work. Let me change this instance and if it works
I'll
> fix the others.

If there are compiler errors, feel free to change it back, but please add a
comment for me one month from now. :)

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
File chrome/browser/extensions/proxy_overridden_bubble_controller.cc (right):

https://codereview.chromium.org/288923004/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:100:
prefs->SetProxyOverriddenBubbleBeenAcknowledged(extension_id, true);
On 2014/05/21 16:30:32, Finnur wrote:
> I find my way generally helps readability and avoids awkward line splits. The
> compiler should optimize this out anyway, so the end result is the same. But
> whatever... (shrug)

If it's just one object, I find inline perfectly easy to read.  If it's
something like:
ExtensionSystem::Get(web_contents()->GetBrowserContext())
    ->extension_service()
    ->pending_extension_manager()
    ->GetPendingIdsForUpdateCheck(...);
I agree whole-heartedly that the extra pointer is worth the readability. :)

https://codereview.chromium.org/288923004/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
(right):

https://codereview.chromium.org/288923004/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:304:
.Set("name", std::string("Extension " + index))
In a separate change, we could probably refactor these to be a bit cleaner since
there's a lot of shared code.  But don't worry about it for this one.

https://codereview.chromium.org/288923004/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:423:
ASSERT_TRUE(LoadExtensionWithAction("1", kId1, Manifest::COMMAND_LINE));
Thanks for also changing the other test cases. :)

https://codereview.chromium.org/288923004/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/settings_api_helpers.cc (right):

https://codereview.chromium.org/288923004/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/settings_api_helpers.cc:111: return
ExtensionRegistry::Get(browser_context)->GetExtensionById(
nit: Maybe
if (!extension_id.empty())
  return ExtensionRegistry...;
return NULL;

(Or ternary, if you prefer)
return extension_id.empty() ? NULL : ExtensionRegistry...;

For one, it saves a small amount of time on looking up the extension.  Probably
more importantly, if we ever decide to do something like
DCHECK(Extension::IdIsValid(id)) in ExtensionRegistry, this won't break it.

https://codereview.chromium.org/288923004/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/settings_api_helpers.h (right):

https://codereview.chromium.org/288923004/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/settings_api_helpers.h:48: // Returns which extension
is overriding the proxy in a given |browser_context|.
nit: Even though we should probably know it, please add a "or NULL if none
exists" to the comment.

[Devlin, 2014-05-21 17:01:53]

On 2014/05/21 16:30:32, Finnur wrote:
> PTAL. 
> 
> Also adding Sky for OWNERS check on settings_api_bubble_helper_views.cc, which
> I'll look at moving into extensions in a followup.

Also, it doesn't look like Sky is on the reviewers list yet, FYI.

[Finnur, 2014-05-21 19:57:08]

On 2014/05/21 17:01:53, Devlin wrote:
> On 2014/05/21 16:30:32, Finnur wrote:
> > PTAL. 
> > 
> > Also adding Sky for OWNERS check on settings_api_bubble_helper_views.cc,
which
> > I'll look at moving into extensions in a followup.
> 
> Also, it doesn't look like Sky is on the reviewers list yet, FYI.

Good catch. Thought you were off the hook, Scott? :)

[sky, 2014-05-21 21:03:28]

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

https://codereview.chromium.org/288923004/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/settings_api_bubble_helper_views.cc:22: namespace
extensions {
How come any of this code is in the extensions namespace?

[Finnur, 2014-05-21 21:29:56]

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

https://codereview.chromium.org/288923004/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/settings_api_bubble_helper_views.cc:22: namespace
extensions {
Umm... because all of it is related to extension bubbles notifying the user
about a particular setting that is being overwritten by an extension?

Is there another namespace you prefer?

[sky, 2014-05-21 21:41:00]

Generally we don't use namespaces in code under src/chrome. See
discussions on chromium-dev.

On Wed, May 21, 2014 at 2:29 PM,  <finnur@chromium.org> wrote:
>
>
https://codereview.chromium.org/288923004/diff/130001/chrome/browser/ui/views...
> File chrome/browser/ui/views/settings_api_bubble_helper_views.cc
> (right):
>
>
https://codereview.chromium.org/288923004/diff/130001/chrome/browser/ui/views...
> chrome/browser/ui/views/settings_api_bubble_helper_views.cc:22:
> namespace extensions {
> Umm... because all of it is related to extension bubbles notifying the
> user about a particular setting that is being overwritten by an
> extension?
>
> Is there another namespace you prefer?
>
> https://codereview.chromium.org/288923004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Devlin, 2014-05-21 21:50:41]

On 2014/05/21 21:41:00, sky wrote:
> Generally we don't use namespaces in code under src/chrome. See
> discussions on chromium-dev.
> 
> On Wed, May 21, 2014 at 2:29 PM,  <mailto:finnur@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/288923004/diff/130001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/settings_api_bubble_helper_views.cc
> > (right):
> >
> >
>
https://codereview.chromium.org/288923004/diff/130001/chrome/browser/ui/views...
> > chrome/browser/ui/views/settings_api_bubble_helper_views.cc:22:
> > namespace extensions {
> > Umm... because all of it is related to extension bubbles notifying the
> > user about a particular setting that is being overwritten by an
> > extension?
> >
> > Is there another namespace you prefer?
> >
> > https://codereview.chromium.org/288923004/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

For better or worse, extensions code has been in its own namespace for years
(and continues to do so).  Also, related discussions on CrDev (e.g. [1]) imply
that most namespaces are bad, and the chrome:: namespace is bad, but are
undecided as to larger namespaces for things like extensions:: and history::.
[2] reiterates that we never came to a decision (as helpful as that is).

Did I miss a thread where we decided against the larger namespaces, like
extensions::?

[1]
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/namesp...
[2]
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/namesp...

[sky, 2014-05-21 22:12:03]

I like namespaces for things like history and extensions. But it doesn't seem to
me that those namespaces apply to the UI for showing them.

I'm LGTMing this patch, we can resolve namespace separately.

[Devlin, 2014-05-21 22:20:25]

On 2014/05/21 22:12:03, sky wrote:
> I like namespaces for things like history and extensions. But it doesn't seem
to
> me that those namespaces apply to the UI for showing them.
> 
> I'm LGTMing this patch, we can resolve namespace separately.

I'd agree for settings_api_bubble_helper_views.cc. :)  Finnur mentioned he's
going to look into moving it into extensions, so that should help with the
namespace issues.

On 2014/05/21 16:30:32, Finnur wrote:
> PTAL. 
> 
> Also adding Sky for OWNERS check on settings_api_bubble_helper_views.cc, which
> I'll look at moving into extensions in a followup.

[Finnur, 2014-05-22 16:03:59]

dbeam: Ping.

[Finnur, 2014-05-23 13:01:19]

On 2014/05/22 16:03:59, Finnur wrote:
> dbeam: Ping.

Monsieur Beam?

[Dan Beam, 2014-05-23 19:14:00]

lgtm

[Finnur, 2014-06-05 00:32:09]

Devlin, mind taking a look at the delta between CLs 5 and 6? 
I set a timeout to 7 days (since install) and changed the bubble text to be
different depending on whether the bubble is pointing at the browser action icon
or not (as requested by Alex).

[Finnur, 2014-06-05 00:32:10]

Devlin, mind taking a look at the delta between CLs 5 and 6? 
I set a timeout to 7 days (since install) and changed the bubble text to be
different depending on whether the bubble is pointing at the browser action icon
or not (as requested by Alex).

[Devlin, 2014-06-05 17:16:31]

https://codereview.chromium.org/288923004/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
(right):

https://codereview.chromium.org/288923004/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:831:
SetInstallTime(kId1, base::Time::Now(), prefs);
Perhaps extensions 1 and 3 should have an old enough install time (otherwise
they are guaranteed not to show by being too new, instead of not being the proxy
overriding extension).

https://codereview.chromium.org/288923004/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/proxy_overridden_bubble_controller.cc (right):

https://codereview.chromium.org/288923004/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:100: return
true;
It seems a bit weird to me that we never check here that the extension is
actually overriding the proxy.  Can we either check it and return false if not,
or DCHECK if we're certain we're only passed in that extension?

https://codereview.chromium.org/288923004/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/proxy_overridden_bubble_controller.h (right):

https://codereview.chromium.org/288923004/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.h:9: 
nit: #include "base/macros.h" (for DISALLOW_COPY_AND_ASSIGN).

https://codereview.chromium.org/288923004/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.h:11: 
nit: forward declare class Profile.

[Finnur, 2014-06-05 19:37:01]

Good points. All addressed.

[Finnur, 2014-06-05 19:37:28]

And now with comments from draft.

https://codereview.chromium.org/288923004/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
(right):

https://codereview.chromium.org/288923004/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:831:
SetInstallTime(kId1, base::Time::Now(), prefs);
Good point!

https://codereview.chromium.org/288923004/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/proxy_overridden_bubble_controller.cc (right):

https://codereview.chromium.org/288923004/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/proxy_overridden_bubble_controller.cc:100: return
true;
It works, but is a bit weird. I agree. Added the check.

[Devlin, 2014-06-05 19:42:55]

LGTM.

https://codereview.chromium.org/288923004/diff/190001/chrome/browser/extensio...
File chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
(right):

https://codereview.chromium.org/288923004/diff/190001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:372:
id, base::Time::Now(), true, false);
nit: please document anonymous bools.

https://codereview.chromium.org/288923004/diff/190001/chrome/browser/extensio...
chrome/browser/extensions/extension_message_bubble_controller_unittest.cc:377:
base::Value::CreateStringValue(id));
nit: indentation mishap.

[Finnur, 2014-06-05 20:15:12]

Committed patchset #9 manually as r275229 (presubmit successful).