Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11663011:
Adds function for activating a tab. (Closed)
|
Created:
8 years ago by Danh Nguyen Modified:
7 years, 11 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, pam+watch_chromium.org, telemetry+watch_chromium.org, Vangelis Kokkevis, anantha Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdds function for activating a tab.
BUG=157495
TEST=src/tools/telemetry/run_tests --browser=system TabTest.testActivateTab
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175440
Patch Set 1 #
Total comments: 12
Patch Set 2 : Updated per Nat's comments #
Total comments: 4
Patch Set 3 : Updated per Dave's comments. #
Total comments: 4
Patch Set 4 : Added unittest. #Patch Set 5 : Removed extra line. #
Total comments: 8
Patch Set 6 : Removed wait on visibility state. #
Total comments: 2
Patch Set 7 : Moved _IsDocumentVisible to module level. #
Messages
Total messages: 19 (0 generated)
Hi Nat, Dave, I need to make these changes in order to add some basic tests for GPU validation. Please take a look whenever you have a chance. Thanks, Danh
https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/bro... File tools/telemetry/telemetry/browser_backend.py (right): https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/bro... tools/telemetry/telemetry/browser_backend.py:48: This doesn't make sense on the tab controller. browser.tabs[2].Activate should be how this works. https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/des... File tools/telemetry/telemetry/desktop_browser_backend.py (right): https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/des... tools/telemetry/telemetry/desktop_browser_backend.py:53: # Required for running debuggable Chrome: Please dont add this. https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/run... File tools/telemetry/telemetry/run_tests.py (right): https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/run... tools/telemetry/telemetry/run_tests.py:116: if extra_options: I do not understand this at all. Why do you need this? https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/tab.py File tools/telemetry/telemetry/tab.py (right): https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/tab... tools/telemetry/telemetry/tab.py:52: def Navigate(self, url): Why? This doesn't fit with the design we have. https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/tab... tools/telemetry/telemetry/tab.py:55: self.Disconnect() and why disconnect?
Hi Nat, Thanks for reviewing. I've just updated the CL per your comments. Could you take another look? Thanks, Danh https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/bro... File tools/telemetry/telemetry/browser_backend.py (right): https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/bro... tools/telemetry/telemetry/browser_backend.py:48: On 2012/12/21 22:15:23, nduca wrote: > This doesn't make sense on the tab controller. > > browser.tabs[2].Activate should be how this works. Done. https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/bro... tools/telemetry/telemetry/browser_backend.py:48: On 2012/12/21 22:15:23, nduca wrote: > This doesn't make sense on the tab controller. > > browser.tabs[2].Activate should be how this works. Done. I'm moving this function to tab.py as you suggested. https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/des... File tools/telemetry/telemetry/desktop_browser_backend.py (right): https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/des... tools/telemetry/telemetry/desktop_browser_backend.py:53: # Required for running debuggable Chrome: On 2012/12/21 22:15:23, nduca wrote: > Please dont add this. Done. https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/run... File tools/telemetry/telemetry/run_tests.py (right): https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/run... tools/telemetry/telemetry/run_tests.py:116: if extra_options: On 2012/12/21 22:15:23, nduca wrote: > I do not understand this at all. Why do you need this? I intended to allow options in gpu validation tests, e.g.: ./gpu_validation/run_test --url_file=../telemetry/examples/top1k --num_tabs=25 Per your comments for https://codereview.chromium.org/11668013/ (unit tests shouldn't have options), I'm going to remove this change. https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/tab.py File tools/telemetry/telemetry/tab.py (right): https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/tab... tools/telemetry/telemetry/tab.py:52: def Navigate(self, url): On 2012/12/21 22:15:23, nduca wrote: > Why? This doesn't fit with the design we have. Removed. I'll move these calls into the tests instead. https://codereview.chromium.org/11663011/diff/1/tools/telemetry/telemetry/tab... tools/telemetry/telemetry/tab.py:55: self.Disconnect() On 2012/12/21 22:15:23, nduca wrote: > and why disconnect? The Disconnect() is the get around so that TabController._UpdateTabList() will work properly after Page.Navigate() call. Navigated tab doesn't have webSocketDebuggerUrl thus breaks TabController._UpdateTabList() and causes problem (https://code.google.com/p/chromium/issues/detail?id=166243). The Disconnect() call gets the webSocketDebuggerUrl back for the navigated tab.
https://chromiumcodereview.appspot.com/11663011/diff/1/tools/telemetry/teleme... File tools/telemetry/telemetry/browser_backend.py (right): https://chromiumcodereview.appspot.com/11663011/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry/browser_backend.py:48: On 2012/12/21 22:15:23, nduca wrote: > This doesn't make sense on the tab controller. > > browser.tabs[2].Activate should be how this works. This is actually similar to how Close() works. tab.Close() calls _tab_controller.CloseTab(), because TabController has a lot of the other logic needed for CloseTab() https://chromiumcodereview.appspot.com/11663011/diff/4002/tools/telemetry/tel... File tools/telemetry/telemetry/desktop_browser_backend.py (right): https://chromiumcodereview.appspot.com/11663011/diff/4002/tools/telemetry/tel... tools/telemetry/telemetry/desktop_browser_backend.py:52: args.append('--enable-stats-table') No --enable-stats-table. Explanation is here: https://codereview.chromium.org/11274046/ (The tl;dr is that it crashes in the reference build.) https://chromiumcodereview.appspot.com/11663011/diff/4002/tools/telemetry/tel... File tools/telemetry/telemetry/tab.py (right): https://chromiumcodereview.appspot.com/11663011/diff/4002/tools/telemetry/tel... tools/telemetry/telemetry/tab.py:60: 'activate/%s' % tab_id, timeout=timeout) Is this call synchronous?
Thanks Dave for your comments. I've changed tab.Activate() to have similar flow as tab.Close(). Please take another look whenever you have a chance. Thanks, Danh https://codereview.chromium.org/11663011/diff/4002/tools/telemetry/telemetry/... File tools/telemetry/telemetry/desktop_browser_backend.py (right): https://codereview.chromium.org/11663011/diff/4002/tools/telemetry/telemetry/... tools/telemetry/telemetry/desktop_browser_backend.py:52: args.append('--enable-stats-table') On 2012/12/27 18:57:25, Dave Tu wrote: > No --enable-stats-table. Explanation is here: > https://codereview.chromium.org/11274046/ (The tl;dr is that it crashes in the > reference build.) Without this flag, telemetry crashes the debug build due to a DCHECK at at chrome_content_browser_client.cc(992). If we add this flag only when using debug build, it won't crash the reference build. https://codereview.chromium.org/11663011/diff/4002/tools/telemetry/telemetry/... File tools/telemetry/telemetry/tab.py (right): https://codereview.chromium.org/11663011/diff/4002/tools/telemetry/telemetry/... tools/telemetry/telemetry/tab.py:60: 'activate/%s' % tab_id, timeout=timeout) On 2012/12/27 18:57:25, Dave Tu wrote: > Is this call synchronous? I've traced this call to Browser::ActivateContents() which doesn't wait for every elements in a tab to redraw. So I think it's not synchronous.
Looks better. You need unit tests. https://codereview.chromium.org/11663011/diff/11001/tools/telemetry/telemetry... File tools/telemetry/telemetry/desktop_browser_backend.py (right): https://codereview.chromium.org/11663011/diff/11001/tools/telemetry/telemetry... tools/telemetry/telemetry/desktop_browser_backend.py:52: if self.browser_type == 'debug': > Without this flag, telemetry crashes the debug build due to a DCHECK at at chrome_content_browser_client.cc(992). Please keep changes separate. This change deals with activating a tab. Dont add fixes for other bugs.
https://chromiumcodereview.appspot.com/11663011/diff/11001/tools/telemetry/te... File tools/telemetry/telemetry/browser_backend.py (right): https://chromiumcodereview.appspot.com/11663011/diff/11001/tools/telemetry/te... tools/telemetry/telemetry/browser_backend.py:67: timeout=timeout) If it's not synchronous then do we need to wait for any condition to ensure that we don't have any races?
What is the race you're worrying about here David? Activating a tab is asynchronous and you dont know when its active. I think its better to explain in the docs that the activation is asynchronous and that the tab may not be visible when it returns. Its very hard to actually wait for the tab to be "visible," anyway. What does visible mean, after all? Onscreen? Drawn? Submitted to the GPU? Visible to the eye? Its complex. So the point is, this api should make no guarantees. If a test needs a specific guarantee, then it will want to look for another api to deal with that. For instance, jbaumans' latency measuremnt api will provide a way to get a callback when a tab is submitted to the OS for rendering. A design doc for which is here: https://docs.google.com/a/google.com/document/d/1UEoVttE9UMNs3ouL0tZ41GAMkOk7... But that's a totally different patch than this patch...
On 2013/01/04 01:01:48, nduca wrote: > What is the race you're worrying about here David? Activating a tab is > asynchronous and you dont know when its active. > > I think its better to explain in the docs that the activation is asynchronous > and that the tab may not be visible when it returns. > > Its very hard to actually wait for the tab to be "visible," anyway. What does > visible mean, after all? Onscreen? Drawn? Submitted to the GPU? Visible to the > eye? Its complex. > > So the point is, this api should make no guarantees. > > If a test needs a specific guarantee, then it will want to look for another api > to deal with that. For instance, jbaumans' latency measuremnt api will provide a > way to get a callback when a tab is submitted to the OS for rendering. A design > doc for which is here: > https://docs.google.com/a/google.com/document/d/1UEoVttE9UMNs3ouL0tZ41GAMkOk7... > > But that's a totally different patch than this patch... That sounds good. I was just concerned about potential future tests that rely on it being synchronous. But the current stress test doesn't require that and warning about it in the API docs is sufficient.
Hi Dave & Nat, Thanks so much for your comments. I've added unit test and the wait on webkitVisibilityState when activating a tab. Please take another look. Thanks, Danh https://codereview.chromium.org/11663011/diff/11001/tools/telemetry/telemetry... File tools/telemetry/telemetry/browser_backend.py (right): https://codereview.chromium.org/11663011/diff/11001/tools/telemetry/telemetry... tools/telemetry/telemetry/browser_backend.py:67: timeout=timeout) On 2013/01/03 21:30:33, Dave Tu wrote: > If it's not synchronous then do we need to wait for any condition to ensure that > we don't have any races? Done. Added the wait for webkitVisibilityState to change from 'hidden' to 'visible'. https://codereview.chromium.org/11663011/diff/11001/tools/telemetry/telemetry... File tools/telemetry/telemetry/desktop_browser_backend.py (right): https://codereview.chromium.org/11663011/diff/11001/tools/telemetry/telemetry... tools/telemetry/telemetry/desktop_browser_backend.py:52: if self.browser_type == 'debug': On 2013/01/03 21:19:09, nduca wrote: > > Without this flag, telemetry crashes the debug build due to a DCHECK at at > chrome_content_browser_client.cc(992). > > Please keep changes separate. This change deals with activating a tab. Dont add > fixes for other bugs. Yes I'll put this fix in a different CL.
You'll want to update your CL description btw.
https://codereview.chromium.org/11663011/diff/20002/tools/telemetry/telemetry... File tools/telemetry/telemetry/browser_backend.py (right): https://codereview.chromium.org/11663011/diff/20002/tools/telemetry/telemetry... tools/telemetry/telemetry/browser_backend.py:73: tab_object = self._tab_dict[debugger_url] Please dont do this. Remove this code, its not safe. Make the tab switch async and make the documentation very clearly state this. https://codereview.chromium.org/11663011/diff/20002/tools/telemetry/telemetry... File tools/telemetry/telemetry/tab.py (right): https://codereview.chromium.org/11663011/diff/20002/tools/telemetry/telemetry... tools/telemetry/telemetry/tab.py:73: """Brings this tab to the foreground.""" """Brings this tab to the foreground, asynchronously. Please note: this is asynchronous. There is a delay between this call and the page's documentVisibilityState becoming 'visible', and yet more delay until the actual tab is visible to the user. None of these delays are included in this call.""" https://codereview.chromium.org/11663011/diff/20002/tools/telemetry/telemetry... tools/telemetry/telemetry/tab.py:122: def IsDocumentVisibilityStateToBeVisible(self): Visibility is a very confusing thing. Document visibility state and becoming visible to the user is a long process, and visibilty state happens before the page is visible to the user. The page still has to render, which takes some time.To know that, you need @jbaumans' visibility patch. Because of the ambiguity around this, I think we should not make anything about visibility a part of the tab api. Your unit test may need some of this code, thats fine. But it shouldn't be part of the official api. https://codereview.chromium.org/11663011/diff/20002/tools/telemetry/telemetry... File tools/telemetry/telemetry/tab_unittest.py (right): https://codereview.chromium.org/11663011/diff/20002/tools/telemetry/telemetry... tools/telemetry/telemetry/tab_unittest.py:25: self.assertTrue(self._tab.IsDocumentVisibilityStateToBeVisible()) s/IsDocumentVisibilityStateToBeVisible/IsDocumentVisible/ this test should be something more like function declared inline or on the module outside the class. def IsDocumentVisible(tab): ... the code you had before... assertTrue(IsDocumentVisible(self._tab))) otherTab = self._browser.tabs.New() WaitFor(lambda: IsDocumentVisible(otherTab)) assertFalse(IsDocumentVisible(self._tab))) etc
Thanks Nat for reviewing and for your comments. I've just made another update. Please take another look. Thanks, Danh https://codereview.chromium.org/11663011/diff/20002/tools/telemetry/telemetry... File tools/telemetry/telemetry/browser_backend.py (right): https://codereview.chromium.org/11663011/diff/20002/tools/telemetry/telemetry... tools/telemetry/telemetry/browser_backend.py:73: tab_object = self._tab_dict[debugger_url] On 2013/01/05 00:44:50, nduca wrote: > Please dont do this. Remove this code, its not safe. Make the tab switch async > and make the documentation very clearly state this. Done. https://codereview.chromium.org/11663011/diff/20002/tools/telemetry/telemetry... File tools/telemetry/telemetry/tab.py (right): https://codereview.chromium.org/11663011/diff/20002/tools/telemetry/telemetry... tools/telemetry/telemetry/tab.py:73: """Brings this tab to the foreground.""" On 2013/01/05 00:44:50, nduca wrote: > """Brings this tab to the foreground, asynchronously. > > Please note: this is asynchronous. There is a delay between this call and the > page's documentVisibilityState becoming 'visible', and yet more delay until the > actual tab is visible to the user. None of these delays are included in this > call.""" Done. Thanks for suggesting the doc string. https://codereview.chromium.org/11663011/diff/20002/tools/telemetry/telemetry... tools/telemetry/telemetry/tab.py:122: def IsDocumentVisibilityStateToBeVisible(self): On 2013/01/05 00:44:50, nduca wrote: > Visibility is a very confusing thing. Document visibility state and becoming > visible to the user is a long process, and visibilty state happens before the > page is visible to the user. The page still has to render, which takes some > time.To know that, you need @jbaumans' visibility patch. > > Because of the ambiguity around this, I think we should not make anything about > visibility a part of the tab api. > > Your unit test may need some of this code, thats fine. But it shouldn't be part > of the official api. Done. https://codereview.chromium.org/11663011/diff/20002/tools/telemetry/telemetry... File tools/telemetry/telemetry/tab_unittest.py (right): https://codereview.chromium.org/11663011/diff/20002/tools/telemetry/telemetry... tools/telemetry/telemetry/tab_unittest.py:25: self.assertTrue(self._tab.IsDocumentVisibilityStateToBeVisible()) On 2013/01/05 00:44:50, nduca wrote: > s/IsDocumentVisibilityStateToBeVisible/IsDocumentVisible/ > > this test should be something more like > > function declared inline or on the module outside the class. > def IsDocumentVisible(tab): > ... the code you had before... > > assertTrue(IsDocumentVisible(self._tab))) > otherTab = self._browser.tabs.New() > WaitFor(lambda: IsDocumentVisible(otherTab)) > assertFalse(IsDocumentVisible(self._tab))) > > etc Done.
lgtm https://codereview.chromium.org/11663011/diff/27001/tools/telemetry/telemetry... File tools/telemetry/telemetry/tab_unittest.py (right): https://codereview.chromium.org/11663011/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/tab_unittest.py:34: def _IsDocumentVisible(self, tab): this doesn't use self. So you can pull it out to be a module level function.
Thanks, Nat. Danh https://codereview.chromium.org/11663011/diff/27001/tools/telemetry/telemetry... File tools/telemetry/telemetry/tab_unittest.py (right): https://codereview.chromium.org/11663011/diff/27001/tools/telemetry/telemetry... tools/telemetry/telemetry/tab_unittest.py:34: def _IsDocumentVisible(self, tab): On 2013/01/07 19:18:31, nduca wrote: > this doesn't use self. So you can pull it out to be a module level function. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danhn@chromium.org/11663011/25002
Retried try job too often on win_aura for step(s) interactive_ui_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danhn@chromium.org/11663011/25002
Message was sent while issue was closed.
Change committed as 175440 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
