Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2564973002:
Add an infobar if a session is being controlled by an automated test. (Closed)
DescriptionAdd 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 #Dependent Patchsets: Messages
Total messages: 48 (17 generated)
Description was changed from ========== Add an infobar if a session is being controlled by ChromeDriver. This infobar is only displayed if the browser is launched with the --enable-chromedriver switch. It also disables the developer mode extensions warning bubble. BUG= ========== to ========== Add an infobar if a session is being controlled by ChromeDriver. This infobar is only displayed if the browser is launched with the --enable-chromedriver switch. It also disables the developer mode extensions warning bubble. BUG=chromedriver:1625 ==========
Description was changed from ========== Add an infobar if a session is being controlled by ChromeDriver. This infobar is only displayed if the browser is launched with the --enable-chromedriver switch. It also disables the developer mode extensions warning bubble. BUG=chromedriver:1625 ========== to ========== Add an infobar if a session is being controlled by ChromeDriver. This infobar is only displayed if the browser is launched with the --enable-chromedriver switch. It also disables the developer mode extensions warning bubble. BUG=chromedriver:1625 TEST=launch with and without --enable-chromedriver, and check for presence of chromedriver infobar ==========
samuong@chromium.org changed reviewers: + rdcronin@google.com
Devlin, can you ptal? As discussed earlier, we'll need to treat load-component-extension as load-extension for a while, so that ChromeDriver can continue to be compatible with older Chrome builds.
friendly ping, devlin it'd be good to know what you think here, especially with the way that load-component-extension is interpreted
rdevlin.cronin@chromium.org changed reviewers: + rdevlin.cronin@chromium.org - rdcronin@google.com
Hey Sam, sorry for the delay - chromium reviews should go to my @chromium, so this got weirdly filtered/lost in my @google. :( (On that note, updating my @google to mention this) https://codereview.chromium.org/2564973002/diff/20001/chrome/app/chromium_str... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/app/chromium_str... chrome/app/chromium_strings.grd:1253: Your browser is being remotely controlled by ChromeDriver. we have a generated_resources.grd file for strings that don't need to be different between Chrome and Chromium. https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:448: LoadExtensionsFromCommandLineFlag(switches::kLoadComponentExtension); I think we should make this a separate change - let's have this one only target the infobar. https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/extension_message_bubble_factory.cc (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_factory.cc:95: return false; On Windows beta and stable, we've already returned true by this point (line 90). https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/chromedriver_infobar_delegate.cc (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/chromedriver_infobar_delegate.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. no longer 2016 :) https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/chromedriver_infobar_delegate.cc:8: #include "chrome/browser/profiles/profile.h" Not all of these includes seem needed. :) https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/chromedriver_infobar_delegate.cc:17: extra newline https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/chromedriver_infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/chromedriver_infobar_delegate.h:5: #ifndef CHROME_BROWSER_UI_STARTUP_CHROMEDRIVER_INFOBAR_DELEGATE_H_ startup probably isn't the right place for this. It probably should just go in chrome/browser/extensions. https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:805: ChromeDriverInfoBarDelegate::Create(browser); This is pretty high-level. I wonder if we could put this a little lower in the stack? Someone in c/b/ui/startup owners could say for sure if this is the right place (it *does* have other infobars, but they're a little more startup-related). https://codereview.chromium.org/2564973002/diff/20001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:564: const char kLoadComponentExtension[] = "load-component-extension"; (as in extension_service.cc, let's make this a separate change)
https://codereview.chromium.org/2564973002/diff/20001/chrome/app/chromium_str... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/app/chromium_str... chrome/app/chromium_strings.grd:1253: Your browser is being remotely controlled by ChromeDriver. On 2017/02/01 23:04:59, Devlin wrote: > we have a generated_resources.grd file for strings that don't need to be > different between Chrome and Chromium. Done. https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:448: LoadExtensionsFromCommandLineFlag(switches::kLoadComponentExtension); On 2017/02/01 23:05:00, Devlin wrote: > I think we should make this a separate change - let's have this one only target > the infobar. Done. https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/extension_message_bubble_factory.cc (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_factory.cc:95: return false; On 2017/02/01 23:05:00, Devlin wrote: > On Windows beta and stable, we've already returned true by this point (line 90). Done. https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/chromedriver_infobar_delegate.cc (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/chromedriver_infobar_delegate.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/01 23:05:00, Devlin wrote: > no longer 2016 :) Done. https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/chromedriver_infobar_delegate.cc:8: #include "chrome/browser/profiles/profile.h" On 2017/02/01 23:05:00, Devlin wrote: > Not all of these includes seem needed. :) Done. https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/chromedriver_infobar_delegate.cc:17: On 2017/02/01 23:05:00, Devlin wrote: > extra newline Done. https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/chromedriver_infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/chromedriver_infobar_delegate.h:5: #ifndef CHROME_BROWSER_UI_STARTUP_CHROMEDRIVER_INFOBAR_DELEGATE_H_ On 2017/02/01 23:05:00, Devlin wrote: > startup probably isn't the right place for this. It probably should just go in > chrome/browser/extensions. Even if ChromeDriver weren't loading an extension, I think it's still reasonable to show a warning to the user. So I don't really see this infobar as an extensions-specific thing, what do you think? https://codereview.chromium.org/2564973002/diff/20001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:564: const char kLoadComponentExtension[] = "load-component-extension"; On 2017/02/01 23:05:00, Devlin wrote: > (as in extension_service.cc, let's make this a separate change) Done.
Nice! A few last small things and I'd like to take another look before it lands, but overall this looks pretty good! You'll need to add a ui/ and infobar/ owner here, and pkasting@ fits both those needs. :) https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/chromedriver_infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/chromedriver_infobar_delegate.h:5: #ifndef CHROME_BROWSER_UI_STARTUP_CHROMEDRIVER_INFOBAR_DELEGATE_H_ On 2017/02/04 00:11:03, samuong wrote: > On 2017/02/01 23:05:00, Devlin wrote: > > startup probably isn't the right place for this. It probably should just go > in > > chrome/browser/extensions. > > Even if ChromeDriver weren't loading an extension, I think it's still reasonable > to show a warning to the user. So I don't really see this infobar as an > extensions-specific thing, what do you think? I don't feel very strongly, so I'll let a ui or ui/startup owner decide. :) https://codereview.chromium.org/2564973002/diff/40001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/extension_message_bubble_factory.cc (right): https://codereview.chromium.org/2564973002/diff/40001/chrome/browser/ui/exten... chrome/browser/ui/extensions/extension_message_bubble_factory.cc:88: base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); We should add a comment here like: // If ChromeDriver is controlling the browser, we don't show the // dev mode bubble because it interferes with focus. This isn't a // security concern because we'll instead show an (even scarier) // infobar. See also <infobar class>. https://codereview.chromium.org/2564973002/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/chromedriver_infobar_delegate.cc (right): https://codereview.chromium.org/2564973002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/chromedriver_infobar_delegate.cc:28: : ConfirmInfoBarDelegate() { nit: this should be automatically called (so you can just have this be ChromeDriverInfoBarDelegate::ChromeDriverInfoBarDelegate() {}) https://codereview.chromium.org/2564973002/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/chromedriver_infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/chromedriver_infobar_delegate.h:16: class ChromeDriverInfoBarDelegate : public ConfirmInfoBarDelegate { We should add a class-level comment here, e.g. // An infobar to inform the user that their browser is being controlled by the ChromeDriver extension. https://codereview.chromium.org/2564973002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2564973002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1748: switches::kEnableChromeDriver, Does this switch need to be propogated to the renderer? (It doesn't look like it...) https://codereview.chromium.org/2564973002/diff/40001/content/public/common/c... File content/public/common/content_switches.h (right): https://codereview.chromium.org/2564973002/diff/40001/content/public/common/c... content/public/common/content_switches.h:112: extern const char kEnableChromeDriver[]; Is this needed in the content/ layer? Or can this go in chrome/common/chrome_switches.h?
Description was changed from ========== Add an infobar if a session is being controlled by ChromeDriver. This infobar is only displayed if the browser is launched with the --enable-chromedriver switch. It also disables the developer mode extensions warning bubble. BUG=chromedriver:1625 TEST=launch with and without --enable-chromedriver, and check for presence of chromedriver infobar ========== to ========== Add an infobar if a session is being controlled by ChromeDriver. This infobar is only displayed if the browser is launched with the --enable-chromedriver switch. It also disables the developer mode extensions warning bubble. Design doc: https://docs.google.com/document/d/1JYj9K61UyxIYavR8_HATYIglR9T_rDwAtLLsD3fbD... BUG=chromedriver:1625 TEST=launch with and without --enable-chromedriver, and check for presence of chromedriver infobar ==========
https://codereview.chromium.org/2564973002/diff/40001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/extension_message_bubble_factory.cc (right): https://codereview.chromium.org/2564973002/diff/40001/chrome/browser/ui/exten... 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: > We should add a comment here like: > // If ChromeDriver is controlling the browser, we don't show the > // dev mode bubble because it interferes with focus. This isn't a > // security concern because we'll instead show an (even scarier) > // infobar. See also <infobar class>. Done. https://codereview.chromium.org/2564973002/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/chromedriver_infobar_delegate.cc (right): https://codereview.chromium.org/2564973002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/chromedriver_infobar_delegate.cc:28: : ConfirmInfoBarDelegate() { On 2017/02/06 16:20:21, Devlin wrote: > nit: this should be automatically called (so you can just have this be > ChromeDriverInfoBarDelegate::ChromeDriverInfoBarDelegate() {}) Done. https://codereview.chromium.org/2564973002/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/chromedriver_infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/chromedriver_infobar_delegate.h:16: class ChromeDriverInfoBarDelegate : public ConfirmInfoBarDelegate { On 2017/02/06 16:20:21, Devlin wrote: > We should add a class-level comment here, e.g. > // An infobar to inform the user that their browser is being controlled by the > ChromeDriver extension. Done. https://codereview.chromium.org/2564973002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2564973002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1748: switches::kEnableChromeDriver, On 2017/02/06 16:20:22, Devlin wrote: > Does this switch need to be propogated to the renderer? (It doesn't look like > it...) Done. It's not needed for this CL, but I might add this back in a future CL. https://codereview.chromium.org/2564973002/diff/40001/content/public/common/c... File content/public/common/content_switches.h (right): https://codereview.chromium.org/2564973002/diff/40001/content/public/common/c... content/public/common/content_switches.h:112: extern const char kEnableChromeDriver[]; On 2017/02/06 16:20:22, Devlin wrote: > Is this needed in the content/ layer? Or can this go in > chrome/common/chrome_switches.h? For this specific CL, it could go in either content/ or chrome/. But ChromeDriver can drive content shell, and in the future I'd like to be able to add other features to this switch, such as exposing synthetic inputs to ChromeDriver. Other content embedders might also want to know whether they're running in a WebDriver context.
samuong@chromium.org changed reviewers: + pfeldman@chromium.org, pkasting@chromium.org
+pkasting@ for ui/ and infobars/ review
also, +pfeldman@ could you let me know if you're ok with my use of, and changes
to, GlobalConfirmInfoBar?
In addition to addressing Devlin's comments, I've also made some changes
requested by the UX team: 1) show the infobar on all tabs and 2) make it white
(which means, set the type to PAGE_ACTION) and 3) use the product icon on the
infobar.
Pavel, in order to do this, I've had to add some overrides to
GlobalConfirmInfoBar. I also had to make sure that MaybeAddInfoBar passes the
InfoBar ptr to the delegate, not just the delegate proxy (otherwise we get a
browser crash when the link is clicked, because the default implementation of
ConfirmInfoBar::LinkClicked dereferences a pointer to the InfoBar).
Finally, git-cl refuses to upload chrome/app/generated_resources.grd "because
it's too large", so I think I'm going to have to land this manually. The diff
for that file is below:
diff --git a/chrome/app/generated_resources.grd
b/chrome/app/generated_resources.grd
index aed2ab476b4f..cad144241b90 100644
--- a/chrome/app/generated_resources.grd
+++ b/chrome/app/generated_resources.grd
@@ -15612,6 +15612,11 @@ read aloud to screenreader users to announce that a
completion is available.">
Enable postscript generation in place of emf generation when printing
to postscript capable printers.
</message>
</if>
+
+ <!-- ChromeDriver info bar -->
+ <message name="IDS_CONTROLLED_BY_CHROMEDRIVER" desc="Message shown when the
browser session is being controlled by ChromeDriver. This message is followed by
a 'Learn more' link.">
+ Your browser is being remotely controlled by ChromeDriver.
+ </message>
</messages>
</release>
</grit>
I don't think we should add code into chrome specifically for ChromeDriver. Our source base is already huge, let's try to limit it. It is unlikely that the infobar is essential for automation.
On 2017/02/07 19:15:44, samuong wrote: > Finally, git-cl refuses to upload chrome/app/generated_resources.grd "because > it's too large", so I think I'm going to have to land this manually. The diff > for that file is below: > > diff --git a/chrome/app/generated_resources.grd > b/chrome/app/generated_resources.grd > index aed2ab476b4f..cad144241b90 100644 > --- a/chrome/app/generated_resources.grd > +++ b/chrome/app/generated_resources.grd > @@ -15612,6 +15612,11 @@ read aloud to screenreader users to announce that a > completion is available."> > Enable postscript generation in place of emf generation when printing > to postscript capable printers. > </message> > </if> > + > + <!-- ChromeDriver info bar --> > + <message name="IDS_CONTROLLED_BY_CHROMEDRIVER" desc="Message shown when the > browser session is being controlled by ChromeDriver. This message is followed by > a 'Learn more' link."> > + Your browser is being remotely controlled by ChromeDriver. > + </message> > </messages> > </release> > </grit> That just means it won't upload the full file, so you can't view a side-by-side diff. The file is still included in the patch set, and can be sent through the CQ normally.
On 2017/02/07 23:08:50, pfeldman wrote: > I don't think we should add code into chrome specifically for ChromeDriver. Our > source base is already huge, let's try to limit it. It is unlikely that the > infobar is essential for automation. I'm always a fan of avoiding more infobars. I'm going to hold off on reviewing this until everyone's in agreement that it's the route forward.
On 2017/02/08 00:35:38, Peter Kasting wrote: > On 2017/02/07 23:08:50, pfeldman wrote: > > I don't think we should add code into chrome specifically for ChromeDriver. > Our > > source base is already huge, let's try to limit it. It is unlikely that the > > infobar is essential for automation. > > I'm always a fan of avoiding more infobars. > > I'm going to hold off on reviewing this until everyone's in agreement that it's > the route forward. FWIW, this went through a reasonably detailed UI review - internal thread at https://groups.google.com/a/google.com/forum/#!searchin/chrome-ui-review/info...
I won't have time to get to this for the next couple of days, so here's the initial drafts I have. https://codereview.chromium.org/2564973002/diff/120001/chrome/browser/devtool... File chrome/browser/devtools/global_confirm_info_bar.cc (right): https://codereview.chromium.org/2564973002/diff/120001/chrome/browser/devtool... chrome/browser/devtools/global_confirm_info_bar.cc:41: gfx::VectorIconId GetVectorIconId() const override; Nit: These last two go just above and just below GetIdentifier() in the above list, respectively (to match base class declaration order) https://codereview.chromium.org/2564973002/diff/120001/chrome/browser/devtool... chrome/browser/devtools/global_confirm_info_bar.cc:46: global_info_bar_->delegate_->set_infobar(info_bar_); I'm worried about this. It seems like |global_info_bar_|'s delegate's infobar should be |global_info_bar_|, not the provided infobar. You said you needed to "fix a crash". Can you provide more info about how this is crashing, what the stack looks like, etc.? I'm especially worried about leaking infobars by doing the wrong thing here, as well as having the delegate pointed at the most-recently-added view when in fact a user may be clicking on an infobar in a different tab, and having that cause visibility/ownership problems or something. https://codereview.chromium.org/2564973002/diff/120001/chrome/browser/devtool... chrome/browser/devtools/global_confirm_info_bar.cc:139: : WARNING_TYPE; Nit: Don't hardcode the fallback type; use ConfirmInfoBarDelegate::GetInfoBarType() instead https://codereview.chromium.org/2564973002/diff/120001/chrome/browser/devtool... chrome/browser/devtools/global_confirm_info_bar.cc:144: : gfx::VectorIconId::VECTOR_ICON_NONE; Nit: Same comment https://codereview.chromium.org/2564973002/diff/120001/chrome/browser/devtool... chrome/browser/devtools/global_confirm_info_bar.cc:229: DCHECK(added_bar); This DCHECK is unsafe; we can't guarantee |added_bar| is non-null. This should be converted to a conditional return and moved to just below the AddInfoBar() call.
I'd suggest that you drop the infobar changes - they are not affecting the security aspect of what is going on. Also, I'm suggesting to save on new flags and reuse remote debugging. Rationale is that ChromeDriver is just one of the automation clients that are using remote debugging interface. Whatever makes sense for ChromeDriver makes sense for any automation / continuous integration framework. I've also learned from Sam that ChromeDriver will later become solely based on the remote debugging interface with no extensions involved. https://codereview.chromium.org/2564973002/diff/120001/chrome/browser/ui/exte... File chrome/browser/ui/extensions/extension_message_bubble_factory.cc (right): https://codereview.chromium.org/2564973002/diff/120001/chrome/browser/ui/exte... chrome/browser/ui/extensions/extension_message_bubble_factory.cc:93: if (command_line->HasSwitch(switches::kEnableChromeDriver)) I'm suggesting that you check for remote debugging being enabled instead. Same check, another flag. https://codereview.chromium.org/2564973002/diff/120001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2564973002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:811: if (command_line_.HasSwitch(switches::kEnableChromeDriver)) ditto
Where does this review stand w.r.t. http://crbug.com/691117 and https://codereview.chromium.org/2685203002/ ? Are we still doing this? I'm kind of confused.
On 2017/02/11 01:49:20, Peter Kasting wrote: > Where does this review stand w.r.t. http://crbug.com/691117 and > https://codereview.chromium.org/2685203002/ ? Are we still doing this? I'm kind > of confused. This change should go in favor of the two you point to.
On 2017/02/11 01:53:41, pfeldman wrote: > On 2017/02/11 01:49:20, Peter Kasting wrote: > > Where does this review stand w.r.t. http://crbug.com/691117 and > > https://codereview.chromium.org/2685203002/ ? Are we still doing this? I'm > kind > > of confused. > > This change should go in favor of the two you point to. I'm going to interpret that as "should go away" and assume this will be closed or else someone will correct me, and thus choose not to review it :)
I've had requests not to make any changes to GlobalConfirmInfoBar, so that we can minimize the size of the patch that we're merging back into the M57 branch. So I've backed out of these changes, which also means: - the infobar is no longer a PAGE_ACTION type, and is back to being yellow - the infobar does not have a product icon on it - there is no "learn more" link on the infobar
My only comment is to call it in a generic manner: --enable-automation, AutomationInfobar, etc. It is likely that Lighthouse would also like to use it the same way ChromeDriver does.
Description was changed from ========== Add an infobar if a session is being controlled by ChromeDriver. This infobar is only displayed if the browser is launched with the --enable-chromedriver switch. It also disables the developer mode extensions warning bubble. Design doc: https://docs.google.com/document/d/1JYj9K61UyxIYavR8_HATYIglR9T_rDwAtLLsD3fbD... BUG=chromedriver:1625 TEST=launch with and without --enable-chromedriver, and check for presence of chromedriver infobar ========== to ========== 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_rDwAtLLsD3fbD... BUG=chromedriver:1625 TEST=launch with and without --enable-automation, and check for presence of automation infobar ==========
Done.
lgtm
https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/star... File chrome/browser/ui/startup/chromedriver_infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/star... 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/star... chrome/browser/ui/startup/chromedriver_infobar_delegate.h:21: static void CreateAndShow(Browser* browser); Nit: Name this Create() in keeping with other infobars. Eliminate the |browser| arg, since it's unused. https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/star... chrome/browser/ui/startup/chromedriver_infobar_delegate.h:29: bool ShouldExpire(const NavigationDetails& details) const override; Nit: ShouldExpire() goes just above GetMessageText() (comes from an earlier ancestor) https://codereview.chromium.org/2564973002/diff/140001/components/infobars/co... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/140001/components/infobars/co... components/infobars/core/infobar_delegate.h:147: CHROMEDRIVER_INFOBAR_DELEGATE = 73, You also need to add a corresponding histogram value.
samuong@chromium.org changed reviewers: + ericwilligers@chromium.org
+ericwilligers@ for tools/metrics/histograms/histograms.xml https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/star... File chrome/browser/ui/startup/chromedriver_infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/star... chrome/browser/ui/startup/chromedriver_infobar_delegate.h:20: ~ChromeDriverInfoBarDelegate() override; On 2017/02/13 23:16:52, Peter Kasting wrote: > Nit: This should be private I get a compile error if I try to make it private: .../unique_ptr.h:63:9: error: calling a private destructor of class 'AutomationInfoBarDelegate' ... .../automation_infobar_delegate.cc:16:46: note: in instantiation of member function 'std::unique_ptr<AutomationInfoBarDelegate, std::default_delete<AutomationInfoBarDelegate> >::~unique_ptr' requested here std::unique_ptr<AutomationInfoBarDelegate> delegate( ^ https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/star... chrome/browser/ui/startup/chromedriver_infobar_delegate.h:21: static void CreateAndShow(Browser* browser); On 2017/02/13 23:16:52, Peter Kasting wrote: > Nit: Name this Create() in keeping with other infobars. > > Eliminate the |browser| arg, since it's unused. Done. https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/star... chrome/browser/ui/startup/chromedriver_infobar_delegate.h:29: bool ShouldExpire(const NavigationDetails& details) const override; On 2017/02/13 23:16:52, Peter Kasting wrote: > Nit: ShouldExpire() goes just above GetMessageText() (comes from an earlier > ancestor) Done. Did this in the .cc file too. https://codereview.chromium.org/2564973002/diff/140001/components/infobars/co... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/140001/components/infobars/co... components/infobars/core/infobar_delegate.h:147: CHROMEDRIVER_INFOBAR_DELEGATE = 73, On 2017/02/13 23:16:52, Peter Kasting wrote: > You also need to add a corresponding histogram value. I'm not very familiar with histograms. I've added an entry in tools/metrics/histograms/histograms.xml, does this need anything else?
c/b/ui/extensions lgtm
https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/star... File chrome/browser/ui/startup/chromedriver_infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/star... chrome/browser/ui/startup/chromedriver_infobar_delegate.h:20: ~ChromeDriverInfoBarDelegate() override; On 2017/02/14 00:19:17, samuong wrote: > On 2017/02/13 23:16:52, Peter Kasting wrote: > > Nit: This should be private > > I get a compile error if I try to make it private: > > .../unique_ptr.h:63:9: error: calling a private destructor of class > 'AutomationInfoBarDelegate' > ... > .../automation_infobar_delegate.cc:16:46: note: in instantiation of member > function 'std::unique_ptr<AutomationInfoBarDelegate, > std::default_delete<AutomationInfoBarDelegate> >::~unique_ptr' requested here > std::unique_ptr<AutomationInfoBarDelegate> delegate( > ^ Declare that unique_ptr as a unique_ptr<ConfirmInfoBarDelegate> and this should clear up. https://codereview.chromium.org/2564973002/diff/140001/components/infobars/co... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/140001/components/infobars/co... components/infobars/core/infobar_delegate.h:147: CHROMEDRIVER_INFOBAR_DELEGATE = 73, On 2017/02/14 00:19:17, samuong wrote: > On 2017/02/13 23:16:52, Peter Kasting wrote: > > You also need to add a corresponding histogram value. > > I'm not very familiar with histograms. I've added an entry in > tools/metrics/histograms/histograms.xml, does this need anything else? I think that's all you need, but you'll need signoff from a histograms OWNER (and they can double-check).
pfeldman@chromium.org changed reviewers: + isherman@chromium.org
https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/star... File chrome/browser/ui/startup/chromedriver_infobar_delegate.h (right): https://codereview.chromium.org/2564973002/diff/140001/chrome/browser/ui/star... chrome/browser/ui/startup/chromedriver_infobar_delegate.h:20: ~ChromeDriverInfoBarDelegate() override; On 2017/02/14 00:23:17, Peter Kasting wrote: > On 2017/02/14 00:19:17, samuong wrote: > > On 2017/02/13 23:16:52, Peter Kasting wrote: > > > Nit: This should be private > > > > I get a compile error if I try to make it private: > > > > .../unique_ptr.h:63:9: error: calling a private destructor of class > > 'AutomationInfoBarDelegate' > > ... > > .../automation_infobar_delegate.cc:16:46: note: in instantiation of member > > function 'std::unique_ptr<AutomationInfoBarDelegate, > > std::default_delete<AutomationInfoBarDelegate> >::~unique_ptr' requested here > > std::unique_ptr<AutomationInfoBarDelegate> delegate( > > ^ > > Declare that unique_ptr as a unique_ptr<ConfirmInfoBarDelegate> and this should > clear up. Ah right, done.
LGTM https://codereview.chromium.org/2564973002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2564973002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:818: // These info bars are not shown when the browser is being controlled by an Nit: Remove "an"
https://codereview.chromium.org/2564973002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2564973002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:818: // These info bars are not shown when the browser is being controlled by an On 2017/02/14 00:34:27, Peter Kasting wrote: > Nit: Remove "an" Done.
histograms.xml lgtm
You'll need someone from tools/metrics/OWNERS e.g. isherman
The CQ bit was checked by ericwilligers@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by samuong@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org, rdevlin.cronin@chromium.org, pkasting@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2564973002/#ps240001 (title: "rebase")
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": 240001, "attempt_start_ts": 1487043908347390,
"parent_rev": "931df34fb40d8130d986b1fc6df37a7f478cf9f9", "commit_rev":
"7a66b24c10161a5db8668729a97ac95cee9068e2"}
Message was sent while issue was closed.
Description was changed from ========== 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_rDwAtLLsD3fbD... BUG=chromedriver:1625 TEST=launch with and without --enable-automation, and check for presence of automation infobar ========== to ========== 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_rDwAtLLsD3fbD... 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/+/7a66b24c10161a5db8668729a97a... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/7a66b24c10161a5db8668729a97a... |
