Move net::FormatUrl and friends outside of //net and into //components

Move net::FormatUrl and friends outside of //net and into //components

net::FormatUrl and related are specifically concerned with display
policies of URLs, which is not something that //net needs to be aware
of, as that's a UX question.

This folds in net::FormatURL along with the existing //components/url_fixer
and //components/secure_display into a common component,
//components/url_formatter, that handles reformatting URLs for user-friendly
or data storage (url_formatter), for use in security prompts (elide_url),
or for reformatting URLs from user input (url_fixer)

(Disabling presubmit since this is intentionally not fixing a legacy API, just moving it for future cleanups)

BUG=486979
NOPRESUBMIT=true

Committed: https://crrev.com/24f64dc221628032b7843f1af6b277a7f0d6c562
Cr-Commit-Position: refs/heads/master@{#342445}

[Ryan Sleevi, 2015-06-10 02:04:29]

rsleevi@chromium.org changed reviewers:
+ jam@chromium.org

[Ryan Sleevi, 2015-06-10 02:04:31]

jam: This is a big CL, but because it's so tightly coupled to //components, I
wanted to run the approach and design by you first. The longer explanation is as
follows, but I wanted to review the dependency graph with you.

As it stands, //mandoline is not using this right now, but one of the functions
being moved (IDNtoUnicode) is part of the Blink::Platform layer, and presumably
will eventually be implemented by html_viewer (right now, html_viewer doesn't
handle IDN display at all; ideally, that would change)

The overall goal of this is to move presentation logic out of //net, which is
what FormatUrl is. FormatUrl is also generally unsafe/wrong for the majority of
its callers (example issue is http://crbug.com/498069 , although more will come
as we audit callers), which are expecting the URL displayed to represent the
logical security boundary (this gets especially messy with file:// URLs, which
//url handles differently than Blink with regards to origins).

Moving FormatUrlForSecurityDisplay (currently in //chrome/browser/ui) will help
resolve these issues, and help keep //net unaware of presentation concerns.

This also helps (but not complete) reduce the dependencies on ICU within //net,
which is an issue for some platforms.

Caveats:
- This code needs to be called from iOS (//iOS)
- This code needs to be called from //content
- This code needs to be called from other //components (e.g. when formatting
prompts & security dialogs or other user-facing elements)
- This code needs to be called from //chrome (naturally)
- This code will (eventually) need to be called by //components/html_viewer

For these reasons, a //component as I've done in this CL seems right.

[Ryan Sleevi, 2015-06-10 02:20:55]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[Ryan Sleevi, 2015-06-10 02:21:05]

The CQ bit was unchecked by rsleevi@chromium.org

[Ryan Sleevi, 2015-06-10 02:23:45]

rsleevi@chromium.org changed reviewers:
+ droger@chromium.org

[Ryan Sleevi, 2015-06-10 02:23:46]

Also adding droger@ for an early design review, to make sure I haven't botched
the iOS-specific concerns. This isn't a request for a full review - please see
my remarks to jam@ and just focus on "Does this dependency graph look right" and
"Have I botched the layering concerns?" . If so, the rest of the review would be
pointless ;)

[droger, 2015-06-10 10:25:40]

droger@chromium.org changed reviewers:
+ stuartmorgan@chromium.org

[droger, 2015-06-10 10:25:42]

This is the first time //ios/web depends on a component.
Adding stuartmorgan who is the main owner of //ios/web.

The high-level architecture looks fine to me.

https://codereview.chromium.org/1171333003/diff/60001/components/url_formatte...
File components/url_formatter/DEPS (right):

https://codereview.chromium.org/1171333003/diff/60001/components/url_formatte...
components/url_formatter/DEPS:4: "-content",
Maybe also disallow ios and mandoline for consistency?

[stuartmorgan, 2015-06-10 16:09:10]

ios/web/ changes LGTM.

(I'm still not at all thrilled by the fact that there's no obvious way to tell
that this component dependency is one of the okay ones--i.e., conceptually below
content rather than above it--but I guess I lost that argument.)

https://codereview.chromium.org/1171333003/diff/60001/components/url_formatte...
File components/url_formatter/DEPS (right):

https://codereview.chromium.org/1171333003/diff/60001/components/url_formatte...
components/url_formatter/DEPS:4: "-content",
On 2015/06/10 10:25:41, droger wrote:
> Maybe also disallow ios and mandoline for consistency?

I'd certainly like to see ios/web explicitly disallowed anywhere content is.

https://codereview.chromium.org/1171333003/diff/60001/ios/web/DEPS
File ios/web/DEPS (right):

https://codereview.chromium.org/1171333003/diff/60001/ios/web/DEPS#newcode3
ios/web/DEPS:3: "+components/url_formatter",
components < crypto

[jam, 2015-06-10 20:06:33]

On 2015/06/10 02:04:31, Ryan Sleevi wrote:
> jam: This is a big CL, but because it's so tightly coupled to //components, I
> wanted to run the approach and design by you first. The longer explanation is
as
> follows, but I wanted to review the dependency graph with you.
> 
> As it stands, //mandoline is not using this right now, but one of the
functions
> being moved (IDNtoUnicode) is part of the Blink::Platform layer, and
presumably
> will eventually be implemented by html_viewer (right now, html_viewer doesn't
> handle IDN display at all; ideally, that would change)
> 
> The overall goal of this is to move presentation logic out of //net, which is
> what FormatUrl is. FormatUrl is also generally unsafe/wrong for the majority
of
> its callers (example issue is http://crbug.com/498069 , although more will
come
> as we audit callers), which are expecting the URL displayed to represent the
> logical security boundary (this gets especially messy with file:// URLs, which
> //url handles differently than Blink with regards to origins).
> 
> Moving FormatUrlForSecurityDisplay (currently in //chrome/browser/ui) will
help
> resolve these issues, and help keep //net unaware of presentation concerns.
> 
> This also helps (but not complete) reduce the dependencies on ICU within
//net,
> which is an issue for some platforms.
> 
> Caveats:
> - This code needs to be called from iOS (//iOS)
> - This code needs to be called from //content
> - This code needs to be called from other //components (e.g. when formatting
> prompts & security dialogs or other user-facing elements)
> - This code needs to be called from //chrome (naturally)
> - This code will (eventually) need to be called by //components/html_viewer
> 
> For these reasons, a //component as I've done in this CL seems right.

Thanks for the clear explanation. I looked through the DEPS files (but nothing
else) and they all make sense. You're right we'll have to start calling this in
html_viewer.

My main question is: do we want to have components/url instead of
components/{url_fixer, url_matcher, url_formatter}? I understand they're
different things, but it almost feels too much to have these three separate
directories. If you think 3 is better, then at the very least add an OWNERS file
in the new directory.

https://codereview.chromium.org/1171333003/diff/60001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/1171333003/diff/60001/content/browser/DEPS#ne...
content/browser/DEPS:6: "+components/url_formatter",
nit: order

[Ryan Sleevi, 2015-06-10 23:44:56]

On 2015/06/10 20:06:33, jam wrote:
> My main question is: do we want to have components/url instead of
> components/{url_fixer, url_matcher, url_formatter}? I understand they're
> different things, but it almost feels too much to have these three separate
> directories. If you think 3 is better, then at the very least add an OWNERS
file
> in the new directory.

This is an excellent question, and one I've struggled with finding a good answer
for.

The arguments in favour is that we have three components dealing with URLs,
doesn't it make sense to consolidate these (into a general "URL manipulation
that has user interactivity" component)?
url_fixer - Used by Omnibox & History (primarily) to 'fix' broken URLs that
users may type into a common form.
  - Input: Invalid URL, Output: Valid URL
url_formatter - Used by lots of things (that shouldn't be using it), but also
Omnibox and History. Generally falls into two use cases
  - Format a URL for presenting to a user (e.g. the things most commonly
botched)
     - Input: Valid URL, Output: Invalid URL (or at least, non-reversibly so)
  - Canonicalize a URL into some internal-stable form (used by Omnibox and
History)
     - Input: Valid URL, Output: "Opaque"
url_matcher - Used by extensions and content settings for 'see if URL X matches
pattern Y', where both URL X and pattern Y are things shown to users.

On the "Single Responsibility", each of these have distinct responsibilities.
fixer and formatter are the most natural to consider combining, despite their
distinct inputs, because of their shared usage in Omnibox & History and in
handling "User stuff". url_matcher probably remains distinct.

On the cons argument, the reasons I didn't do it in this CL
  - Keep the refactoring 'simple' for the existing owners
  - Because of the great dependency change here (things depending on //net now
depending on url_formatter), I wanted to make sure separately that url_fixer
could meet the same dependency requirements (common dependency for all
platforms) as url_formatter
  - Code bloat (they're not strictly coupled)
  - Not wanting to run afoul of components policy (for whether or not url_fixer
*needs* to be shared across impls)

As I get more OWNERS approval for this CL (validating both design and
implementation), then barring major issues, I'll file a follow-up bug to work
with the url_fixer OWNERs to better answer that question for you. Does that
work?

[Ryan Sleevi, 2015-06-10 23:50:35]

rsleevi@chromium.org changed reviewers:
+ mkwst@chromium.org, pkasting@chromium.org, sky@chromium.org

[Ryan Sleevi, 2015-06-10 23:50:37]

I'm going to start collecting stamps now, but in the event anyone starts closely
reviewing, one caveat:
It seems appropriate to convert this component from a static_library into a
shared_library, since it's linked in to so many modules (browser and renderer
alike)

I'll also be addressing the review feedback as it comes in.

mkwst@chromium.org: Please review changes in 
content/shell

pkasting@chromium.org: Please review changes in 
components/omnibox
chrome/browser/omnibox
[general layering]
And see the message immediately before this one from me regarding whether
url_fixer and url_formatter should be combined.

sky@chromium.org: Please review changes in 
chrome/ (general renaming rubber-stamp; DEPs are the exciting bits)

[Ryan Sleevi, 2015-06-10 23:54:11]

rsleevi@chromium.org changed reviewers:
+ jochen@chromium.org

[Ryan Sleevi, 2015-06-10 23:54:12]

jochen@chromium.org: Please review changes in 
components/ (wide-reaching rubber-stamp into the DEPS changes; see my initial
remarks to jam@ for the explanation)

[Peter Kasting, 2015-06-11 00:14:20]

On 2015/06/10 23:50:37, Ryan Sleevi wrote:
> mailto:pkasting@chromium.org: Please review changes in 
> components/omnibox
> chrome/browser/omnibox
> [general layering]
> And see the message immediately before this one from me regarding whether
> url_fixer and url_formatter should be combined.

Yes, I would combine these.  I wrote much of both, and I can't really grasp the
distinction you're trying to make, so I have little hope that the majority of
readers would.

That said, it's also non-obvious to me what FormatUrl() is doing wrong for these
various callers you refer to.  Reading FormatUrlForSecurityDisplay(), it seems
to be primarily about stripping the path/query/ref from standard-scheme URLs. 
I'm not sure why it (or its callers) are not doing something like
FormatUrl(url.GetOrigin()) if they want to strip these.

[Ryan Sleevi, 2015-06-11 00:23:28]

On 2015/06/11 00:14:20, Peter Kasting wrote:
> Yes, I would combine these.  I wrote much of both, and I can't really grasp
the
> distinction you're trying to make, so I have little hope that the majority of
> readers would.

url_fixer seems tightly coupled with the omnibox/search engine logic, and
doesn't seem to have general utility outside of that. It's an input handling
that has a semantic transformation of the URL. (e.g. chrome:// is translated to
about: in some cases)

url_formatter is generic for all presentation logic, and is used extensively
(see above). I wanted to avoid accidentally creating a "url_utils" of 'all the
random things in one component that aren't tightly related'.

> That said, it's also non-obvious to me what FormatUrl() is doing wrong for
these
> various callers you refer to.  Reading FormatUrlForSecurityDisplay(), it seems
> to be primarily about stripping the path/query/ref from standard-scheme URLs. 
> I'm not sure why it (or its callers) are not doing something like
> FormatUrl(url.GetOrigin()) if they want to strip these.

Because url.GetOrigin() returns the semantic tuple-origin (scheme host port),
but not the security origin (e.g. how permissions and privileges are scoped).
Some of the discussion is captured on
https://groups.google.com/a/chromium.org/d/msg/net-dev/cXLW9bAmwt8/9uTi62ZCXc0J
about the overloading of the term "origin" for different things. I think that's
a separate discussion you should have with chrome-security-enamel@ if it makes
you react negatively.

[Peter Kasting, 2015-06-11 00:30:05]

On 2015/06/11 00:23:28, Ryan Sleevi wrote:
> On 2015/06/11 00:14:20, Peter Kasting wrote:
> > Yes, I would combine these.  I wrote much of both, and I can't really grasp
> the
> > distinction you're trying to make, so I have little hope that the majority
of
> > readers would.
> 
> url_fixer seems tightly coupled with the omnibox/search engine logic, and
> doesn't seem to have general utility outside of that. It's an input handling
> that has a semantic transformation of the URL. (e.g. chrome:// is translated
to
> about: in some cases)

This is usable -- and should be used, even where it isn't being used -- on any
place we can allow url-like input from users.  For example, various pieces of
our settings logic.  It's not tightly coupled to the omnibox.

> > That said, it's also non-obvious to me what FormatUrl() is doing wrong for
> these
> > various callers you refer to.  Reading FormatUrlForSecurityDisplay(), it
seems
> > to be primarily about stripping the path/query/ref from standard-scheme
URLs. 
> > I'm not sure why it (or its callers) are not doing something like
> > FormatUrl(url.GetOrigin()) if they want to strip these.
> 
> Because url.GetOrigin() returns the semantic tuple-origin (scheme host port),
> but not the security origin (e.g. how permissions and privileges are scoped).
> Some of the discussion is captured on
>
https://groups.google.com/a/chromium.org/d/msg/net-dev/cXLW9bAmwt8/9uTi62ZCXc0J
> about the overloading of the term "origin" for different things. I think
that's
> a separate discussion you should have with chrome-security-enamel@ if it makes
> you react negatively.

Is it correct to say that this is primarily problematic because of sandboxed
iframes (in particular, that it looks like they're introducing some sort of new
concept of what the security origin is)?  I'm not telling you to do things
differently, I'm just trying to understand whether I'm reading the other thread
correctly.

If so, I think the APIs and their comments need to do a much better job of
helping authors understand this distinction.  Added bonus if there's some way to
actually make it impossible to use the APIs incorrectly, although I can't think
of a way to do that.

[Ryan Sleevi, 2015-06-11 00:41:06]

On 2015/06/11 00:30:05, Peter Kasting wrote:
> Is it correct to say that this is primarily problematic because of sandboxed
> iframes (in particular, that it looks like they're introducing some sort of
new
> concept of what the security origin is)?  I'm not telling you to do things
> differently, I'm just trying to understand whether I'm reading the other
thread
> correctly.

It's a problem with file:// URLs as we tighten the policies related to that in
Blink
It's a problem with blob: and filesystem: URLs (whose security origin is the
contents of the inner_url)
It's a problem with sandboxed iframes (for which the origin is a unique/opaque
identifier)
It's a problem with usability when they want to omit the port from something
like https://foo.example:443 (which should be https://foo.example, or,
especially as powerful features are restricted to https, potentially just
"foo.example")

> If so, I think the APIs and their comments need to do a much better job of
> helping authors understand this distinction.  Added bonus if there's some way
to
> actually make it impossible to use the APIs incorrectly, although I can't
think
> of a way to do that.

I think you'll find there is violent agreement about that. Part of the goal of
moving this out of //net is to have chrome-security-enamel (which are going
through and auditing "URLs as presented to users") have a closer ownership of
this code, documentation, and usage patterns, and to get //net out of the
business of presentation logic (especially as //net gets embedded in more
places, such as Cronet)

[Peter Kasting, 2015-06-11 05:12:22]

OK, thanks for the explanations.

chrome/browser/autocomplete, components/omnibox, and infobar files LGTM.

I would still suggest combining the url fixer and formatter components; I think
they go together logically and we just don't benefit much from trying to split
them.

https://codereview.chromium.org/1171333003/diff/60001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/1171333003/diff/60001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider.cc:236:
net::UnescapeRule::SPACES, nullptr, nullptr, nullptr),
Man, this formatting seems gross :/

[jochen (gone - plz use gerrit), 2015-06-11 12:42:33]

if you want to use this component from html_viewer, you'll need to use a
separate gyp file for your component. components.gyp pulls in content, so we can
depend on it from html_viewer

[jam, 2015-06-15 16:48:24]

On 2015/06/11 05:12:22, Peter Kasting wrote:
> OK, thanks for the explanations.
> 
> chrome/browser/autocomplete, components/omnibox, and infobar files LGTM.
> 
> I would still suggest combining the url fixer and formatter components; I
think
> they go together logically and we just don't benefit much from trying to split

+1. I think the fact that different headers are used by different pieces of code
is a bit orthogonal from the fact that these url-related helper classes/methods
are related. Another way of putting it is that just like net/ or base/ has many
users, we don't split out classes there into separate components depending on
who/how they're called.

[Ryan Sleevi, 2015-06-15 16:55:09]

On 2015/06/15 16:48:24, jam wrote:
> 
> +1. I think the fact that different headers are used by different pieces of
code
> is a bit orthogonal from the fact that these url-related helper
classes/methods
> are related. Another way of putting it is that just like net/ or base/ has
many
> users, we don't split out classes there into separate components depending on
> who/how they're called.

Sure we do. The most obvious split is who needs to use ICU (this code does, the
existing code does not) or when there are natural dependency differences. This
makes the url_fixer code usable on less platforms, and requires that the
url_fixer code adhere to the same (stronger) dependency contract.

I tried to keep this CL minimal for focusing on url_formatter, since I still
believe that should be separate, but I have neither the time nor energy for
disagreeing with the two of you, so I'll just combine them in the same CL.

[Ryan Sleevi, 2015-06-15 16:55:16]

On 2015/06/15 16:48:24, jam wrote:
> 
> +1. I think the fact that different headers are used by different pieces of
code
> is a bit orthogonal from the fact that these url-related helper
classes/methods
> are related. Another way of putting it is that just like net/ or base/ has
many
> users, we don't split out classes there into separate components depending on
> who/how they're called.

Sure we do. The most obvious split is who needs to use ICU (this code does, the
existing code does not) or when there are natural dependency differences. This
makes the url_fixer code usable on less platforms, and requires that the
url_fixer code adhere to the same (stronger) dependency contract.

I tried to keep this CL minimal for focusing on url_formatter, since I still
believe that should be separate, but I have neither the time nor energy for
disagreeing with the two of you, so I'll just combine them in the same CL.

[Ryan Sleevi, 2015-07-31 02:30:38]

rsleevi@chromium.org changed reviewers:
+ felt@chromium.org

[Ryan Sleevi, 2015-07-31 02:30:40]

OK! Time to pick up this CL again.

John, Peter, Adrienne: This incorporates the changes discussed over email.

I'd suggest the following review order:
components/url_formatter/DEPS
components/url_formatter.gypi
components/url_formatter/BUILD.gn
components/url_formatter/OWNERS

Make sure these look sane first. I'm a little uncomfortable with
url_formatter.gypi depending on //ui/gfx, but that was inherited from
//components/secure_display (by virtue of elide_url). It seems like ElideUrl
should be moved back to chrome/browser/ui, while FormatUrlForSecurityDisplay
kept in //components/url_formatter

Also inherited from secure_display is that the dep on ui/gfx is unconditional,
but the actual dependency is conditional (!Android). Again, this is true for the
existing code today, but it seems like this should either be a conditional dep
(!Android) or move back out to c/b/ui.

Stuart: Heads up on the above discussion, since iOS ends up using
FormatUrlForSecurityDisplay. It appears //ui/gfx is fine for iOS, but I wanted
to confirm this with you.


With the DEPS for url_formatter sorted, it may help to spot check the .gypi/.gn
files. I'm going to take another pass tomorrow to make sure they all make sense,
but it should be mechanical.

Further note: While I realize this is now an even large CL, I combined the
url_formatter introduction and url_fixer move into one CL to avoid having to
churn DEPS. Otherwise, the introduction of url_formatter and moving url_fixer to
align in separate CLs would have seen nearly all the same DEPS files touched,
since many of the places depending on url_formatter already depend on url_fixer.

[Ryan Sleevi, 2015-07-31 02:34:31]

On 2015/06/11 12:42:33, jochen wrote:
> if you want to use this component from html_viewer, you'll need to use a
> separate gyp file for your component. components.gyp pulls in content, so we
can
> depend on it from html_viewer

jochen: I don't see any GYP files for html_viewer (
https://code.google.com/p/chromium/codesearch#search/&q=html_viewer%20file:%5...
); this doesn't apply, right? It only seems to use
//components/html_viewer/BUILD.gn, to which the
//components/url_formatter/BUILD.gn should be sufficient, AFAICT.

[jochen (gone - plz use gerrit), 2015-07-31 07:07:29]

On 2015/07/31 at 02:34:31, rsleevi wrote:
> On 2015/06/11 12:42:33, jochen wrote:
> > if you want to use this component from html_viewer, you'll need to use a
> > separate gyp file for your component. components.gyp pulls in content, so we
can
> > depend on it from html_viewer
> 
> jochen: I don't see any GYP files for html_viewer (
https://code.google.com/p/chromium/codesearch#search/&q=html_viewer%20file:%5...
); this doesn't apply, right? It only seems to use
//components/html_viewer/BUILD.gn, to which the
//components/url_formatter/BUILD.gn should be sufficient, AFAICT.

right, html_viewer (and mandoline) are GN only targets

[jam, 2015-07-31 15:25:12]

lgtm (I just looked at the files you mentioned + content). let me know if you
want me to look at more.

On 2015/07/31 02:30:40, Ryan Sleevi wrote:
> OK! Time to pick up this CL again.
> 
> John, Peter, Adrienne: This incorporates the changes discussed over email.
> 
> I'd suggest the following review order:
> components/url_formatter/DEPS
> components/url_formatter.gypi
> components/url_formatter/BUILD.gn
> components/url_formatter/OWNERS

only one owner (ignoring per-file) isn't good (looks like initially brettw was
in there but he removed himself).

how about making palmer/felt/you also owners in there?

> 
> Make sure these look sane first. I'm a little uncomfortable with
> url_formatter.gypi depending on //ui/gfx, but that was inherited from
> //components/secure_display (by virtue of elide_url). It seems like ElideUrl
> should be moved back to chrome/browser/ui, while FormatUrlForSecurityDisplay
> kept in //components/url_formatter
> 
> Also inherited from secure_display is that the dep on ui/gfx is unconditional,
> but the actual dependency is conditional (!Android). Again, this is true for
the
> existing code today, but it seems like this should either be a conditional dep
> (!Android) or move back out to c/b/ui.

can you fix this in this cl (by making the dependency conditional)?

> 
> Stuart: Heads up on the above discussion, since iOS ends up using
> FormatUrlForSecurityDisplay. It appears //ui/gfx is fine for iOS, but I wanted
> to confirm this with you.
> 
> 
> With the DEPS for url_formatter sorted, it may help to spot check the
.gypi/.gn
> files. I'm going to take another pass tomorrow to make sure they all make
sense,
> but it should be mechanical.
> 
> Further note: While I realize this is now an even large CL, I combined the
> url_formatter introduction and url_fixer move into one CL to avoid having to
> churn DEPS. Otherwise, the introduction of url_formatter and moving url_fixer
to
> align in separate CLs would have seen nearly all the same DEPS files touched,
> since many of the places depending on url_formatter already depend on
url_fixer.

[jam, 2015-07-31 17:25:17]

https://codereview.chromium.org/1171333003/diff/130001/components/url_formatt...
File components/url_formatter/DEPS (right):

https://codereview.chromium.org/1171333003/diff/130001/components/url_formatt...
components/url_formatter/DEPS:7: "-mandoline",
it seems redundant to add these lines, since they're not added by higher level.

[Ryan Sleevi, 2015-07-31 17:30:30]

https://codereview.chromium.org/1171333003/diff/130001/components/url_formatt...
File components/url_formatter/DEPS (right):

https://codereview.chromium.org/1171333003/diff/130001/components/url_formatt...
components/url_formatter/DEPS:7: "-mandoline",
On 2015/07/31 17:25:17, jam wrote:
> it seems redundant to add these lines, since they're not added by higher
level.

See the previous discussion about making sure it was consistent in rejecting any
of the targets. It's absolutely not necessary, just more of an explicit "Don't
do this"

[Peter Kasting, 2015-07-31 20:36:36]

Since I already signed off on the earlier change on on the general direction of
this one, I'm assuming you don't need my re-review.  Let me know if that's not
true.

[jam, 2015-07-31 20:59:05]

On 2015/07/31 17:30:30, Ryan Sleevi wrote:
>
https://codereview.chromium.org/1171333003/diff/130001/components/url_formatt...
> File components/url_formatter/DEPS (right):
> 
>
https://codereview.chromium.org/1171333003/diff/130001/components/url_formatt...
> components/url_formatter/DEPS:7: "-mandoline",
> On 2015/07/31 17:25:17, jam wrote:
> > it seems redundant to add these lines, since they're not added by higher
> level.
> 
> See the previous discussion about making sure it was consistent in rejecting
any
> of the targets. It's absolutely not necessary, just more of an explicit "Don't
> do this"

just read that now. i think it's unnecessary, and the other places shouldn't do
it either. i don't like to see an unnecessary pattern spread but given how minor
it is, i won't worry too much about it.

[Ryan Sleevi, 2015-07-31 21:00:54]

On 2015/07/31 20:36:36, Peter Kasting wrote:
> Since I already signed off on the earlier change on on the general direction
of
> this one, I'm assuming you don't need my re-review.  Let me know if that's not
> true.

Peter: Just wanted to make sure you and John were happy with the DEPS & OWNERs.
The rest is mechanical, but I wanted to make sure you had no concerns.

[Peter Kasting, 2015-07-31 21:13:10]

On 2015/07/31 21:00:54, Ryan Sleevi wrote:
> On 2015/07/31 20:36:36, Peter Kasting wrote:
> > Since I already signed off on the earlier change on on the general direction
> of
> > this one, I'm assuming you don't need my re-review.  Let me know if that's
not
> > true.
> 
> Peter: Just wanted to make sure you and John were happy with the DEPS &
OWNERs.
> The rest is mechanical, but I wanted to make sure you had no concerns.

It would be nice to not be the only owner -- I would put in brettw, and
potentially use a comment to mark him as a backup owner, depending on whether
he's amenable.

[Ryan Sleevi, 2015-07-31 21:57:51]

On 2015/07/31 21:13:10, Peter Kasting wrote:
> It would be nice to not be the only owner -- I would put in brettw, and
> potentially use a comment to mark him as a backup owner, depending on whether
> he's amenable.

Confirmed with Brett that he's comfortable as a (backup) reviewer, and indicated
as such.

The original url_fixer component only had you, and I'm not sure that
myself/felt/palmer are good reviewers for any of the url_fixer/url_formatter
bits (always happy to take prepasses though), so I didn't add us all as OWNERs
for that. The code itself has so rarely changed (other than shifting the deps
around), so hopefully this isn't too onerous - and anyone who starts sending
pkasting lots of reviews for will make a good owner ;)

[Ryan Sleevi, 2015-08-03 19:39:19]

OK, sending this to the CQ now. Brett's comfortable OWNING, and pkasting/jam@
are being used as the overall rubberstamp for the mechanical changes in the
majority of the files.

The only change from what you reviewed was sorting out build deps - making "gn
check" happy and splitting url_formatter into its own .gyp file to avoid a
dependency on content from iOS (the issue jochen@ raised turned out to be
relevant for iOS, though it wasn't for mandoline)

Thanks all for the patience in the overall (large) review.

[Ryan Sleevi, 2015-08-03 19:39:30]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2015-08-03 19:39:31]

The CQ bit was unchecked by rsleevi@chromium.org

[Ryan Sleevi, 2015-08-03 19:39:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
stuartmorgan@chromium.org, pkasting@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1171333003/#ps290001
(title: "Rebased")

[Ryan Sleevi, 2015-08-03 19:39:33]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2015-08-03 19:40:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171333003/290001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1171333003/290001

[commit-bot: I haz the power, 2015-08-03 19:53:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-03 19:53:30]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ryan Sleevi, 2015-08-03 20:32:49]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2015-08-03 20:33:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171333003/290001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1171333003/290001

[commit-bot: I haz the power, 2015-08-03 20:47:22]

Committed patchset #16 (id:290001)

[commit-bot: I haz the power, 2015-08-03 20:47:51]

Patchset 16 (id:??) landed as
https://crrev.com/1659865c3eb47166c82378bb840801135b057a09
Cr-Commit-Position: refs/heads/master@{#341605}

[jochen (gone - plz use gerrit), 2015-08-04 07:03:21]

A revert of this CL (patchset #16 id:290001) has been created in
https://codereview.chromium.org/1260033005/ by jochen@chromium.org.

The reason for reverting is: breaks gn_check on Android:
https://build.chromium.org/p/chromium.linux/builders/Android%20GN/builds/2879....

[Ryan Sleevi, 2015-08-05 23:12:10]

rsleevi@chromium.org changed reviewers:
+ brettw@chromium.org

[Ryan Sleevi, 2015-08-05 23:12:11]

Brett: Huge CL, but just asking you to review .gn and elide_url.cc (diff with PS
18 -
https://codereview.chromium.org/1171333003/diff2/330001:350001/components/url...
) and elide_url_unittest.cc (diff with PS 18 -
https://codereview.chromium.org/1171333003/diff2/330001:350001/components/url...
)

Would you prefer instead of the nogncheck I remove the entire target from .gn?
Or does this work?

[brettw, 2015-08-06 22:25:50]

GN LGTM, this is what the nogncheck is for.

https://codereview.chromium.org/1171333003/diff/350001/components/url_formatt...
File components/url_formatter/BUILD.gn (right):

https://codereview.chromium.org/1171333003/diff/350001/components/url_formatt...
components/url_formatter/BUILD.gn:5: static_library("url_formatter") {
This should be a source_set too.

https://codereview.chromium.org/1171333003/diff/350001/components/url_formatt...
components/url_formatter/BUILD.gn:27: deps -= [ "//ui/gfx" ]
In GYP this is how you would do it, but in GN I prefer the opposite:
  if (!is_android) {
    deps += [ "//ui/gfx" ]
  }
and then don't add it to the list above. Same below.

https://codereview.chromium.org/1171333003/diff/350001/components/url_formatt...
components/url_formatter/BUILD.gn:45: ":url_formatter",
This goes first.

[Ryan Sleevi, 2015-08-06 23:16:32]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2015-08-06 23:16:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, jam@chromium.org, stuartmorgan@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1171333003/#ps390001
(title: "Rebased yet again")

[commit-bot: I haz the power, 2015-08-06 23:17:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171333003/390001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1171333003/390001

[commit-bot: I haz the power, 2015-08-06 23:22:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 23:22:59]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ryan Sleevi, 2015-08-06 23:41:43]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2015-08-06 23:41:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, jam@chromium.org, stuartmorgan@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1171333003/#ps410001
(title: "vim keybinding fail")

[commit-bot: I haz the power, 2015-08-06 23:42:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171333003/410001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1171333003/410001

[commit-bot: I haz the power, 2015-08-07 01:11:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 01:11:08]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ryan Sleevi, 2015-08-07 01:13:11]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2015-08-07 01:14:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171333003/410001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1171333003/410001

[commit-bot: I haz the power, 2015-08-07 01:21:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 01:21:30]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ryan Sleevi, 2015-08-07 01:27:46]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2015-08-07 01:29:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171333003/410001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1171333003/410001

[commit-bot: I haz the power, 2015-08-07 02:19:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 02:19:05]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ryan Sleevi, 2015-08-07 17:37:35]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2015-08-07 17:37:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, jam@chromium.org, stuartmorgan@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1171333003/#ps430001
(title: "Rebase again now that CQ is fixed")

[commit-bot: I haz the power, 2015-08-07 17:38:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171333003/430001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1171333003/430001

[commit-bot: I haz the power, 2015-08-07 19:37:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 19:37:34]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Ryan Sleevi, 2015-08-07 19:41:15]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2015-08-07 19:41:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171333003/430001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1171333003/430001

[commit-bot: I haz the power, 2015-08-07 21:39:34]

Committed patchset #23 (id:430001)

[commit-bot: I haz the power, 2015-08-07 21:40:12]

Patchset 23 (id:??) landed as
https://crrev.com/24f64dc221628032b7843f1af6b277a7f0d6c562
Cr-Commit-Position: refs/heads/master@{#342445}

[Miguel Garcia, 2015-08-10 09:04:35]

Just back from vacation apologies for the late reply.

>Make sure these look sane first. I'm a little uncomfortable with
>url_formatter.gypi depending on //ui/gfx, but that was inherited from
>//components/secure_display (by virtue of elide_url). It seems like ElideUrl
>should be moved back to chrome/browser/ui, while FormatUrlForSecurityDisplay
>kept in //components/url_formatter

Please don't move back ElideUrl to chrome/browser/ui. We want to use
it to elide the host in webpush notifications (which live in //ui) I
am planning to complete this work and we are tracking it in
https://code.google.com/p/chromium/issues/detail?id=509834

I originally moved ElideUrl to it's own directory in //ui exactly
because of the dependency you are worried about but during code review
was strongly advised to make it a component. (see patch set #1 in
https://chromiumcodereview.appspot.com/1233313005/)




On Fri, Aug 7, 2015 at 10:40 PM, commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com>
wrote:
> Patchset 23 (id:??) landed as
> https://crrev.com/24f64dc221628032b7843f1af6b277a7f0d6c562
> Cr-Commit-Position: refs/heads/master@{#342445}
>
> https://codereview.chromium.org/1171333003/

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