Replace usage of WebVector::isEmpty with WebVector::empty().

Replace usage of WebVector::isEmpty with WebVector::empty().

std:: containers expose empty() to check if a contain is empty or not, this change will bring that the WebVector API is inline with these expectations.

BUG=708389

[slangley, 2017-04-05 05:34:43]

slangley@chromium.org changed reviewers:
+ esprehn@chromium.org

[haraken, 2017-04-05 05:41:14]

This CL LGTM.

(That said, I think I should add that removing WebVector would not have a big
benefit because our plan is to replace public APIs with Mojo, where WTF::Vector
is implicitly mapped to std::vector. Once we remove public APIs, WebVector
should be gone as a natural consequence.)

[esprehn, 2017-04-05 06:50:15]

esprehn@chromium.org changed reviewers:
+ haraken@chromium.org

[esprehn, 2017-04-05 06:50:15]

WebVector serves the purpose of preventing people from using std::vector in core
or web. It's used at all layers of blink to pass values out through the public
API. I don't think we should remove it, but we should make it movable into a
std::vector to avoid copies.

Where was this change discussed? I'll defer to other people but personally I
think we shouldn't bring std::vector into core or remove WebVector.

[haraken, 2017-04-05 06:54:15]

On 2017/04/05 06:50:15, esprehn wrote:
> WebVector serves the purpose of preventing people from using std::vector in
core
> or web. It's used at all layers of blink to pass values out through the public
> API. I don't think we should remove it, but we should make it movable into a
> std::vector to avoid copies.
> 
> Where was this change discussed? I'll defer to other people but personally I
> think we shouldn't bring std::vector into core or remove WebVector.

I'm personally okay with replacing WebVector used in public APIs with
std::vector (we should keep using WTF::Vector in Blink of course).

However I don't understand the benefit since our plan is to move more things
from content/ to Blink and thus remove public APIs. If that's our plan, it would
be nicer to invest on that effort instead of investing on a gray-zone
refactoring like this.

[slangley, 2017-04-05 07:13:55]

As WebVector is essentially a wrapper around std::vector it wasn't (and
probably still isn't) clear to me what value this additional level of
abstraction is providing, so in the interests of removing unnecessary
abstractions I thought I'd set about euthanizing it (slowly).

However if there is some other orthogonal plan that would render WebVector
obsolete in the near future anyway then I agree the purpose of this CL is
moot.

On Wed, Apr 5, 2017 at 4:54 PM, <haraken@chromium.org> wrote:

> On 2017/04/05 06:50:15, esprehn wrote:
> > WebVector serves the purpose of preventing people from using std::vector
> in
> core
> > or web. It's used at all layers of blink to pass values out through the
> public
> > API. I don't think we should remove it, but we should make it movable
> into a
> > std::vector to avoid copies.
> >
> > Where was this change discussed? I'll defer to other people but
> personally I
> > think we shouldn't bring std::vector into core or remove WebVector.
>
> I'm personally okay with replacing WebVector used in public APIs with
> std::vector (we should keep using WTF::Vector in Blink of course).
>
> However I don't understand the benefit since our plan is to move more
> things
> from content/ to Blink and thus remove public APIs. If that's our plan, it
> would
> be nicer to invest on that effort instead of investing on a gray-zone
> refactoring like this.
>
>
>
> https://codereview.chromium.org/2799563002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[slangley, 2017-04-05 07:13:57]

As WebVector is essentially a wrapper around std::vector it wasn't (and
probably still isn't) clear to me what value this additional level of
abstraction is providing, so in the interests of removing unnecessary
abstractions I thought I'd set about euthanizing it (slowly).

However if there is some other orthogonal plan that would render WebVector
obsolete in the near future anyway then I agree the purpose of this CL is
moot.

On Wed, Apr 5, 2017 at 4:54 PM, <haraken@chromium.org> wrote:

> On 2017/04/05 06:50:15, esprehn wrote:
> > WebVector serves the purpose of preventing people from using std::vector
> in
> core
> > or web. It's used at all layers of blink to pass values out through the
> public
> > API. I don't think we should remove it, but we should make it movable
> into a
> > std::vector to avoid copies.
> >
> > Where was this change discussed? I'll defer to other people but
> personally I
> > think we shouldn't bring std::vector into core or remove WebVector.
>
> I'm personally okay with replacing WebVector used in public APIs with
> std::vector (we should keep using WTF::Vector in Blink of course).
>
> However I don't understand the benefit since our plan is to move more
> things
> from content/ to Blink and thus remove public APIs. If that's our plan, it
> would
> be nicer to invest on that effort instead of investing on a gray-zone
> refactoring like this.
>
>
>
> https://codereview.chromium.org/2799563002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[esprehn, 2017-04-05 07:14:24]

If the public apis use std::vector then you have to use it in core since
public/platform is used in core and web. Similarly if public/web uses
std::vector you'll have to use it inside web/ where we haven't allowed it
before.

[slangley, 2017-04-05 07:47:20]

Is there a technical reason you would not want to use std::vector directly
in place of WebVector in core/ & web/?



On Wed, Apr 5, 2017 at 5:14 PM, <esprehn@chromium.org> wrote:

> If the public apis use std::vector then you have to use it in core since
> public/platform is used in core and web. Similarly if public/web uses
> std::vector you'll have to use it inside web/ where we haven't allowed it
> before.
>
> https://codereview.chromium.org/2799563002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[slangley, 2017-04-05 07:47:20]

Is there a technical reason you would not want to use std::vector directly
in place of WebVector in core/ & web/?



On Wed, Apr 5, 2017 at 5:14 PM, <esprehn@chromium.org> wrote:

> If the public apis use std::vector then you have to use it in core since
> public/platform is used in core and web. Similarly if public/web uses
> std::vector you'll have to use it inside web/ where we haven't allowed it
> before.
>
> https://codereview.chromium.org/2799563002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[haraken, 2017-04-05 07:49:37]

On 2017/04/05 07:47:20, slangley wrote:
> Is there a technical reason you would not want to use std::vector directly
> in place of WebVector in core/ & web/?

WTF::Vector is optimized for Blink's workflow (e.g., supporting inline
capacity). So:

- We should use WTF::Vector in Blink
- We should use std::vector in Chromium

We need to use something else at the boundary. Currently it is WebVector.

[slangley, 2017-04-05 07:51:32]

Yes I understand there is a separate need that is being met by WTF::
container types.

On Wed, Apr 5, 2017 at 5:49 PM, <haraken@chromium.org> wrote:

> On 2017/04/05 07:47:20, slangley wrote:
> > Is there a technical reason you would not want to use std::vector
> directly
> > in place of WebVector in core/ & web/?
>
> WTF::Vector is optimized for Blink's workflow (e.g., supporting inline
> capacity). So:
>
> - We should use WTF::Vector in Blink
> - We should use std::vector in Chromium
>
> We need to use something else at the boundary. Currently it is WebVector.
>
>
> https://codereview.chromium.org/2799563002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[slangley, 2017-04-05 07:57:02]

Yes I understand there is a separate need that is being met by WTF::
container types.

On Wed, Apr 5, 2017 at 5:49 PM, <haraken@chromium.org> wrote:

> On 2017/04/05 07:47:20, slangley wrote:
> > Is there a technical reason you would not want to use std::vector
> directly
> > in place of WebVector in core/ & web/?
>
> WTF::Vector is optimized for Blink's workflow (e.g., supporting inline
> capacity). So:
>
> - We should use WTF::Vector in Blink
> - We should use std::vector in Chromium
>
> We need to use something else at the boundary. Currently it is WebVector.
>
>
> https://codereview.chromium.org/2799563002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[esprehn, 2017-04-05 08:36:18]

On 2017/04/05 at 07:47:20, slangley wrote:
> Is there a technical reason you would not want to use std::vector directly
> in place of WebVector in core/ & web/?
> 

The reason was social, WebVector has a very limited API surface which was to
discourage it's use.

The long term plan was to onion soup the codebase so we didn't need to pass
stuff through the public API at all.

[esprehn, 2017-04-07 00:06:46]

esprehn@chromium.org changed reviewers:
+ dglazkov@chromium.org

[esprehn, 2017-04-07 00:06:46]

btw I don't have authority here, I was just giving history and why I made this
decision in the past. I defer the decision about what to do here to haraken@,
dglazkov@ and jam@

[esprehn, 2017-04-07 00:06:58]

esprehn@chromium.org changed reviewers:
- esprehn@chromium.org

[dcheng, 2017-04-07 00:11:00]

On 2017/04/07 00:06:58, esprehn wrote:
> mailto:esprehn@chromium.org changed reviewers:
> - mailto:esprehn@chromium.org

FWIW, I'm OK with this renaming change, but I'm not sure if we want to just use
std::vector outright: WebVector lives in platform, so it's used in some places
in core+modules. If we change, we'll end up with std::vector living side-by-side
with WTF::Vector, but with more painful conversions. It seems to me that the
long-term plan of migrating the public API to mojo will probably be the best way
to go.

[haraken, 2017-04-07 00:56:59]

On 2017/04/07 00:11:00, dcheng wrote:
> On 2017/04/07 00:06:58, esprehn wrote:
> > mailto:esprehn@chromium.org changed reviewers:
> > - mailto:esprehn@chromium.org
> 
> FWIW, I'm OK with this renaming change, but I'm not sure if we want to just
use
> std::vector outright: WebVector lives in platform, so it's used in some places
> in core+modules. If we change, we'll end up with std::vector living
side-by-side
> with WTF::Vector, but with more painful conversions. It seems to me that the
> long-term plan of migrating the public API to mojo will probably be the best
way
> to go.

Just to clarify, we have decided not to remove WebVector. We should instead
invest on removing public APIs.

Though landing this renaming CL looks fine.

[slangley, 2017-04-07 01:35:12]

It seems the overall sentiment is that there are various not immediately
obvious reasons to keep WebVector, and I am happy to abide by the opinions
of the more experienced engineers.

Having API compatibility with std::vector is nice to have, but not urgent.
I am happy to gradually converge the API of WebVector if there's no
substantial pushback.



On Fri, Apr 7, 2017 at 10:56 AM, <haraken@chromium.org> wrote:

> On 2017/04/07 00:11:00, dcheng wrote:
> > On 2017/04/07 00:06:58, esprehn wrote:
> > > mailto:esprehn@chromium.org changed reviewers:
> > > - mailto:esprehn@chromium.org
> >
> > FWIW, I'm OK with this renaming change, but I'm not sure if we want to
> just
> use
> > std::vector outright: WebVector lives in platform, so it's used in some
> places
> > in core+modules. If we change, we'll end up with std::vector living
> side-by-side
> > with WTF::Vector, but with more painful conversions. It seems to me that
> the
> > long-term plan of migrating the public API to mojo will probably be the
> best
> way
> > to go.
>
> Just to clarify, we have decided not to remove WebVector. We should instead
> invest on removing public APIs.
>
> Though landing this renaming CL looks fine.
>
>
>
> https://codereview.chromium.org/2799563002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haraken, 2017-04-07 01:38:31]

On 2017/04/07 01:35:12, slangley wrote:
> It seems the overall sentiment is that there are various not immediately
> obvious reasons to keep WebVector, and I am happy to abide by the opinions
> of the more experienced engineers.
> 
> Having API compatibility with std::vector is nice to have, but not urgent.
> I am happy to gradually converge the API of WebVector if there's no
> substantial pushback.
> 

Just to clarify, my point is that this is a kind of gray-zone refactoring. There
is some pros (as you described) and cons (as Elliott and Dcheng described). A
gray-zone refactoring is not a good thing. We should invest on more important
things.

[slangley, 2017-04-07 01:41:55]

It seems the overall sentiment is that there are various not immediately
obvious reasons to keep WebVector, and I am happy to abide by the opinions
of the more experienced engineers.

Having API compatibility with std::vector is nice to have, but not urgent.
I am happy to gradually converge the API of WebVector if there's no
substantial pushback.



On Fri, Apr 7, 2017 at 10:56 AM, <haraken@chromium.org> wrote:

> On 2017/04/07 00:11:00, dcheng wrote:
> > On 2017/04/07 00:06:58, esprehn wrote:
> > > mailto:esprehn@chromium.org changed reviewers:
> > > - mailto:esprehn@chromium.org
> >
> > FWIW, I'm OK with this renaming change, but I'm not sure if we want to
> just
> use
> > std::vector outright: WebVector lives in platform, so it's used in some
> places
> > in core+modules. If we change, we'll end up with std::vector living
> side-by-side
> > with WTF::Vector, but with more painful conversions. It seems to me that
> the
> > long-term plan of migrating the public API to mojo will probably be the
> best
> way
> > to go.
>
> Just to clarify, we have decided not to remove WebVector. We should instead
> invest on removing public APIs.
>
> Though landing this renaming CL looks fine.
>
>
>
> https://codereview.chromium.org/2799563002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[slangley, 2017-04-07 01:42:57]

Description was changed from

==========
Replace usage of WebVector::isEmpty with WebVector::empty().

This work is to move WebVector to being compliant with the std::vector API,
which is preliminary work to removing WebVector entirely and replace it with
std::vector (which is currently shims anyway).

BUG=708389
==========

to

==========
Replace usage of WebVector::isEmpty with WebVector::empty().

std:: containers expose empty() to check if a contain is empty or not, this
change will bring that the WebVector API is inline with these expectations.

BUG=708389
==========

[slangley, 2017-04-07 01:45:32]

ptal.

I updated the bug & description to highlight that this is purely to make the API
consistent with what's exposed by std:: container types, and there is no
intention to kill WebVector.

[haraken, 2017-04-07 01:49:19]

On 2017/04/07 01:45:32, slangley wrote:
> ptal.
> 
> I updated the bug & description to highlight that this is purely to make the
API
> consistent with what's exposed by std:: container types, and there is no
> intention to kill WebVector.

LGTM

[slangley, 2017-04-07 02:47:43]

slangley@chromium.org changed reviewers:
+ esprehn@chromium.org, pfeldman@chromium.org

[slangley, 2017-04-07 02:47:43]

+esprehn@ - for content/child & content/renderer OWNERS.
+pfeldman@ - for content/shell/test_runner.

[esprehn, 2017-04-07 10:27:05]

esprehn@chromium.org changed reviewers:
- esprehn@chromium.org

[pfeldman, 2017-04-07 18:20:54]

shell lgtm