Warn user in case extension delays network traffic too much.

Warn user in case extension delays network traffic too much.

This CL adds a badge to the wrench menu and warning messages to chrome://extensions in case an extension delays network traffic too much and thereby causes a bad user experience.

BUG=82618
TEST=Install an extension using the webRequest API in a debug build. In debug build the extension should delay the network traffic enough to cause warning messages. You can use https://adblockplus.org/development-builds/experimental-adblock-plus-for-chrome-builds-available-with-better-blocking for example.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=104929

[battre, 2011-10-06 16:16:29]

Please review this CL. Its intention is to warn the user if one or more of his
extensions use the webRequest API and decrease the performance too much because
of bad practices.

chrome-ui-leads@google.com for UX, UI and string changes
(chrome/app/generated_resources.grd).
I have uploaded screenshots of the UX here: http://www.dropmocks.com/maxNG
- ux1.png: user has loaded a website and an extension was slow enough to trigger
a warning. The user is shown a badge on the wrench menu.
- ux2.png: user clicks on wrench menu and sees that extensions caused problems
[note that this menu is only shown after a new window is opened, see
http://crbug.com/id=96608]
- ux3.png: user has clicked on menu item and is redirected to
chrome://settings/extensions which shows a warning. This warning might occur on
several extensions that used the webRequest API and slowed down chrome together.
- ux4.png: user disabled or removes the extension --> the warning vanishes
- ux5.png: after re-enabling the extension (or restarting chrome) the warning is
gone

sail for use of GlobalError (ExtensionGlobalError)

mpcomplete for extensions related stuff

finnur for chrome/browser/{ui/webui/options, resources/options}
arv for owners approval of chrome/browser/{ui/webui/options, resources/options}

Please note that this is a first version intended for M16. We want to have a
feature complete webRequest API that is experimental (i.e. behind a flag) for
M16. For M17 we can iterate on the UX.

Thanks,
Dominic

[sail, 2011-10-06 16:48:43]

LGTM!
ExtensionGlobalError looks good.

[glen, 2011-10-06 17:04:30]

Inlining for Brian's sake, and I defer to his final judgement.

I assume that if the user clicks on the menu item, we remove the
badge? We don't want to annoy users in the case where they don't care
about the performance degradation (as that will desensitize them to
our warning messages, which we still need for other purposes).

> <message name="IDS_EXTENSION_WARNINGS_WRENCH_MENU_ITEM"
> desc="The wrench menu item indicating that extensions caused problems">
>  Extensions caused problems
> </message>

"Misbehaving extension"

(It's OK to use the same string if multiple extensions are misbehaving)

> <message name="IDS_EXTENSION_WARNINGS_TITLE_SINGULAR" desc="The title of a>
section in chrome://extensions which contains the (single) warning that relates>
to one particular extension">>  Warning:> </message>
> <message name="IDS_EXTENSION_WARNINGS_TITLE_PLURAL" desc="The title of a s
> ection in chrome://extensions which contains all warnings that relate to one
par> ticular extension">>  Warnings:> </message>

It's not necessary to use a plural here - 'Warning' with the bullet
points you have is enough.

> <message name="IDS_EXTENSION_WARNINGS_MODERATE_NETWORK_DELAY" desc="Warnin> g
message indicating that an extension caused moderate network delays for web re>
quests">> This extension caused or contributed to moderate network delays that
slowed
> down <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>.> </message>>
<message name="IDS_EXTENSION_WARNINGS_EXCESSIVE_NETWORK_DELAY" desc="Warni
> ng message indicating that an extension caused excessive network delays for
web > requests">> This extension caused or contributed to excessive network
delays that sl> owed down <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>.
It is recommend> ed to uninstall this extension.

We don't need two messages here - we shouldn't expose users to the
idea that there are multiple levels of badness (they shouldn't have to
wonder about the difference between our definitions of moderate and
excessive).

"This extension is slowing down Chrome. You should disable it to
restore Chrome's performance."




On Thu, Oct 6, 2011 at 9:16 AM,  <battre@chromium.org> wrote:
> Reviewers: chrome-ui-leads_google.com, sail, Matt Perry, Finnur, arv,
>
> Message:
> Please review this CL. Its intention is to warn the user if one or more of
> his
> extensions use the webRequest API and decrease the performance too much
> because
> of bad practices.
>
> chrome-ui-leads@google.com for UX, UI and string changes
> (chrome/app/generated_resources.grd).
> I have uploaded screenshots of the UX here: http://www.dropmocks.com/maxNG
> - ux1.png: user has loaded a website and an extension was slow enough to
> trigger
> a warning. The user is shown a badge on the wrench menu.
> - ux2.png: user clicks on wrench menu and sees that extensions caused
> problems
> [note that this menu is only shown after a new window is opened, see
> http://crbug.com/id=96608]
> - ux3.png: user has clicked on menu item and is redirected to
> chrome://settings/extensions which shows a warning. This warning might occur
> on
> several extensions that used the webRequest API and slowed down chrome
> together.
> - ux4.png: user disabled or removes the extension --> the warning vanishes
> - ux5.png: after re-enabling the extension (or restarting chrome) the
> warning is
> gone
>
> sail for use of GlobalError (ExtensionGlobalError)
>
> mpcomplete for extensions related stuff
>
> finnur for chrome/browser/{ui/webui/options, resources/options}
> arv for owners approval of chrome/browser/{ui/webui/options,
> resources/options}
>
> Please note that this is a first version intended for M16. We want to have a
> feature complete webRequest API that is experimental (i.e. behind a flag)
> for
> M16. For M17 we can iterate on the UX.
>
> Thanks,
> Dominic
>
> Description:
> Warn user in case extension delays network traffic too much.
>
> This CL adds a badge to the wrench menu and warning messages to
> chrome://extensions in case an extension delays network traffic too much and
> thereby causes a bad user experience.
>
> BUG=82618
> TEST=no
>
>
> Please review this at http://codereview.chromium.org/8176001/
>
> SVN Base: http://git.chromium.org/chromium/src.git@master
>
> Affected files:
>  M chrome/app/chrome_command_ids.h
>  M chrome/app/generated_resources.grd
>  A chrome/browser/extensions/extension_global_error.h
>  A chrome/browser/extensions/extension_global_error.cc
>  M chrome/browser/extensions/extension_service.h
>  M chrome/browser/extensions/extension_service.cc
>  M chrome/browser/extensions/extension_service_unittest.cc
>  M chrome/browser/extensions/extension_ui_unittest.cc
>  M chrome/browser/extensions/extension_webrequest_api.h
>  M chrome/browser/extensions/extension_webrequest_api.cc
>  M chrome/browser/extensions/extension_webrequest_time_tracker.h
>  M chrome/browser/extensions/extension_webrequest_time_tracker.cc
>  M chrome/browser/extensions/extension_webrequest_time_tracker_unittest.cc
>  M chrome/browser/resources/options/extension_list.js
>  M chrome/browser/resources/options/extension_settings.css
>  M chrome/browser/ui/webui/options/extension_settings_handler.h
>  M chrome/browser/ui/webui/options/extension_settings_handler.cc
>  M chrome/chrome_browser.gypi
>  M chrome/common/chrome_notification_types.h
>
>
>

[battre, 2011-10-06 20:06:19]

Hi.

On Thu, Oct 6, 2011 at 7:04 PM, Glen Murphy <glen@google.com> wrote:

> Inlining for Brian's sake, and I defer to his final judgement.
>
> I assume that if the user clicks on the menu item, we remove the
> badge? We don't want to annoy users in the case where they don't care
> about the performance degradation (as that will desensitize them to
> our warning messages, which we still need for other purposes).
>

Done.

The current behavior is now:

Heuristic triggers a warning
-> Badge is shown
-> User clicks on Wrench Menu and "Misbehaving extension"
-> Badge is suppressed for the currently existing type of warning for the
rest of the browsing session, chrome://extensions is shown, the error
messages remain on chrome://extensions until they are resolved

If the user resolves the problem temporarily and it appears again:
-> Badge is not shown(!), chrome://extensions still shows the warning


> > <message name="IDS_EXTENSION_WARNINGS_WRENCH_MENU_ITEM"
> > desc="The wrench menu item indicating that extensions caused problems">
> >  Extensions caused problems
> > </message>
>
> "Misbehaving extension"
>
> (It's OK to use the same string if multiple extensions are misbehaving)
>

Done.


> > <message name="IDS_EXTENSION_WARNINGS_TITLE_SINGULAR" desc="The title of
> a> section in chrome://extensions which contains the (single) warning that
> relates> to one particular extension">>  Warning:> </message>
> > <message name="IDS_EXTENSION_WARNINGS_TITLE_PLURAL" desc="The title of a
> s
> > ection in chrome://extensions which contains all warnings that relate to
> one par> ticular extension">>  Warnings:> </message>
>
> It's not necessary to use a plural here - 'Warning' with the bullet
> points you have is enough.
>

Done.


> > <message name="IDS_EXTENSION_WARNINGS_MODERATE_NETWORK_DELAY"
> desc="Warnin> g message indicating that an extension caused moderate network
> delays for web re> quests">> This extension caused or contributed to
> moderate network delays that slowed
> > down <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>.> </message>>
> <message name="IDS_EXTENSION_WARNINGS_EXCESSIVE_NETWORK_DELAY" desc="Warni
> > ng message indicating that an extension caused excessive network delays
> for web > requests">> This extension caused or contributed to excessive
> network delays that sl> owed down <ph name="PRODUCT_NAME">$1<ex>Google
> Chrome</ex></ph>. It is recommend> ed to uninstall this extension.
>
> We don't need two messages here - we shouldn't expose users to the
> idea that there are multiple levels of badness (they shouldn't have to
> wonder about the difference between our definitions of moderate and
> excessive).
>
> "This extension is slowing down Chrome. You should disable it to
> restore Chrome's performance."
>
>
Done.

I trigger this now for previously considered excessive network delays. We
still need to find a good definition for this.

Best regards,
Dominic



> On Thu, Oct 6, 2011 at 9:16 AM,  <battre@chromium.org> wrote:
> > Reviewers: chrome-ui-leads_google.com, sail, Matt Perry, Finnur, arv,
> >
> > Message:
> > Please review this CL. Its intention is to warn the user if one or more
> of
> > his
> > extensions use the webRequest API and decrease the performance too much
> > because
> > of bad practices.
> >
> > chrome-ui-leads@google.com for UX, UI and string changes
> > (chrome/app/generated_resources.grd).
> > I have uploaded screenshots of the UX here:
> http://www.dropmocks.com/maxNG
> > - ux1.png: user has loaded a website and an extension was slow enough to
> > trigger
> > a warning. The user is shown a badge on the wrench menu.
> > - ux2.png: user clicks on wrench menu and sees that extensions caused
> > problems
> > [note that this menu is only shown after a new window is opened, see
> > http://crbug.com/id=96608]
> > - ux3.png: user has clicked on menu item and is redirected to
> > chrome://settings/extensions which shows a warning. This warning might
> occur
> > on
> > several extensions that used the webRequest API and slowed down chrome
> > together.
> > - ux4.png: user disabled or removes the extension --> the warning
> vanishes
> > - ux5.png: after re-enabling the extension (or restarting chrome) the
> > warning is
> > gone
> >
> > sail for use of GlobalError (ExtensionGlobalError)
> >
> > mpcomplete for extensions related stuff
> >
> > finnur for chrome/browser/{ui/webui/options, resources/options}
> > arv for owners approval of chrome/browser/{ui/webui/options,
> > resources/options}
> >
> > Please note that this is a first version intended for M16. We want to
> have a
> > feature complete webRequest API that is experimental (i.e. behind a flag)
> > for
> > M16. For M17 we can iterate on the UX.
> >
> > Thanks,
> > Dominic
> >
> > Description:
> > Warn user in case extension delays network traffic too much.
> >
> > This CL adds a badge to the wrench menu and warning messages to
> > chrome://extensions in case an extension delays network traffic too much
> and
> > thereby causes a bad user experience.
> >
> > BUG=82618
> > TEST=no
> >
> >
> > Please review this at http://codereview.chromium.org/8176001/
> >
> > SVN Base: http://git.chromium.org/chromium/src.git@master
> >
> > Affected files:
> >  M chrome/app/chrome_command_ids.h
> >  M chrome/app/generated_resources.grd
> >  A chrome/browser/extensions/extension_global_error.h
> >  A chrome/browser/extensions/extension_global_error.cc
> >  M chrome/browser/extensions/extension_service.h
> >  M chrome/browser/extensions/extension_service.cc
> >  M chrome/browser/extensions/extension_service_unittest.cc
> >  M chrome/browser/extensions/extension_ui_unittest.cc
> >  M chrome/browser/extensions/extension_webrequest_api.h
> >  M chrome/browser/extensions/extension_webrequest_api.cc
> >  M chrome/browser/extensions/extension_webrequest_time_tracker.h
> >  M chrome/browser/extensions/extension_webrequest_time_tracker.cc
> >  M
> chrome/browser/extensions/extension_webrequest_time_tracker_unittest.cc
> >  M chrome/browser/resources/options/extension_list.js
> >  M chrome/browser/resources/options/extension_settings.css
> >  M chrome/browser/ui/webui/options/extension_settings_handler.h
> >  M chrome/browser/ui/webui/options/extension_settings_handler.cc
> >  M chrome/chrome_browser.gypi
> >  M chrome/common/chrome_notification_types.h
> >
> >
> >
>



-- 
Dominic Battré | Software Engineer | battre@google.com | +49 (0) 89 - 83 930
9152

[Matt Perry, 2011-10-06 22:55:54]

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.h:76: class ExtensionServiceWarning
{
The warning stuff isn't really related to the ExtensionService. Can you move it
to a different class, like an ExtensionWarningsSet that ExtensionService owns?

Also, the grouping of warnings by types seems confusing to me. How about just
flat list of (extension_id, warning_type) pairs?

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.h:102: const std::set<std::string>
extension_ids() const;
underscore_style methods must be inline. also, when returning a complex type,
return it by ref: const std::set&

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.h:588: // triggers a
chrome::NOTIFICATION_EXTENSION_WARING message.
typo: WARING (and below)

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webrequest_api.cc (right):

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_api.cc:517: // We do not report
these.
why not?

The goal of the 2 buckets was to catch 2 bad cases:
- excessive: an extension takes a really long time on a small number of
requests, noticeably slowing down a couple page loads.
- moderate: an extension slows down many requests a medium amount. it might not
be noticeable in any given instance, but chrome "feels" slower overall.

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webrequest_time_tracker.cc (right):

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_time_tracker.cc:105:
std::set<std::string> ExtensionWebRequestTimeTracker::GetExtensionIds(
this is probably OK for now, but not really correct. It will blame all
extensions that happened to intercept a request that any 1 extension slowed
down. See my TODO in Analyze. I think what we should do is:
- wait until moderate or excessive delays hits the threshold.
- when it does, go back through all requests, and tally up individual block
times for extensions.
- only blame either (a) the slowest extension, or (b) the extensions that have
contributed more than X delay, where X is to be determined.

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webrequest_time_tracker.h (right):

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_time_tracker.h:23: class
ExtensionWebRequestTimeTrackerDelegate {
This is good for testing, but I don't think the ExtensionWebRequestEventRouter
should be the delegate (it has nothing to do with the event routing). How about
a default delegate in this file that implements the notification?

http://codereview.chromium.org/8176001/diff/5004/chrome/common/chrome_notific...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/8176001/diff/5004/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:525: NOTIFICATION_EXTENSION_WARNING,
This should really be called NOTIFICATION_EXTENSION_WARNING_CHANGED. Also update
the comment to indicate it can be called when a warning is suppressed.

[battre, 2011-10-07 14:09:24]

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.h:76: class ExtensionServiceWarning
{
On 2011/10/06 22:55:54, Matt Perry wrote:
> The warning stuff isn't really related to the ExtensionService. Can you move
it
> to a different class, like an ExtensionWarningsSet that ExtensionService owns?
> 
> Also, the grouping of warnings by types seems confusing to me. How about just
> flat list of (extension_id, warning_type) pairs?

Done.

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.h:102: const std::set<std::string>
extension_ids() const;
On 2011/10/06 22:55:54, Matt Perry wrote:
> underscore_style methods must be inline. also, when returning a complex type,
> return it by ref: const std::set&

Done.

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.h:588: // triggers a
chrome::NOTIFICATION_EXTENSION_WARING message.
On 2011/10/06 22:55:54, Matt Perry wrote:
> typo: WARING (and below)

Done.

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webrequest_api.cc (right):

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_api.cc:517: // We do not report
these.
On 2011/10/06 22:55:54, Matt Perry wrote:
> why not?
> 
> The goal of the 2 buckets was to catch 2 bad cases:
> - excessive: an extension takes a really long time on a small number of
> requests, noticeably slowing down a couple page loads.
> - moderate: an extension slows down many requests a medium amount. it might
not
> be noticeable in any given instance, but chrome "feels" slower overall.

Done.

I had not understood this properly before. Thanks.

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webrequest_time_tracker.cc (right):

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_time_tracker.cc:105:
std::set<std::string> ExtensionWebRequestTimeTracker::GetExtensionIds(
On 2011/10/06 22:55:54, Matt Perry wrote:
> this is probably OK for now, but not really correct. It will blame all
> extensions that happened to intercept a request that any 1 extension slowed
> down. See my TODO in Analyze. I think what we should do is:
> - wait until moderate or excessive delays hits the threshold.
> - when it does, go back through all requests, and tally up individual block
> times for extensions.
> - only blame either (a) the slowest extension, or (b) the extensions that have
> contributed more than X delay, where X is to be determined.

I agree.

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webrequest_time_tracker.h (right):

http://codereview.chromium.org/8176001/diff/5004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_time_tracker.h:23: class
ExtensionWebRequestTimeTrackerDelegate {
On 2011/10/06 22:55:54, Matt Perry wrote:
> This is good for testing, but I don't think the ExtensionWebRequestEventRouter
> should be the delegate (it has nothing to do with the event routing). How
about
> a default delegate in this file that implements the notification?

Done.

http://codereview.chromium.org/8176001/diff/5004/chrome/common/chrome_notific...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/8176001/diff/5004/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:525: NOTIFICATION_EXTENSION_WARNING,
On 2011/10/06 22:55:54, Matt Perry wrote:
> This should really be called NOTIFICATION_EXTENSION_WARNING_CHANGED. Also
update
> the comment to indicate it can be called when a warning is suppressed.

Done.

I have changed the label and the comment. I did not add the comment to indicate
that NOTIFICATION_EXTENSION_WARNING_CHANGED can be triggered when a warning is
suppressed because I have renamed the warning suppression in ExtensionWarningSet
to be called "badge suppression".

[Matt Perry, 2011-10-07 19:18:38]

almost there

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_warning_set.h (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_warning_set.h:16: class ExtensionWarning {
This class should be an implementation detail of ExtensionWarningSet. The
warning type can be a public type on ExtensionWarningSet, but how it's grouped
internally should be hidden from consumers. (make it an inner private class)

Also, FYI, this entire class could be replaced by
  typedef std::pair<WarningType, std::string> ExtensionWarning;

std::pair even comes with a builtin operator< (which does the same lexicographic
comparison).

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_warning_set_unittest.cc (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_warning_set_unittest.cc:60: // Check that
ClearWarnings deletes exaclty the specified warnings and
typo: exactly

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webrequest_time_tracker.cc (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_time_tracker.cc:38: size_t
total_num_messages,
since these params are not used for now, just remove them until they're needed.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_time_tracker.cc:58: class
DefaultExtensionWebRequestTimeTrackerDelegate
nit: can this go inside the anonymous namespace? then the name could be
shortened if you like.

[Finnur, 2011-10-10 09:59:24]

I was sick on Friday, so I didn't get to this until now.

Reviewed:
chrome/browser/resources/options/extension_list.js
chrome/browser/resources/options/extension_settings.css
chrome/browser/ui/webui/options/extension_settings_handler.h
chrome/browser/ui/webui/options/extension_settings_handler.cc
... as requested (and bits and pieces elsewhere).

General feedback: "TEST=no" should instead be a note to QA on how to test this,
at least at a high-level what testing this involves.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_warning_set.cc (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_warning_set.cc:118: }
nit: no braces on single-line if-else clauses.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_list.js (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:244:
warningsDiv.classList.add('extension-warnings');
nit: You use plural here (warnings) but singular for all style names below (I
think I'd prefer singular).

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:256:
warningList.appendChild(warningEntry);
For non-network-slowdown warnings this will look weird, since it will be
'Warning:' heading only and no warning text.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.cc:214:
NotificationService::AllSources());
Shouldn't this just check for notifications from its own profile?

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.cc:642: case
ExtensionWarning::kInvalid:
nit: indentation is off.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.cc:734:
Value::CreateStringValue(GetLocalizedWarning(*iter)));
Hmm... For all warnings we add in the future (besides 'network slowdown'), we'll
pass an empty string down to the page...

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/extension_settings_handler.h (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.h:170:
ExtensionWarning::WarningType warning_type);
nit: static function should be at the top of the private section.

However, this feels like the wrong place for this function. Shouldn't it be in
the ExtensionWarning class, or there about?

[Finnur, 2011-10-10 10:02:39]

Glen...

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_list.js (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:242: if
(extension.warnings.length > 0) {
I guess this is a question for Glen...
Was the intent here for this to be always visible (whether the details section
is expanded or not)?

[Finnur, 2011-10-10 10:37:27]

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.cc:214:
NotificationService::AllSources());
See http://codereview.chromium.org/8199022/ for reference.

On 2011/10/10 09:59:24, Finnur wrote:
> Shouldn't this just check for notifications from its own profile?

[battre, 2011-10-10 13:16:36]

Thanks for the reviews.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_warning_set.cc (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_warning_set.cc:118: }
On 2011/10/10 09:59:24, Finnur wrote:
> nit: no braces on single-line if-else clauses.

Done.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_warning_set.h (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_warning_set.h:16: class ExtensionWarning {
On 2011/10/07 19:18:38, Matt Perry wrote:
> This class should be an implementation detail of ExtensionWarningSet. The
> warning type can be a public type on ExtensionWarningSet, but how it's grouped
> internally should be hidden from consumers. (make it an inner private class)
> 
> Also, FYI, this entire class could be replaced by
>   typedef std::pair<WarningType, std::string> ExtensionWarning;
> 
> std::pair even comes with a builtin operator< (which does the same
lexicographic
> comparison).

Done.

I have moved the definition of ExtensionWarning into extension_warning_set.cc

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_warning_set_unittest.cc (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_warning_set_unittest.cc:60: // Check that
ClearWarnings deletes exaclty the specified warnings and
On 2011/10/07 19:18:38, Matt Perry wrote:
> typo: exactly

Done.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webrequest_time_tracker.cc (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_time_tracker.cc:38: size_t
total_num_messages,
On 2011/10/07 19:18:38, Matt Perry wrote:
> since these params are not used for now, just remove them until they're
needed.

Done.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webrequest_time_tracker.cc:58: class
DefaultExtensionWebRequestTimeTrackerDelegate
On 2011/10/07 19:18:38, Matt Perry wrote:
> nit: can this go inside the anonymous namespace? then the name could be
> shortened if you like.

Done.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_list.js (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:242: if
(extension.warnings.length > 0) {
On 2011/10/10 10:02:39, Finnur wrote:
> I guess this is a question for Glen...
> Was the intent here for this to be always visible (whether the details section
> is expanded or not)?

This was my intent: Always show it if and only if there is a warning. The
warning should be an exception. In case there is a warning it should be visible.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:244:
warningsDiv.classList.add('extension-warnings');
On 2011/10/10 09:59:24, Finnur wrote:
> nit: You use plural here (warnings) but singular for all style names below (I
> think I'd prefer singular).

Given the confusion from the comment below, I have chosen to make the style and
the div name plural. It is one header for all warning*s*.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:256:
warningList.appendChild(warningEntry);
On 2011/10/10 09:59:24, Finnur wrote:
> For non-network-slowdown warnings this will look weird, since it will be
> 'Warning:' heading only and no warning text.

I am not sure I understand this. This is the reason for the if-statement in line
242. If there are any warnings, we see "Warning:" followed by a bullet list of
all warnings. If there are no warnings, we don't see the "Warning:" caption.

There will be other warnings types in the future (conflicting modifications to
network requests it the next on my list). These warnings will also have textual
descriptions.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.cc:214:
NotificationService::AllSources());
On 2011/10/10 09:59:24, Finnur wrote:
> Shouldn't this just check for notifications from its own profile?

Done.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.cc:642: case
ExtensionWarning::kInvalid:
On 2011/10/10 09:59:24, Finnur wrote:
> nit: indentation is off.

Done.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.cc:649:
l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));
please note that there is no default case here. Adding new enumeration values to
the type without a new case branch here creates a compiler error.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.cc:734:
Value::CreateStringValue(GetLocalizedWarning(*iter)));
On 2011/10/10 09:59:24, Finnur wrote:
> Hmm... For all warnings we add in the future (besides 'network slowdown'),
we'll
> pass an empty string down to the page...

No, we'll trigger a compile error (on GCC and Clang, not for MSVC) that forces
authors of new warnings to add new messages.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/extension_settings_handler.h (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.h:170:
ExtensionWarning::WarningType warning_type);
On 2011/10/10 09:59:24, Finnur wrote:
> nit: static function should be at the top of the private section.
> 
> However, this feels like the wrong place for this function. Shouldn't it be in
> the ExtensionWarning class, or there about?

I have moved the function up. I did not move it into ExtensionWarning class. How
strongly do you feel about this? My opinion would be that localization should
not be in the core of an Extension class but rather part of the UI. The switch
statement in GetLocalizedWarning is written in such a way that adding a new enum
value triggers a compile failure if you don't extend GetLocalizedWarning.

[Finnur, 2011-10-10 15:06:55]

My files LGTM, w/couple of nits.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/resources/op...
File chrome/browser/resources/options/extension_list.js (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/resources/op...
chrome/browser/resources/options/extension_list.js:256:
warningList.appendChild(warningEntry);
Ignore this comment, addressed elsewhere.

On 2011/10/10 13:16:36, battre wrote:
> On 2011/10/10 09:59:24, Finnur wrote:
> > For non-network-slowdown warnings this will look weird, since it will be
> > 'Warning:' heading only and no warning text.
> 
> I am not sure I understand this. This is the reason for the if-statement in
line
> 242. If there are any warnings, we see "Warning:" followed by a bullet list of
> all warnings. If there are no warnings, we don't see the "Warning:" caption.
> 
> There will be other warnings types in the future (conflicting modifications to
> network requests it the next on my list). These warnings will also have
textual
> descriptions.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.cc:649:
l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));
Hmm... This is a little too subtle for my taste, but it comes with the added
benefit of a compile-time error. Can't argue with that.

Can you at least add a NOTREACHED() on line 651 (before the return), for the
benefit of Windows developers?

On 2011/10/10 13:16:36, battre wrote:
> please note that there is no default case here. Adding new enumeration values
to
> the type without a new case branch here creates a compiler error.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/extension_settings_handler.h (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.h:170:
ExtensionWarning::WarningType warning_type);
I don't feel strongly about it but if someone adds another UI class that also
uses the ExtensionWarning, they'd probably look in the ExtensionWarning class
for such a translate function, rather than here. If they don't find it, they
might duplicate the method and if they do find it in this class they would
probably be more likely to move it to the ExtensionWarning class (so they can
share it) rather than store it in some other common file they have access
to/need to create.

If the translate function were to have potentially different outcome based on
which UI class it was used in, then it would look a bit different.

But, maybe it is just me. 

On 2011/10/10 13:16:36, battre wrote:
> On 2011/10/10 09:59:24, Finnur wrote:
> > nit: static function should be at the top of the private section.
> > 
> > However, this feels like the wrong place for this function. Shouldn't it be
in
> > the ExtensionWarning class, or there about?
> 
> I have moved the function up. I did not move it into ExtensionWarning class.
How
> strongly do you feel about this? My opinion would be that localization should
> not be in the core of an Extension class but rather part of the UI. The switch
> statement in GetLocalizedWarning is written in such a way that adding a new
enum
> value triggers a compile failure if you don't extend GetLocalizedWarning.

[battre, 2011-10-10 15:55:57]

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.cc:649:
l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));
On 2011/10/10 15:06:56, Finnur wrote:
> Hmm... This is a little too subtle for my taste, but it comes with the added
> benefit of a compile-time error. Can't argue with that.
> 
> Can you at least add a NOTREACHED() on line 651 (before the return), for the
> benefit of Windows developers?
> 
> On 2011/10/10 13:16:36, battre wrote:
> > please note that there is no default case here. Adding new enumeration
values
> to
> > the type without a new case branch here creates a compiler error.
> 

Done.

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/extension_settings_handler.h (right):

http://codereview.chromium.org/8176001/diff/15003/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/extension_settings_handler.h:170:
ExtensionWarning::WarningType warning_type);
On 2011/10/10 15:06:56, Finnur wrote:
> I don't feel strongly about it but if someone adds another UI class that also
> uses the ExtensionWarning, they'd probably look in the ExtensionWarning class
> for such a translate function, rather than here. If they don't find it, they
> might duplicate the method and if they do find it in this class they would
> probably be more likely to move it to the ExtensionWarning class (so they can
> share it) rather than store it in some other common file they have access
> to/need to create.
> 
> If the translate function were to have potentially different outcome based on
> which UI class it was used in, then it would look a bit different.
> 
> But, maybe it is just me. 
> 
> On 2011/10/10 13:16:36, battre wrote:
> > On 2011/10/10 09:59:24, Finnur wrote:
> > > nit: static function should be at the top of the private section.
> > > 
> > > However, this feels like the wrong place for this function. Shouldn't it
be
> in
> > > the ExtensionWarning class, or there about?
> > 
> > I have moved the function up. I did not move it into ExtensionWarning class.
> How
> > strongly do you feel about this? My opinion would be that localization
should
> > not be in the core of an Extension class but rather part of the UI. The
switch
> > statement in GetLocalizedWarning is written in such a way that adding a new
> enum
> > value triggers a compile failure if you don't extend GetLocalizedWarning.
> 

Convinced. Done.

[Glen Murphy, 2011-10-10 17:39:08]

On 2011/10/10 10:02:39, Finnur wrote:
> Glen...
> 
>
http://codereview.chromium.org/8176001/diff/15003/chrome/browser/resources/op...
> File chrome/browser/resources/options/extension_list.js (right):
> 
>
http://codereview.chromium.org/8176001/diff/15003/chrome/browser/resources/op...
> chrome/browser/resources/options/extension_list.js:242: if
> (extension.warnings.length > 0) {
> I guess this is a question for Glen...
> Was the intent here for this to be always visible (whether the details section
> is expanded or not)?

Yes, the warnings should always be visible.

[Matt Perry, 2011-10-10 19:00:57]

lgtm

[commit-bot: I haz the power, 2011-10-11 15:35:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/8176001/27001

[commit-bot: I haz the power, 2011-10-11 18:53:28]

Change committed as 104929