ImageResource: remove unnecessary vector copying during iteration.

ImageResource: remove unnecessary vector copying during iteration.

Iterations that don't update the underlying collection, can be done
in-place.

R=
BUG=

Committed: https://crrev.com/af6993cee55aaf307b3f73e4e0ca5f05aa0413f4
Cr-Commit-Position: refs/heads/master@{#437482}

[sof, 2016-12-07 08:33:42]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 08:33:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2016-12-07 09:00:02]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 09:00:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2016-12-07 09:11:42]

sigbjornf@opera.com changed reviewers:
+ hiroshige@chromium.org, tkent@chromium.org

[sof, 2016-12-07 09:11:42]

please take a look.

I've been trying to understand these asVector() uses for a while, but have
failed -- no particular reason to have these, right?

[commit-bot: I haz the power, 2016-12-07 11:32:35]

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

[commit-bot: I haz the power, 2016-12-07 11:32:36]

Dry run: This issue passed the CQ dry run.

[tkent, 2016-12-07 23:29:19]

tkent@chromium.org changed reviewers:
+ yhirano@chromium.org

[tkent, 2016-12-07 23:38:43]

+yhirano

On 2016/12/07 at 09:11:42, sigbjornf wrote:
> I've been trying to understand these asVector() uses for a while, but have
failed -- no particular reason to have these, right?

It looks safe to remove asVector() with the current ImageResourceObserver
implementations.  However, an ImageResourceObserver implementation might break
the assumption in the future.  The defensive copy isn't bad.  I'd like to
entrust this to an owner of core/fetch.

[yhirano, 2016-12-08 01:59:49]

On 2016/12/07 23:38:43, tkent wrote:
> +yhirano
> 
> On 2016/12/07 at 09:11:42, sigbjornf wrote:
> > I've been trying to understand these asVector() uses for a while, but have
> failed -- no particular reason to have these, right?
> 
> It looks safe to remove asVector() with the current ImageResourceObserver
> implementations.  However, an ImageResourceObserver implementation might break
> the assumption in the future.  The defensive copy isn't bad.  I'd like to
> entrust this to an owner of core/fetch.

We copy a collection before iterating-and-calling a method on the collection
because the method may mutate the collection (especially via addClient /
removeClient / addObserver / removeObserver).

[sof, 2016-12-08 06:30:50]

On 2016/12/08 01:59:49, yhirano wrote:
> On 2016/12/07 23:38:43, tkent wrote:
> > +yhirano
> > 
> > On 2016/12/07 at 09:11:42, sigbjornf wrote:
> > > I've been trying to understand these asVector() uses for a while, but have
> > failed -- no particular reason to have these, right?
> > 
> > It looks safe to remove asVector() with the current ImageResourceObserver
> > implementations.  However, an ImageResourceObserver implementation might
break
> > the assumption in the future.  The defensive copy isn't bad.  I'd like to
> > entrust this to an owner of core/fetch.
> 
> We copy a collection before iterating-and-calling a method on the collection
> because the method may mutate the collection (especially via addClient /
> removeClient / addObserver / removeObserver).

Sure, and checkModifications() on HashTable iterators catch when that doesn't
hold. Which of the predicates in these loops perform side-effects?

[yhirano, 2016-12-08 06:50:05]

On 2016/12/08 06:30:50, sof wrote:
> On 2016/12/08 01:59:49, yhirano wrote:
> > On 2016/12/07 23:38:43, tkent wrote:
> > > +yhirano
> > > 
> > > On 2016/12/07 at 09:11:42, sigbjornf wrote:
> > > > I've been trying to understand these asVector() uses for a while, but
have
> > > failed -- no particular reason to have these, right?
> > > 
> > > It looks safe to remove asVector() with the current ImageResourceObserver
> > > implementations.  However, an ImageResourceObserver implementation might
> break
> > > the assumption in the future.  The defensive copy isn't bad.  I'd like to
> > > entrust this to an owner of core/fetch.
> > 
> > We copy a collection before iterating-and-calling a method on the collection
> > because the method may mutate the collection (especially via addClient /
> > removeClient / addObserver / removeObserver).
> 
> Sure, and checkModifications() on HashTable iterators catch when that doesn't
> hold. Which of the predicates in these loops perform side-effects?

I didn't know checkModifications(), thanks. Currently I don't understand all
ImageResourceObserver subclasses implementations, but it might make sense to
forbid addObserver / removeObserver in some functions. If we do so, such
restrictions must be explicitly stated in ImageResourceObserver.h.

[sof, 2016-12-08 07:09:59]

On 2016/12/08 06:50:05, yhirano wrote:
> On 2016/12/08 06:30:50, sof wrote:
> > On 2016/12/08 01:59:49, yhirano wrote:
> > > On 2016/12/07 23:38:43, tkent wrote:
> > > > +yhirano
> > > > 
> > > > On 2016/12/07 at 09:11:42, sigbjornf wrote:
> > > > > I've been trying to understand these asVector() uses for a while, but
> have
> > > > failed -- no particular reason to have these, right?
> > > > 
> > > > It looks safe to remove asVector() with the current
ImageResourceObserver
> > > > implementations.  However, an ImageResourceObserver implementation might
> > break
> > > > the assumption in the future.  The defensive copy isn't bad.  I'd like
to
> > > > entrust this to an owner of core/fetch.
> > > 
> > > We copy a collection before iterating-and-calling a method on the
collection
> > > because the method may mutate the collection (especially via addClient /
> > > removeClient / addObserver / removeObserver).
> > 
> > Sure, and checkModifications() on HashTable iterators catch when that
doesn't
> > hold. Which of the predicates in these loops perform side-effects?
> 
> I didn't know checkModifications(), thanks. Currently I don't understand all
> ImageResourceObserver subclasses implementations, but it might make sense to
> forbid addObserver / removeObserver in some functions. If we do so, such
> restrictions must be explicitly stated in ImageResourceObserver.h.

While such a clarification is separately worthwhile, the copying that this CL
removes, is redundant and overly defensive.

[yhirano, 2016-12-08 07:14:34]

On 2016/12/08 07:09:59, sof wrote:
> On 2016/12/08 06:50:05, yhirano wrote:
> > On 2016/12/08 06:30:50, sof wrote:
> > > On 2016/12/08 01:59:49, yhirano wrote:
> > > > On 2016/12/07 23:38:43, tkent wrote:
> > > > > +yhirano
> > > > > 
> > > > > On 2016/12/07 at 09:11:42, sigbjornf wrote:
> > > > > > I've been trying to understand these asVector() uses for a while,
but
> > have
> > > > > failed -- no particular reason to have these, right?
> > > > > 
> > > > > It looks safe to remove asVector() with the current
> ImageResourceObserver
> > > > > implementations.  However, an ImageResourceObserver implementation
might
> > > break
> > > > > the assumption in the future.  The defensive copy isn't bad.  I'd like
> to
> > > > > entrust this to an owner of core/fetch.
> > > > 
> > > > We copy a collection before iterating-and-calling a method on the
> collection
> > > > because the method may mutate the collection (especially via addClient /
> > > > removeClient / addObserver / removeObserver).
> > > 
> > > Sure, and checkModifications() on HashTable iterators catch when that
> doesn't
> > > hold. Which of the predicates in these loops perform side-effects?
> > 
> > I didn't know checkModifications(), thanks. Currently I don't understand all
> > ImageResourceObserver subclasses implementations, but it might make sense to
> > forbid addObserver / removeObserver in some functions. If we do so, such
> > restrictions must be explicitly stated in ImageResourceObserver.h.
> 
> While such a clarification is separately worthwhile, the copying that this CL
> removes, is redundant and overly defensive.

What this CL is doing is changing contracts between ImageResource and
ImageResourceObserver. That must be explicitly stated in the public interface,
not only in the implementation.

[sof, 2016-12-08 07:27:03]

On 2016/12/08 07:14:34, yhirano wrote:
> On 2016/12/08 07:09:59, sof wrote:
> > On 2016/12/08 06:50:05, yhirano wrote:
> > > On 2016/12/08 06:30:50, sof wrote:
> > > > On 2016/12/08 01:59:49, yhirano wrote:
> > > > > On 2016/12/07 23:38:43, tkent wrote:
> > > > > > +yhirano
> > > > > > 
> > > > > > On 2016/12/07 at 09:11:42, sigbjornf wrote:
> > > > > > > I've been trying to understand these asVector() uses for a while,
> but
> > > have
> > > > > > failed -- no particular reason to have these, right?
> > > > > > 
> > > > > > It looks safe to remove asVector() with the current
> > ImageResourceObserver
> > > > > > implementations.  However, an ImageResourceObserver implementation
> might
> > > > break
> > > > > > the assumption in the future.  The defensive copy isn't bad.  I'd
like
> > to
> > > > > > entrust this to an owner of core/fetch.
> > > > > 
> > > > > We copy a collection before iterating-and-calling a method on the
> > collection
> > > > > because the method may mutate the collection (especially via addClient
/
> > > > > removeClient / addObserver / removeObserver).
> > > > 
> > > > Sure, and checkModifications() on HashTable iterators catch when that
> > doesn't
> > > > hold. Which of the predicates in these loops perform side-effects?
> > > 
> > > I didn't know checkModifications(), thanks. Currently I don't understand
all
> > > ImageResourceObserver subclasses implementations, but it might make sense
to
> > > forbid addObserver / removeObserver in some functions. If we do so, such
> > > restrictions must be explicitly stated in ImageResourceObserver.h.
> > 
> > While such a clarification is separately worthwhile, the copying that this
CL
> > removes, is redundant and overly defensive.
> 
> What this CL is doing is changing contracts between ImageResource and
> ImageResourceObserver. That must be explicitly stated in the public interface,
> not only in the implementation.

Done.

[yhirano, 2016-12-08 07:29:21]

https://codereview.chromium.org/2555103004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResourceObserver.h (right):

https://codereview.chromium.org/2555103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResourceObserver.h:63: virtual
ResourcePriority computeResourcePriority() const {
This function has the same restriction.

[sof, 2016-12-08 07:32:51]

https://codereview.chromium.org/2555103004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResourceObserver.h (right):

https://codereview.chromium.org/2555103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResourceObserver.h:63: virtual
ResourcePriority computeResourcePriority() const {
On 2016/12/08 07:29:21, yhirano wrote:
> This function has the same restriction.

Done.

[yhirano, 2016-12-08 07:34:09]

lgtm

[sof, 2016-12-08 07:34:28]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-12-08 07:34:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 08:56:35]

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

[commit-bot: I haz the power, 2016-12-08 08:56:36]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sof, 2016-12-08 09:02:51]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-12-08 09:03:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 10:37:27]

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

[commit-bot: I haz the power, 2016-12-08 10:37:28]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sof, 2016-12-08 10:45:08]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-12-08 10:45:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2016-12-08 10:46:01]

lgtm. I think we should add assertions that these callbacks actually does not
add/remove observers, but I'd like to do it later (after I modifies
ImageResource largely probably next week:
https://codereview.chromium.org/2469873002/)

One trybot is consistently failing, but the tests are already failing/flaky
without this CL?

[sof, 2016-12-08 10:52:51]

On 2016/12/08 10:46:01, hiroshige wrote:
> lgtm. I think we should add assertions that these callbacks actually does not
> add/remove observers, but I'd like to do it later (after I modifies
> ImageResource largely probably next week:
> https://codereview.chromium.org/2469873002/)
> 

ok, it wouldn't hurt, but the likelihood it happening is low for these methods.
Having HashTable enforce it on updates during iteration has proven to have too
large an overhead.

> One trybot is consistently failing, but the tests are already failing/flaky
> without this CL?

It's flaky for all atm.

[sof, 2016-12-08 11:00:18]

On 2016/12/08 10:52:51, sof wrote:
> On 2016/12/08 10:46:01, hiroshige wrote:
> > lgtm. I think we should add assertions that these callbacks actually does
not
> > add/remove observers, but I'd like to do it later (after I modifies
> > ImageResource largely probably next week:
> > https://codereview.chromium.org/2469873002/)
> > 
> 
> ok, it wouldn't hurt, but the likelihood it happening is low for these
methods.
> Having HashTable enforce it on updates during iteration has proven to have too
> large an overhead.
> 
> > One trybot is consistently failing, but the tests are already failing/flaky
> > without this CL?
> 
> It's flaky for all atm.

It'd be useful to at least document which imageChanged() observer
implementations depend on (un)registering observers.

[commit-bot: I haz the power, 2016-12-08 12:06:24]

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

[commit-bot: I haz the power, 2016-12-08 12:06:26]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[hiroshige, 2016-12-08 14:33:37]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2016-12-08 14:34:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 15:54:02]

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

[commit-bot: I haz the power, 2016-12-08 15:54:03]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sof, 2016-12-08 16:23:34]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-12-08 16:24:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 17:55:05]

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

[commit-bot: I haz the power, 2016-12-08 17:55:06]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sof, 2016-12-09 06:15:00]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-12-09 06:15:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 06:20:55]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481264100264290,
"parent_rev": "fcc8ae27cce0a55d64344e65c2d396d8c34027bb", "commit_rev":
"a8743ac9d6898fc7707efa839bd99587af96300c"}

[commit-bot: I haz the power, 2016-12-09 06:21:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-09 06:24:24]

Description was changed from

==========
ImageResource: remove unnecessary vector copying during iteration.

Iterations that don't update the underlying collection, can be done
in-place.

R=
BUG=
==========

to

==========
ImageResource: remove unnecessary vector copying during iteration.

Iterations that don't update the underlying collection, can be done
in-place.

R=
BUG=

Committed: https://crrev.com/af6993cee55aaf307b3f73e4e0ca5f05aa0413f4
Cr-Commit-Position: refs/heads/master@{#437482}
==========

[commit-bot: I haz the power, 2016-12-09 06:24:25]

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