Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23619077:
Introduce CrosBrowserWithOOBE, which exposes an oobe property. (Closed)
|
Created:
7 years, 3 months ago by achuithb Modified:
7 years, 2 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, telemetry+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIntroduce CrosBrowserWithOOBE, which exposes an oobe property.
This is only visible when the create_browser_with_oobe browser option is specified.
BUG=294183
TEST=manual
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226476
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227008
Patch Set 1 #Patch Set 2 : minor #
Total comments: 4
Patch Set 3 : add option enable_oobe_property #
Total comments: 6
Patch Set 4 : nduca comments #Patch Set 5 : minor comment #Patch Set 6 : create_browser_with_oobe #
Messages
Total messages: 27 (0 generated)
Please take a look, Tim.
Also Tony.
This class does make sense as the oobe is a feature that is only available on CrOS. However, this would be the first platform-specific browser class, just for the oobe feature. Do we have a need for specialized features on other platforms or for other CrOS features? In any case, This module shouldn't be in backends/, so I think we should probably create a browsers/ package if we're going down this route. Another option could be to expose an method in browser.py to get a list of all web contents instead of just tabs. I'll defer to Tony for this change.
On 2013/09/20 19:34:42, Tim Song wrote: > This class does make sense as the oobe is a feature that is only available on > CrOS. However, this would be the first platform-specific browser class, just for > the oobe feature. Do we have a need for specialized features on other platforms > or for other CrOS features? At the moment, I'm not planning for anything besides oobe. I think we want to avoid adding specialized features on other platforms as much as possible. > In any case, This module shouldn't be in backends/, so I think we should > probably create a browsers/ package if we're going down this route. I'm not really clear on the logic of backends myself. backends/chrome/ has everything related to chrome, including the finders, inspector, cros_interface, websocket, tracing, etc. It seems like a reasonable place to put cros_browser.py. > Another option could be to expose an method in browser.py to get a list of all > web contents instead of just tabs. There was a discussion about this sometime ago and we don't want to do it that way. > I'll defer to Tony for this change. Sounds good. Thanks for taking a look.
Tony, can you take a look? What do you think of this?
Dave, could you please take a look?
This stinks but we need to do this. However, I see an edit that makes me think @achuith, you missed the target that I had discussed with you. Please provide context.
https://codereview.chromium.org/23619077/diff/2001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/backends/chrome/cros_browser.py (right): https://codereview.chromium.org/23619077/diff/2001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/backends/chrome/cros_browser.py:16: return self._browser_backend.oobe i'm a bit puzzled, did this get added some time before? do all browsers have an oobe method? That seems really bad, please revisit so that this exposure surface is considerably narrowed.
PTAL, Nat https://codereview.chromium.org/23619077/diff/2001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/backends/chrome/cros_browser.py (right): https://codereview.chromium.org/23619077/diff/2001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/backends/chrome/cros_browser.py:16: return self._browser_backend.oobe On 2013/09/27 16:23:49, nduca wrote: > i'm a bit puzzled, did this get added some time before? do all browsers have an > oobe method? That seems really bad, please revisit so that this exposure surface > is considerably narrowed. Nope, only CrOSBrowserBackend has oobe. Note the assert on line 11 - we know that CrOSBrowser can only be attached to CrOSBrowserBackend. We don't want to pollute the public api of Browser/BrowserBackend with oobe - it is restricted to CrOSBrowser/CrOSBrowserBackend. This class exists merely to expose oobe.
On 2013/09/27 16:23:43, nduca wrote: > This stinks but we need to do this. However, I see an edit that makes me think > @achuith, you missed the target that I had discussed with you. Please provide > context. I don't quite follow? Is this the target for splitting BrowserOptions from FinderOptions?
lgtm
https://codereview.chromium.org/23619077/diff/2001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py (right): https://codereview.chromium.org/23619077/diff/2001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py:39: b = cros_browser.CrOSBrowser(backend, actually not lgtm, i think. you're making this in all cases. that means anyone on cros is getting a cros browser. that's not what you said you'd do --- only tests that ask for oobe testing should get this class right?
Nat, I've added a cros-specific browser option enable_oobe_property for this. Let me know what you think. https://codereview.chromium.org/23619077/diff/2001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py (right): https://codereview.chromium.org/23619077/diff/2001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py:39: b = cros_browser.CrOSBrowser(backend, On 2013/10/01 00:21:01, nduca wrote: > actually not lgtm, i think. you're making this in all cases. that means anyone > on cros is getting a cros browser. that's not what you said you'd do --- only > tests that ask for oobe testing should get this class right? Done.
https://codereview.chromium.org/23619077/diff/19001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/backends/chrome/cros_browser.py (right): https://codereview.chromium.org/23619077/diff/19001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/backends/chrome/cros_browser.py:15: def oobe(self): docstringplease at the chrometobefest go ask 10 random people what they think an "oobe" is :) https://codereview.chromium.org/23619077/diff/19001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py (right): https://codereview.chromium.org/23619077/diff/19001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py:41: if browser_options.enable_oobe_property: create_browser_with_oobe https://codereview.chromium.org/23619077/diff/19001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py:42: return cros_browser.CrOSBrowser(backend, platform) lets call it CrOSBrowserWithOOBE and update the names accordingly?
lgtm with these changes. please be cautious as tests come online using this surface area and overcommunicate to telemetry@ about how the surface area of this evolves. Better we all are aware of what is happening here than beign surprised down the road.
Thanks for the review! https://codereview.chromium.org/23619077/diff/19001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/backends/chrome/cros_browser.py (right): https://codereview.chromium.org/23619077/diff/19001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/backends/chrome/cros_browser.py:15: def oobe(self): On 2013/10/02 06:53:50, nduca wrote: > docstringplease at the chrometobefest go ask 10 random people what they think > an "oobe" is :) Done. https://codereview.chromium.org/23619077/diff/19001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py (right): https://codereview.chromium.org/23619077/diff/19001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py:41: if browser_options.enable_oobe_property: On 2013/10/02 06:53:50, nduca wrote: > create_browser_with_oobe Done. https://codereview.chromium.org/23619077/diff/19001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py:42: return cros_browser.CrOSBrowser(backend, platform) On 2013/10/02 06:53:50, nduca wrote: > lets call it CrOSBrowserWithOOBE and update the names accordingly? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/23619077/34001
Retried try job too often on win7_aura for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/23619077/34001
Message was sent while issue was closed.
Change committed as 226476
Message was sent while issue was closed.
Looks like this change broke the login_CryptohomeIncognitoTelemetry test in BVT, it failed on all platforms tonight with the message: 'CrosBrowserOptions' object has no attribute 'create_browser_with_oobe' https://wmatrix.googleplex.com/failures/bvt?tests=login_CryptohomeIncognitoTe...
Message was sent while issue was closed.
On 2013/10/03 14:19:33, kamrik wrote: > Looks like this change broke the login_CryptohomeIncognitoTelemetry test in BVT, > it failed on all platforms tonight with the message: > 'CrosBrowserOptions' object has no attribute 'create_browser_with_oobe' > https://wmatrix.googleplex.com/failures/bvt?tests=login_CryptohomeIncognitoTe... Ugh, one of the files didn't update for some reason. Reverting and trying again.
Message was sent while issue was closed.
On 2013/10/03 20:28:46, achuith.bhandarkar wrote: > On 2013/10/03 14:19:33, kamrik wrote: > > Looks like this change broke the login_CryptohomeIncognitoTelemetry test in > BVT, > > it failed on all platforms tonight with the message: > > 'CrosBrowserOptions' object has no attribute 'create_browser_with_oobe' > > > https://wmatrix.googleplex.com/failures/bvt?tests=login_CryptohomeIncognitoTe... > > Ugh, one of the files didn't update for some reason. Reverting and trying again. https://codereview.chromium.org/25417008/
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/23619077/63001
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/23619077/63001
Message was sent while issue was closed.
Change committed as 227008 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
