[OnionSoup] Move background_sync.mojom from //content to //third_party/WebKit

[OnionSoup] Move background_sync.mojom from //content to
//third_party/WebKit

This patch is the first step of moving
background_sync.mojom from //content to //third_party/WebKit

BUG=610374
Committed: https://crrev.com/30a0e93e8d121381d3674db975ba7ac7838ec7cf
Cr-Commit-Position: refs/heads/master@{#392779}

[juncai, 2016-05-09 18:02:10]

Description was changed from

==========
Move background sync to blink

BUG=None
==========

to

==========
[OnionSoup] Move background_sync.mojom from //content to 
//third_party/WebKit

This patch is the first step of moving
background_sync.mojom from //content to //third_party/WebKit

BUG=610374
==========

[juncai, 2016-05-09 18:14:58]

juncai@chromium.org changed reviewers:
+ avi@chromium.org, esprehn@chromium.org

[juncai, 2016-05-09 18:14:59]

esprehn@chromium.org: Please review changes in 
//third_party/WebKit/public/

avi@chromium.org: Please review changes in 
//content/

[Avi (use Gerrit), 2016-05-09 18:38:34]

This seems like just a move, yes?

LGTM stamp

[juncai, 2016-05-09 18:42:00]

On 2016/05/09 18:38:34, Avi wrote:
> This seems like just a move, yes?
> 
> LGTM stamp

Yes, just moved mojom file from //content to //third_party//WebKit, no new code
added.

[esprehn, 2016-05-09 18:51:37]

lgtm

[juncai, 2016-05-09 18:59:49]

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

[juncai, 2016-05-09 18:59:50]

tsepez@chromium.org: Please review changes in 

//content/common/background_sync_service.mojom
//content/public/common/background_sync.mojom
//content/public/common/service_worker_event_status.mojom

[Marijn Kruisselbrink, 2016-05-09 19:02:37]

drive-by nit

https://codereview.chromium.org/1960843002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/background_sync/OWNERS (right):

https://codereview.chromium.org/1960843002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/background_sync/OWNERS:4: # Changes
to Mojo interfaces require a security review to avoid
You probably should have a similar OWNERS file change in 
third_party/WebKit/public/platform/modules/serviceworker/?

[juncai, 2016-05-09 19:11:03]

https://codereview.chromium.org/1960843002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/background_sync/OWNERS (right):

https://codereview.chromium.org/1960843002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/background_sync/OWNERS:4: # Changes
to Mojo interfaces require a security review to avoid
On 2016/05/09 19:02:37, Marijn Kruisselbrink wrote:
> You probably should have a similar OWNERS file change in 
> third_party/WebKit/public/platform/modules/serviceworker/?

Done.

[juncai, 2016-05-10 02:39:30]

juncai@chromium.org changed reviewers:
+ dcheng@chromium.org

[juncai, 2016-05-10 02:39:31]

Noticed that tsepez@ is OOO. Added dcheng@ as the reviewer.

dcheng@:
I guess moving those mojom files:
//content/common/background_sync_service.mojom
//content/public/common/background_sync.mojom
//content/public/common/service_worker_event_status.mojom

needs a security review, can you take a look? Thanks.

[dcheng, 2016-05-10 05:06:03]

I didn't look at the existing type converters very closely: where they
previously reviewed by a security OWNER?

https://codereview.chromium.org/1960843002/diff/80001/content/browser/backgro...
File content/browser/background_sync/background_sync_registration.h (right):

https://codereview.chromium.org/1960843002/diff/80001/content/browser/backgro...
content/browser/background_sync/background_sync_registration.h:85: };
Can we try to clean up these type converters in a followup CL? We should be
using StructTraits where possible.

(Note that type converters should also be a separate file named something like
foo_type_converters.* with appropriate mojom OWNERS rules)

[iclelland, 2016-05-10 13:46:21]

On 2016/05/10 05:06:03, dcheng wrote:
> I didn't look at the existing type converters very closely: where they
> previously reviewed by a security OWNER?
> 
>
https://codereview.chromium.org/1960843002/diff/80001/content/browser/backgro...
> File content/browser/background_sync/background_sync_registration.h (right):
> 
>
https://codereview.chromium.org/1960843002/diff/80001/content/browser/backgro...
> content/browser/background_sync/background_sync_registration.h:85: };
> Can we try to clean up these type converters in a followup CL? We should be
> using StructTraits where possible.
> 
> (Note that type converters should also be a separate file named something like
> foo_type_converters.* with appropriate mojom OWNERS rules)

background_sync_type_converters were added here:
https://codereview.chromium.org/1106193002/

service_worker_type_converters added here:
https://codereview.chromium.org/1220943003/

In both cases, tsepez reviewed the Mojo changes (as a required Mojo security
review), but I'm not sure now if his attention was primarily on the .mojom
interface definition files, or whether he looked at the converters specifically
as well.

[juncai, 2016-05-10 20:40:14]

https://codereview.chromium.org/1960843002/diff/80001/content/browser/backgro...
File content/browser/background_sync/background_sync_registration.h (right):

https://codereview.chromium.org/1960843002/diff/80001/content/browser/backgro...
content/browser/background_sync/background_sync_registration.h:85: };
On 2016/05/10 05:06:03, dcheng wrote:
> Can we try to clean up these type converters in a followup CL? We should be
> using StructTraits where possible.
> 
> (Note that type converters should also be a separate file named something like
> foo_type_converters.* with appropriate mojom OWNERS rules)

It seems that the implementation of these were deleted at:
https://codereview.chromium.org/1282013004/diff/670001/content/browser/backgr...

I guess here it can be safely removed too.

Done.

[dcheng, 2016-05-10 20:44:40]

mojom lgtm

[juncai, 2016-05-10 21:20:19]

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

[juncai, 2016-05-10 21:20:20]

I guess 
//content/public/common/background_sync.mojom
//content/public/common/service_worker_event_status.mojom
also need a security review. Added palmer@chromium.org to review them.

palmer@, please take a look. Thanks!

[palmer, 2016-05-10 22:29:38]

LGTM rubberstamp.

[juncai, 2016-05-10 22:40:54]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-05-10 22:40:55]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1960843002/#ps100001
(title: "address dcheng@'s comment")

[commit-bot: I haz the power, 2016-05-10 22:41:17]

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

[commit-bot: I haz the power, 2016-05-11 00:10:05]

Description was changed from

==========
[OnionSoup] Move background_sync.mojom from //content to 
//third_party/WebKit

This patch is the first step of moving
background_sync.mojom from //content to //third_party/WebKit

BUG=610374
==========

to

==========
[OnionSoup] Move background_sync.mojom from //content to 
//third_party/WebKit

This patch is the first step of moving
background_sync.mojom from //content to //third_party/WebKit

BUG=610374
==========

[commit-bot: I haz the power, 2016-05-11 00:10:07]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-11 00:11:21]

Description was changed from

==========
[OnionSoup] Move background_sync.mojom from //content to 
//third_party/WebKit

This patch is the first step of moving
background_sync.mojom from //content to //third_party/WebKit

BUG=610374
==========

to

==========
[OnionSoup] Move background_sync.mojom from //content to
//third_party/WebKit

This patch is the first step of moving
background_sync.mojom from //content to //third_party/WebKit

BUG=610374

Committed: https://crrev.com/30a0e93e8d121381d3674db975ba7ac7838ec7cf
Cr-Commit-Position: refs/heads/master@{#392779}
==========

[commit-bot: I haz the power, 2016-05-11 00:11:23]

Patchset 6 (id:??) landed as
https://crrev.com/30a0e93e8d121381d3674db975ba7ac7838ec7cf
Cr-Commit-Position: refs/heads/master@{#392779}

[michaeln, 2016-05-11 22:30:43]

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

[michaeln, 2016-05-11 22:30:44]

https://codereview.chromium.org/1960843002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/BUILD.gn (right):

https://codereview.chromium.org/1960843002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/BUILD.gn:186:
"platform/modules/geolocation/geolocation.mojom",
Drive by question?

This has probably already been discussed plenty times over, but...

The practice of putting interface definition files for external services
utilized by webkit/modules into the webkit corner of the source tree seems odd?
Its easy to imagine some of these services being reused in non-webkit contexts.

Why are the interface files defining a service co-resident with the sources that
import the service rather then the sources that export the service?

For interfaces that are highly specific to blink, it would make sense as 
declaration of an interface that an embedder should provide. But for more
generic/reusable services its odd.