Create interface to manage manual exceptions.

Create interface to manage manual exceptions.

Adds a new dialog that allows a user to edit the list of manual exceptions for
managed mode.

Authentication is not yet tied in.

R=bauerb@chromium.org, markusheintz@chromium.org, jhawkins@chromium.org
BUG=171370
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192937

[Bernhard Bauer, 2013-03-27 14:41:11]

https://codereview.chromium.org/12594029/diff/2001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://codereview.chromium.org/12594029/diff/2001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:259:
!url.has_ref();
How about using CanonicalizeHost() (from gurl.h)? It will return false for an
invalid host, and as a bonus you'll get a nice canonicalized hostname back.

[Sergiu, 2013-03-27 15:26:45]

Fixed that and the stuff from the other side.

https://codereview.chromium.org/12594029/diff/2001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://codereview.chromium.org/12594029/diff/2001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:259:
!url.has_ref();
On 2013/03/27 14:41:11, Bernhard Bauer wrote:
> How about using CanonicalizeHost() (from gurl.h)? It will return false for an
> invalid host, and as a bonus you'll get a nice canonicalized hostname back.

I've changed it to that (the one from url_canon.h) but I'm not sure we want to
show the user the canonical version, that one is escaped.

[Bernhard Bauer, 2013-03-27 15:55:12]

https://codereview.chromium.org/12594029/diff/2001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://codereview.chromium.org/12594029/diff/2001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:259:
!url.has_ref();
On 2013/03/27 15:26:46, Sergiu wrote:
> On 2013/03/27 14:41:11, Bernhard Bauer wrote:
> > How about using CanonicalizeHost() (from gurl.h)? It will return false for
an
> > invalid host, and as a bonus you'll get a nice canonicalized hostname back.
> 
> I've changed it to that (the one from url_canon.h) but I'm not sure we want to
> show the user the canonical version, that one is escaped.

By escaped you mean punycode? We also use that in any other place I can think of
(e.g. history, omnibox).

We also use canonicalized hostnames internally in ManagedUserService, so that's
what we should pass to it. If we really want to show the hosts unescaped, we
should do that just for the UI.

[Sergiu, 2013-03-27 19:07:06]

On 2013/03/27 15:55:12, Bernhard Bauer wrote:
>
https://codereview.chromium.org/12594029/diff/2001/chrome/browser/ui/webui/op...
> File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):
> 
>
https://codereview.chromium.org/12594029/diff/2001/chrome/browser/ui/webui/op...
> chrome/browser/ui/webui/options/managed_user_settings_handler.cc:259:
> !url.has_ref();
> On 2013/03/27 15:26:46, Sergiu wrote:
> > On 2013/03/27 14:41:11, Bernhard Bauer wrote:
> > > How about using CanonicalizeHost() (from gurl.h)? It will return false for
> an
> > > invalid host, and as a bonus you'll get a nice canonicalized hostname
back.
> > 
> > I've changed it to that (the one from url_canon.h) but I'm not sure we want
to
> > show the user the canonical version, that one is escaped.
> 
> By escaped you mean punycode? We also use that in any other place I can think
of
> (e.g. history, omnibox).
> 
> We also use canonicalized hostnames internally in ManagedUserService, so
that's
> what we should pass to it. If we really want to show the hosts unescaped, we
> should do that just for the UI.

I think I've done it now, check out the new version, I get the canonical form
when adding the host and the GURL transforms the URL to a canonical form when
building it anyway so that is fine.

[Bernhard Bauer, 2013-04-02 11:08:36]

https://codereview.chromium.org/12594029/diff/7003/chrome/browser/resources/o...
File chrome/browser/resources/options/managed_user_settings.js (right):

https://codereview.chromium.org/12594029/diff/7003/chrome/browser/resources/o...
chrome/browser/resources/options/managed_user_settings.js:202: var
exceptionsList = $('manual-exceptions');
I think you could inline this now.

https://codereview.chromium.org/12594029/diff/7003/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://codereview.chromium.org/12594029/diff/7003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:40: bool
CheckHostname(std::string host, std::string* output_string) {
This method has too many side effects for my taste to just be called "Check..."
Maybe CanonicalizeHostname()?

https://codereview.chromium.org/12594029/diff/7003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:234:
DCHECK(CheckHostname(pattern, &canonical_host));
This will leave |canonical_host| empty in a non-DCHECK-enabled build. What you
want is to store the return value in a variable and only DCHECK that.

https://codereview.chromium.org/12594029/diff/7003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:279: is_valid =
CheckHostname(pattern_string, NULL);
I think you could move the declaration of |is_valid| to here:

 bool is_valid = (url.is_valid() && url.IsStandard()) ||
                 CheckHostname(pattern_string, NULL);

[Sergiu, 2013-04-02 14:55:38]

https://codereview.chromium.org/12594029/diff/7003/chrome/browser/resources/o...
File chrome/browser/resources/options/managed_user_settings.js (right):

https://codereview.chromium.org/12594029/diff/7003/chrome/browser/resources/o...
chrome/browser/resources/options/managed_user_settings.js:202: var
exceptionsList = $('manual-exceptions');
On 2013/04/02 11:08:36, Bernhard Bauer wrote:
> I think you could inline this now.

Done.

https://codereview.chromium.org/12594029/diff/7003/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://codereview.chromium.org/12594029/diff/7003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:40: bool
CheckHostname(std::string host, std::string* output_string) {
On 2013/04/02 11:08:36, Bernhard Bauer wrote:
> This method has too many side effects for my taste to just be called
"Check..."
> Maybe CanonicalizeHostname()?

Renamed to CanonicalizeHost, as the other function is called, although
"canonicalize" is not really a verb in English :P

https://codereview.chromium.org/12594029/diff/7003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:234:
DCHECK(CheckHostname(pattern, &canonical_host));
On 2013/04/02 11:08:36, Bernhard Bauer wrote:
> This will leave |canonical_host| empty in a non-DCHECK-enabled build. What you
> want is to store the return value in a variable and only DCHECK that.

Ah, got it.

https://codereview.chromium.org/12594029/diff/7003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:279: is_valid =
CheckHostname(pattern_string, NULL);
On 2013/04/02 11:08:36, Bernhard Bauer wrote:
> I think you could move the declaration of |is_valid| to here:
> 
>  bool is_valid = (url.is_valid() && url.IsStandard()) ||
>                  CheckHostname(pattern_string, NULL);

True, done.

[Bernhard Bauer, 2013-04-02 15:07:55]

LGTM with a nit:

https://codereview.chromium.org/12594029/diff/7003/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://codereview.chromium.org/12594029/diff/7003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:40: bool
CheckHostname(std::string host, std::string* output_string) {
On 2013/04/02 14:55:38, Sergiu wrote:
> On 2013/04/02 11:08:36, Bernhard Bauer wrote:
> > This method has too many side effects for my taste to just be called
> "Check..."
> > Maybe CanonicalizeHostname()?
> 
> Renamed to CanonicalizeHost, as the other function is called, although
> "canonicalize" is not really a verb in English :P

It's not? It has a wiktionary entry: http://en.wiktionary.org/wiki/canonicalize.

https://codereview.chromium.org/12594029/diff/16002/chrome/browser/resources/...
File chrome/browser/resources/options/managed_user_settings.js (right):

https://codereview.chromium.org/12594029/diff/16002/chrome/browser/resources/...
chrome/browser/resources/options/managed_user_settings.js:215: var
exceptionsList = $('manual-exceptions');
This can be inlined too.

[Sergiu, 2013-04-02 15:22:55]

Hey James, please take a look at this.

Thanks!

> > Renamed to CanonicalizeHost, as the other function is called, although
> > "canonicalize" is not really a verb in English :P
> 
> It's not? It has a wiktionary entry:
http://en.wiktionary.org/wiki/canonicalize.

Interesting, Mirriam-Webster doesn't have it.

https://codereview.chromium.org/12594029/diff/16002/chrome/browser/resources/...
File chrome/browser/resources/options/managed_user_settings.js (right):

https://codereview.chromium.org/12594029/diff/16002/chrome/browser/resources/...
chrome/browser/resources/options/managed_user_settings.js:215: var
exceptionsList = $('manual-exceptions');
On 2013/04/02 15:07:56, Bernhard Bauer wrote:
> This can be inlined too.

Done.

[James Hawkins, 2013-04-02 15:57:32]

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
File chrome/browser/resources/options/managed_user_exceptions_area.html (right):

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
chrome/browser/resources/options/managed_user_exceptions_area.html:6: <div
class="manual-exception-pattern-column"
Why use class when there's only one use of each of pattern-column and
behavior-column?

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
File chrome/browser/resources/options/managed_user_exceptions_area.js (left):

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
chrome/browser/resources/options/managed_user_exceptions_area.js:15: * @param
{string} contentType The type of the list.
What is up with all the code removal in this file?

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
File chrome/browser/resources/options/managed_user_settings.css (right):

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
chrome/browser/resources/options/managed_user_settings.css:35: display:
-webkit-box;
s/box/flex/

See:

https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-d...

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
File chrome/browser/resources/options/managed_user_settings.js (right):

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
chrome/browser/resources/options/managed_user_settings.js:64:
exceptionsButtons[0].onclick = function(event) {
This seems fragile; why are we only adding onclick to the first button in the
list?

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
chrome/browser/resources/options/managed_user_settings.js:67:
window.history.replaceState({pageName: page.name},
What's with this replaceState?

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:66: // Read the
manual url and host lists from the current profile and add them to
s/Read/Reads/

s/add/adds/

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/managed_user_settings_handler.h (right):

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/managed_user_settings_handler.h:39: // Called to
decide whether a given pattern is valid, or if it should be
Optional nit: s/Called to decide/Decides/

[Sergiu, 2013-04-02 17:14:09]

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
File chrome/browser/resources/options/managed_user_exceptions_area.html (right):

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
chrome/browser/resources/options/managed_user_exceptions_area.html:6: <div
class="manual-exception-pattern-column"
On 2013/04/02 15:57:33, James Hawkins wrote:
> Why use class when there's only one use of each of pattern-column and
> behavior-column?

Bernhard was worried about collisions but in the end I named them in an unique
way but never changed them back, did that now.

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
File chrome/browser/resources/options/managed_user_exceptions_area.js (left):

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
chrome/browser/resources/options/managed_user_exceptions_area.js:15: * @param
{string} contentType The type of the list.
On 2013/04/02 15:57:33, James Hawkins wrote:
> What is up with all the code removal in this file?

This is Rietveld being a bit confused, I picked up the CL form our intern and
she copied this file from content_settings_exceptions_area.js. So for some
strange reason Rietveld shows the diff vs that file instead of a completely new
one.

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
File chrome/browser/resources/options/managed_user_settings.css (right):

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
chrome/browser/resources/options/managed_user_settings.css:35: display:
-webkit-box;
On 2013/04/02 15:57:33, James Hawkins wrote:
> s/box/flex/
> 
> See:
> 
>
https://groups.google.com/a/chromium.org/forum/?fromgroups=#%21topic/chromium...

Done.

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
File chrome/browser/resources/options/managed_user_settings.js (right):

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
chrome/browser/resources/options/managed_user_settings.js:64:
exceptionsButtons[0].onclick = function(event) {
On 2013/04/02 15:57:33, James Hawkins wrote:
> This seems fragile; why are we only adding onclick to the first button in the
> list?

Yeah, that wasn't exactly right since there is only one button in this case,
replaced the class name with an id and put the event on that.

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
chrome/browser/resources/options/managed_user_settings.js:67:
window.history.replaceState({pageName: page.name},
On 2013/04/02 15:57:33, James Hawkins wrote:
> What's with this replaceState?

I thought this was needed by looking at the content settings code but come to
think of it I don't think it is. Removed.

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:66: // Read the
manual url and host lists from the current profile and add them to
On 2013/04/02 15:57:33, James Hawkins wrote:
> s/Read/Reads/
> 
> s/add/adds/

Done.

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/managed_user_settings_handler.h (right):

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/managed_user_settings_handler.h:39: // Called to
decide whether a given pattern is valid, or if it should be
On 2013/04/02 15:57:33, James Hawkins wrote:
> Optional nit: s/Called to decide/Decides/

Done.

[Bernhard Bauer, 2013-04-02 17:18:55]

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
File chrome/browser/resources/options/managed_user_exceptions_area.js (left):

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
chrome/browser/resources/options/managed_user_exceptions_area.js:15: * @param
{string} contentType The type of the list.
On 2013/04/02 17:14:09, Sergiu wrote:
> On 2013/04/02 15:57:33, James Hawkins wrote:
> > What is up with all the code removal in this file?
> 
> This is Rietveld being a bit confused, I picked up the CL form our intern and
> she copied this file from content_settings_exceptions_area.js. So for some
> strange reason Rietveld shows the diff vs that file instead of a completely
new
> one.

To be exact, this is due to the output of git diff, which uses a heuristic to
determine whether a new file is a copy of an existing one.

[James Hawkins, 2013-04-02 17:19:09]

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
File chrome/browser/resources/options/managed_user_exceptions_area.js (left):

https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
chrome/browser/resources/options/managed_user_exceptions_area.js:15: * @param
{string} contentType The type of the list.
On 2013/04/02 17:14:09, Sergiu wrote:
> On 2013/04/02 15:57:33, James Hawkins wrote:
> > What is up with all the code removal in this file?
> 
> This is Rietveld being a bit confused, I picked up the CL form our intern and
> she copied this file from content_settings_exceptions_area.js. So for some
> strange reason Rietveld shows the diff vs that file instead of a completely
new
> one.

OK.  I'm very concerned about how much code is now exactly the same in both of
these classes.  I think we need to think about how to share code here.

[Sergiu, 2013-04-03 13:28:50]

On 2013/04/02 17:19:09, James Hawkins wrote:
>
https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
> File chrome/browser/resources/options/managed_user_exceptions_area.js (left):
> 
>
https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
> chrome/browser/resources/options/managed_user_exceptions_area.js:15: * @param
> {string} contentType The type of the list.
> On 2013/04/02 17:14:09, Sergiu wrote:
> > On 2013/04/02 15:57:33, James Hawkins wrote:
> > > What is up with all the code removal in this file?
> > 
> > This is Rietveld being a bit confused, I picked up the CL form our intern
and
> > she copied this file from content_settings_exceptions_area.js. So for some
> > strange reason Rietveld shows the diff vs that file instead of a completely
> new
> > one.
> 
> OK.  I'm very concerned about how much code is now exactly the same in both of
> these classes.  I think we need to think about how to share code here.

Well, I'm open to suggestions but I'm not sure what the best solution is other
than this. What I'm doing is using the UI from content settings exceptions with
a different model and custom sorting behind it. So I don't really think that the
correct place would be in there. I've tried to keep only useful code, other than
that I'm not sure what should be a better approach.

[James Hawkins, 2013-04-05 18:38:25]

On 2013/04/03 13:28:50, Sergiu wrote:
> On 2013/04/02 17:19:09, James Hawkins wrote:
> >
>
https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
> > File chrome/browser/resources/options/managed_user_exceptions_area.js
(left):
> > 
> >
>
https://codereview.chromium.org/12594029/diff/21007/chrome/browser/resources/...
> > chrome/browser/resources/options/managed_user_exceptions_area.js:15: *
@param
> > {string} contentType The type of the list.
> > On 2013/04/02 17:14:09, Sergiu wrote:
> > > On 2013/04/02 15:57:33, James Hawkins wrote:
> > > > What is up with all the code removal in this file?
> > > 
> > > This is Rietveld being a bit confused, I picked up the CL form our intern
> and
> > > she copied this file from content_settings_exceptions_area.js. So for some
> > > strange reason Rietveld shows the diff vs that file instead of a
completely
> > new
> > > one.
> > 
> > OK.  I'm very concerned about how much code is now exactly the same in both
of
> > these classes.  I think we need to think about how to share code here.
> 
> Well, I'm open to suggestions but I'm not sure what the best solution is other
> than this. What I'm doing is using the UI from content settings exceptions
with
> a different model and custom sorting behind it. So I don't really think that
the
> correct place would be in there. I've tried to keep only useful code, other
than
> that I'm not sure what should be a better approach.

My suggestion is to create a base class for shared code.  I don't know what it
would look like, as that would be tantamount to me doing the refactoring.  I'm
not going to block this CL (any longer) on that, but please add a TODO and file
a Cleanup bug.

LGTM

[commit-bot: I haz the power, 2013-04-08 09:35:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergiu@chromium.org/12594029/38001

[commit-bot: I haz the power, 2013-04-08 09:43:08]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-08 21:28:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergiu@chromium.org/12594029/48001

[commit-bot: I haz the power, 2013-04-08 22:53:26]

Change committed as 192937