Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2777783002:
Remove user agent from HeadlessBrowserContextOptions. (Closed)
DescriptionRemove user agent from HeadlessBrowserContextOptions.
BUG=
Review-Url: https://codereview.chromium.org/2777783002
Cr-Commit-Position: refs/heads/master@{#460987}
Committed: https://chromium.googlesource.com/chromium/src/+/255d63a0a5669310b7e28f3f01ff7c75c9645d06
Patch Set 1 #
Total comments: 4
Patch Set 2 : Remove user agent from BrowserContextOptions #Patch Set 3 : Remove --user-agent flag #
Messages
Total messages: 19 (10 generated)
irisu@chromium.org changed reviewers: + skyostil@chromium.org
Thanks! One comment below. https://codereview.chromium.org/2777783002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_content_browser_client.cc (right): https://codereview.chromium.org/2777783002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_content_browser_client.cc:192: command_line->AppendSwitchNative(switches::kUserAgent, I'm wondering how things worked before when we didn't tell the renderer about the user agent? Would you mind adding a simple test for this? In any case something isn't quite wired up right because the user agent is an option on the BrowserContext[1], which means you can have multiple user agents in one headless instance, whereas this code uses a single global user agent. Does it look like we can support multiple user agents in the first place? [1] https://cs.chromium.org/chromium/src/headless/lib/browser/headless_browser_co...
https://codereview.chromium.org/2777783002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_content_browser_client.cc (right): https://codereview.chromium.org/2777783002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_content_browser_client.cc:192: command_line->AppendSwitchNative(switches::kUserAgent, On 2017/03/27 15:52:08, Sami wrote: > I'm wondering how things worked before when we didn't tell the renderer about > the user agent? Would you mind adding a simple test for this? > > In any case something isn't quite wired up right because the user agent is an > option on the BrowserContext[1], which means you can have multiple user agents > in one headless instance, whereas this code uses a single global user agent. > Does it look like we can support multiple user agents in the first place? > > [1] > https://cs.chromium.org/chromium/src/headless/lib/browser/headless_browser_co... It looks as though the user agent header is set using HeadlessContentClient::GetUserAgent(), which takes it from the HeadlessBrowser::Options rather than the BrowserContextOptions. https://cs.chromium.org/chromium/src/headless/lib/headless_content_client.cc?... I checked this by modifying that method to return a hardcoded string and running: cr run headless_shell --remote-debugging-port=9222 --disable-gpu http://www.whatsmyua.com/ To my understanding, there is only one instance of the HeadlessContentClient for each HeadlessBrowser, is that correct?
https://codereview.chromium.org/2777783002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_content_browser_client.cc (right): https://codereview.chromium.org/2777783002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_content_browser_client.cc:192: command_line->AppendSwitchNative(switches::kUserAgent, On 2017/03/29 05:51:33, irisu wrote: > On 2017/03/27 15:52:08, Sami wrote: > > I'm wondering how things worked before when we didn't tell the renderer about > > the user agent? Would you mind adding a simple test for this? > > > > In any case something isn't quite wired up right because the user agent is an > > option on the BrowserContext[1], which means you can have multiple user agents > > in one headless instance, whereas this code uses a single global user agent. > > Does it look like we can support multiple user agents in the first place? > > > > [1] > > > https://cs.chromium.org/chromium/src/headless/lib/browser/headless_browser_co... > > It looks as though the user agent header is set using > HeadlessContentClient::GetUserAgent(), which takes it from the > HeadlessBrowser::Options rather than the BrowserContextOptions. > https://cs.chromium.org/chromium/src/headless/lib/headless_content_client.cc?... > > I checked this by modifying that method to return a hardcoded string and > running: cr run headless_shell --remote-debugging-port=9222 --disable-gpu > http://www.whatsmyua.com/ > > To my understanding, there is only one instance of the HeadlessContentClient for > each HeadlessBrowser, is that correct? That's right, that's the default user agent for the entire browser. Maybe we should be using this instead to override the user agent: https://chromedevtools.github.io/debugger-protocol-viewer/tot/Network/#method... Also, given this we should probably remove the per-context user agent option.
https://codereview.chromium.org/2777783002/diff/1/headless/lib/browser/headle... File headless/lib/browser/headless_content_browser_client.cc (right): https://codereview.chromium.org/2777783002/diff/1/headless/lib/browser/headle... headless/lib/browser/headless_content_browser_client.cc:192: command_line->AppendSwitchNative(switches::kUserAgent, On 2017/03/29 10:46:07, Sami wrote: > On 2017/03/29 05:51:33, irisu wrote: > > On 2017/03/27 15:52:08, Sami wrote: > > > I'm wondering how things worked before when we didn't tell the renderer > about > > > the user agent? Would you mind adding a simple test for this? > > > > > > In any case something isn't quite wired up right because the user agent is > an > > > option on the BrowserContext[1], which means you can have multiple user > agents > > > in one headless instance, whereas this code uses a single global user agent. > > > Does it look like we can support multiple user agents in the first place? > > > > > > [1] > > > > > > https://cs.chromium.org/chromium/src/headless/lib/browser/headless_browser_co... > > > > It looks as though the user agent header is set using > > HeadlessContentClient::GetUserAgent(), which takes it from the > > HeadlessBrowser::Options rather than the BrowserContextOptions. > > > https://cs.chromium.org/chromium/src/headless/lib/headless_content_client.cc?... > > > > I checked this by modifying that method to return a hardcoded string and > > running: cr run headless_shell --remote-debugging-port=9222 --disable-gpu > > http://www.whatsmyua.com/ > > > > To my understanding, there is only one instance of the HeadlessContentClient > for > > each HeadlessBrowser, is that correct? > > That's right, that's the default user agent for the entire browser. Maybe we > should be using this instead to override the user agent: > https://chromedevtools.github.io/debugger-protocol-viewer/tot/Network/#method... > > Also, given this we should probably remove the per-context user agent option. Removed user agent from BrowserContextOptions. I've created another patch which uses devtools instead of the command line switch to set the user agent. Is that what you had in mind? If so, I can remove the flag from this patch and this patch would only remove the user agent from BrowserContextOptions. https://codereview.chromium.org/2785083002
On 2017/03/30 07:06:07, irisu wrote: > Removed user agent from BrowserContextOptions. > > I've created another patch which uses devtools instead of the command line > switch to set the user agent. Is that what you had in mind? If so, I can remove > the flag from this patch and this patch would only remove the user agent from > BrowserContextOptions. > https://codereview.chromium.org/2785083002 Thanks, that seems like the way to go! lgtm to remove the setting from BrowserContextOptions with this patch. We could still add --user-agent to set the global user agent, but I'm leaning against it with the idea of keeping headless shell as simple as possible and making DevTools the primary way to interact with the browser.
Description was changed from ========== Add user-agent command line flag to headless. This is needed to pass testDeviceName and testUserAgent from chromedriver tests. BUG=604324 ========== to ========== Remove user agent from HeadlessBrowserContextOptions. BUG= ==========
The CQ bit was checked by irisu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by irisu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2777783002/#ps40001 (title: "Remove --user-agent flag")
On 2017/03/30 11:27:47, Sami wrote: > On 2017/03/30 07:06:07, irisu wrote: > > Removed user agent from BrowserContextOptions. > > > > I've created another patch which uses devtools instead of the command line > > switch to set the user agent. Is that what you had in mind? If so, I can > remove > > the flag from this patch and this patch would only remove the user agent from > > BrowserContextOptions. > > https://codereview.chromium.org/2785083002 > > Thanks, that seems like the way to go! lgtm to remove the setting from > BrowserContextOptions with this patch. > > We could still add --user-agent to set the global user agent, but I'm leaning > against it with the idea of keeping headless shell as simple as possible and > making DevTools the primary way to interact with the browser. Agreed. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490924067205400,
"parent_rev": "9200fc1d36c47ee62054a4e642c171defe986bbd", "commit_rev":
"255d63a0a5669310b7e28f3f01ff7c75c9645d06"}
Message was sent while issue was closed.
Description was changed from ========== Remove user agent from HeadlessBrowserContextOptions. BUG= ========== to ========== Remove user agent from HeadlessBrowserContextOptions. BUG= Review-Url: https://codereview.chromium.org/2777783002 Cr-Commit-Position: refs/heads/master@{#460987} Committed: https://chromium.googlesource.com/chromium/src/+/255d63a0a5669310b7e28f3f01ff... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/255d63a0a5669310b7e28f3f01ff... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
