Specify the first-party-for-cookies URL for HTTP requests...

Specify the first-party-for-cookies URL for HTTP requests
issued by plugins.

Fix formatting nits.

R=abarth,darin,eroman
BUG=36957
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=40331

[wtc, 2010-02-11 03:41:28]

Could you suggest someone who knows WebKit and plugins to
work with me on this bug?

This patch still needs polishing, so you just need to review
the approach, and answer my questions (marked with TODO
comments).  Thanks.

[abarth-chromium, 2010-02-11 05:08:00]

I'd prefer if we called the value first_party_for_cookies in network-related
code.  Other than that, looks like a reasonable approach.

[darin (slow to review), 2010-02-11 09:18:55]

http://codereview.chromium.org/601038/diff/1/5
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/601038/diff/1/5#newcode559
webkit/glue/webplugin_impl.cc:559: // TODO(wtc): Is there a better way to
construct a WebURL object from
nope, we should probably add one.

http://codereview.chromium.org/601038/diff/1/5#newcode913
webkit/glue/webplugin_impl.cc:913: // TODO(wtc): Do we need to call CompleteURL
to convert a const char* to a
it seems to me that page_url should already be an absolute url, so calling
CompleteURL should be unnecessary.  just doing GURL(page_url) is probably
sufficient.

http://codereview.chromium.org/601038/diff/1/5#newcode1001
webkit/glue/webplugin_impl.cc:1001: GURL complete_page_url =
CompleteURL(page_url);
ditto.

[eroman, 2010-02-19 00:09:23]

Rather than plumbing through the page URL, it looks like you are only accessing
it from WebPluginImpl::RouteToFrame().

Would it be sufficient to simply get at the information directly in
RouteToFrame()? Probably something like:

  webframe_->top()->url()

As a side-comment, do you know why WebPluginProxy::HandleURLRequest() is using
all those C-Strings for URLs rather than GURL or std::string? (cstrings always
concern me, for problems like embedded nulls; although technically if the URL
was canonicalized we shouldn't have those, but still).

Otherwise the general approach looks sound, but I don't really know much on the
plugins setup.

[wtc, 2010-02-27 01:18:57]

abarth, darin, eroman:

This CL is now ready for review.  Please review Patch Set 4.

Thanks to eroman's observation, I found that the URL of the
main page is readily available in WebPluginImpl, so I don't
need to plumb it through from WebPluginDelegateProxy.

darin: webframe_->top() can return NULL.  Do I need a null
check for webframe_->top() in those two places?

[abarth_gmail.com, 2010-02-27 01:30:51]

LGTM modulo null checking issues.

[eroman, 2010-02-27 01:53:16]

LGTM

[darin (slow to review), 2010-02-27 05:59:04]

http://codereview.chromium.org/601038/diff/6006/6007
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/601038/diff/6006/6007#newcode560
webkit/glue/webplugin_impl.cc:560:
request.setFirstPartyForCookies(webframe_->top()->url());
it should be safe to assume that top will be non-null here.

http://codereview.chromium.org/601038/diff/6006/6007#newcode956
webkit/glue/webplugin_impl.cc:956:
info.request.setFirstPartyForCookies(webframe_->top()->url());
would it help if we exposed a WebDocument::firstPartyForCookies property?  adam?

[abarth-chromium, 2010-02-27 07:12:27]

http://codereview.chromium.org/601038/diff/6006/6007
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/601038/diff/6006/6007#newcode956
webkit/glue/webplugin_impl.cc:956:
info.request.setFirstPartyForCookies(webframe_->top()->url());
Yeah.  That's probably a good idea.  I feel slightly sad about proliferating the
API, but it's better than assuming the semantics here.

[wtc, 2010-03-02 19:25:12]

I filed webkit bug https://bugs.webkit.org/show_bug.cgi?id=35592
on exposing the WebDocument::firstPartyForCookies property.

[wtc, 2010-03-04 00:58:50]

On 2010/02/19 00:09:23, eroman wrote:
>
> As a side-comment, do you know why WebPluginProxy::HandleURLRequest() is using
> all those C-Strings for URLs rather than GURL or std::string? (cstrings always
> concern me, for problems like embedded nulls; although technically if the URL
> was canonicalized we shouldn't have those, but still).

I forgot to respond to this.  I also noticed this problem when
I wrote the original patch.  I don't know why, and didn't
investigate how much work it is to change them to pass GURLs.