New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 875703 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

wpt/client-hints/accept_ch.tentative.https.html test is failing

Project Member Reported by tbansal@chromium.org, Aug 20

Issue description

wpt/client-hints/accept_ch.tentative.https.html test is failing, and the test was disabled in https://chromium-review.googlesource.com/c/chromium/src/+/1179511.

We need to figure out the root cause, and re-enable the test. 
 
Cc: yukiy@google.com yukishiino@chromium.org

Comment 2 by y...@yoav.ws, Aug 23

Looking at https://wpt.fyi/results/client-hints?label=experimental I see a lot of other red tests.

Tarun - will you be able to take a look?
Components: Blink>Bindings
Just FYI, yukiy@ is working on re-implementation of event listeners at https://chromium-review.googlesource.com/c/chromium/src/+/1172234 , and that patch will be going to fix this issue.

Cc: mkwst@chromium.org
As pointed out by mkwst@, seems like the tests are still mostly red :/
tbansal@, yukiy@ - any plans to work on that in the near future?
Cc: -yukishiino@chromium.org -yukiy@google.com yukiy@chromium.org
Owner: yukishiino@chromium.org
Assigning to yukishiino@ for triage.
Cc: yukishiino@chromium.org
Owner: tbansal@chromium.org
I checked the following test and the test successfully passes *when* I allowed a popup window on the page.
https://w3c-test.org/client-hints/accept_ch.tentative.https.html

I guess that the test is failing just because it cannot open a new popup window.  Event listener related things should have already been fixed as yukiy@ already completed their work.

Status: Started (was: Assigned)
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 12

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

commit 837bac24105bf8e59c1079f8ef7804d79e43b721
Author: Tarun Bansal <tbansal@chromium.org>
Date: Wed Dec 12 06:59:01 2018

Expose client hints as a keyed service

Instead of static class, expose client hints as a keyed service.
Next CL would add a new abstract class client_hints_controller
in //content, and make client hints in //chrome, a derived class
of the client_hints_controller. This would allow
content to directly call the methods to determine which client
hint headers should be added.

Change-Id: I8971e77a23dd49f2f25ba8036ca8ea76c24ca2a4
Bug: 875703
Reviewed-on: https://chromium-review.googlesource.com/c/1368208
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615826}
[modify] https://crrev.com/837bac24105bf8e59c1079f8ef7804d79e43b721/chrome/browser/BUILD.gn
[modify] https://crrev.com/837bac24105bf8e59c1079f8ef7804d79e43b721/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/837bac24105bf8e59c1079f8ef7804d79e43b721/chrome/browser/client_hints/client_hints.cc
[modify] https://crrev.com/837bac24105bf8e59c1079f8ef7804d79e43b721/chrome/browser/client_hints/client_hints.h
[add] https://crrev.com/837bac24105bf8e59c1079f8ef7804d79e43b721/chrome/browser/client_hints/client_hints_factory.cc
[add] https://crrev.com/837bac24105bf8e59c1079f8ef7804d79e43b721/chrome/browser/client_hints/client_hints_factory.h

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 9

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

commit 1ff665c8af38fa71e6e885a9545b281cf043931c
Author: Tarun Bansal <tbansal@chromium.org>
Date: Wed Jan 09 16:35:52 2019

Move client hints to content

Currently, the logic to attach client hint headers lives in
//chrome. This CL moves that logic to the //content layer.

In content/browser/frame_host/navigation_request.cc,
a delegate is called to determine which client hints should
be attached when a navigation starts or is redirected.

Client hints is a property of Web platform, and so it's
reasonable for the related logic to live in //content.
Further, doing so would make it possible to write appropriate
web platform tests to test its functionality.

Note that client_hints_browsertest.cc already tests all
of the client hints pipeline end-to-end.

Change-Id: I34b8a197252de3d1ea339e4086ed664ebb41f671
Bug: 875703
Reviewed-on: https://chromium-review.googlesource.com/c/1376709
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Luke Halliwell <halliwell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621176}
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/android_webview/browser/aw_browser_context.cc
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/android_webview/browser/aw_browser_context.h
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/chrome/browser/client_hints/client_hints.cc
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/chrome/browser/client_hints/client_hints.h
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/chrome/browser/client_hints/client_hints_factory.cc
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/chrome/browser/client_hints/client_hints_factory.h
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/chrome/browser/profiles/off_the_record_profile_impl.cc
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/chrome/browser/profiles/off_the_record_profile_impl.h
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/chrome/browser/profiles/profile_impl.cc
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/chrome/browser/profiles/profile_impl.h
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/chrome/test/base/testing_profile.cc
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/chrome/test/base/testing_profile.h
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/chromecast/browser/cast_browser_context.cc
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/chromecast/browser/cast_browser_context.h
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/content/public/browser/BUILD.gn
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/content/public/browser/browser_context.h
[add] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/content/public/browser/client_hints_controller_delegate.h
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/content/public/test/test_browser_context.cc
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/content/public/test/test_browser_context.h
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/content/shell/browser/shell_browser_context.cc
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/content/shell/browser/shell_browser_context.h
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/headless/lib/browser/headless_browser_context_impl.cc
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/headless/lib/browser/headless_browser_context_impl.h
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/webrunner/browser/webrunner_browser_context.cc
[modify] https://crrev.com/1ff665c8af38fa71e6e885a9545b281cf043931c/webrunner/browser/webrunner_browser_context.h

Comment 10 by tbansal@chromium.org, Today (13 hours ago)

I have moved the attaching of headers to the //content layer. I think the steps left are:

1. Add PersistClientHintsHeader() method to client_hints_controller_delegate.h. Then, https://cs.chromium.org/chromium/src/chrome/browser/client_hints/client_hints_observer.cc would need to call this method. client_hints_controller_delegate.h would then modify the settings instead of the client_hints_observer.cc doing it directly.

2. Add a test-only derived class for https://cs.chromium.org/chromium/src/content/public/browser/client_hints_controller_delegate.h

3. Plumb it via test content shell similar to OnSetPermission(): https://cs.chromium.org/search/?q=OnSetPermission+f:test&type=cs

We will need to use the binding to set the client hints preferences on disk.

4. Connect the binding: https://cs.chromium.org/chromium/src/content/shell/test_runner/test_runner.cc?rcl=89140af1a339c40bc2e1398939df41e3729d97c7&l=565

5.  Use the binding  to set the client hints https://cs.chromium.org/chromium/src/third_party/blink/web_tests/external/wpt/resources/chromium/generic_sensor_mocks.js?rcl=89140af1a339c40bc2e1398939df41e3729d97c7&l=196

Comment 11 by tbansal@chromium.org, Today (13 hours ago)

For the tests, the test-only derived class would maintain an in-memory record of which origins opted-in to which subset of client hints, and for how long. Essentially, the test delegate is an in-memory substitute for the on-disk delegate:

https://cs.chromium.org/chromium/src/chrome/browser/client_hints/client_hints.cc?sq=package:chromium&g=0&l=218

The values for different client hints are currently decided by client_hints.cc which is in //chrome. For the test delegate, it's unclear if the values really matter since the purpose is to just check if ACL header is supported or not. However, if it's necessary, we would need to move the value-generation part to //content instead of //chrome.

Sign in to add a comment