Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(30)

Issue 2564973002: Add an infobar if a session is being controlled by an automated test. (Closed)

Created:
4 years ago by samuong
Modified:
3 years, 10 months ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add an infobar if a session is being controlled by an automated test. This infobar is only displayed if the browser is launched with the --enable-automation switch. It also disables the developer mode extensions warning bubble. Design doc: https://docs.google.com/document/d/1JYj9K61UyxIYavR8_HATYIglR9T_rDwAtLLsD3fbDQg/edit BUG=chromedriver:1625 TEST=launch with and without --enable-automation, and check for presence of automation infobar Review-Url: https://codereview.chromium.org/2564973002 Cr-Commit-Position: refs/heads/master@{#450257} Committed: https://chromium.googlesource.com/chromium/src/+/7a66b24c10161a5db8668729a97ac95cee9068e2

Patch Set 1 #

Patch Set 2 : tweak wording, temporarily re-introduce load-component-extension #

Total comments: 18

Patch Set 3 : address review comments #

Total comments: 10

Patch Set 4 : address second round of review comments #

Patch Set 5 : make the infobar global (show on all tabs), return a vector icon id #

Patch Set 6 : add overrides to GlobalConfirmInfoBar, make sure infobar ptr is set properly #

Patch Set 7 : change wording of infobar message to match suggestion from ux team #

Total comments: 7

Patch Set 8 : avoid changes to GlobalConfirmInfoBar #

Total comments: 11

Patch Set 9 : make this into a generic "test automation" feature, rather than chromedriver-specific #

Patch Set 10 : address review comments #

Patch Set 11 : make destructor private #

Total comments: 2

Patch Set 12 : fix nit #

Patch Set 13 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+123 lines, -21 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/extensions/extension_message_bubble_factory.cc View 1 2 3 4 5 6 7 8 2 chunks +9 lines, -0 lines 0 comments Download
A chrome/browser/ui/startup/automation_infobar_delegate.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +33 lines, -0 lines 0 comments Download
A chrome/browser/ui/startup/automation_infobar_delegate.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +41 lines, -0 lines 0 comments Download
M chrome/browser/ui/startup/startup_browser_creator_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +10 lines, -1 line 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/chrome_launcher.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +17 lines, -20 lines 0 comments Download
M components/infobars/core/infobar_delegate.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 48 (17 generated)
samuong
Devlin, can you ptal? As discussed earlier, we'll need to treat load-component-extension as load-extension for ...
3 years, 11 months ago (2017-01-24 23:32:38 UTC) #4
samuong
friendly ping, devlin it'd be good to know what you think here, especially with the ...
3 years, 11 months ago (2017-01-26 21:13:24 UTC) #5
Devlin
Hey Sam, sorry for the delay - chromium reviews should go to my @chromium, so ...
3 years, 10 months ago (2017-02-01 23:05:00 UTC) #7
samuong
https://codereview.chromium.org/2564973002/diff/20001/chrome/app/chromium_strings.grd File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/app/chromium_strings.grd#newcode1253 chrome/app/chromium_strings.grd:1253: Your browser is being remotely controlled by ChromeDriver. On ...
3 years, 10 months ago (2017-02-04 00:11:03 UTC) #8
Devlin
Nice! A few last small things and I'd like to take another look before it ...
3 years, 10 months ago (2017-02-06 16:20:22 UTC) #9
samuong
https://codereview.chromium.org/2564973002/diff/40001/chrome/browser/ui/extensions/extension_message_bubble_factory.cc File chrome/browser/ui/extensions/extension_message_bubble_factory.cc (right): https://codereview.chromium.org/2564973002/diff/40001/chrome/browser/ui/extensions/extension_message_bubble_factory.cc#newcode88 chrome/browser/ui/extensions/extension_message_bubble_factory.cc:88: base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); On 2017/02/06 16:20:21, Devlin wrote: ...
3 years, 10 months ago (2017-02-07 19:11:10 UTC) #11
samuong
+pkasting@ for ui/ and infobars/ review also, +pfeldman@ could you let me know if you're ...
3 years, 10 months ago (2017-02-07 19:15:44 UTC) #13
pfeldman
I don't think we should add code into chrome specifically for ChromeDriver. Our source base ...
3 years, 10 months ago (2017-02-07 23:08:50 UTC) #14
Devlin
On 2017/02/07 19:15:44, samuong wrote: > Finally, git-cl refuses to upload chrome/app/generated_resources.grd "because > it's ...
3 years, 10 months ago (2017-02-07 23:38:59 UTC) #15
Peter Kasting
On 2017/02/07 23:08:50, pfeldman wrote: > I don't think we should add code into chrome ...
3 years, 10 months ago (2017-02-08 00:35:38 UTC) #16
Devlin
On 2017/02/08 00:35:38, Peter Kasting wrote: > On 2017/02/07 23:08:50, pfeldman wrote: > > I ...
3 years, 10 months ago (2017-02-08 01:32:51 UTC) #17
Peter Kasting
I won't have time to get to this for the next couple of days, so ...
3 years, 10 months ago (2017-02-08 08:23:44 UTC) #18
pfeldman
I'd suggest that you drop the infobar changes - they are not affecting the security ...
3 years, 10 months ago (2017-02-08 18:35:36 UTC) #19
Peter Kasting
Where does this review stand w.r.t. http://crbug.com/691117 and https://codereview.chromium.org/2685203002/ ? Are we still doing this? ...
3 years, 10 months ago (2017-02-11 01:49:20 UTC) #20
pfeldman
On 2017/02/11 01:49:20, Peter Kasting wrote: > Where does this review stand w.r.t. http://crbug.com/691117 and ...
3 years, 10 months ago (2017-02-11 01:53:41 UTC) #21
Peter Kasting
On 2017/02/11 01:53:41, pfeldman wrote: > On 2017/02/11 01:49:20, Peter Kasting wrote: > > Where ...
3 years, 10 months ago (2017-02-11 02:13:19 UTC) #22
samuong
I've had requests not to make any changes to GlobalConfirmInfoBar, so that we can minimize ...
3 years, 10 months ago (2017-02-13 22:17:01 UTC) #23
pfeldman
My only comment is to call it in a generic manner: --enable-automation, AutomationInfobar, etc. It ...
3 years, 10 months ago (2017-02-13 22:20:05 UTC) #24
samuong
Done.
3 years, 10 months ago (2017-02-13 22:39:58 UTC) #26
pfeldman
lgtm
3 years, 10 months ago (2017-02-13 22:51:29 UTC) #27
Peter Kasting
https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/startup/chromedriver_infobar_delegate.h File chrome/browser/ui/startup/chromedriver_infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/startup/chromedriver_infobar_delegate.h#newcode20 chrome/browser/ui/startup/chromedriver_infobar_delegate.h:20: ~ChromeDriverInfoBarDelegate() override; Nit: This should be private https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/startup/chromedriver_infobar_delegate.h#newcode21 chrome/browser/ui/startup/chromedriver_infobar_delegate.h:21: ...
3 years, 10 months ago (2017-02-13 23:16:52 UTC) #28
samuong
+ericwilligers@ for tools/metrics/histograms/histograms.xml https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/startup/chromedriver_infobar_delegate.h File chrome/browser/ui/startup/chromedriver_infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/startup/chromedriver_infobar_delegate.h#newcode20 chrome/browser/ui/startup/chromedriver_infobar_delegate.h:20: ~ChromeDriverInfoBarDelegate() override; On 2017/02/13 23:16:52, Peter ...
3 years, 10 months ago (2017-02-14 00:19:17 UTC) #30
Devlin
c/b/ui/extensions lgtm
3 years, 10 months ago (2017-02-14 00:22:10 UTC) #31
Peter Kasting
https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/startup/chromedriver_infobar_delegate.h File chrome/browser/ui/startup/chromedriver_infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/startup/chromedriver_infobar_delegate.h#newcode20 chrome/browser/ui/startup/chromedriver_infobar_delegate.h:20: ~ChromeDriverInfoBarDelegate() override; On 2017/02/14 00:19:17, samuong wrote: > On ...
3 years, 10 months ago (2017-02-14 00:23:17 UTC) #32
samuong
https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/startup/chromedriver_infobar_delegate.h File chrome/browser/ui/startup/chromedriver_infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/startup/chromedriver_infobar_delegate.h#newcode20 chrome/browser/ui/startup/chromedriver_infobar_delegate.h:20: ~ChromeDriverInfoBarDelegate() override; On 2017/02/14 00:23:17, Peter Kasting wrote: > ...
3 years, 10 months ago (2017-02-14 00:26:54 UTC) #34
Peter Kasting
LGTM https://codereview.chromium.org/2564973002/diff/200001/chrome/browser/ui/startup/startup_browser_creator_impl.cc File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2564973002/diff/200001/chrome/browser/ui/startup/startup_browser_creator_impl.cc#newcode818 chrome/browser/ui/startup/startup_browser_creator_impl.cc:818: // These info bars are not shown when ...
3 years, 10 months ago (2017-02-14 00:34:27 UTC) #35
samuong
https://codereview.chromium.org/2564973002/diff/200001/chrome/browser/ui/startup/startup_browser_creator_impl.cc File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2564973002/diff/200001/chrome/browser/ui/startup/startup_browser_creator_impl.cc#newcode818 chrome/browser/ui/startup/startup_browser_creator_impl.cc:818: // These info bars are not shown when the ...
3 years, 10 months ago (2017-02-14 00:43:11 UTC) #36
Ilya Sherman
histograms.xml lgtm
3 years, 10 months ago (2017-02-14 00:49:49 UTC) #37
Eric Willigers
You'll need someone from tools/metrics/OWNERS e.g. isherman
3 years, 10 months ago (2017-02-14 00:52:56 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2564973002/240001
3 years, 10 months ago (2017-02-14 03:45:46 UTC) #45
commit-bot: I haz the power
3 years, 10 months ago (2017-02-14 05:11:38 UTC) #48
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/7a66b24c10161a5db8668729a97a...

Powered by Google App Engine
This is Rietveld 408576698