Move elide_url to its own component

Move elide_url to its own component

So it can be used for elements in the ui/ directory for example.
In particular there will be a follow up to use it from
https://codereview.chromium.org/1235253009/

TBR=cbenzel,danakj
BUG=509834
Committed: https://crrev.com/dbcfb120974345eb846b92bf224fef89426b30cf
Cr-Commit-Position: refs/heads/master@{#340068}

[Miguel Garcia, 2015-07-16 16:29:41]

miguelg@chromium.org changed reviewers:
+ palmer@chromium.org

[Miguel Garcia, 2015-07-16 16:29:42]

Some trybots will fail (the patch needs a rebase and I have not even ran the GN
version yet) but I wanted to get some early feedback to see if this is a good
idea.

The immediate motivation for this is that we want to elide notifications and
rather than hack it around it seemed cleaner to move this to ui/ so it can be
used outside the chromium ui per se.

[tfarina, 2015-07-16 16:52:28]

tfarina@chromium.org changed reviewers:
+ tfarina@chromium.org

[tfarina, 2015-07-16 16:52:29]

Does this needs to be in ui/? Can we have it in components/ instead? If not,
why?

There are some precedent in components/, as we have components like url_fixer
and url_matcher.

I think Peter would be also a good candidate to review this. I think he is very
familiar with Elide code. Also could you get Sadrul to review ui/?

Thanks,

[noyau (Ping after 24h), 2015-07-16 17:00:25]

On 2015/07/16 16:29:42, Miguel Garcia wrote:
> Some trybots will fail (the patch needs a rebase and I have not even ran the
GN
> version yet) but I wanted to get some early feedback to see if this is a good
> idea.
> 
> The immediate motivation for this is that we want to elide notifications and
> rather than hack it around it seemed cleaner to move this to ui/ so it can be
> used outside the chromium ui per se.

elide is used on iOS, and it would help greatly to move it to components/
instead of ui/

[palmer, 2015-07-16 17:42:58]

This LG to M overall, but I agree that components/ is probably the best place
for it (since more callers can call it).

https://codereview.chromium.org/1233313005/diff/1/ui/secure_display/OWNERS
File ui/secure_display/OWNERS (right):

https://codereview.chromium.org/1233313005/diff/1/ui/secure_display/OWNERS#ne...
ui/secure_display/OWNERS:1: palmer@chromium.org
Add felt, too.

[Miguel Garcia, 2015-07-21 17:38:42]

Ok, I created a component out of this, there are a few changes worth noticing

Added secure_display_export.h so that the component builds keep working. This
seems to be a pattern in most components
Ifdefed the functions in elide_url.h that should not be run on android.
ElideHost and ElideUrl requires gfx::GetStringWidthF which is not implemented in
android
Nothing failed there because on android nothing is yet using it.

FormatUrlForSecurityDisplay is in fact used in android and now the unittest runs
in the android bots as well.

Added felt to the owners file
 

On 2015/07/16 17:42:58, palmer wrote:
> This LG to M overall, but I agree that components/ is probably the best place
> for it (since more callers can call it).
> 
> https://codereview.chromium.org/1233313005/diff/1/ui/secure_display/OWNERS
> File ui/secure_display/OWNERS (right):
> 
>
https://codereview.chromium.org/1233313005/diff/1/ui/secure_display/OWNERS#ne...
> ui/secure_display/OWNERS:1: mailto:palmer@chromium.org
> Add felt, too.

[Miguel Garcia, 2015-07-21 17:41:22]

miguelg@chromium.org changed reviewers:
+ sdefresne@chromium.org

[Miguel Garcia, 2015-07-21 17:41:23]

[+sdefresne] for component/OWNERS

[Miguel Garcia, 2015-07-21 17:50:14]

miguelg@chromium.org changed reviewers:
+ jochen@chromium.org
- sdefresne@chromium.org

[Miguel Garcia, 2015-07-21 17:50:15]

Changed main OWNER to jochen since he has both chrome and components ownership.

Sylvain please feel free to comment as well if you prefer to.

[Miguel Garcia, 2015-07-21 18:02:48]

CC felt so she knows she will now become the proud owner of a postmortem

[Miguel Garcia, 2015-07-21 18:47:40]

erm I meant component, too many things at the same time.

On Tue, Jul 21, 2015 at 7:02 PM, <miguelg@chromium.org> wrote:

> CC felt so she knows she will now become the proud owner of a postmortem
>
> https://codereview.chromium.org/1233313005/
>

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

[palmer, 2015-07-21 20:27:17]

Awesome, thank you! LGTM.

[jochen (gone - plz use gerrit), 2015-07-22 09:31:55]

lgtm

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
File components/secure_display/BUILD.gn (right):

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
components/secure_display/BUILD.gn:3: source_set("secure_display") {
i guess you mean component() here? source_set is "static_library" in gyp

[noyau (Ping after 24h), 2015-07-22 09:43:59]

noyau@chromium.org changed reviewers:
+ noyau@chromium.org

[noyau (Ping after 24h), 2015-07-22 09:44:00]

https://codereview.chromium.org/1233313005/diff/120001/components/secure_disp...
File components/secure_display/elide_url.cc (right):

https://codereview.chromium.org/1233313005/diff/120001/components/secure_disp...
components/secure_display/elide_url.cc:108: #endif
nit

#endif // !defined(OS_ANDROID)

https://codereview.chromium.org/1233313005/diff/120001/components/secure_disp...
File components/secure_display/elide_url.h (right):

https://codereview.chromium.org/1233313005/diff/120001/components/secure_disp...
components/secure_display/elide_url.h:71: }  // namespace
nit:

// namespace secure_display

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
File components/secure_display.gypi (right):

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
components/secure_display.gypi:10: 'type': '<(component)',
Most simple components are of type static_library, is this code really worth
another DLL on Windows?

sdefresne@, is there a doc somewhere explicitly explaining the pro and cons of
components vs static libs in components/ ?

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
File components/secure_display/elide_url.cc (right):

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
components/secure_display/elide_url.cc:312: const std::string& languages) {
Instead of ifdefing most of the file for this one function, can't this function
be moved to another file and this file excluded from compilation on Android?

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
File components/secure_display/elide_url.h (right):

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
components/secure_display/elide_url.h:53: #endif
nit: everywhere you added a #define please recall the condition in the #endif.

#endif // !defined(OS_ANDROID)

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
components/secure_display/elide_url.h:71: }  // namespace
nit: Please repeat the name of the namespace in the closing comment

}  // namespace secure_display

[Miguel Garcia, 2015-07-22 12:33:07]

https://codereview.chromium.org/1233313005/diff/120001/components/secure_disp...
File components/secure_display/elide_url.cc (right):

https://codereview.chromium.org/1233313005/diff/120001/components/secure_disp...
components/secure_display/elide_url.cc:108: #endif
On 2015/07/22 09:44:00, noyau wrote:
> nit
> 
> #endif // !defined(OS_ANDROID)

Done.

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
File components/secure_display.gypi (right):

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
components/secure_display.gypi:10: 'type': '<(component)',
On 2015/07/22 09:44:00, noyau wrote:
> Most simple components are of type static_library, is this code really worth
> another DLL on Windows?
> 
> sdefresne@, is there a doc somewhere explicitly explaining the pro and cons of
> components vs static libs in components/ ?

I moved the gn file to be component as well to be consistent happy to change it
(now or in a separate cl) if sdefresne has feedback.

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
File components/secure_display/BUILD.gn (right):

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
components/secure_display/BUILD.gn:3: source_set("secure_display") {
On 2015/07/22 09:31:55, jochen wrote:
> i guess you mean component() here? source_set is "static_library" in gyp 
indeed I meant component, that said I'd like to know exactly what the different
implications are since it works both ways

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
File components/secure_display/elide_url.cc (right):

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
components/secure_display/elide_url.cc:312: const std::string& languages) {
On 2015/07/22 09:44:00, noyau wrote:
> Instead of ifdefing most of the file for this one function, can't this
function
> be moved to another file and this file excluded from compilation on Android?

We eventually want this to work in Android as well so I think I am going to
leave it as is for now.

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
File components/secure_display/elide_url.h (right):

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
components/secure_display/elide_url.h:53: #endif
On 2015/07/22 09:44:00, noyau wrote:
> nit: everywhere you added a #define please recall the condition in the #endif.
> 
> #endif // !defined(OS_ANDROID)
> 

Done. I also fixed the previous ifdefs in these files.

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
components/secure_display/elide_url.h:71: }  // namespace
On 2015/07/22 09:44:00, noyau wrote:
> nit: Please repeat the name of the namespace in the closing comment
> 
> }  // namespace secure_display
>  

Done.

[Miguel Garcia, 2015-07-22 12:33:13]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2015-07-22 12:33:14]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1233313005/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-07-22 12:33:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233313005/200001

[noyau (Ping after 24h), 2015-07-22 12:44:34]

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
File components/secure_display/elide_url.h (right):

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
components/secure_display/elide_url.h:71: }  // namespace
On 2015/07/22 12:33:07, Miguel Garcia wrote:
> On 2015/07/22 09:44:00, noyau wrote:
> > nit: Please repeat the name of the namespace in the closing comment
> > 
> > }  // namespace secure_display
> >  
> 
> Done.

Apparently not…

[Miguel Garcia, 2015-07-22 12:45:31]

The CQ bit was unchecked by miguelg@chromium.org

[commit-bot: I haz the power, 2015-07-22 12:45:32]

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

[commit-bot: I haz the power, 2015-07-22 12:45:33]

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...)

[Miguel Garcia, 2015-07-22 12:56:01]

On 2015/07/22 12:44:34, noyau wrote:
>
https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
> File components/secure_display/elide_url.h (right):
> 
>
https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
> components/secure_display/elide_url.h:71: }  // namespace
> On 2015/07/22 12:33:07, Miguel Garcia wrote:
> > On 2015/07/22 09:44:00, noyau wrote:
> > > nit: Please repeat the name of the namespace in the closing comment
> > > 
> > > }  // namespace secure_display
> > >  
> > 
> > Done.
> 
> Apparently not…

My bad! will fix before landing. Sorry about that.

[Miguel Garcia, 2015-07-22 12:58:04]

miguelg@chromium.org changed reviewers:
+ danakj@chromium.org

[Miguel Garcia, 2015-07-22 12:58:05]

It seems I need to add owners for the different deps I am adding.

I can't imagine why someone will have a problem with base/ or gurl/ so I am
TBRing those.

Adding:
danakj for ui/gfx and cbentzel for net base

[noyau (Ping after 24h), 2015-07-22 13:26:53]

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
File components/secure_display.gypi (right):

https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
components/secure_display.gypi:10: 'type': '<(component)',
On 2015/07/22 12:33:06, Miguel Garcia wrote:
> On 2015/07/22 09:44:00, noyau wrote:
> > Most simple components are of type static_library, is this code really worth
> > another DLL on Windows?
> > 
> > sdefresne@, is there a doc somewhere explicitly explaining the pro and cons
of
> > components vs static libs in components/ ?
> 
> I moved the gn file to be component as well to be consistent happy to change
it
> (now or in a separate cl) if sdefresne has feedback.

From Sylvain, himself quoting from brettw on chromium-dev:
> A component requires linking. A GN source set requires no linking.
> 
> The only reason components are desirable in the first place is that some
> bigger things can be linked together and not require a full rebuild of
> chrome when one thing changes.
>
> IMO we already have too many components. A component should be typically 
> something on the level of a toplevel directory like net or base.
>
> So, my understanding is that code in //components should probably be just a 
> bunch of static_library/source_set.

Please make this a static library.

[Miguel Garcia, 2015-07-22 14:58:31]

Thanks for the extra info, will make it a static library before landing it

On Wed, Jul 22, 2015 at 2:26 PM, <noyau@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
> File components/secure_display.gypi (right):
>
>
>
https://codereview.chromium.org/1233313005/diff/140001/components/secure_disp...
> components/secure_display.gypi:10: 'type': '<(component)',
> On 2015/07/22 12:33:06, Miguel Garcia wrote:
>
>> On 2015/07/22 09:44:00, noyau wrote:
>> > Most simple components are of type static_library, is this code
>>
> really worth
>
>> > another DLL on Windows?
>> >
>> > sdefresne@, is there a doc somewhere explicitly explaining the pro
>>
> and cons of
>
>> > components vs static libs in components/ ?
>>
>
>  I moved the gn file to be component as well to be consistent happy to
>>
> change it
>
>> (now or in a separate cl) if sdefresne has feedback.
>>
>
> From Sylvain, himself quoting from brettw on chromium-dev:
>
>> A component requires linking. A GN source set requires no linking.
>>
>
>  The only reason components are desirable in the first place is that
>>
> some
>
>> bigger things can be linked together and not require a full rebuild of
>> chrome when one thing changes.
>>
>
>  IMO we already have too many components. A component should be
>>
> typically
>
>> something on the level of a toplevel directory like net or base.
>>
>
>  So, my understanding is that code in //components should probably be
>>
> just a
>
>> bunch of static_library/source_set.
>>
>
> Please make this a static library.
>
> https://codereview.chromium.org/1233313005/
>

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

[noyau (Ping after 24h), 2015-07-22 15:03:51]

On 2015/07/22 14:58:31, Miguel Garcia wrote:
> Thanks for the extra info, will make it a static library before landing it
> 
Note that for a static library you can remove all the export header stuff you
had to create.

[jochen (gone - plz use gerrit), 2015-07-22 15:08:12]

On 2015/07/22 at 15:03:51, noyau wrote:
> On 2015/07/22 14:58:31, Miguel Garcia wrote:
> > Thanks for the extra info, will make it a static library before landing it
> > 
> Note that for a static library you can remove all the export header stuff you
had to create.

https://www.chromium.org/developers/how-tos/component-build has some more info

lots of components allow for faster incremental compile times - while the source
set itself isn't linked together, it'll still have to be linked into the
resulting binaries, while a shared library doesn't have to be relinked if it
didn't change

[Miguel Garcia, 2015-07-22 18:29:55]

miguelg@chromium.org changed reviewers:
+ cbentzel@chromium.org

[Miguel Garcia, 2015-07-22 18:29:56]

+cbentzel for the net/base DEPS

[Miguel Garcia, 2015-07-23 10:05:25]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2015-07-23 10:05:26]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1233313005/#ps220001
(title: "move to static library")

[commit-bot: I haz the power, 2015-07-23 10:05:45]

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

[cbentzel, 2015-07-23 10:11:39]

On 2015/07/23 10:05:45, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1233313005/220001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1233313005/220001

I don't see any net/base/DEPS change. Am I missing something?

[noyau (Ping after 24h), 2015-07-23 10:44:08]

On 2015/07/23 10:11:39, cbentzel wrote:
> On 2015/07/23 10:05:45, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1233313005/220001
> > View timeline at
> >  https://chromium-cq-status.appspot.com/patch-timeline/1233313005/220001
> 
> I don't see any net/base/DEPS change. Am I missing something?

The deps for the new component adds net/base, and it needs a net/base owner to
approve its use.

[commit-bot: I haz the power, 2015-07-23 10:45:19]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-07-23 10:45:49]

Patchset 12 (id:??) landed as
https://crrev.com/dbcfb120974345eb846b92bf224fef89426b30cf
Cr-Commit-Position: refs/heads/master@{#340068}

[noyau (Ping after 24h), 2015-07-23 11:13:57]

https://codereview.chromium.org/1233313005/diff/220001/components/secure_disp...
File components/secure_display.gypi (right):

https://codereview.chromium.org/1233313005/diff/220001/components/secure_disp...
components/secure_display.gypi:18: 'SECURE_DISPLAY_IMPLEMENTATION',
Is this define still needed?

[Miguel Garcia, 2015-07-23 14:34:50]

On 2015/07/23 11:13:57, noyau wrote:
>
https://codereview.chromium.org/1233313005/diff/220001/components/secure_disp...
> File components/secure_display.gypi (right):
> 
>
https://codereview.chromium.org/1233313005/diff/220001/components/secure_disp...
> components/secure_display.gypi:18: 'SECURE_DISPLAY_IMPLEMENTATION',
> Is this define still needed?

It's not, thanks a lot for checking.