Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6269001:
Fixing omnibox page content search test (Closed)
|
Created:
9 years, 11 months ago by rohitbm Modified:
9 years, 3 months ago CC:
chromium-reviews Visibility:
Public. |
DescriptionFixing omnibox page content search test
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71523
Patch Set 1 #Patch Set 2 : '' #
Total comments: 4
Patch Set 3 : '' #Patch Set 4 : '' #Messages
Total messages: 10 (0 generated)
http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py File functional/omnibox.py (right): http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py#newcod... functional/omnibox.py:297: search_text = 'British throne' You don't need this variable. Pass this value directly as you are not using it any where else. http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py#newcod... functional/omnibox.py:300: self.AppendTab(pyauto.GURL(url)) Why are you opening a new tab?
On 2011/01/13 02:18:54, sunandt wrote: > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py > File functional/omnibox.py (right): > > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py#newcod... > functional/omnibox.py:297: search_text = 'British throne' > You don't need this variable. Pass this value directly as you are not using it > any where else. Search_text variable holds an import value for the test and mainly this variable makes value more meaningful so I used a variable here. I see that other tests are doing the same. If you think I should still remove this variable, I will do that. > > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py#newcod... > functional/omnibox.py:300: self.AppendTab(pyauto.GURL(url)) > Why are you opening a new tab? I thought to make test more spicy. Let me know if I should not open a new tab.
http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py File functional/omnibox.py (right): http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py#newcod... functional/omnibox.py:282: """Determines if omnibox returns a visited page when searching using page s/visited/previously visited/ http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py#newcod... functional/omnibox.py:282: """Determines if omnibox returns a visited page when searching using page Simplify this sentence. I don't understand it.
On 2011/01/14 01:55:26, rohitbm wrote: > On 2011/01/13 02:18:54, sunandt wrote: > > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py > > File functional/omnibox.py (right): > > > > > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py#newcod... > > functional/omnibox.py:297: search_text = 'British throne' > > You don't need this variable. Pass this value directly as you are not using it > > any where else. > Search_text variable holds an import value for the test and mainly this variable > makes value more meaningful so I used a variable here. I see that other tests > are doing the same. > If you think I should still remove this variable, I will do that. I don't think we should use an extra variable when 1. It's not being used to store some result 2. Not being reused If possible modify other tests where it's not needed. We can reduce some lines of code as well. > > > > > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py#newcod... > > functional/omnibox.py:300: self.AppendTab(pyauto.GURL(url)) > > Why are you opening a new tab? > I thought to make test more spicy. Let me know if I should not open a new tab. Not really needed in this case I guess. Opening another tab would just increase the execution time of the test.
On 2011/01/14 21:43:25, sunandt wrote: > On 2011/01/14 01:55:26, rohitbm wrote: > > On 2011/01/13 02:18:54, sunandt wrote: > > > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py > > > File functional/omnibox.py (right): > > > > > > > > > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py#newcod... > > > functional/omnibox.py:297: search_text = 'British throne' > > > You don't need this variable. Pass this value directly as you are not using > it > > > any where else. > > Search_text variable holds an import value for the test and mainly this > variable > > makes value more meaningful so I used a variable here. I see that other tests > > are doing the same. > > If you think I should still remove this variable, I will do that. > I don't think we should use an extra variable when > 1. It's not being used to store some result > 2. Not being reused > If possible modify other tests where it's not needed. We can reduce some lines > of code as well. Done. > > > > > > > > > > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py#newcod... > > > functional/omnibox.py:300: self.AppendTab(pyauto.GURL(url)) > > > Why are you opening a new tab? > > I thought to make test more spicy. Let me know if I should not open a new tab. > Not really needed in this case I guess. Opening another tab would just increase > the execution time of the test. Done.
On 2011/01/14 19:49:57, Nirnimesh wrote: > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py > File functional/omnibox.py (right): > > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py#newcod... > functional/omnibox.py:282: """Determines if omnibox returns a visited page when > searching using page > s/visited/previously visited/ Done. > > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py#newcod... > functional/omnibox.py:282: """Determines if omnibox returns a visited page when > searching using page > Simplify this sentence. I don't understand it. Done.
LGTM
On 2011/01/14 23:01:32, rohitbm wrote: > On 2011/01/14 21:43:25, sunandt wrote: > > On 2011/01/14 01:55:26, rohitbm wrote: > > > On 2011/01/13 02:18:54, sunandt wrote: > > > > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py > > > > File functional/omnibox.py (right): > > > > > > > > > > > > > > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py#newcod... > > > > functional/omnibox.py:297: search_text = 'British throne' > > > > You don't need this variable. Pass this value directly as you are not > using > > it > > > > any where else. > > > Search_text variable holds an import value for the test and mainly this > > variable > > > makes value more meaningful so I used a variable here. I see that other > tests > > > are doing the same. > > > If you think I should still remove this variable, I will do that. > > I don't think we should use an extra variable when > > 1. It's not being used to store some result > > 2. Not being reused > > If possible modify other tests where it's not needed. We can reduce some lines > > of code as well. > Done. > > > > > > > > > > > > > > > > http://codereview.chromium.org/6269001/diff/2001/functional/omnibox.py#newcod... > > > > functional/omnibox.py:300: self.AppendTab(pyauto.GURL(url)) > > > > Why are you opening a new tab? > > > I thought to make test more spicy. Let me know if I should not open a new > tab. > > Not really needed in this case I guess. Opening another tab would just > increase > > the execution time of the test. > Done. LGTM |
