Beginnings of the script bubble.

Beginnings of the script bubble.

BUG=153137, 135503
TBR=arv@chromium.org

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

[Aaron Boodman, 2012-09-30 04:08:14]

yoz: c/b/e/component_loader*
kalman: everything else

Note: The changes to ComponentLoader are to make it so that you don't have to
hardcode a public key in the manifest, and an ID in the code. It will now just
generate an ID on the fly. I'll remove unnecessary hardcoded stuff from other
component extensions in a subsequent CL.

[Aaron Boodman, 2012-10-01 07:49:32]

redirecting kalman bit to jyasskin

[Jeffrey Yasskin, 2012-10-02 00:26:31]

http://codereview.chromium.org/11014009/diff/9002/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.cc (right):

http://codereview.chromium.org/11014009/diff/9002/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:153: if
(extension_service_->is_ready()) {
Comment why you need to handle the !is_ready() case with a separate code path.

http://codereview.chromium.org/11014009/diff/9002/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.h (right):

http://codereview.chromium.org/11014009/diff/9002/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.h:44: std::string Add(const
std::string& manifest_contents,
Comment the meaning of the returned string, please.

http://codereview.chromium.org/11014009/diff/9002/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.h:90: void AddScriptBubble();
Comment what this function does, and possibly how it differs from
GetScriptBubble in a way that makes both necessary.

http://codereview.chromium.org/11014009/diff/9002/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.h:105: std::string id;
optional: I'd prefer "extension_id" here since it's not the
ComponentExtensionInfo's id.

http://codereview.chromium.org/11014009/diff/9002/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader_unittest.cc (right):

http://codereview.chromium.org/11014009/diff/9002/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader_unittest.cc:175:
ASSERT_FALSE(extension_id.empty());
I wonder if ASSERT_NE("", extension_id) would give more useful information on
failure. (Or EXPECT)

http://codereview.chromium.org/11014009/diff/9002/chrome/browser/extensions/s...
File chrome/browser/extensions/script_bubble_controller.cc (right):

http://codereview.chromium.org/11014009/diff/9002/chrome/browser/extensions/s...
chrome/browser/extensions/script_bubble_controller.cc:42: int tab_id =
ExtensionTabUtil::GetTabId(web_contents);
This should be SessionId::IdForTab(web_contents);

http://codereview.chromium.org/11014009/diff/9002/chrome/browser/extensions/s...
chrome/browser/extensions/script_bubble_controller.cc:45:
page_action->SetAppearance(tab_id, ExtensionAction::ACTIVE);
I don't think this automatically tells the location bar to update. You need to
call TabHelper::location_bar_controller()->NotifyChange.

http://codereview.chromium.org/11014009/diff/9002/chrome/browser/extensions/s...
File chrome/browser/extensions/script_bubble_controller.h (right):

http://codereview.chromium.org/11014009/diff/9002/chrome/browser/extensions/s...
chrome/browser/extensions/script_bubble_controller.h:17: explicit
ScriptBubbleController(TabHelper* tab_helper);
Are there lifetime requirements on tab_helper vs *this? If so, document them
here.

http://codereview.chromium.org/11014009/diff/9002/chrome/common/extensions/fe...
File chrome/common/extensions/feature_switch.h (right):

http://codereview.chromium.org/11014009/diff/9002/chrome/common/extensions/fe...
chrome/common/extensions/feature_switch.h:21: class ScopedOverride {
You should DISALLOW_COPY_AND_ASSIGN on this or it's pretty easy to accidentally
set the FeatureSwitch back to its default. I'd probably make the constructor
public and remove FeatureSwitch::Override, especially since
"the_feature->Override(true);" has no effect but would compile fine in your
version.

http://codereview.chromium.org/11014009/diff/9002/chrome/common/extensions/fe...
File chrome/common/extensions/feature_switch_unittest.cc (right):

http://codereview.chromium.org/11014009/diff/9002/chrome/common/extensions/fe...
chrome/common/extensions/feature_switch_unittest.cc:36:
TEST_F(FeatureSwitchDisabledTest, DefaultDisabled1) {
Please give tests names associated with what they're testing, not just numbers.

[Yoyo Zhou, 2012-10-02 01:21:01]

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/component_loader.cc (right):

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/component_loader.cc:59: return
Extension::GenerateIdForPath(path);
Is this a good idea? The path to a component extension is likely to be different
on different platforms, at least, and probably also different Chrome install
locations. I think it's desirable to have a consistent extension ID.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/component_loader.h (right):

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/component_loader.h:43: // deferred until LoadAll is
called.
Should have a note about what it's returning.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/component_loader.h:90: void AddScriptBubble();
This should be private.

[Aaron Boodman, 2012-10-02 01:28:52]

Here are the responses to jyasskin's comments.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/component_loader.cc (right):

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/component_loader.cc:153: if
(extension_service_->is_ready()) {
On 2012/10/02 00:26:31, Jeffrey Yasskin wrote:
> Comment why you need to handle the !is_ready() case with a separate code path.

Don't need to. Just simplified instead.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/component_loader.h (right):

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/component_loader.h:44: std::string Add(const
std::string& manifest_contents,
On 2012/10/02 00:26:31, Jeffrey Yasskin wrote:
> Comment the meaning of the returned string, please.

Done.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/component_loader.h:44: std::string Add(const
std::string& manifest_contents,
On 2012/10/02 00:26:31, Jeffrey Yasskin wrote:
> Comment the meaning of the returned string, please.

Done.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/component_loader.h:90: void AddScriptBubble();
On 2012/10/02 00:26:31, Jeffrey Yasskin wrote:
> Comment what this function does, and possibly how it differs from
> GetScriptBubble in a way that makes both necessary.

Done.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/component_loader.h:105: std::string id;
On 2012/10/02 00:26:31, Jeffrey Yasskin wrote:
> optional: I'd prefer "extension_id" here since it's not the
> ComponentExtensionInfo's id.

Done.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/component_loader_unittest.cc (right):

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/component_loader_unittest.cc:175:
ASSERT_FALSE(extension_id.empty());
On 2012/10/02 00:26:31, Jeffrey Yasskin wrote:
> I wonder if ASSERT_NE("", extension_id) would give more useful information on
> failure. (Or EXPECT)

Done, throughout.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/script_bubble_controller.cc (right):

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/script_bubble_controller.cc:42: int tab_id =
ExtensionTabUtil::GetTabId(web_contents);
On 2012/10/02 00:26:31, Jeffrey Yasskin wrote:
> This should be SessionId::IdForTab(web_contents);

Why? I prefer to use ExtensionTabUtil because semantically, I want the extension
system's notion of the tab id, not the session system's.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/script_bubble_controller.cc:45:
page_action->SetAppearance(tab_id, ExtensionAction::ACTIVE);
On 2012/10/02 00:26:31, Jeffrey Yasskin wrote:
> I don't think this automatically tells the location bar to update. You need to
> call TabHelper::location_bar_controller()->NotifyChange.

Done.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/script_bubble_controller.h (right):

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/script_bubble_controller.h:17: explicit
ScriptBubbleController(TabHelper* tab_helper);
On 2012/10/02 00:26:31, Jeffrey Yasskin wrote:
> Are there lifetime requirements on tab_helper vs *this? If so, document them
> here.

Done.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/common/exten...
File chrome/common/extensions/feature_switch.h (right):

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/common/exten...
chrome/common/extensions/feature_switch.h:21: class ScopedOverride {
On 2012/10/02 00:26:31, Jeffrey Yasskin wrote:
> You should DISALLOW_COPY_AND_ASSIGN on this or it's pretty easy to
accidentally
> set the FeatureSwitch back to its default. I'd probably make the constructor
> public and remove FeatureSwitch::Override, especially since
> "the_feature->Override(true);" has no effect but would compile fine in your
> version.

good point, done.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/common/exten...
File chrome/common/extensions/feature_switch_unittest.cc (right):

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/common/exten...
chrome/common/extensions/feature_switch_unittest.cc:36:
TEST_F(FeatureSwitchDisabledTest, DefaultDisabled1) {
On 2012/10/02 00:26:31, Jeffrey Yasskin wrote:
> Please give tests names associated with what they're testing, not just
numbers.

Done.

[Aaron Boodman, 2012-10-02 01:42:22]

Here are the responses to Yoyo's comments.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/component_loader.cc (right):

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/component_loader.cc:59: return
Extension::GenerateIdForPath(path);
On 2012/10/02 01:21:01, Yoyo Zhou wrote:
> Is this a good idea? The path to a component extension is likely to be
different
> on different platforms, at least, and probably also different Chrome install
> locations. I think it's desirable to have a consistent extension ID.

Good point, I reverted this bit.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/component_loader.h (right):

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/component_loader.h:43: // deferred until LoadAll is
called.
On 2012/10/02 01:21:01, Yoyo Zhou wrote:
> Should have a note about what it's returning.

Done with last patchset.

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/component_loader.h:90: void AddScriptBubble();
On 2012/10/02 01:21:01, Yoyo Zhou wrote:
> This should be private.

It's called by the unit tests. This is necessary because I don't want to
accidentally start testing other component extension behavior over time.

[Jeffrey Yasskin, 2012-10-02 01:45:51]

LGTM

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/script_bubble_controller.cc (right):

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/script_bubble_controller.cc:42: int tab_id =
ExtensionTabUtil::GetTabId(web_contents);
On 2012/10/02 01:28:52, Aaron Boodman wrote:
> On 2012/10/02 00:26:31, Jeffrey Yasskin wrote:
> > This should be SessionId::IdForTab(web_contents);
> 
> Why? I prefer to use ExtensionTabUtil because semantically, I want the
extension
> system's notion of the tab id, not the session system's.

They're the same id: ExtensionTabUtil::GetTabId just forwards to
SessionId::IdForTab. You also used SessionId in your test. We're calling a mix
of both throughout the extensions system, but we should probably remove
ExtensionTabId in order to have as few APIs to get a numeric ID for a tab as
possible.

https://chromiumcodereview.appspot.com/11014009/diff/16005/chrome/common/exte...
File chrome/common/extensions/feature_switch.h (right):

https://chromiumcodereview.appspot.com/11014009/diff/16005/chrome/common/exte...
chrome/common/extensions/feature_switch.h:26: friend FeatureSwitch;
And now I think you can get rid of this friend statement.

https://chromiumcodereview.appspot.com/11014009/diff/16005/chrome/common/exte...
File chrome/common/extensions/feature_switch_unittest.cc (right):

https://chromiumcodereview.appspot.com/11014009/diff/16005/chrome/common/exte...
chrome/common/extensions/feature_switch_unittest.cc:36:
TEST_F(FeatureSwitchDisabledTest, DefaultDisabled_NoSwitchValue) {
Note that "Disabled" already appears in the fixture name, so you don't
absolutely need to duplicate it in the test name.

[Yoyo Zhou, 2012-10-02 02:51:40]

LGTM

[Aaron Boodman, 2012-10-02 07:46:40]

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/script_bubble_controller.cc (right):

https://chromiumcodereview.appspot.com/11014009/diff/9002/chrome/browser/exte...
chrome/browser/extensions/script_bubble_controller.cc:42: int tab_id =
ExtensionTabUtil::GetTabId(web_contents);
On 2012/10/02 01:45:51, Jeffrey Yasskin wrote:
> On 2012/10/02 01:28:52, Aaron Boodman wrote:
> > On 2012/10/02 00:26:31, Jeffrey Yasskin wrote:
> > > This should be SessionId::IdForTab(web_contents);
> > 
> > Why? I prefer to use ExtensionTabUtil because semantically, I want the
> extension
> > system's notion of the tab id, not the session system's.
> 
> They're the same id: ExtensionTabUtil::GetTabId just forwards to
> SessionId::IdForTab. You also used SessionId in your test. We're calling a mix
> of both throughout the extensions system, but we should probably remove
> ExtensionTabId in order to have as few APIs to get a numeric ID for a tab as
> possible.

The SessionID in the unit test was a paste-o from some other test. I've replaced
it with ExtensionTabUtil. I realize they are currently the same ID, but having a
distinction in the code theoretically makes it easier for them to not be the
same ID in the future. And it also makes the code more self-documenting. Anyway,
I think ExtensionTabUtil::GetTabId is much more common in the extension system
today, so there's consistency.

https://chromiumcodereview.appspot.com/11014009/diff/16005/chrome/common/exte...
File chrome/common/extensions/feature_switch_unittest.cc (right):

https://chromiumcodereview.appspot.com/11014009/diff/16005/chrome/common/exte...
chrome/common/extensions/feature_switch_unittest.cc:36:
TEST_F(FeatureSwitchDisabledTest, DefaultDisabled_NoSwitchValue) {
On 2012/10/02 01:45:51, Jeffrey Yasskin wrote:
> Note that "Disabled" already appears in the fixture name, so you don't
> absolutely need to duplicate it in the test name.

Done.