Add Mojo types and service definitions for Background Sync API

Add Mojo types and service definitions for Background Sync API

This commit depends on the Blink CL at
https://codereview.chromium.org/1096503002/
for changes to platform objects.

BUG=479251
Committed: https://crrev.com/5347685266d20a08f84e4135bfb5fbb0ca182063
Cr-Commit-Position: refs/heads/master@{#327745}

[iclelland, 2015-04-27 14:37:58]

iclelland@chromium.org changed reviewers:
+ jkarlin@chromium.org, rockot@chromium.org

[iclelland, 2015-04-27 14:42:02]

On 2015/04/27 14:37:58, iclelland wrote:
> mailto:iclelland@chromium.org changed reviewers:
> + mailto:jkarlin@chromium.org, mailto:rockot@chromium.org

rockot: Could you comment on the appropriateness of the nested namespaces here?

I was getting conflicts between the names in this patch, and identically-named
proto definitions in the content namespace being defined in
content/browser/background_sync/background_sync_manager.h

jkarlin: The methods in background_sync_service have been updated since
https://codereview.chromium.org/1072293002/ to reflect the requirements of the
background sync manager (origin, tag on unregister(), etc)

I'll upload a dependent CL with the renderer-side code soon as well.

[jkarlin, 2015-04-27 18:25:11]

Nice work. First pass comments below:

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
File content/child/background_sync/background_sync_type_converters.cc (right):

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:5: #include
"content/child/background_sync/background_sync_type_converters.h"
New line after this line

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:24: }
Since you have the switch, you should call NOTREACHED() in the event of an
invalid enum value. 

E.g.,

switch (input) {
     case content::background_sync::SYNC_PERIODICITY_Periodic:
       return blink::WebSyncRegistration::PeriodicityPeriodic;
     case content::background_sync::SYNC_PERIODICITY_OneShot:
       return blink::WebSyncRegistration::PeriodicityOneShot;
   }
NOTREACHED();
return blink::WebSyncRegistration::PeriodicityPeriodic;


Alternatively, you can use COMPILE_ASSERTs to verify that the enums are the same
and simply static_cast between them. This doesn't provide protection against
invalid values but has its own advantages (simplicity). It would require a _MAX
value for each enum so that you know that you've verified all of them. See
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
for an example.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:42: return
result;
ditto

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:63: return
result;
ditto

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:83: return
result;
ditto

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:118: return
result;
ditto

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:122:
blink::WebSyncRegistration* TypeConverter<blink::WebSyncRegistration *,
This naked pointer scares me. Return a scoped_ptr<blink::WebSyncRegistration>
instead.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:157: } //
namespace mojo
two spaces after }

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
File content/child/background_sync/background_sync_type_converters.h (right):

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.h:83: } //
namespace mojo
two spaces between code and comment

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
File content/child/background_sync/background_sync_type_converters_unittest.cc
(right):

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters_unittest.cc:5:
#include "content/child/background_sync/background_sync_type_converters.h"
blank line after this include

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters_unittest.cc:9:
namespace test {
is this namespace used for anything?

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters_unittest.cc:137:
blink::WebSyncRegistration* out = ConvertTo<blink::WebSyncRegistration*>(in);
out is leaked

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters_unittest.cc:157:
blink::WebSyncRegistration* out = ConvertTo<blink::WebSyncRegistration*>(in);
out is leaked

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters_unittest.cc:169: }
add namespace comments to these closing braces

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
File content/common/background_sync.mojom (right):

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync.mojom:5: module content.background_sync;
I'd remove the namespace, and see comments below about naming the enums.

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync.mojom:10: Online,
These files are in chromium code and should use chromium style guide.

Suggest:

SYNC_NETWORK_STATE_ANY, SYNC_NETWORK_STATE_AVOID_CELLULAR,
SYNC_NETWORK_STATE_ONLINE

MACRO_CASE is required for Chromium enums and expanding the name is better in
the namespace. This also won't collide with the proto enums.

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync.mojom:15: AvoidDraining,
SYNC_POWER_STATE_AUTO and SYNC_POWER_STATE_DRAINING

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync.mojom:19: Periodic,
SYNC_PERIODICITY_PERIODIC and SYNC_PERIODICITY_ONE_SHOT

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync.mojom:27: uint64 minPeriodMs = 0;
min_period_ms

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync.mojom:28: SyncNetworkState networkState = Online;
network_state

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync.mojom:29: SyncPowerState powerState = Auto;
power_state

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
File content/common/background_sync_service.mojom (right):

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync_service.mojom:11: int64
serviceWorkerRegistrationId) => (SyncRegistration options);
service_worker_registration_id here and below

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync_service.mojom:18: Unregister(SyncPeriodicity
periodicity, int32 id, string tag, string origin,
int64 id

[iclelland, 2015-04-28 12:41:30]

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
File content/child/background_sync/background_sync_type_converters.cc (right):

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:5: #include
"content/child/background_sync/background_sync_type_converters.h"
On 2015/04/27 18:25:11, jkarlin wrote:
> New line after this line

Done.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:24: }
On 2015/04/27 18:25:10, jkarlin wrote:
> Since you have the switch, you should call NOTREACHED() in the event of an
> invalid enum value. 
> 
> E.g.,
> 
> switch (input) {
>      case content::background_sync::SYNC_PERIODICITY_Periodic:
>        return blink::WebSyncRegistration::PeriodicityPeriodic;
>      case content::background_sync::SYNC_PERIODICITY_OneShot:
>        return blink::WebSyncRegistration::PeriodicityOneShot;
>    }
> NOTREACHED();
> return blink::WebSyncRegistration::PeriodicityPeriodic;

I thought that the compiler flags always included -Wswitch, so there was no need
to assert at run time that all values were handled. It doesn't look like
NOTREACHED costs anything in a release build, though, so it can't hurt.

> 
> 
> Alternatively, you can use COMPILE_ASSERTs to verify that the enums are the
same
> and simply static_cast between them. This doesn't provide protection against
> invalid values but has its own advantages (simplicity). It would require a
_MAX
> value for each enum so that you know that you've verified all of them. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> for an example.
> 

It looks like only one side needs to define _MAX -- in that example, WebSocket.h
*does not* define _MAX, but ppb_websocket.h *does*. As long as that's okay, then
I'll use the static cast instead.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:42: return
result;
On 2015/04/27 18:25:10, jkarlin wrote:
> ditto

Done.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:63: return
result;
On 2015/04/27 18:25:11, jkarlin wrote:
> ditto

Done.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:83: return
result;
On 2015/04/27 18:25:11, jkarlin wrote:
> ditto

Done.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:118: return
result;
On 2015/04/27 18:25:11, jkarlin wrote:
> ditto

Done.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:122:
blink::WebSyncRegistration* TypeConverter<blink::WebSyncRegistration *,
On 2015/04/27 18:25:10, jkarlin wrote:
> This naked pointer scares me. Return a scoped_ptr<blink::WebSyncRegistration>
> instead.

Done.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:157: } //
namespace mojo
On 2015/04/27 18:25:11, jkarlin wrote:
> two spaces after }

Done.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
File content/child/background_sync/background_sync_type_converters.h (right):

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.h:83: } //
namespace mojo
On 2015/04/27 18:25:11, jkarlin wrote:
> two spaces between code and comment

Done.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
File content/child/background_sync/background_sync_type_converters_unittest.cc
(right):

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters_unittest.cc:5:
#include "content/child/background_sync/background_sync_type_converters.h"
On 2015/04/27 18:25:11, jkarlin wrote:
> blank line after this include

Done.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters_unittest.cc:9:
namespace test {
On 2015/04/27 18:25:11, jkarlin wrote:
> is this namespace used for anything?

Not by me, but it was a pattern I'd seen in mojo tests
(/src/mojo/shell/url_resolver_unittest.cc,
src/mojo/common/common_type_converters_unittest.cc), so I cargo-culted it in
there.
Removed, as unit tests still run fine without.
Done.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters_unittest.cc:137:
blink::WebSyncRegistration* out = ConvertTo<blink::WebSyncRegistration*>(in);
On 2015/04/27 18:25:11, jkarlin wrote:
> out is leaked

Fixed with scoped_ptr.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters_unittest.cc:157:
blink::WebSyncRegistration* out = ConvertTo<blink::WebSyncRegistration*>(in);
On 2015/04/27 18:25:11, jkarlin wrote:
> out is leaked

Fixed with scoped_ptr.

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters_unittest.cc:169: }
On 2015/04/27 18:25:11, jkarlin wrote:
> add namespace comments to these closing braces

Done.

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
File content/common/background_sync.mojom (right):

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync.mojom:5: module content.background_sync;
On 2015/04/27 18:25:11, jkarlin wrote:
> I'd remove the namespace, and see comments below about naming the enums.

Done.

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync.mojom:10: Online,
On 2015/04/27 18:25:11, jkarlin wrote:
> These files are in chromium code and should use chromium style guide.
> 
> Suggest:
> 
> SYNC_NETWORK_STATE_ANY, SYNC_NETWORK_STATE_AVOID_CELLULAR,
> SYNC_NETWORK_STATE_ONLINE
> 
> MACRO_CASE is required for Chromium enums and expanding the name is better in
> the namespace. This also won't collide with the proto enums.

The mojo bindings generator already prefixes these with "SYNC_NETWORK_STATE_",
because of the enum name. I'll uppercase the values to complete these, and
update the corresponding code everywhere.

The proto enums collide with the type name, not the enum values. It was
"SyncNetworkState" that was the issue when I included both headers.

I'm changing the enums to be named BackgroundSyncNetworkState, etc., and the
values will look like BACKGROUND_SYNC_NETWORK_STATE_ANY. This lets me remove the
extra namespace, and clean up code elsewhere.

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync.mojom:15: AvoidDraining,
On 2015/04/27 18:25:11, jkarlin wrote:
> SYNC_POWER_STATE_AUTO and SYNC_POWER_STATE_DRAINING

Done, as noted above (BACKGROUND_SYNC_POWER_STATE_AUTO, etc).

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync.mojom:19: Periodic,
On 2015/04/27 18:25:11, jkarlin wrote:
> SYNC_PERIODICITY_PERIODIC and SYNC_PERIODICITY_ONE_SHOT

Done, as noted above (BACKGROUND_SYNC_PERIODICITY_PERIODIC, etc).

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync.mojom:27: uint64 minPeriodMs = 0;
On 2015/04/27 18:25:11, jkarlin wrote:
> min_period_ms

Done.

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync.mojom:28: SyncNetworkState networkState = Online;
On 2015/04/27 18:25:11, jkarlin wrote:
> network_state

Done.

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync.mojom:29: SyncPowerState powerState = Auto;
On 2015/04/27 18:25:11, jkarlin wrote:
> power_state

Done.

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
File content/common/background_sync_service.mojom (right):

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync_service.mojom:11: int64
serviceWorkerRegistrationId) => (SyncRegistration options);
On 2015/04/27 18:25:11, jkarlin wrote:
> service_worker_registration_id here and below

Done.

https://codereview.chromium.org/1106193002/diff/1/content/common/background_s...
content/common/background_sync_service.mojom:18: Unregister(SyncPeriodicity
periodicity, int32 id, string tag, string origin,
On 2015/04/27 18:25:11, jkarlin wrote:
> int64 id

Done.

[iclelland, 2015-04-28 12:53:40]

iclelland@chromium.org changed reviewers:
+ tsepez@chromium.org

[iclelland, 2015-04-28 12:57:38]

+r tsepez@ -- Could I get an OWNERS review for the new .mojom files in
content/common?

[Tom Sepez, 2015-04-28 16:05:09]

mojom LGTM.

[jkarlin, 2015-04-29 13:49:27]

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
File content/child/background_sync/background_sync_type_converters.cc (right):

https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...
content/child/background_sync/background_sync_type_converters.cc:24: }
On 2015/04/28 12:41:29, iclelland wrote:
> On 2015/04/27 18:25:10, jkarlin wrote:
> > Since you have the switch, you should call NOTREACHED() in the event of an
> > invalid enum value. 
> > 
> > E.g.,
> > 
> > switch (input) {
> >      case content::background_sync::SYNC_PERIODICITY_Periodic:
> >        return blink::WebSyncRegistration::PeriodicityPeriodic;
> >      case content::background_sync::SYNC_PERIODICITY_OneShot:
> >        return blink::WebSyncRegistration::PeriodicityOneShot;
> >    }
> > NOTREACHED();
> > return blink::WebSyncRegistration::PeriodicityPeriodic;
> 
> I thought that the compiler flags always included -Wswitch, so there was no
need
> to assert at run time that all values were handled. It doesn't look like
> NOTREACHED costs anything in a release build, though, so it can't hurt.

-Wswitch doesn't protect against invalid values (outside the enumeration's
range) from being passed in.

> 
> > 
> > 
> > Alternatively, you can use COMPILE_ASSERTs to verify that the enums are the
> same
> > and simply static_cast between them. This doesn't provide protection against
> > invalid values but has its own advantages (simplicity). It would require a
> _MAX
> > value for each enum so that you know that you've verified all of them. See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> > for an example.
> > 
> 
> It looks like only one side needs to define _MAX -- in that example,
WebSocket.h
> *does not* define _MAX, but ppb_websocket.h *does*. As long as that's okay,
then
> I'll use the static cast instead.

I agree that WebSocket.h doesn't have a _MAX, but it should. Otherwise how can
you convert in both directions safely?

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
File content/child/background_sync/background_sync_type_converters.cc (right):

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
content/child/background_sync/background_sync_type_converters.cc:20:
COMPILE_ASSERT_MATCHING_ENUM(BACKGROUND_SYNC_PERIODICITY_MAX,
Should compare to PeriodicityMax. Otherwise, what if Periodicity has an extra
value that doesn't translate to BACKGROUND_SYNC_? If that's too painful, the
switch with a NOTREACHED at the end works fine too.

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
content/child/background_sync/background_sync_type_converters.cc:28:
COMPILE_ASSERT_MATCHING_ENUM(BACKGROUND_SYNC_NETWORK_STATE_MAX,
ditto

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
content/child/background_sync/background_sync_type_converters.cc:34:
COMPILE_ASSERT_MATCHING_ENUM(BACKGROUND_SYNC_POWER_STATE_MAX,
ditto

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
content/child/background_sync/background_sync_type_converters.cc:90: auto result
= make_scoped_ptr(new blink::WebSyncRegistration());
nit: scoped_ptr<blink::WebSyncRegistration> result(new
blink::WebSyncRegistration()); instead of auto here? Auto doesn't save much
space here and makes the code harder to read.

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
content/child/background_sync/background_sync_type_converters.cc:100: return
result;
scoped_ptr.h says that this should be result.Pass(), though I imagine compilers
can optimize this.

[jkarlin, 2015-04-29 15:07:59]

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
File content/child/background_sync/background_sync_type_converters.cc (right):

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
content/child/background_sync/background_sync_type_converters.cc:100: return
result;
On 2015/04/29 13:49:26, jkarlin wrote:
> scoped_ptr.h says that this should be result.Pass(), though I imagine
compilers
> can optimize this.

rsmith@ created a bug for this at https://crbug.com/482477.

[iclelland, 2015-04-29 19:05:25]

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
File content/child/background_sync/background_sync_type_converters.cc (right):

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
content/child/background_sync/background_sync_type_converters.cc:20:
COMPILE_ASSERT_MATCHING_ENUM(BACKGROUND_SYNC_PERIODICITY_MAX,
On 2015/04/29 13:49:26, jkarlin wrote:
> Should compare to PeriodicityMax. Otherwise, what if Periodicity has an extra
> value that doesn't translate to BACKGROUND_SYNC_? If that's too painful, the
> switch with a NOTREACHED at the end works fine too.

Done. It's spelled PeriodicityLast in WebSyncRegistration.h, so it's easy,
actually.

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
content/child/background_sync/background_sync_type_converters.cc:28:
COMPILE_ASSERT_MATCHING_ENUM(BACKGROUND_SYNC_NETWORK_STATE_MAX,
On 2015/04/29 13:49:27, jkarlin wrote:
> ditto

Done.

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
content/child/background_sync/background_sync_type_converters.cc:34:
COMPILE_ASSERT_MATCHING_ENUM(BACKGROUND_SYNC_POWER_STATE_MAX,
On 2015/04/29 13:49:27, jkarlin wrote:
> ditto

Done.

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
content/child/background_sync/background_sync_type_converters.cc:90: auto result
= make_scoped_ptr(new blink::WebSyncRegistration());
On 2015/04/29 13:49:26, jkarlin wrote:
> nit: scoped_ptr<blink::WebSyncRegistration> result(new
> blink::WebSyncRegistration()); instead of auto here? Auto doesn't save much
> space here and makes the code harder to read.

Changed to use a bare pointer here, and wrap it in scoped_ptr at the end of the
method, so to be clearer.

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
content/child/background_sync/background_sync_type_converters.cc:100: return
result;
On 2015/04/29 13:49:26, jkarlin wrote:
> scoped_ptr.h says that this should be result.Pass(), though I imagine
compilers
> can optimize this.

Changed to construct and return in the same statement, following the examples in
https://www.chromium.org/developers/smart-pointer-guidelines

[iclelland, 2015-04-29 19:05:49]

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

[iclelland, 2015-04-29 19:08:23]

+r jochen@ -- Can you review the changes to the build files?
content/common/BUILD.gn
content/content_common_mojo_bindings.gyp
content/public/common/BUILD.gn

Thanks

[jkarlin, 2015-04-29 19:09:20]

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
File content/child/background_sync/background_sync_type_converters.cc (right):

https://codereview.chromium.org/1106193002/diff/60001/content/child/backgroun...
content/child/background_sync/background_sync_type_converters.cc:100: return
result;
On 2015/04/29 19:05:24, iclelland wrote:
> On 2015/04/29 13:49:26, jkarlin wrote:
> > scoped_ptr.h says that this should be result.Pass(), though I imagine
> compilers
> > can optimize this.
> 
> Changed to construct and return in the same statement, following the examples
in
> https://www.chromium.org/developers/smart-pointer-guidelines

FYI, the previously mentioned bug, https://crbug.com/482477 is resolved, and
recommends your original implementation.

[jochen (gone - plz use gerrit), 2015-04-30 07:19:17]

lgtm

[jkarlin, 2015-04-30 12:47:37]

lgtm with nits

https://codereview.chromium.org/1106193002/diff/120001/content/child/backgrou...
File content/child/background_sync/background_sync_type_converters.cc (right):

https://codereview.chromium.org/1106193002/diff/120001/content/child/backgrou...
content/child/background_sync/background_sync_type_converters.cc:21:
PeriodicityOneShot);
This check is unnecessary now that you have the one below.

https://codereview.chromium.org/1106193002/diff/120001/content/child/backgrou...
content/child/background_sync/background_sync_type_converters.cc:31:
NetworkStateOnline);
ditto

https://codereview.chromium.org/1106193002/diff/120001/content/child/backgrou...
content/child/background_sync/background_sync_type_converters.cc:39:
PowerStateAvoidDraining);
ditto

https://codereview.chromium.org/1106193002/diff/120001/content/child/backgrou...
content/child/background_sync/background_sync_type_converters.cc:106: return
scoped_ptr<blink::WebSyncRegistration>(result);
Sorry to give you the run-around on this. I prefer your original implementation
as the pointer is never naked.

scoped_ptr<WebSyncRegistration> result(new blink::WebSyncRegistration());
...
return result;


But if you want to keep this form, then "return make_scoped_ptr(result);" is
preferable.

[iclelland, 2015-04-30 14:49:53]

https://codereview.chromium.org/1106193002/diff/120001/content/child/backgrou...
File content/child/background_sync/background_sync_type_converters.cc (right):

https://codereview.chromium.org/1106193002/diff/120001/content/child/backgrou...
content/child/background_sync/background_sync_type_converters.cc:21:
PeriodicityOneShot);
On 2015/04/30 12:47:37, jkarlin wrote:
> This check is unnecessary now that you have the one below.

I thought this check was critical -- to ensure that no one has sneaked in any
new mismatching values after OneShot -- i.e., MAX could still match Last, but
there is one new value on each side that isn't covered by the tests.

https://codereview.chromium.org/1106193002/diff/120001/content/child/backgrou...
content/child/background_sync/background_sync_type_converters.cc:106: return
scoped_ptr<blink::WebSyncRegistration>(result);
On 2015/04/30 12:47:37, jkarlin wrote:
> Sorry to give you the run-around on this. I prefer your original
implementation
> as the pointer is never naked.
> 
> scoped_ptr<WebSyncRegistration> result(new blink::WebSyncRegistration());
> ...
> return result;
> 
> 
> But if you want to keep this form, then "return make_scoped_ptr(result);" is
> preferable.

No problem; I'll revert to the original construct.
Done.

[jkarlin, 2015-04-30 14:58:38]

https://codereview.chromium.org/1106193002/diff/120001/content/child/backgrou...
File content/child/background_sync/background_sync_type_converters.cc (right):

https://codereview.chromium.org/1106193002/diff/120001/content/child/backgrou...
content/child/background_sync/background_sync_type_converters.cc:21:
PeriodicityOneShot);
On 2015/04/30 14:49:52, iclelland wrote:
> On 2015/04/30 12:47:37, jkarlin wrote:
> > This check is unnecessary now that you have the one below.
> 
> I thought this check was critical -- to ensure that no one has sneaked in any
> new mismatching values after OneShot -- i.e., MAX could still match Last, but
> there is one new value on each side that isn't covered by the tests.

Fair, works with me.

[iclelland, 2015-04-30 17:28:29]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2015-04-30 17:28:30]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jkarlin@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1106193002/#ps140001
(title: "Revert scoped_ptr construction change")

[commit-bot: I haz the power, 2015-04-30 17:29:01]

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

[commit-bot: I haz the power, 2015-04-30 18:32:18]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-04-30 18:33:15]

Patchset 8 (id:??) landed as
https://crrev.com/5347685266d20a08f84e4135bfb5fbb0ca182063
Cr-Commit-Position: refs/heads/master@{#327745}