[ServiceWorker] Implement WebServiceWorkerContextClient::openWindow().

[ServiceWorker] Implement WebServiceWorkerContextClient::openWindow().

It is sending the request from the render process to the browser process
which then opens the URL using ContentBrowserClient::OpenURL().

This is going to be used by sw.client.openWindow(), see Blink CL:
https://codereview.chromium.org/850413002

BUG=437151
Committed: https://crrev.com/eb40d5503d848125b533185e95f3fb659336509e
Cr-Commit-Position: refs/heads/master@{#314697}

[mlamouri (slow - plz ping), 2015-01-15 22:06:02]

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

[mlamouri (slow - plz ping), 2015-01-15 22:07:20]

mlamouri@chromium.org changed reviewers:
+ nasko@chromium.org

[mlamouri (slow - plz ping), 2015-01-15 22:07:20]

michaeln@, could you review the changes in:
- content/browser/service_worker/

nasko@, could you do a security review of:
- content/common/service_worker/service_worker_messages.h

[mlamouri (slow - plz ping), 2015-01-15 22:41:19]

It seems that the WindowOpenedObserver doesn't work fine with "about:blank". I
will investigate that tomorrow.

[nasko, 2015-01-17 00:05:28]

Just a couple of nits. 

Is ServiceWorker supposed to be able to open windows? Sorry, not familiar with
the API developments there. Can you point me to the spec?

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:1156: if
(render_process_id == ChildProcessHost::kInvalidUniqueID &&
RenderProcessHost is what hosts Blink renderers. ChildProcessHost is for all
other types of processes. While this statement might be ok in practice, it feels
strange to mix up the two types.

https://codereview.chromium.org/843583005/diff/1/content/renderer/service_wor...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/843583005/diff/1/content/renderer/service_wor...
content/renderer/service_worker/service_worker_script_context.cc:450: int
request_id, const ServiceWorkerClientInfo& client, bool dummy_client) {
nit: if params don't fit on the line of the method, each param gets its own line

[michaeln, 2015-01-17 00:25:32]

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_provider_host.cc:247: int
embedded_worker_id,
why is this first param here?

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:227:
GetContentClient()->browser()->OpenURL(browser_context, params);
imo, storage_partition ptr would make sense as an optional input to this method

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:227:
GetContentClient()->browser()->OpenURL(browser_context, params);
can this method return nullptr for a web_contents, just checking?

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:883:
IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_GetClientInfoSuccess,
Hmmm...

This is not new, but these two ClientInfo completion msg handlers really do not
belong in this class. Notice they're the only ones in here that aren't sent as a
result of something that happens in the serviceworker script context. The proxy
for stuff about client documents is the providerhost class. These msgs should
land on that class.

Oooops, somebody has mistakenly considered a pending clientinfo call as the sw
being in midst of handling an event (see HasInflightRequests). Nope, it's
waiting for a client document to respond to that query.

Thats odd, the way its put together, the only class that can call
host->GetClientInfo() is the version class because the messages that are sent as
a result (a hidden side effect) are delivered to the version class.

Arghh... if a renderer process dies prior to responsing to the GetClientInfo
query, theres a leak and OnGetClientDocuments will never complete.

Actually the whole bit with clientinfo looks rough to my eye. First and
foremost, the only piece of information that is dynamic in the clientinfo is the
focused state. Really this message boils down to "are you focused". It seems
like having the renderer tell the browserprocess when the focus state changes,
and having a bool for that somewhere on the browserside in the
ServiceWorkerProviderHost, would simplify things a whole lot. There probably
already is a message for that state change.

It might be better to back up and de-confusifying ^^^ prior to adding more to
it? Somebody had a bright idea earlier,
https://codereview.chromium.org/771103002/#msg16 :)

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:1142: context_wrapper,
nit: make_scoped_refptr(context_->wrapper()) might be more readable

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:1188: const
ServiceWorkerClientInfo& client_info) {
how is the client_info.client_id field filled in, looks like the renderer always
returns 0 for that value?

also can you please add a ctor to the ServiceWorkerClientInfo struct?

[mlamouri (slow - plz ping), 2015-01-26 13:19:08]

Comments applied. PTAL

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_provider_host.cc:247: int
embedded_worker_id,
On 2015/01/17 at 00:25:32, michaeln wrote:
> why is this first param here?

No longer apply with the rebase.

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:227:
GetContentClient()->browser()->OpenURL(browser_context, params);
On 2015/01/17 at 00:25:32, michaeln wrote:
> imo, storage_partition ptr would make sense as an optional input to this
method

Let's discuss about that in the CL related to ContentBrowserClient::OpenURL.

> can this method return nullptr for a web_contents, just checking?

No, unless it's not supported (Android/iOS). I've added a DCHECK() to make that
more clear.

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:883:
IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_GetClientInfoSuccess,
On 2015/01/17 at 00:25:32, michaeln wrote:
> Hmmm...
> 
> This is not new, but these two ClientInfo completion msg handlers really do
not belong in this class. Notice they're the only ones in here that aren't sent
as a result of something that happens in the serviceworker script context. The
proxy for stuff about client documents is the providerhost class. These msgs
should land on that class.
> 
> Oooops, somebody has mistakenly considered a pending clientinfo call as the sw
being in midst of handling an event (see HasInflightRequests). Nope, it's
waiting for a client document to respond to that query.
> 
> Thats odd, the way its put together, the only class that can call
host->GetClientInfo() is the version class because the messages that are sent as
a result (a hidden side effect) are delivered to the version class.
> 
> Arghh... if a renderer process dies prior to responsing to the GetClientInfo
query, theres a leak and OnGetClientDocuments will never complete.
> 
> Actually the whole bit with clientinfo looks rough to my eye. First and
foremost, the only piece of information that is dynamic in the clientinfo is the
focused state. Really this message boils down to "are you focused". It seems
like having the renderer tell the browserprocess when the focus state changes,
and having a bool for that somewhere on the browserside in the
ServiceWorkerProviderHost, would simplify things a whole lot. There probably
already is a message for that state change.
> 
> It might be better to back up and de-confusifying ^^^ prior to adding more to
it? Somebody had a bright idea earlier,
https://codereview.chromium.org/771103002/#msg16 :)

I've changed how this is being handled in
https://codereview.chromium.org/871013003
This CL is re-using that new GetClientInfo() method.

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:1142: context_wrapper,
On 2015/01/17 at 00:25:32, michaeln wrote:
> nit: make_scoped_refptr(context_->wrapper()) might be more readable

Done.

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:1156: if
(render_process_id == ChildProcessHost::kInvalidUniqueID &&
On 2015/01/17 at 00:05:28, nasko (very slow until 02-02) wrote:
> RenderProcessHost is what hosts Blink renderers. ChildProcessHost is for all
other types of processes. While this statement might be ok in practice, it feels
strange to mix up the two types.

My understanding was that a RenderProcess was a type of ChildProcess. Anyhow,
RPH::GetID() says:
"// This will never return ChildProcessHost::kInvalidUniqueID."
so I guess checking against that is the right move, isn't?

https://codereview.chromium.org/843583005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:1188: const
ServiceWorkerClientInfo& client_info) {
On 2015/01/17 at 00:25:32, michaeln wrote:
> how is the client_info.client_id field filled in, looks like the renderer
always returns 0 for that value?

Fixed.

> also can you please add a ctor to the ServiceWorkerClientInfo struct?

Done as part of https://codereview.chromium.org/871013003

https://codereview.chromium.org/843583005/diff/1/content/renderer/service_wor...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/843583005/diff/1/content/renderer/service_wor...
content/renderer/service_worker/service_worker_script_context.cc:450: int
request_id, const ServiceWorkerClientInfo& client, bool dummy_client) {
On 2015/01/17 at 00:05:28, nasko (very slow until 02-02) wrote:
> nit: if params don't fit on the line of the method, each param gets its own
line

Done.

[mlamouri (slow - plz ping), 2015-01-26 15:13:10]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org
- nasko@chromium.org

[mlamouri (slow - plz ping), 2015-01-26 15:13:11]

I've merged https://codereview.chromium.org/844313002/ into this CL per jochen@
request.

Reviewers:
+jochen@ for content/ and chrome/
+mkwst@ for security review

CC:
+nasko@ and creis@ who were on the hook to review 844313002 but are busy this
week.

[mlamouri (slow - plz ping), 2015-01-26 15:17:24]

After discussion with jochen@, we decided to restrict this to same origin,
keeping the option to open to cross origin later. Given that the restriction is
there for security reason, I've added a check on the browser process and the
worker process will be killed if a cross origin goes trough.

[jochen (gone - plz use gerrit), 2015-01-26 15:43:53]

can you add a browser test (or layout test with the blink cl)?

https://codereview.chromium.org/843583005/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/843583005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:172: void
DocumentAvailableInMainFrame() override {
any reason you're waiting until the document is parsed (instead of going for
RenderViewCreated)?

https://codereview.chromium.org/843583005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:206: };
disallow copy/assign

https://codereview.chromium.org/843583005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:206: };
disallow copy/assign

https://codereview.chromium.org/843583005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:217: : nullptr;
can you also bail out if the process is an isolated guest?

The comment says that an <appview> is not isolated (in which case, it's in the
same storage partition as regular content anyways)... i guess it's fine to allow
window.open for appviews

https://codereview.chromium.org/843583005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:240: if
(render_process_host->GetHandle() != base::kNullProcessHandle)
should be render_process_host->ReceivedBadMessage()

[mlamouri (slow - plz ping), 2015-01-26 16:01:41]

Service Worker is using LayoutTests for anything that can be tested using test
harness to be able to share them with W3C. This is the case here so I will add
LayoutTests.

https://codereview.chromium.org/843583005/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/843583005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:172: void
DocumentAvailableInMainFrame() override {
On 2015/01/26 at 15:43:53, jochen (slow) wrote:
> any reason you're waiting until the document is parsed (instead of going for
RenderViewCreated)?

The document needs to be controlled which will happen before
|DocumentAvailableInMainFrame| is called.

https://codereview.chromium.org/843583005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:206: };
On 2015/01/26 at 15:43:53, jochen (slow) wrote:
> disallow copy/assign

Done.

https://codereview.chromium.org/843583005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:217: : nullptr;
On 2015/01/26 at 15:43:53, jochen (slow) wrote:
> can you also bail out if the process is an isolated guest?
> 
> The comment says that an <appview> is not isolated (in which case, it's in the
same storage partition as regular content anyways)... i guess it's fine to allow
window.open for appviews

Done. Though, the issue isn't really storage partition here. Without the check,
the storage partition would match what's expected given that the URL would be
opened in a new browser tab - and will have a browser storage partition.

https://codereview.chromium.org/843583005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:240: if
(render_process_host->GetHandle() != base::kNullProcessHandle)
On 2015/01/26 at 15:43:53, jochen (slow) wrote:
> should be render_process_host->ReceivedBadMessage()

Done.

[jochen (gone - plz use gerrit), 2015-01-27 09:54:45]

https://codereview.chromium.org/843583005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/843583005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:172: void
DocumentAvailableInMainFrame() override {
i guess you'll want the first signal after didCreateData source which is
didStartProvisionalLoad

https://codereview.chromium.org/843583005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:243:
DCHECK(web_contents);
why not if (web_contents) new Window....

[mlamouri (slow - plz ping), 2015-01-27 15:42:08]

Review comments applied. PTAL

https://codereview.chromium.org/843583005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/843583005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:172: void
DocumentAvailableInMainFrame() override {
On 2015/01/27 at 09:54:45, jochen (slow) wrote:
> i guess you'll want the first signal after didCreateData source which is
didStartProvisionalLoad

My mistake, the controllee is added via a url request interceptor. I'm now using
didCommitProvisionalLoad signal.

https://codereview.chromium.org/843583005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:243:
DCHECK(web_contents);
On 2015/01/27 at 09:54:45, jochen (slow) wrote:
> why not if (web_contents) new Window....

Because |web_contents| should not be nullptr here. Temporarily, Android and iOS
will return nullptr but we shouldn't run this code on iOS anyway and I will
handle Android in a follow-up.

[jochen (gone - plz use gerrit), 2015-01-27 15:42:58]

lgtm

[michaeln, 2015-01-27 23:49:09]

lgtm 2

[Mike West, 2015-01-28 13:34:59]

LGTM 3. Would you mind adding links to the subsequent patches in the CL
description?

[Michael van Ouwerkerk, 2015-01-30 16:21:01]

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

[Michael van Ouwerkerk, 2015-01-30 16:21:02]

https://codereview.chromium.org/843583005/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/843583005/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:220: const
WindowOpenedCallback& callback) {
Please document that this callback will be called on the IO thread.

https://codereview.chromium.org/843583005/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:244: false);
nit: please comment what false is for here. It is "is_renderer_initiated"
apparently. Shouldn't it be true then, as this call comes from a javascript call
in the renderer?

[Bernhard Bauer, 2015-02-04 18:01:35]

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

[Bernhard Bauer, 2015-02-04 18:01:36]

Drive-by review!

https://codereview.chromium.org/843583005/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/843583005/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:157: using
WindowOpenedCallback = base::Callback<void(int,int)>;
Nit: space after comma.

https://codereview.chromium.org/843583005/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:169: ,
callback_(callback)
Comma goes on the previous line, same for for opening brace. (Closing brace can
go on the next line or the same one. In general, clang-format will Do The Right
Thing here and cause annoying reviewers to shut up ;-)

[mlamouri (slow - plz ping), 2015-02-04 23:23:23]

Drive-by review comments applied. Landing.

https://codereview.chromium.org/843583005/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/843583005/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:220: const
WindowOpenedCallback& callback) {
On 2015/01/30 at 16:21:02, Michael van Ouwerkerk wrote:
> Please document that this callback will be called on the IO thread.

Done.

https://codereview.chromium.org/843583005/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:244: false);
On 2015/01/30 at 16:21:01, Michael van Ouwerkerk wrote:
> nit: please comment what false is for here. It is "is_renderer_initiated"
apparently. Shouldn't it be true then, as this call comes from a javascript call
in the renderer?

Right. Changed to true and added the comment.

https://codereview.chromium.org/843583005/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/843583005/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:157: using
WindowOpenedCallback = base::Callback<void(int,int)>;
On 2015/02/04 at 18:01:36, Bernhard Bauer wrote:
> Nit: space after comma.

Done.

https://codereview.chromium.org/843583005/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:169: ,
callback_(callback)
On 2015/02/04 at 18:01:36, Bernhard Bauer wrote:
> Comma goes on the previous line, same for for opening brace. (Closing brace
can go on the next line or the same one. In general, clang-format will Do The
Right Thing here and cause annoying reviewers to shut up ;-)

Done. I will being annoyed by reviewers :)

https://codereview.chromium.org/843583005/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:169: ,
callback_(callback)
On 2015/02/04 at 18:01:36, Bernhard Bauer wrote:
> Comma goes on the previous line, same for for opening brace. (Closing brace
can go on the next line or the same one. In general, clang-format will Do The
Right Thing here and cause annoying reviewers to shut up ;-)

Done. I like to be annoyed by reviewers :)

[mlamouri (slow - plz ping), 2015-02-04 23:23:28]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-02-04 23:24:46]

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

[commit-bot: I haz the power, 2015-02-05 00:57:37]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-05 00:59:26]

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