Mojo: add support for cookies to HTMLViewer.

Mojo: add support for cookies to HTMLViewer.

BUG=394644
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284865

[darin (slow to review), 2014-07-22 23:36:48]

The |origin| plumbing in this CL is all a bit incomplete. Doing it properly is
gated on the impersonation stuff I was talking about on mojo-dev@.

[jamesr, 2014-07-22 23:53:44]

lgtm

i think your CookieStoreImpl will leak until my binding patch lands

https://codereview.chromium.org/409883007/diff/20001/mojo/services/html_viewe...
File mojo/services/html_viewer/webcookiejar_impl.cc (right):

https://codereview.chromium.org/409883007/diff/20001/mojo/services/html_viewe...
mojo/services/html_viewer/webcookiejar_impl.cc:37: // Wait to ensure the cookie
was set before advancing. That way any
in content's cookie jar Gets are blocking but Sets aren't. do we have to make
this blocking because network requests go on a different pipe and don't have
FIFO with our set calls?

https://codereview.chromium.org/409883007/diff/20001/mojo/services/html_viewe...
mojo/services/html_viewer/webcookiejar_impl.cc:51:
store_.WaitForIncomingMethodCall();
or is it that if we allow multiple set calls to be made asynchronously we won't
know what to wait for here?

https://codereview.chromium.org/409883007/diff/20001/mojo/services/network/co...
File mojo/services/network/cookie_store_impl.h (right):

https://codereview.chromium.org/409883007/diff/20001/mojo/services/network/co...
mojo/services/network/cookie_store_impl.h:28: };
DISALLOW_blahblahblah ?

https://codereview.chromium.org/409883007/diff/20001/mojo/services/public/int...
File mojo/services/public/interfaces/network/cookie_store.mojom (right):

https://codereview.chromium.org/409883007/diff/20001/mojo/services/public/int...
mojo/services/public/interfaces/network/cookie_store.mojom:8: Get(string url) =>
(string cookies);
would be nice to have a type for URLs in order to do some validation (it'd still
serialize as a string, i suppose)

[darin (slow to review), 2014-07-23 00:09:21]

On 2014/07/22 23:53:44, jamesr wrote:
> lgtm
> 
> i think your CookieStoreImpl will leak until my binding patch lands

Hmm, this should get destroyed from ~MessageLoop.


>
https://codereview.chromium.org/409883007/diff/20001/mojo/services/html_viewe...
> File mojo/services/html_viewer/webcookiejar_impl.cc (right):
> 
>
https://codereview.chromium.org/409883007/diff/20001/mojo/services/html_viewe...
> mojo/services/html_viewer/webcookiejar_impl.cc:37: // Wait to ensure the
cookie
> was set before advancing. That way any
> in content's cookie jar Gets are blocking but Sets aren't. do we have to make
> this blocking because network requests go on a different pipe and don't have
> FIFO with our set calls?

Yes, exactly. We can have that once we have support for associated message
pipes,
or we could invent some other solution.


>
https://codereview.chromium.org/409883007/diff/20001/mojo/services/html_viewe...
> mojo/services/html_viewer/webcookiejar_impl.cc:51:
> store_.WaitForIncomingMethodCall();
> or is it that if we allow multiple set calls to be made asynchronously we
won't
> know what to wait for here?

Yes, that would also be an issue. I'm purposefully keeping things simple for
now.


>
https://codereview.chromium.org/409883007/diff/20001/mojo/services/network/co...
> File mojo/services/network/cookie_store_impl.h (right):
> 
>
https://codereview.chromium.org/409883007/diff/20001/mojo/services/network/co...
> mojo/services/network/cookie_store_impl.h:28: };
> DISALLOW_blahblahblah ?

Fixed.


> 
>
https://codereview.chromium.org/409883007/diff/20001/mojo/services/public/int...
> File mojo/services/public/interfaces/network/cookie_store.mojom (right):
> 
>
https://codereview.chromium.org/409883007/diff/20001/mojo/services/public/int...
> mojo/services/public/interfaces/network/cookie_store.mojom:8: Get(string url)
=>
> (string cookies);
> would be nice to have a type for URLs in order to do some validation (it'd
still
> serialize as a string, i suppose)

Yeah, Chrome IPC just serializes a GURL as a std::string. I'm torn on this
issue.

Thanks for the review!

[darin (slow to review), 2014-07-23 00:14:07]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-07-23 00:15:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/darin@chromium.org/409883007/40001

[jamesr, 2014-07-23 00:24:49]

We seem to keep hitting mojom types that serialize as a simpler type but have
additional validation (fixed arrays are one, struct fields that can't really be
null seem like another)

[commit-bot: I haz the power, 2014-07-23 06:36:16]

Change committed as 284865