New issue
Advanced search Search tips

Issue 792676 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Oct 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug

Blocking:
issue 721395



Sign in to add a comment

Convert headless specific cookie accesses to Mojo

Project Member Reported by rdsmith@chromium.org, Dec 6 2017

Issue description

Convert all instances of access to net::CookieStore over to network::mojom::CookieManager. 

There are uses of cookie store in the following headless specific files:
	headless/lib
	headless/lib/browser
	headless/public/util
	headless/public/util/testing

 
Blocking: 721395
Cc: -rdsmith@chromium.org

Comment 3 by dxie@chromium.org, May 17 2018

Labels: -Pri-3 Proj-Servicification-Canary OS-Chrome Pri-1
Labels: -OS-Chrome OS-Linux OS-Mac OS-Windows
This is applicable on all desktop platforms (other than Chrome OS) but I'm not sure it blocks the canary. I guess it would if people are running automation against an installation of Chrome canary.

Comment 5 by dxie@chromium.org, Jun 7 2018

Labels: -Proj-Servicification-Canary Proj-Servicification

Comment 6 by dxie@chromium.org, Jun 12 2018

Labels: Hotlist-KnownIssue
Cc: pfeldman@chromium.org
There is only a single reference to net::CookieStore remaining in //headless. It is in HeadlessURLRequestContextGetter which is unused when the Network Service flag is enabled.

pfeldman@, what's the plan for configuring StoragePartitions in headless?
Cc: -pfeldman@chromium.org caseq@chromium.org
Over to caseq.
Status: Assigned (was: Available)
Reilly, I was going to work on getting rid of HeadlessURLRequestContextGetter in a very near future, so it'll hopefully be gone in a matter of few days.
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac

commit 1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac
Author: Andrey Kosyakov <caseq@chromium.org>
Date: Thu Oct 11 19:40:33 2018

Support network service in headless

- extract all URLRequestContextGettter and NetworkContext creation logic into HeadlessRequestContextManager;
- reconcile network service and non-network service code paths as much as possible;
- fix a couple of issues with handling of redirect URLs in DevToolsURLLoaderInterceptor that
    were different from URLRequestJob-based interception and caused headless tests to fail;
- plumb a call to NetworkServiceTestHelper::RegisterNetworkBinders() in headless utility
    process when running tests to let network service tests inject the magic client certificates;
- update traffic annotations as they just moved;

Bug:  838291 , 792676 

Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ia29143a85b0c0293fae13079ec9f88ff5ba924f9
Reviewed-on: https://chromium-review.googlesource.com/c/1263566
Reviewed-by: Georges Khalil <georgesak@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598893}
[modify] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/content/browser/devtools/devtools_url_loader_interceptor.cc
[modify] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/headless/BUILD.gn
[modify] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/headless/lib/browser/DEPS
[modify] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/headless/lib/browser/headless_browser_context_impl.cc
[modify] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/headless/lib/browser/headless_browser_context_impl.h
[modify] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/headless/lib/browser/headless_browser_impl.cc
[modify] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/headless/lib/browser/headless_browser_impl.h
[modify] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/headless/lib/browser/headless_content_browser_client.cc
[modify] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/headless/lib/browser/headless_content_browser_client.h
[add] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/headless/lib/browser/headless_request_context_manager.cc
[add] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/headless/lib/browser/headless_request_context_manager.h
[delete] https://crrev.com/2863ae23721602d503a4a553659481deb2df15a1/headless/lib/browser/headless_url_request_context_getter.cc
[delete] https://crrev.com/2863ae23721602d503a4a553659481deb2df15a1/headless/lib/browser/headless_url_request_context_getter.h
[modify] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/headless/lib/utility/headless_content_utility_client.cc
[modify] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/headless/lib/utility/headless_content_utility_client.h
[modify] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/headless/test/data/protocol/helpers/http-interceptor.js
[modify] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/headless/test/headless_test_launcher.cc
[modify] https://crrev.com/1ca23fca21d3b83ee0d03cf1c6c011bdc3fb9fac/tools/traffic_annotation/summary/annotations.xml

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ed8c927801fdf271cff29cb95a46546cef0a9239

commit ed8c927801fdf271cff29cb95a46546cef0a9239
Author: Mostyn Bramley-Moore <mostynb@vewd.com>
Date: Fri Oct 12 09:11:23 2018

[jumbo] fixup after recent headless network service work

Followup to https://chromium-review.googlesource.com/c/chromium/src/+/1263566
which introduced a namespace clash between ::network and ::headless::network.

Also, rename one of two kProductName's.

TBR=caseq@chromium.org

Bug:  838291 , 792676 
Change-Id: Ie1fc9ab2096a09651ba2dd45aa262ab30bfd3efc
Reviewed-on: https://chromium-review.googlesource.com/c/1277992
Commit-Queue: Mostyn Bramley-Moore <mostynb@vewd.com>
Reviewed-by: Daniel Bratell <bratell@opera.com>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599150}
[modify] https://crrev.com/ed8c927801fdf271cff29cb95a46546cef0a9239/headless/lib/browser/headless_request_context_manager.cc
[modify] https://crrev.com/ed8c927801fdf271cff29cb95a46546cef0a9239/headless/public/headless_browser.cc

Status: Fixed (was: Assigned)
Project Member

Comment 13 by bugdroid1@chromium.org, Oct 17

Labels: merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1edc83ad4eeceb156b76adfb7b4bb4802c36dbf7

commit 1edc83ad4eeceb156b76adfb7b4bb4802c36dbf7
Author: Mostyn Bramley-Moore <mostynb@vewd.com>
Date: Wed Oct 17 18:13:22 2018

[jumbo] fixup after recent headless network service work

Followup to https://chromium-review.googlesource.com/c/chromium/src/+/1263566
which introduced a namespace clash between ::network and ::headless::network.

Also, rename one of two kProductName's.

TBR=caseq@chromium.org

Bug:  838291 , 792676 
Change-Id: Ie1fc9ab2096a09651ba2dd45aa262ab30bfd3efc
Reviewed-on: https://chromium-review.googlesource.com/c/1277992
Commit-Queue: Mostyn Bramley-Moore <mostynb@vewd.com>
Reviewed-by: Daniel Bratell <bratell@opera.com>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#599150}(cherry picked from commit ed8c927801fdf271cff29cb95a46546cef0a9239)
Reviewed-on: https://chromium-review.googlesource.com/c/1286663
Reviewed-by: Mostyn Bramley-Moore <mostynb@vewd.com>
Cr-Commit-Position: refs/branch-heads/3578@{#91}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/1edc83ad4eeceb156b76adfb7b4bb4802c36dbf7/headless/lib/browser/headless_request_context_manager.cc
[modify] https://crrev.com/1edc83ad4eeceb156b76adfb7b4bb4802c36dbf7/headless/public/headless_browser.cc

Labels: CommitLog-Audit-Violation Merge-Without-Approval M-71
Here's a summary of the rules that were executed: 
 - OnlyMergeApprovedChange: Rule Failed -- Revision 1edc83ad4eeceb156b76adfb7b4bb4802c36dbf7 was merged to refs/branch-heads/3578 branch with no merge approval from a TPM! 
Please explain why this change was merged to the branch!
 - AcknowledgeMerge: Notification Required -- 
Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/1edc83ad4eeceb156b76adfb7b4bb4802c36dbf7

Commit: 1edc83ad4eeceb156b76adfb7b4bb4802c36dbf7
Author: mostynb@vewd.com
Commiter: mostynb@vewd.com
Date: 2018-10-17 18:13:22 +0000 UTC

[jumbo] fixup after recent headless network service work

Followup to https://chromium-review.googlesource.com/c/chromium/src/+/1263566
which introduced a namespace clash between ::network and ::headless::network.

Also, rename one of two kProductName's.

TBR=caseq@chromium.org

Bug:  838291 , 792676 
Change-Id: Ie1fc9ab2096a09651ba2dd45aa262ab30bfd3efc
Reviewed-on: https://chromium-review.googlesource.com/c/1277992
Commit-Queue: Mostyn Bramley-Moore <mostynb@vewd.com>
Reviewed-by: Daniel Bratell <bratell@opera.com>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#599150}(cherry picked from commit ed8c927801fdf271cff29cb95a46546cef0a9239)
Reviewed-on: https://chromium-review.googlesource.com/c/1286663
Reviewed-by: Mostyn Bramley-Moore <mostynb@vewd.com>
Cr-Commit-Position: refs/branch-heads/3578@{#91}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
Cc: mstensho@chromium.org
May I pls know why did this got merge to M71 without merge request and approval? Pls see comment #14.


+mstensho@ cl reviewer. 
Cc: most...@vewd.com
I don't know (I wasn't aware of the merge).

But my guess is that the original fix - https://chromium-review.googlesource.com/c/chromium/src/+/1263566 - landed before branching, but the compilation fix - https://chromium-review.googlesource.com/c/1277992 landed after branching, so it had to be merged, in order to be able to compile on the branch as well.

But I'm just guessing. Mostyn, you probably know?
@govind: the merge was approved in https://bugs.chromium.org/p/chromium/issues/detail?id=895751

I guess I should have added 895751 in the commit message- sorry (this was the first time I landed something on a release branch).
(I meant to say 792676 should have been added to the commit message.)
Labels: -Merge-Without-Approval
Ok, got it. Removing "Merge-Without-Approval" per comment #18. Thank you.

Sign in to add a comment