Improve error messaging of webRequest API in case of conflicts

Improve error messaging of webRequest API in case of conflicts


BUG=126438
TBR=jochen@chromium.org,estade@chromium.org



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

[battre, 2012-05-22 20:09:50]

Hi Matt,

could you review this please?

Thanks,
Dominic

[Matt Perry, 2012-05-22 20:46:56]

You should run this by someone from the UI team. I seem to remember we went with
a generic warning because the UI team (Glen?) thought the more detailed warning
was too cryptic for end users.

[battre, 2012-05-22 21:14:28]

+glen as suggested by Matt

I would like to give better error messaging because
a) it helps extension authors (they don't know currently which kind of conflicts
exist)
b) it helps users because it gives them advise on which extensions conflict and
might need to be disabled

[Glen Murphy, 2012-05-23 22:59:52]

Can you provide screenshots of the implementation?


On Tue, May 22, 2012 at 2:14 PM,  <battre@chromium.org> wrote:
> +glen as suggested by Matt
>
> I would like to give better error messaging because
> a) it helps extension authors (they don't know currently which kind of
> conflicts
> exist)
> b) it helps users because it gives them advise on which extensions conflict
> and
> might need to be disabled
>
> https://chromiumcodereview.appspot.com/10407105/

[battre, 2012-05-24 17:52:31]

Sure. http://www/~battre/screenshot-warnings.png

There is no change in the way warnings are displayed. The only change is in the
strings. The screenshot shows one example.

Thanks,
Dominic

On 2012/05/23 22:59:52, Glen Murphy wrote:
> Can you provide screenshots of the implementation?
> 
> 
> On Tue, May 22, 2012 at 2:14 PM,  <mailto:battre@chromium.org> wrote:
> > +glen as suggested by Matt
> >
> > I would like to give better error messaging because
> > a) it helps extension authors (they don't know currently which kind of
> > conflicts
> > exist)
> > b) it helps users because it gives them advise on which extensions conflict
> > and
> > might need to be disabled
> >
> > https://chromiumcodereview.appspot.com/10407105/

[Aaron Boodman, 2012-05-24 20:17:37]

Can we put the title in the parenthesis instead of the extension ID?

On Thu, May 24, 2012 at 10:52 AM,  <battre@chromium.org> wrote:
> Sure. http://www/~battre/screenshot-warnings.png
>
> There is no change in the way warnings are displayed. The only change is in
> the
> strings. The screenshot shows one example.
>
> Thanks,
> Dominic
>
>
> On 2012/05/23 22:59:52, Glen Murphy wrote:
>>
>> Can you provide screenshots of the implementation?
>
>
>
>> On Tue, May 22, 2012 at 2:14 PM,  <mailto:battre@chromium.org> wrote:
>> > +glen as suggested by Matt
>> >
>> > I would like to give better error messaging because
>> > a) it helps extension authors (they don't know currently which kind of
>> > conflicts
>> > exist)
>> > b) it helps users because it gives them advise on which extensions
>> > conflict
>> > and
>> > might need to be disabled
>> >
>> > https://chromiumcodereview.appspot.com/10407105/
>
>
>
> https://chromiumcodereview.appspot.com/10407105/

[battre, 2012-05-24 20:44:23]

Yes, I see five options (none is pretty)

- Store the extension ID first and then replace it via a RegEx with the actual
extension name pretty late.

- Delay the warning string generation to a later point, basically substituting
const string16& ExtensionWarning::message() with
ExtensionWarning::message(ExtensionInfoMap*), slightly nasty unless I can
convince the owners of GetStringFUTF16 to introduce
UI_EXPORT string16 GetStringFUTF16(int message_id, const std::vector<string16>&
a);

- Plumb the ExtensionInfoMap into the methods of web_request_api_helpers.cc
(codewise pretty nasty)

- Copy the extension names into EventResponseDeltas (pretty expensive in my
opinion)

- Leave it as is because the warnings are only shown in Developer Mode and in
which the extension ID is also shown. I agree that this is slightly nasty if you
look for a specific extension and have 80 extensions installed (I have seen this
number in some of the crash reports).

I think I prefer the second option. WDYT?

Best regards,
Dominic

On 2012/05/24 20:17:37, Aaron Boodman wrote:
> Can we put the title in the parenthesis instead of the extension ID?
> 
> On Thu, May 24, 2012 at 10:52 AM,  <mailto:battre@chromium.org> wrote:
> > Sure. http://www/%7Ebattre/screenshot-warnings.png
> >
> > There is no change in the way warnings are displayed. The only change is in
> > the
> > strings. The screenshot shows one example.
> >
> > Thanks,
> > Dominic
> >
> >
> > On 2012/05/23 22:59:52, Glen Murphy wrote:
> >>
> >> Can you provide screenshots of the implementation?
> >
> >
> >
> >> On Tue, May 22, 2012 at 2:14 PM, &nbsp;<mailto:battre@chromium.org> wrote:
> >> > +glen as suggested by Matt
> >> >
> >> > I would like to give better error messaging because
> >> > a) it helps extension authors (they don't know currently which kind of
> >> > conflicts
> >> > exist)
> >> > b) it helps users because it gives them advise on which extensions
> >> > conflict
> >> > and
> >> > might need to be disabled
> >> >
> >> > https://chromiumcodereview.appspot.com/10407105/
> >
> >
> >
> > https://chromiumcodereview.appspot.com/10407105/

[battre, 2012-09-04 14:58:57]

I have revived this old CL. I had to change quite a lot. Please take another
look.

Here are screenshots of the new warnings:
http://www/~battre/fixed-extension-conflict-message.png
http://www/~battre/fixed-extension-conflict-message-2.png
http://www/~battre/fixed-extension-conflict-message-3.png

Thanks,
Dominic

[battre, 2012-09-04 15:05:53]

Oh, I have added Aaron as a reviewer. I would like to use this as the foundation
for partially getting rid of host permissions for the declarative web request
api. If an extension redirects in a way that is incompatible with
http://crbug.com/145456, we would show this warning.

[Aaron Boodman, 2012-09-05 08:11:11]

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.h:623: ExtensionWarningService*
extension_warnings() {
Should be a member of ExtensionSystem, not ExtensionService.

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_warning_set.h (right):

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:23: class ExtensionWarning {
Move all this into the extensions namespace and remove the 'Extension' prefix
from names.

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:76: const std::string
GetMessage(ExtensionService* extension_service) const;
Hm, odd that this takes |extension_service|.

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:107: class ExtensionWarningSet
{
Can ExtensionWarningSet and ExtensionWarningService be combined? I don't see the
distinction  between them, and I always prefer simple data objects to 'services'
that are designed to fit a certain place

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:109: ExtensionWarningSet();
It seems like a map is a better fit for this class (from Extension* or
extension_id to std::vector<Warning>)

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:140: //
chrome::NOTIFICATION_EXTENSION_WARNING message if such warnings existed.
We've been trying to reduce usage of notifications across the codebase. Can this
be phrased as an observer instead?

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:187:
std::set<linked_ptr<ExtensionWarning> > warnings_;
We have been discouraged from using linked_ptr and encouraged to explicitly
delete items in the containing class' destructor. The argument is that
linked_ptr is essentially ref-counting and can make reasoning about lifetime
harder.

[Matt Perry, 2012-09-06 00:21:37]

lgtm

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/web_request_api_helpers.cc
(right):

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api_helpers.cc:576: static
std::string FindSetRequestHeader(
move helpers to the top of the file in an anon namespace

[battre, 2012-09-06 14:25:48]

Hi Aaron,

please take another look. I have addressed several of your issues but I disagree
with a couple of them. I have created separate CLs for them and would like to
ask you to check whether you think that we should land them.

Thanks,
Dominic

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/web_request_api_helpers.cc
(right):

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api_helpers.cc:576: static
std::string FindSetRequestHeader(
On 2012/09/06 00:21:37, Matt Perry wrote:
> move helpers to the top of the file in an anon namespace

I have added a TODO for this. I would like to do that in a separate CL because
the entire file is sorted based on relevant scope (helpers directly for their
user).

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.h:623: ExtensionWarningService*
extension_warnings() {
On 2012/09/05 08:11:11, Aaron Boodman wrote:
> Should be a member of ExtensionSystem, not ExtensionService.

Done.

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_warning_set.h (right):

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:23: class ExtensionWarning {
On 2012/09/05 08:11:11, Aaron Boodman wrote:
> Move all this into the extensions namespace and remove the 'Extension' prefix
> from names.

I have moved it into an extensions namespace. As removing the Extension prefix
requires renaming the files, I would like to do that in a separate CL. I have
added a TODO.

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:76: const std::string
GetMessage(ExtensionService* extension_service) const;
On 2012/09/05 08:11:11, Aaron Boodman wrote:
> Hm, odd that this takes |extension_service|.

I need to translate extension_ids to extension names and need to do this on the
UI thread, i.e. I cannot do it at construction time.

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:107: class ExtensionWarningSet
{
On 2012/09/05 08:11:11, Aaron Boodman wrote:
> Can ExtensionWarningSet and ExtensionWarningService be combined? I don't see
the
> distinction  between them, and I always prefer simple data objects to
'services'
> that are designed to fit a certain place

The reason for this class is that I need to pass a
"std::set<linked_ptr<ExtensionWarning> >" from the IO thread to the UI thread.
As linked_ptr is not thread safe, I cannot pass the
"std::set<linked_ptr<ExtensionWarning> >" directly but need to pass a wrapper.
This wrapper ensures that the IO thread cannot retain a reference to the
ExtensionWarning (because it passes a scoped_ptr<ExtensionWarning>). Also we
ensure that the ExtensionWarningSet does not retain any reference because of the
Release() implementation.

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:109: ExtensionWarningSet();
On 2012/09/05 08:11:11, Aaron Boodman wrote:
> It seems like a map is a better fit for this class (from Extension* or
> extension_id to std::vector<Warning>)

We need set logic. For each pair of (extension_id, warning_type) we want to show
one error message. If we follow your suggestion, we would need to use a
std::map<std::string, std::map<ExtensionWarning::WarningType,
linked_ptr<WarningMessage> > >. This seems overkill to me.

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:140: //
chrome::NOTIFICATION_EXTENSION_WARNING message if such warnings existed.
On 2012/09/05 08:11:11, Aaron Boodman wrote:
> We've been trying to reduce usage of notifications across the codebase. Can
this
> be phrased as an observer instead?

I have created https://chromiumcodereview.appspot.com/10908119/ for this. I
would prefer to keep the current approach (IMO more readable, less coupled), but
that's your call.

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:187:
std::set<linked_ptr<ExtensionWarning> > warnings_;
On 2012/09/05 08:11:11, Aaron Boodman wrote:
> We have been discouraged from using linked_ptr and encouraged to explicitly
> delete items in the containing class' destructor. The argument is that
> linked_ptr is essentially ref-counting and can make reasoning about lifetime
> harder.

I have created https://chromiumcodereview.appspot.com/10909096 but I would
highly appreciate not having to land it. I find this very ugly.

I don't see the point why people want immutable, passive objects not to be
refcounted.

I believe that in this CL the ownership of the linked_ptrs are now very clear
(either in ExtensionWarningSet or in ExtensionWarningService) - or temporarily
in a std::set<linked_ptr<ExtensionWarning> > while in flight between the two
classes.

[Aaron Boodman, 2012-09-06 15:33:31]

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_warning_set.h (right):

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:23: class ExtensionWarning {
On 2012/09/06 14:25:48, battre wrote:
> On 2012/09/05 08:11:11, Aaron Boodman wrote:
> > Move all this into the extensions namespace and remove the 'Extension'
prefix
> > from names.
> 
> I have moved it into an extensions namespace. As removing the Extension prefix
> requires renaming the files, I would like to do that in a separate CL. I have
> added a TODO.

ok

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:76: const std::string
GetMessage(ExtensionService* extension_service) const;
On 2012/09/06 14:25:48, battre wrote:
> On 2012/09/05 08:11:11, Aaron Boodman wrote:
> > Hm, odd that this takes |extension_service|.
> 
> I need to translate extension_ids to extension names and need to do this on
the
> UI thread, i.e. I cannot do it at construction time.

Can you pass ExtensionSet instead?

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:107: class ExtensionWarningSet
{
On 2012/09/06 14:25:48, battre wrote:
> On 2012/09/05 08:11:11, Aaron Boodman wrote:
> > Can ExtensionWarningSet and ExtensionWarningService be combined? I don't see
> the
> > distinction  between them, and I always prefer simple data objects to
> 'services'
> > that are designed to fit a certain place
> 
> The reason for this class is that I need to pass a
> "std::set<linked_ptr<ExtensionWarning> >" from the IO thread to the UI thread.
> As linked_ptr is not thread safe, I cannot pass the
> "std::set<linked_ptr<ExtensionWarning> >" directly but need to pass a wrapper.
> This wrapper ensures that the IO thread cannot retain a reference to the
> ExtensionWarning (because it passes a scoped_ptr<ExtensionWarning>). Also we
> ensure that the ExtensionWarningSet does not retain any reference because of
the
> Release() implementation.

Did you consider making this just std::set<ExtensionWarning> and passing by
value? It's less efficient, but significantly simpler, and this class should be
infrequently used (right?). We pass strings between threads all the time by
value, so this would follow convention.

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:109: ExtensionWarningSet();
On 2012/09/06 14:25:48, battre wrote:
> On 2012/09/05 08:11:11, Aaron Boodman wrote:
> > It seems like a map is a better fit for this class (from Extension* or
> > extension_id to std::vector<Warning>)
> 
> We need set logic. For each pair of (extension_id, warning_type) we want to
show
> one error message. If we follow your suggestion, we would need to use a
> std::map<std::string, std::map<ExtensionWarning::WarningType,
> linked_ptr<WarningMessage> > >. This seems overkill to me.

I see. OK.

[battre, 2012-09-10 17:21:37]

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_warning_set.h (right):

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:76: const std::string
GetMessage(ExtensionService* extension_service) const;
On 2012/09/06 15:33:31, Aaron Boodman wrote:
> On 2012/09/06 14:25:48, battre wrote:
> > On 2012/09/05 08:11:11, Aaron Boodman wrote:
> > > Hm, odd that this takes |extension_service|.
> > 
> > I need to translate extension_ids to extension names and need to do this on
> the
> > UI thread, i.e. I cannot do it at construction time.
> 
> Can you pass ExtensionSet instead?

Done.

http://codereview.chromium.org/10407105/diff/16002/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:107: class ExtensionWarningSet
{
On 2012/09/06 15:33:31, Aaron Boodman wrote:
> On 2012/09/06 14:25:48, battre wrote:
> > On 2012/09/05 08:11:11, Aaron Boodman wrote:
> > > Can ExtensionWarningSet and ExtensionWarningService be combined? I don't
see
> > the
> > > distinction  between them, and I always prefer simple data objects to
> > 'services'
> > > that are designed to fit a certain place
> > 
> > The reason for this class is that I need to pass a
> > "std::set<linked_ptr<ExtensionWarning> >" from the IO thread to the UI
thread.
> > As linked_ptr is not thread safe, I cannot pass the
> > "std::set<linked_ptr<ExtensionWarning> >" directly but need to pass a
wrapper.
> > This wrapper ensures that the IO thread cannot retain a reference to the
> > ExtensionWarning (because it passes a scoped_ptr<ExtensionWarning>). Also we
> > ensure that the ExtensionWarningSet does not retain any reference because of
> the
> > Release() implementation.
> 
> Did you consider making this just std::set<ExtensionWarning> and passing by
> value? It's less efficient, but significantly simpler, and this class should
be
> infrequently used (right?). We pass strings between threads all the time by
> value, so this would follow convention.

Done.

[battre, 2012-09-11 21:56:41]

Aaron, I forgot to submit a useful message last time (I was in a hurry). I think
this is ready for another review. (not complaining - I was just not sure whether
my last message made that clear)

Thanks,
Dominic

https://chromiumcodereview.appspot.com/10407105/diff/18004/chrome/browser/ext...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/10407105/diff/18004/chrome/browser/ext...
chrome/browser/extensions/extension_service.cc:814:
std::set<ExtensionWarning::WarningType> warning_types;
I will move this into the if statement.

[Aaron Boodman, 2012-09-18 18:48:56]

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:818: &warning_types);
Why not put this in UNLOAD rather than duplicating it for DISABLE and UNINSTALL.

Also, can you set up WarningService to listen to notifications from
ExtensionService, rather than having the code in here?

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_warning_set.h (right):

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_EXTENSION_WARNING_SET_H_
Seems like this file should be called extension_warning_service. There's only
one line of code here that implements ExtensionWarningSet (a typedef).

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:47: // We allow copy&assign
for passing containers of ExtensionWarnings between
More generally, it's a value type. We have a lot of these in Chrome. I'm not
sure it's really worth calling out.

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:51: void operator=(const
ExtensionWarning& other);
What is this used by?

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:113: // etc.) trigger a
warning badge in the UI and and provide means to resolve
It seems like a layering violation to have this class directly poke at Chrome's
UI (the global error service).

Can ExtensionWarningService just become ExtensionWarningSet - a nice
dependency-free data class, with an observer interface, that something in the UI
implements to wire up to the error service?

ExtensionSystem would own one instance of ExtensionWarningSet, and would create
the observer that connects it to the error service.

It seems like that would also make testing this class easier. No mocking
necessary.

[Aaron Boodman, 2012-09-18 18:50:06]

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_warning_set.h (right):

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_EXTENSION_WARNING_SET_H_
On 2012/09/18 18:48:56, Aaron Boodman wrote:
> Seems like this file should be called extension_warning_service. There's only
> one line of code here that implements ExtensionWarningSet (a typedef).

Sorry, this comment only applies if you don't do the thing I suggested elsewhere
about making a dependency-free ExtensionWarningSet and getting rid of
ExtensionWarningService.

[battre, 2012-09-20 14:23:07]

Hi Aaron,

Please take another look.

Thanks,
Dominic

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:814:
std::set<ExtensionWarning::WarningType> warning_types;
On 2012/09/11 21:56:41, battre wrote:
> I will move this into the if statement.

Done.

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:818: &warning_types);
On 2012/09/18 18:48:56, Aaron Boodman wrote:
> Why not put this in UNLOAD rather than duplicating it for DISABLE and
UNINSTALL.
> 
> Also, can you set up WarningService to listen to notifications from
> ExtensionService, rather than having the code in here?

Done.

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_warning_set.h (right):

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:47: // We allow copy&assign
for passing containers of ExtensionWarnings between
On 2012/09/18 18:48:56, Aaron Boodman wrote:
> More generally, it's a value type. We have a lot of these in Chrome. I'm not
> sure it's really worth calling out.

Done.

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:51: void operator=(const
ExtensionWarning& other);
On 2012/09/18 18:48:56, Aaron Boodman wrote:
> What is this used by?

I have fixed the operator= definition (to return ExtensionWarning&). I think if
you define a copy constructor you should also define an assignment operator.

http://codereview.chromium.org/10407105/diff/18004/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:113: // etc.) trigger a
warning badge in the UI and and provide means to resolve
On 2012/09/18 18:48:56, Aaron Boodman wrote:
> It seems like a layering violation to have this class directly poke at
Chrome's
> UI (the global error service).
> 
> Can ExtensionWarningService just become ExtensionWarningSet - a nice
> dependency-free data class, with an observer interface, that something in the
UI
> implements to wire up to the error service?
> 
> ExtensionSystem would own one instance of ExtensionWarningSet, and would
create
> the observer that connects it to the error service.
> 
> It seems like that would also make testing this class easier. No mocking
> necessary.

Done. Except that I left some mocking.

[battre, 2012-09-28 15:34:20]

Aaron, I have fixed a SEGFAULT and addressed your comments. Could you please
have another look?

Glen, are you ok with the better strings?

Thanks,
Dominic

[battre, 2012-10-15 20:42:53]

Aaron: Another ping, as you declared email bankruptcy.

Best regards,
Dominic

On 2012/09/28 15:34:20, battre wrote:
> Aaron, I have fixed a SEGFAULT and addressed your comments. Could you please
> have another look?
> 
> Glen, are you ok with the better strings?
> 
> Thanks,
> Dominic

[Aaron Boodman, 2012-10-16 06:06:05]

http://codereview.chromium.org/10407105/diff/37004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_warning_set.cc (right):

http://codereview.chromium.org/10407105/diff/37004/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.cc:220: registrar_.Add(this,
chrome::NOTIFICATION_EXTENSION_UNLOADED,
Sigh, this isn't what I had in mind for a "dumb data class".

I guess this makes more sense with ExtensionWarningSet and
ExtensionWarningService combined together in the same file, and just the single
member of ExtensionSystem.

Pre-emptive LGTM so we don't have to have another review round.

[battre, 2012-10-19 13:56:51]

Hi Aaron,

I am terribly sorry. I pinged you for a messy CL. I had two different branches,
addressed your most recent comments on the wrong branch and uploaded it.

I have updated everything and think this is now quite nice from an architectural
point of view:

ExtensionWarningSet is a simple std::set<ExtensionWarning> data structure,
nothing fancy.

ExtensionWarningService manages the currently active ExtensionWarnings and can
be observed. It manages only ExtensionWarnings and has no UI itself.

ExtensionWarningBadgeService observes ExtensionWarningService and takes care of
displaying a warning badge and suppressing extensions that the user has
acknowledged by clicking on the warning badge.

PTAL and sorry again.

Thanks,
Dominic

[battre, 2012-11-05 17:15:08]

@Aaron: Friendly ping, trying to revive this. Please review this CL.

Thanks,
Dominic

[Aaron Boodman, 2012-11-08 23:02:55]

lgtm w/ nits - sorry for laggy review

http://codereview.chromium.org/10407105/diff/65006/chrome/browser/extensions/...
File chrome/browser/extensions/extension_warning_service.h (right):

http://codereview.chromium.org/10407105/diff/65006/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_service.h:55:
std::set<ExtensionWarning::WarningType>* result) const;
Nit: You can just return this. The copy is optimized away by the compiler.

http://codereview.chromium.org/10407105/diff/65006/chrome/browser/extensions/...
File chrome/browser/extensions/extension_warning_set.h (right):

http://codereview.chromium.org/10407105/diff/65006/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:20: // This class is used to
represent warnings if extensions misbehave.
We only use this class in certain kinds of misbehavior. Consider adding a note
that it is used with ExtensionWarningService

[battre, 2012-11-13 17:27:33]

http://codereview.chromium.org/10407105/diff/65006/chrome/browser/extensions/...
File chrome/browser/extensions/extension_warning_service.h (right):

http://codereview.chromium.org/10407105/diff/65006/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_service.h:55:
std::set<ExtensionWarning::WarningType>* result) const;
On 2012/11/08 23:02:55, Aaron Boodman wrote:
> Nit: You can just return this. The copy is optimized away by the compiler.

Done.

http://codereview.chromium.org/10407105/diff/65006/chrome/browser/extensions/...
File chrome/browser/extensions/extension_warning_set.h (right):

http://codereview.chromium.org/10407105/diff/65006/chrome/browser/extensions/...
chrome/browser/extensions/extension_warning_set.h:20: // This class is used to
represent warnings if extensions misbehave.
On 2012/11/08 23:02:55, Aaron Boodman wrote:
> We only use this class in certain kinds of misbehavior. Consider adding a note
> that it is used with ExtensionWarningService

Done.

[battre, 2012-11-13 17:32:34]

I will TBR jochen for OWNERs approval of chrome/*.gypi changes and code deletion
in chrome/common/chrome_notification_types.h.

I will TBR estade for OWNERs approval of
chrome/browser/ui/webui/extensions/extension_settings_handler.{h,cc} as this has
been reviewed by aa and is mostly a minor refactoring of existing calls that are
not particular to chrome/browser/ui/webui.

[commit-bot: I haz the power, 2012-11-13 18:17:07]

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

[jochen (gone - plz use gerrit), 2012-11-13 18:23:46]

gypi changes lgtm

[battre, 2012-11-13 18:31:52]

Taking out of commit queue as Jochen feels that
chrome/browser/ui/webui/extensions/ should have a full owners review.

Evan, can you please review
chrome/browser/ui/webui/extensions/extension_settings_handler.*?

Thanks,
Dominic

[Evan Stade, 2012-11-13 20:21:54]

webui lgtm

http://codereview.chromium.org/10407105/diff/78004/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10407105/diff/78004/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:94:
registrar_.RemoveAll();
I don't think this is necessary

http://codereview.chromium.org/10407105/diff/78004/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:96: if
(registered_for_notifications_) {
might be nice if there were a scoped object to handle removing |this| as
observer. Also might be more hassle than it's worth.

http://codereview.chromium.org/10407105/diff/78004/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.h (right):

http://codereview.chromium.org/10407105/diff/78004/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.h:68: public
base::SupportsWeakPtr<ExtensionSettingsHandler>{
can you add a space before the curly

[battre, 2012-11-14 12:30:53]

Done.

Jochen, could you please review the changes between patch set 24 and 26?

Thanks,
Dominic

http://codereview.chromium.org/10407105/diff/78004/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10407105/diff/78004/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:94:
registrar_.RemoveAll();
On 2012/11/13 20:21:54, Evan Stade wrote:
> I don't think this is necessary

You are right.

Done.

http://codereview.chromium.org/10407105/diff/78004/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:96: if
(registered_for_notifications_) {
On 2012/11/13 20:21:54, Evan Stade wrote:
> might be nice if there were a scoped object to handle removing |this| as
> observer. Also might be more hassle than it's worth.

Done.

http://codereview.chromium.org/10407105/diff/78004/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.h (right):

http://codereview.chromium.org/10407105/diff/78004/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.h:68: public
base::SupportsWeakPtr<ExtensionSettingsHandler>{
On 2012/11/13 20:21:54, Evan Stade wrote:
> can you add a space before the curly

Done.

[jochen (gone - plz use gerrit), 2012-11-14 12:42:55]

lgtm

[commit-bot: I haz the power, 2012-11-14 12:57:37]

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

[commit-bot: I haz the power, 2012-11-14 14:44:13]

Change committed as 167673

[Evan Stade, 2012-11-14 21:50:15]

lgtm

http://codereview.chromium.org/10407105/diff/86005/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.h (right):

http://codereview.chromium.org/10407105/diff/86005/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.h:244:
ScopedObserver<extensions::ExtensionWarningService,
I did not even know this existed. That is cool.