[Extensions] Convert APIs to use movable types [8]

[Extensions] Convert APIs to use movable types [8]

Generated extensions code used to rely on linked_ptrs for non-copyable types in
containers. Now, we can make these types movable. Update the following APIs
to use movable types in this CL:
- omnibox.json
- page_action.json
- page_capture.json
- passwords_private.idl
- permissions.json
- platform_keys.idl
- platform_keys_internal.idl
- preferences_private.json
- privacy.json

BUG=595949
TBR=mpearson@chromium.org (trivial c/b/autocomplete/ change)

Committed: https://crrev.com/7edeeeeb5245c12eba7c32cd683f1fb6b159d1b9
Cr-Commit-Position: refs/heads/master@{#383829}

[Devlin, 2016-03-25 13:33:02]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-03-25 13:33:11]

Patchset #1 (id:20001) has been deleted

[Devlin, 2016-03-25 13:39:48]

rdevlin.cronin@chromium.org changed reviewers:
+ lazyboy@chromium.org, stevenjb@chromium.org

[Devlin, 2016-03-25 13:39:48]

Steven, can you take a look at the PasswordsPrivate API?  See comments in files
- non-trivial review this time.

Istiaque, please take a look at everything else. :)

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h
(right):

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h:122:
base::ObserverList<Observer> observers_;
Looking through the code, I can't see any reason this used to be an
ObserverListThreadSafe - it looks as though it's always called on the same
thread.  ObserverListThreadSafe also has more overhead and has to copy the
arguments.

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.cc
(right):

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.cc:68:
StartOrStopListeningForChanges();
Flow:
- Starts listening
- Adds self as observer
- Delegate immediately sends "OnFooChanged" notifications, which used to cause
this to then SendFooListToListeners
- Continues to line 70/71, where everything is re-sent.
This results in two updates being sent immediately for extensions registering
for listeners, which made the API test fail.  The reason this didn't fail before
is because the ObserverListThreadSafe made these updates asynchronous.

Note that even with this change, we still have the side effect of notifying
*all* observers every time a new observer is added, even if nothing's changed. 
The fix would be to either dispatch only to the new listener or to provide a
GetFoo() api method - but that's a separate issue, and since this is a private
API, not really a big deal.

[lazyboy, 2016-03-25 17:57:07]

"everything else" lgtm.

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/omnibox/omnibox_unittest.cc (left):

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:82:
EXPECT_TRUE(params->suggest_results[0].get());
Here and in other places: should keep an equivalent of this, e.g.:
EXPECT_GT(params->suggest_results.size(), 0u);

https://codereview.chromium.org/1833773002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/platform_keys.idl (right):

https://codereview.chromium.org/1833773002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/platform_keys.idl:10:
[use_movable_types=true]namespace platformKeys {
nit: space before "namespace"

[stevenjb, 2016-03-29 00:52:37]

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right):

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:327: length =
*style.length;
nit: int length = style.length ? :

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:331: : std::max(0,
static_cast<int>(description.length()) +
optional nit: clean this up too (casting seems backwards here).

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h
(right):

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h:122:
base::ObserverList<Observer> observers_;
On 2016/03/25 13:39:48, Devlin wrote:
> Looking through the code, I can't see any reason this used to be an
> ObserverListThreadSafe - it looks as though it's always called on the same
> thread.  ObserverListThreadSafe also has more overhead and has to copy the
> arguments.

I looked at the code review and I'm not sure why I didn't call this out in the
review (other than it being a large CL with other issues that took a while to
get right).

On inspection, I don't see any indication that observers_ would be called from a
different thread. As far as I am aware, the extensions api code, while mostly
asynchronous, all occurs on the UI thread. I can see how the author might have
suspected that the event router would call Add/RemoveObsverver asynchronously,
but inspecting the code it appears that it does not.

In other words, this looks fine :)

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.cc
(right):

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.cc:68:
StartOrStopListeningForChanges();
On 2016/03/25 13:39:48, Devlin wrote:
> Flow:
> - Starts listening
> - Adds self as observer
> - Delegate immediately sends "OnFooChanged" notifications, which used to cause
> this to then SendFooListToListeners
> - Continues to line 70/71, where everything is re-sent.
> This results in two updates being sent immediately for extensions registering
> for listeners, which made the API test fail.  The reason this didn't fail
before
> is because the ObserverListThreadSafe made these updates asynchronous.
> 
> Note that even with this change, we still have the side effect of notifying
> *all* observers every time a new observer is added, even if nothing's changed.

> The fix would be to either dispatch only to the new listener or to provide a
> GetFoo() api method - but that's a separate issue, and since this is a private
> API, not really a big deal.

I'd actually rather fix this separately. I agree that the current behavior is a
side effect of inappropriately using ObserverListThreadSafe is strange and bad,
but I'd rather fix this more correctly separately.

Could you file a bug for this and assign it to me? I'll fix or reassign
appropriately. Thanks!

[Devlin, 2016-03-29 14:19:27]

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.cc
(right):

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.cc:68:
StartOrStopListeningForChanges();
On 2016/03/29 00:52:37, stevenjb wrote:
> On 2016/03/25 13:39:48, Devlin wrote:
> > Flow:
> > - Starts listening
> > - Adds self as observer
> > - Delegate immediately sends "OnFooChanged" notifications, which used to
cause
> > this to then SendFooListToListeners
> > - Continues to line 70/71, where everything is re-sent.
> > This results in two updates being sent immediately for extensions
registering
> > for listeners, which made the API test fail.  The reason this didn't fail
> before
> > is because the ObserverListThreadSafe made these updates asynchronous.
> > 
> > Note that even with this change, we still have the side effect of notifying
> > *all* observers every time a new observer is added, even if nothing's
changed.
> 
> > The fix would be to either dispatch only to the new listener or to provide a
> > GetFoo() api method - but that's a separate issue, and since this is a
private
> > API, not really a big deal.
> 
> I'd actually rather fix this separately. I agree that the current behavior is
a
> side effect of inappropriately using ObserverListThreadSafe is strange and
bad,
> but I'd rather fix this more correctly separately.
> 
> Could you file a bug for this and assign it to me? I'll fix or reassign
> appropriately. Thanks!

Unfortunately, the API test fails without this fix (since the listener gets
notified too many times).  I could proceed with this band-aid for now or change
the API test and add a TODO, or I can wait for the proper fix before landing
this one (I have a slight preference for one of the former so that this isn't
blocked).  Preferences?

[stevenjb, 2016-03-29 16:28:27]

I don't want to block this, but I would like to understand why exactly
these changes are required now. (i.e. if we don't make the ObserverList
change, why does this start failing?)



On Tue, Mar 29, 2016 at 7:19 AM, <rdevlin.cronin@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
> File
>
>
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.cc
> (right):
>
>
>
https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
>
>
chrome/browser/extensions/api/passwords_private/passwords_private_event_router.cc:68:
> StartOrStopListeningForChanges();
> On 2016/03/29 00:52:37, stevenjb wrote:
> > On 2016/03/25 13:39:48, Devlin wrote:
> > > Flow:
> > > - Starts listening
> > > - Adds self as observer
> > > - Delegate immediately sends "OnFooChanged" notifications, which
> used to cause
> > > this to then SendFooListToListeners
> > > - Continues to line 70/71, where everything is re-sent.
> > > This results in two updates being sent immediately for extensions
> registering
> > > for listeners, which made the API test fail. The reason this didn't
> fail
> > before
> > > is because the ObserverListThreadSafe made these updates
> asynchronous.
> > >
> > > Note that even with this change, we still have the side effect of
> notifying
> > > *all* observers every time a new observer is added, even if
> nothing's changed.
> >
> > > The fix would be to either dispatch only to the new listener or to
> provide a
> > > GetFoo() api method - but that's a separate issue, and since this is
> a private
> > > API, not really a big deal.
> >
> > I'd actually rather fix this separately. I agree that the current
> behavior is a
> > side effect of inappropriately using ObserverListThreadSafe is strange
> and bad,
> > but I'd rather fix this more correctly separately.
> >
> > Could you file a bug for this and assign it to me? I'll fix or
> reassign
> > appropriately. Thanks!
>
> Unfortunately, the API test fails without this fix (since the listener
> gets notified too many times). I could proceed with this band-aid for
> now or change the API test and add a TODO, or I can wait for the proper
> fix before landing this one (I have a slight preference for one of the
> former so that this isn't blocked). Preferences?
>
> https://codereview.chromium.org/1833773002/
>

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

[Devlin, 2016-03-29 16:42:23]

On 2016/03/29 16:28:27, stevenjb wrote:
> I don't want to block this, but I would like to understand why exactly
> these changes are required now. (i.e. if we don't make the ObserverList
> change, why does this start failing?)

If we don't make the ObserverList change, it wouldn't, but it wouldn't compile -
ObserverListThreadSafe takes a closure and notifies the observer asynchronously,
which means it either needs to own or make a copy of any of the arguments passed
in (like the vector of current entries, ownership of which has to stay with the
delegate).  Moving this to a regular observer list means we just pass in
references rather than copying.  Previously, we *did* just do a copy - but
linked_ptrs are copyable.  Note that this wasn't good either, but it compiled.

[stevenjb, 2016-03-29 16:51:36]

I see. That is the bit of information I was missing.

In that case, go ahead and just add a a TODO + bug. Thanks!


On Tue, Mar 29, 2016 at 9:42 AM, <rdevlin.cronin@chromium.org> wrote:

> On 2016/03/29 16:28:27, stevenjb wrote:
> > I don't want to block this, but I would like to understand why exactly
> > these changes are required now. (i.e. if we don't make the ObserverList
> > change, why does this start failing?)
>
> If we don't make the ObserverList change, it wouldn't, but it wouldn't
> compile -
> ObserverListThreadSafe takes a closure and notifies the observer
> asynchronously,
> which means it either needs to own or make a copy of any of the arguments
> passed
> in (like the vector of current entries, ownership of which has to stay
> with the
> delegate). Moving this to a regular observer list means we just pass in
> references rather than copying. Previously, we *did* just do a copy - but
> linked_ptrs are copyable. Note that this wasn't good either, but it
> compiled.
>
> https://codereview.chromium.org/1833773002/
>

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

[Devlin, 2016-03-29 20:20:35]

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right):

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:327: length =
*style.length;
On 2016/03/29 00:52:37, stevenjb wrote:
> nit: int length = style.length ? :
Done.

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:331: : std::max(0,
static_cast<int>(description.length()) +
On 2016/03/29 00:52:37, stevenjb wrote:
> optional nit: clean this up too (casting seems backwards here).

Wrong how?

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/omnibox/omnibox_unittest.cc (left):

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:82:
EXPECT_TRUE(params->suggest_results[0].get());
On 2016/03/25 17:57:07, lazyboy wrote:
> Here and in other places: should keep an equivalent of this, e.g.:
> EXPECT_GT(params->suggest_results.size(), 0u);

Done, though note that's not an equivalent check - previously, if the vector was
empty, it'd crash just as it would in this patch set. ;)

https://codereview.chromium.org/1833773002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/platform_keys.idl (right):

https://codereview.chromium.org/1833773002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/platform_keys.idl:10:
[use_movable_types=true]namespace platformKeys {
On 2016/03/25 17:57:07, lazyboy wrote:
> nit: space before "namespace"

Done.

[stevenjb, 2016-03-29 20:31:45]

lgtm

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right):

https://codereview.chromium.org/1833773002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/omnibox/omnibox_api.cc:331: : std::max(0,
static_cast<int>(description.length()) +
On 2016/03/29 20:20:35, Devlin wrote:
> On 2016/03/29 00:52:37, stevenjb wrote:
> > optional nit: clean this up too (casting seems backwards here).
> 
> Wrong how?

Oh, I guess we have to cast to an int to do the signed arithmetic. But then we
assign it to a size_t. It's just kind of confusing and awkward looking, but it's
fine.

[Devlin, 2016-03-29 20:35:19]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-03-29 20:35:19]

The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/1833773002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-03-29 20:36:02]

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

[commit-bot: I haz the power, 2016-03-29 20:52:50]

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

[commit-bot: I haz the power, 2016-03-29 20:52:51]

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

[Devlin, 2016-03-29 20:57:38]

Description was changed from

==========
[Extensions] Convert APIs to use movable types [8]

Generated extensions code used to rely on linked_ptrs for non-copyable types in
containers. Now, we can make these types movable. Update the following APIs
to use movable types in this CL:
- omnibox.json
- page_action.json
- page_capture.json
- passwords_private.idl
- permissions.json
- platform_keys.idl
- platform_keys_internal.idl
- preferences_private.json
- privacy.json

BUG=595949
==========

to

==========
[Extensions] Convert APIs to use movable types [8]

Generated extensions code used to rely on linked_ptrs for non-copyable types in
containers. Now, we can make these types movable. Update the following APIs
to use movable types in this CL:
- omnibox.json
- page_action.json
- page_capture.json
- passwords_private.idl
- permissions.json
- platform_keys.idl
- platform_keys_internal.idl
- preferences_private.json
- privacy.json

BUG=595949
TBR=mpearson@chromium.org (trivial c/b/autocomplete/ change)
==========

[Devlin, 2016-03-29 20:57:45]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2016-03-29 20:58:06]

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

[commit-bot: I haz the power, 2016-03-29 21:25:12]

Description was changed from

==========
[Extensions] Convert APIs to use movable types [8]

Generated extensions code used to rely on linked_ptrs for non-copyable types in
containers. Now, we can make these types movable. Update the following APIs
to use movable types in this CL:
- omnibox.json
- page_action.json
- page_capture.json
- passwords_private.idl
- permissions.json
- platform_keys.idl
- platform_keys_internal.idl
- preferences_private.json
- privacy.json

BUG=595949
TBR=mpearson@chromium.org (trivial c/b/autocomplete/ change)
==========

to

==========
[Extensions] Convert APIs to use movable types [8]

Generated extensions code used to rely on linked_ptrs for non-copyable types in
containers. Now, we can make these types movable. Update the following APIs
to use movable types in this CL:
- omnibox.json
- page_action.json
- page_capture.json
- passwords_private.idl
- permissions.json
- platform_keys.idl
- platform_keys_internal.idl
- preferences_private.json
- privacy.json

BUG=595949
TBR=mpearson@chromium.org (trivial c/b/autocomplete/ change)
==========

[commit-bot: I haz the power, 2016-03-29 21:25:14]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-03-29 21:26:06]

Description was changed from

==========
[Extensions] Convert APIs to use movable types [8]

Generated extensions code used to rely on linked_ptrs for non-copyable types in
containers. Now, we can make these types movable. Update the following APIs
to use movable types in this CL:
- omnibox.json
- page_action.json
- page_capture.json
- passwords_private.idl
- permissions.json
- platform_keys.idl
- platform_keys_internal.idl
- preferences_private.json
- privacy.json

BUG=595949
TBR=mpearson@chromium.org (trivial c/b/autocomplete/ change)
==========

to

==========
[Extensions] Convert APIs to use movable types [8]

Generated extensions code used to rely on linked_ptrs for non-copyable types in
containers. Now, we can make these types movable. Update the following APIs
to use movable types in this CL:
- omnibox.json
- page_action.json
- page_capture.json
- passwords_private.idl
- permissions.json
- platform_keys.idl
- platform_keys_internal.idl
- preferences_private.json
- privacy.json

BUG=595949
TBR=mpearson@chromium.org (trivial c/b/autocomplete/ change)

Committed: https://crrev.com/7edeeeeb5245c12eba7c32cd683f1fb6b159d1b9
Cr-Commit-Position: refs/heads/master@{#383829}
==========

[commit-bot: I haz the power, 2016-03-29 21:26:07]

Patchset 2 (id:??) landed as
https://crrev.com/7edeeeeb5245c12eba7c32cd683f1fb6b159d1b9
Cr-Commit-Position: refs/heads/master@{#383829}