Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5322001:
Before starting translation reset cache in AutoFillManager. To call Reset() f... (Closed)
|
Created:
10 years, 1 month ago by honten.org Modified:
9 years, 7 months ago CC:
chromium-reviews, ben+cc_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, Paweł Hajdan Jr., James Hawkins, nkostylev+cc_chromium.org, davemoore+watch_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionBefore starting translation reset cache in AutoFillManager. To call Reset() function, append Reset() virtual function in RenderViewHostDelegate::AutoFill and derive it from AutoFillManager.
BUG=58576
TEST= 1.launch English version Chrome 2. make sure autofill is enable and there is autofill data entry in settings 3.navigate to a non-English website e.g.: https://secure.fnac.pt/Account/Logon/LogonNewAccount.aspx?NID=-15&RNID=-15&PrevNID=0&pagepar=SID%3d22120814-4186-d926-4f7b-0dece96fade2|Origin%3dFnacAff|OrderInSession%3d1|TTL%3d070420110015|bl%3dHGAChead&PageRedir=https://www2.fnac.pt/Account/Profil/default.asp&PageAuth=yes&LogonType=ACCOUNT 4. when translation infobar show up, click "Translate" to translate to English 5. focus in "E-mail" field 6. press Down arrow 7. select the autofill data entry by double click on it 8. autofill data should be fill in all text boxes
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69088
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69199
Patch Set 1 #Patch Set 2 : '' #
Total comments: 4
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 6
Patch Set 5 : wrote TranslateAndFormFill test. #
Total comments: 7
Patch Set 6 : fix some style in autofill_browsertest.cc #
Total comments: 1
Patch Set 7 : Fix typo #
Total comments: 2
Patch Set 8 : roll back browsertest #Patch Set 9 : add Reset() #
Messages
Total messages: 56 (0 generated)
James, Thanks to your advice, I suppose I can write some fix for this bug. Could you review it? For now still I don't have any test.Do you have any idea? Because this bug happens when the page is translated. It might be a little bit difficult to write independent test code. Thanks,
On 2010/11/23 05:55:36, honten wrote: > James, > > Thanks to your advice, I suppose I can write some fix for this bug. > > Could you review it? > > For now still I don't have any test.Do you have any idea? Because this bug > happens when the page is translated. It might be a little bit difficult to write > independent test code. > > Thanks, Take a look at the translate test code to see what's done there and try to replicate.
http://codereview.chromium.org/5322001/diff/5001/chrome/browser/autofill/auto... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/5322001/diff/5001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_manager.h:65: // This methodo is one of RenderViewHostDelegate::AutoFill implementation. Please remove these two comments and move the method declaration below |ShowAutoFillDialog()|. http://codereview.chromium.org/5322001/diff/5001/chrome/browser/renderer_host... File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/5322001/diff/5001/chrome/browser/renderer_host... chrome/browser/renderer_host/render_view_host.cc:2006: // Before start translation, reset cache in AutoFillManager first. Please expand this comment to explain why.
James, Thank you for your review. Could you give me your thought about the comment? Thanks, http://codereview.chromium.org/5322001/diff/5001/chrome/browser/renderer_host... File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/5322001/diff/5001/chrome/browser/renderer_host... chrome/browser/renderer_host/render_view_host.cc:2006: // Before start translation, reset cache in AutoFillManager first. I want to replace the comment as following, what do you think? Unfortunately form control element names are not always unique enough. If the labels change as a result of translation, we should be tracking those changes and updating the caches in AutoFillManater. Ideally we'd have a better way to uniquely identify form control elements, but we don't have that yet. So before start translation, we clear the current form and re-parse it in AutoFillManter first to get the new labels. On 2010/11/23 19:18:53, James Hawkins wrote: > Please expand this comment to explain why.
http://codereview.chromium.org/5322001/diff/5001/chrome/browser/renderer_host... File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/5322001/diff/5001/chrome/browser/renderer_host... chrome/browser/renderer_host/render_view_host.cc:2006: // Before start translation, reset cache in AutoFillManager first. On 2010/11/23 19:32:22, honten wrote: > I want to replace the comment as following, what do you think? > > Unfortunately form control element names are not always unique enough. If the > labels change as a result of translation, we should be tracking those changes > and updating the caches in AutoFillManater. Ideally we'd have a better way to > uniquely identify form control elements, but we don't have that yet. So before > start translation, we clear the current form and re-parse it in AutoFillManter > first to get the new labels. > On 2010/11/23 19:18:53, James Hawkins wrote: > > Please expand this comment to explain why. > Way too verbose. The last two sentences suffice.
Ok, Thanks!! On 2010/11/23 19:35:33, James Hawkins wrote: > http://codereview.chromium.org/5322001/diff/5001/chrome/browser/renderer_host... > File chrome/browser/renderer_host/render_view_host.cc (right): > > http://codereview.chromium.org/5322001/diff/5001/chrome/browser/renderer_host... > chrome/browser/renderer_host/render_view_host.cc:2006: // Before start > translation, reset cache in AutoFillManager first. > On 2010/11/23 19:32:22, honten wrote: > > I want to replace the comment as following, what do you think? > > > > Unfortunately form control element names are not always unique enough. If the > > labels change as a result of translation, we should be tracking those changes > > and updating the caches in AutoFillManater. Ideally we'd have a better way to > > uniquely identify form control elements, but we don't have that yet. So > before > > start translation, we clear the current form and re-parse it in AutoFillManter > > first to get the new labels. > > On 2010/11/23 19:18:53, James Hawkins wrote: > > > Please expand this comment to explain why. > > > > Way too verbose. The last two sentences suffice.
James, I cannot find any RnderViewHost unit test in the source code. Instead of that, I found functional test as following, chrome/test/functional/translate.py Also I found autofill.py. Is is enough to wirte functional test here with Python? Thanks, On 2010/11/23 19:13:58, James Hawkins wrote: > On 2010/11/23 05:55:36, honten wrote: > > James, > > > > Thanks to your advice, I suppose I can write some fix for this bug. > > > > Could you review it? > > > > For now still I don't have any test.Do you have any idea? Because this bug > > happens when the page is translated. It might be a little bit difficult to > write > > independent test code. > > > > Thanks, > > Take a look at the translate test code to see what's done there and try to > replicate.
On 2010/11/23 21:16:58, honten wrote: > James, > > I cannot find any RnderViewHost unit test in the source code. Instead of that, I > found functional test as following, > chrome/test/functional/translate.py > > Also I found autofill.py. > > Is is enough to wirte functional test here with Python? > Are you asking if I'll be satisfied if you write a functional test in python, or merely if it's possible to write such a test?
James, I mean "you're satisfied with functional test only, not writing unit test." Last time when I fixed a bug in chrome project, I wrote unit tests, so I wonder if it's sufficient to write only functional tests. Thanks, On Tue, Nov 23, 2010 at 1:22 PM, <jhawkins@chromium.org> wrote: > On 2010/11/23 21:16:58, honten wrote: >> >> James, > >> I cannot find any RnderViewHost unit test in the source code. Instead of >> that, > > I >> >> found functional test as following, >> chrome/test/functional/translate.py > >> Also I found autofill.py. > >> Is is enough to wirte functional test here with Python? > > > Are you asking if I'll be satisfied if you write a functional test in > python, or > merely if it's possible to write such a test? > > http://codereview.chromium.org/5322001/ >
On 2010/11/23 21:27:10, takano.naoki_gmail.com wrote: > James, > > I mean "you're satisfied with functional test only, not writing unit test." > > Last time when I fixed a bug in chrome project, I wrote unit tests, so > I wonder if it's sufficient to write only functional tests. > Yes it's fine.
Ok, I try. Thanks!! On Tue, Nov 23, 2010 at 1:31 PM, <jhawkins@chromium.org> wrote: > On 2010/11/23 21:27:10, takano.naoki_gmail.com wrote: >> >> James, > >> I mean "you're satisfied with functional test only, not writing unit >> test." > >> Last time when I fixed a bug in chrome project, I wrote unit tests, so >> I wonder if it's sufficient to write only functional tests. > > > Yes it's fine. > > http://codereview.chromium.org/5322001/ >
James, Ilya I'm trying to write test code, but I have a couple of questions.. It looks like very hard for me to implement this case for now. The reason is following, 1, At first, as I mentioned, I tried to use translate.py, but I cannot find any form related API in PYAUTO. Of course I looked into autofill.py, but it actually fill the profile directly with Javascript, but it doesn't check anything in real html pages. 2, After that, I referred to translate_manager_unittest.cc, but it does not looks to trigger any rendering. I found how to get AutoFillManager() instance to handle forms, but the instance is not called even if translation happens. 3, Right now, I start to refer to form_structure_browsertest.cc, but I don't know this is right approach or not. I found the related mail thread as following http://groups.google.com/a/chromium.org/group/chromium-reviews/browse_thread/... I found it, but I cannot confirm it. Because my local source is a little bit old. After I get it, I'll confirm it. Do you have any other idea or any test pattern I should refer to? Thanks, On Tue, Nov 23, 2010 at 1:33 PM, Naoki Takano <takano.naoki@gmail.com> wrote: > Ok, I try. > > Thanks!! > > On Tue, Nov 23, 2010 at 1:31 PM, <jhawkins@chromium.org> wrote: >> On 2010/11/23 21:27:10, takano.naoki_gmail.com wrote: >>> >>> James, >> >>> I mean "you're satisfied with functional test only, not writing unit >>> test." >> >>> Last time when I fixed a bug in chrome project, I wrote unit tests, so >>> I wonder if it's sufficient to write only functional tests. >> >> >> Yes it's fine. >> >> http://codereview.chromium.org/5322001/ >> >
autofill_browsertest.cc may provide some insight. You may be able to take the AutoFillTest.BasicFormFill test and modify it to add a navigation that triggers translate. On 2010/11/25 06:41:19, takano.naoki_gmail.com wrote: > James, Ilya > > I'm trying to write test code, but I have a couple of questions.. > It looks like very hard for me to implement this case for now. > > The reason is following, > 1, At first, as I mentioned, I tried to use translate.py, but I cannot > find any form related API in PYAUTO. Of course I looked into > autofill.py, but it actually fill the profile directly with > Javascript, but it doesn't check anything in real html pages. > 2, After that, I referred to translate_manager_unittest.cc, but it > does not looks to trigger any rendering. I found how to get > AutoFillManager() instance to handle forms, but the instance is not > called even if translation happens. > 3, Right now, I start to refer to form_structure_browsertest.cc, but I > don't know this is right approach or not. I found the related mail > thread as following > http://groups.google.com/a/chromium.org/group/chromium-reviews/browse_thread/... > I found it, but I cannot confirm it. Because my local source is a > little bit old. After I get it, I'll confirm it. > > Do you have any other idea or any test pattern I should refer to? > > Thanks, > > On Tue, Nov 23, 2010 at 1:33 PM, Naoki Takano <mailto:takano.naoki@gmail.com> wrote: > > Ok, I try. > > > > Thanks!! > > > > On Tue, Nov 23, 2010 at 1:31 PM, mailto: <jhawkins@chromium.org> wrote: > >> On 2010/11/23 21:27:10, http://takano.naoki_gmail.com wrote: > >>> > >>> James, > >> > >>> I mean "you're satisfied with functional test only, not writing unit > >>> test." > >> > >>> Last time when I fixed a bug in chrome project, I wrote unit tests, so > >>> I wonder if it's sufficient to write only functional tests. > >> > >> > >> Yes it's fine. > >> > >> http://codereview.chromium.org/5322001/ > >> > >
Thank you for you suggestion!! I'll try it. On Mon, Nov 29, 2010 at 9:32 AM, <dhollowa@chromium.org> wrote: > autofill_browsertest.cc may provide some insight. You may be able to take > the > AutoFillTest.BasicFormFill test and modify it to add a navigation that > triggers > translate. > > On 2010/11/25 06:41:19, takano.naoki_gmail.com wrote: >> >> James, Ilya > >> I'm trying to write test code, but I have a couple of questions.. >> It looks like very hard for me to implement this case for now. > >> The reason is following, >> 1, At first, as I mentioned, I tried to use translate.py, but I cannot >> find any form related API in PYAUTO. Of course I looked into >> autofill.py, but it actually fill the profile directly with >> Javascript, but it doesn't check anything in real html pages. >> 2, After that, I referred to translate_manager_unittest.cc, but it >> does not looks to trigger any rendering. I found how to get >> AutoFillManager() instance to handle forms, but the instance is not >> called even if translation happens. >> 3, Right now, I start to refer to form_structure_browsertest.cc, but I >> don't know this is right approach or not. I found the related mail >> thread as following > > http://groups.google.com/a/chromium.org/group/chromium-reviews/browse_thread/... >> >> I found it, but I cannot confirm it. Because my local source is a >> little bit old. After I get it, I'll confirm it. > >> Do you have any other idea or any test pattern I should refer to? > >> Thanks, > >> On Tue, Nov 23, 2010 at 1:33 PM, Naoki Takano >> <mailto:takano.naoki@gmail.com> > > wrote: >> >> > Ok, I try. >> > >> > Thanks!! >> > >> > On Tue, Nov 23, 2010 at 1:31 PM, mailto: <jhawkins@chromium.org> wrote: >> >> On 2010/11/23 21:27:10, http://takano.naoki_gmail.com wrote: >> >>> >> >>> James, >> >> >> >>> I mean "you're satisfied with functional test only, not writing unit >> >>> test." >> >> >> >>> Last time when I fixed a bug in chrome project, I wrote unit tests, so >> >>> I wonder if it's sufficient to write only functional tests. >> >> >> >> >> >> Yes it's fine. >> >> >> >> http://codereview.chromium.org/5322001/ >> >> >> > > > > > http://codereview.chromium.org/5322001/ >
I tried to add translate trigger.
It looks triggered but fails due to network error.
The error message says.
"The translation failed because of a problem with the network connection."
I guess InProcessBrowserTest does not support any network connection.
As you know, translation needs network connection to translate the text.
For example, if there is a simple code to navigate to google in test class
derived from InProcessBrowserTest as following,
ui_test_utils::NavigateToURL(browser(), GURL("http://www.google.com/"));
ui_test_utils::WaitForNavigationInCurrentTab(browser());
ui_test_utils::WaitForNewTab(browser()); // just wait to operate mouse.
Once we run browser test, we can see the browser. I cannot see goole web page,
but the location bar shows "www.google.com".
Even if I try to reload, I cannot get any real web page.
Is this correct, or is there any other trick I have to do?
Thanks,
On 2010/11/29 19:00:31, takano.naoki_gmail.com wrote:
> Thank you for you suggestion!!
>
> I'll try it.
>
> On Mon, Nov 29, 2010 at 9:32 AM, <mailto:dhollowa@chromium.org> wrote:
> > autofill_browsertest.cc may provide some insight. You may be able to
take
> > the
> > AutoFillTest.BasicFormFill test and modify it to add a navigation that
> > triggers
> > translate.
> >
> > On 2010/11/25 06:41:19, http://takano.naoki_gmail.com wrote:
> >>
> >> James, Ilya
> >
> >> I'm trying to write test code, but I have a couple of questions..
> >> It looks like very hard for me to implement this case for now.
> >
> >> The reason is following,
> >> 1, At first, as I mentioned, I tried to use translate.py, but I cannot
> >> find any form related API in PYAUTO. Of course I looked into
> >> autofill.py, but it actually fill the profile directly with
> >> Javascript, but it doesn't check anything in real html pages.
> >> 2, After that, I referred to translate_manager_unittest.cc, but it
> >> does not looks to trigger any rendering. I found how to get
> >> AutoFillManager() instance to handle forms, but the instance is not
> >> called even if translation happens.
> >> 3, Right now, I start to refer to form_structure_browsertest.cc, but I
> >> don't know this is right approach or not. I found the related mail
> >> thread as following
> >
> >
>
http://groups.google.com/a/chromium.org/group/chromium-reviews/browse_thread/...
> >>
> >> I found it, but I cannot confirm it. Because my local source is a
> >> little bit old. After I get it, I'll confirm it.
> >
> >> Do you have any other idea or any test pattern I should refer to?
> >
> >> Thanks,
> >
> >> On Tue, Nov 23, 2010 at 1:33 PM, Naoki Takano
> >> <mailto:takano.naoki@gmail.com>
> >
> > wrote:
> >>
> >> > Ok, I try.
> >> >
> >> > Thanks!!
> >> >
> >> > On Tue, Nov 23, 2010 at 1:31 PM, mailto: <jhawkins@chromium.org>
wrote:
> >> >> On 2010/11/23 21:27:10, http://takano.naoki_gmail.com wrote:
> >> >>>
> >> >>> James,
> >> >>
> >> >>> I mean "you're satisfied with functional test only, not writing unit
> >> >>> test."
> >> >>
> >> >>> Last time when I fixed a bug in chrome project, I wrote unit tests, so
> >> >>> I wonder if it's sufficient to write only functional tests.
> >> >>
> >> >>
> >> >> Yes it's fine.
> >> >>
> >> >> http://codereview.chromium.org/5322001/
> >> >>
> >> >
> >
> >
> >
> > http://codereview.chromium.org/5322001/
> >
Mock the translation process? On Mon, Nov 29, 2010 at 11:10 PM, <Takano.Naoki@gmail.com> wrote: > I tried to add translate trigger. > It looks triggered but fails due to network error. > > The error message says. > "The translation failed because of a problem with the network connection." > > I guess InProcessBrowserTest does not support any network connection. > As you know, translation needs network connection to translate the text. > > For example, if there is a simple code to navigate to google in test class > derived from InProcessBrowserTest as following, > ui_test_utils::NavigateToURL(browser(), GURL("http://www.google.com/")); > ui_test_utils::WaitForNavigationInCurrentTab(browser()); > ui_test_utils::WaitForNewTab(browser()); // just wait to operate mouse. > > Once we run browser test, we can see the browser. I cannot see goole web > page, > but the location bar shows "www.google.com". > > Even if I try to reload, I cannot get any real web page. > > Is this correct, or is there any other trick I have to do? > > Thanks, > > > > > On 2010/11/29 19:00:31, takano.naoki_gmail.com wrote: > >> Thank you for you suggestion!! >> > > I'll try it. >> > > On Mon, Nov 29, 2010 at 9:32 AM, <mailto:dhollowa@chromium.org> wrote: >> > autofill_browsertest.cc may provide some insight. You may be able >> to >> > take > >> > the >> > AutoFillTest.BasicFormFill test and modify it to add a navigation that >> > triggers >> > translate. >> > >> > On 2010/11/25 06:41:19, http://takano.naoki_gmail.com wrote: >> >> >> >> James, Ilya >> > >> >> I'm trying to write test code, but I have a couple of questions.. >> >> It looks like very hard for me to implement this case for now. >> > >> >> The reason is following, >> >> 1, At first, as I mentioned, I tried to use translate.py, but I cannot >> >> find any form related API in PYAUTO. Of course I looked into >> >> autofill.py, but it actually fill the profile directly with >> >> Javascript, but it doesn't check anything in real html pages. >> >> 2, After that, I referred to translate_manager_unittest.cc, but it >> >> does not looks to trigger any rendering. I found how to get >> >> >> AutoFillManager() instance to handle forms, but the instance is not >> >> called even if translation happens. >> >> 3, Right now, I start to refer to form_structure_browsertest.cc, but I >> >> don't know this is right approach or not. I found the related mail >> >> thread as following >> > >> > >> > > > http://groups.google.com/a/chromium.org/group/chromium-reviews/browse_thread/... > >> >> >> >> I found it, but I cannot confirm it. Because my local source is a >> >> little bit old. After I get it, I'll confirm it. >> > >> >> Do you have any other idea or any test pattern I should refer to? >> > >> >> Thanks, >> > >> >> On Tue, Nov 23, 2010 at 1:33 PM, Naoki Takano >> >> <mailto:takano.naoki@gmail.com> >> > >> > wrote: >> >> >> >> > Ok, I try. >> >> > >> >> > Thanks!! >> >> > >> >> > On Tue, Nov 23, 2010 at 1:31 PM, mailto: <jhawkins@chromium.org >> > >> > wrote: > >> >> >> On 2010/11/23 21:27:10, http://takano.naoki_gmail.com wrote: >> >> >>> >> >> >>> James, >> >> >> >> >> >>> I mean "you're satisfied with functional test only, not writing >> unit >> >> >>> test." >> >> >> >> >> >>> Last time when I fixed a bug in chrome project, I wrote unit tests, >> so >> >> >>> I wonder if it's sufficient to write only functional tests. >> >> >> >> >> >> >> >> >> Yes it's fine. >> >> >> >> >> >> http://codereview.chromium.org/5322001/ >> >> >> >> >> > >> > >> > >> > >> > http://codereview.chromium.org/5322001/ >> > >> > > > > http://codereview.chromium.org/5322001/ >
Yes, And the Translate Tool Bar shows the network error. On Tue, Nov 30, 2010 at 10:55 AM, James Hawkins <jhawkins@chromium.org> wrote: > Mock the translation process? > > On Mon, Nov 29, 2010 at 11:10 PM, <Takano.Naoki@gmail.com> wrote: >> >> I tried to add translate trigger. >> It looks triggered but fails due to network error. >> >> The error message says. >> "The translation failed because of a problem with the network connection." >> >> I guess InProcessBrowserTest does not support any network connection. >> As you know, translation needs network connection to translate the text. >> >> For example, if there is a simple code to navigate to google in test class >> derived from InProcessBrowserTest as following, >> ui_test_utils::NavigateToURL(browser(), GURL("http://www.google.com/")); >> ui_test_utils::WaitForNavigationInCurrentTab(browser()); >> ui_test_utils::WaitForNewTab(browser()); // just wait to operate mouse. >> >> Once we run browser test, we can see the browser. I cannot see goole web >> page, >> but the location bar shows "www.google.com". >> >> Even if I try to reload, I cannot get any real web page. >> >> Is this correct, or is there any other trick I have to do? >> >> Thanks, >> >> >> >> On 2010/11/29 19:00:31, takano.naoki_gmail.com wrote: >>> >>> Thank you for you suggestion!! >> >>> I'll try it. >> >>> On Mon, Nov 29, 2010 at 9:32 AM, <mailto:dhollowa@chromium.org> wrote: >>> > autofill_browsertest.cc may provide some insight. You may be able >>> > to >> >> take >>> >>> > the >>> > AutoFillTest.BasicFormFill test and modify it to add a navigation that >>> > triggers >>> > translate. >>> > >>> > On 2010/11/25 06:41:19, http://takano.naoki_gmail.com wrote: >>> >> >>> >> James, Ilya >>> > >>> >> I'm trying to write test code, but I have a couple of questions.. >>> >> It looks like very hard for me to implement this case for now. >>> > >>> >> The reason is following, >>> >> 1, At first, as I mentioned, I tried to use translate.py, but I cannot >>> >> find any form related API in PYAUTO. Of course I looked into >>> >> autofill.py, but it actually fill the profile directly with >>> >> Javascript, but it doesn't check anything in real html pages. >>> >> 2, After that, I referred to translate_manager_unittest.cc, but it >>> >> does not looks to trigger any rendering. I found how to get >>> >> AutoFillManager() instance to handle forms, but the instance is not >>> >> called even if translation happens. >>> >> 3, Right now, I start to refer to form_structure_browsertest.cc, but I >>> >> don't know this is right approach or not. I found the related mail >>> >> thread as following >>> > >>> > >> >> >> http://groups.google.com/a/chromium.org/group/chromium-reviews/browse_thread/... >>> >>> >> >>> >> I found it, but I cannot confirm it. Because my local source is a >>> >> little bit old. After I get it, I'll confirm it. >>> > >>> >> Do you have any other idea or any test pattern I should refer to? >>> > >>> >> Thanks, >>> > >>> >> On Tue, Nov 23, 2010 at 1:33 PM, Naoki Takano >>> >> <mailto:takano.naoki@gmail.com> >>> > >>> > wrote: >>> >> >>> >> > Ok, I try. >>> >> > >>> >> > Thanks!! >>> >> > >>> >> > On Tue, Nov 23, 2010 at 1:31 PM, >>> >> > mailto: <jhawkins@chromium.org> >> >> wrote: >>> >>> >> >> On 2010/11/23 21:27:10, http://takano.naoki_gmail.com wrote: >>> >> >>> >>> >> >>> James, >>> >> >> >>> >> >>> I mean "you're satisfied with functional test only, not writing >>> >> >>> unit >>> >> >>> test." >>> >> >> >>> >> >>> Last time when I fixed a bug in chrome project, I wrote unit >>> >> >>> tests, so >>> >> >>> I wonder if it's sufficient to write only functional tests. >>> >> >> >>> >> >> >>> >> >> Yes it's fine. >>> >> >> >>> >> >> http://codereview.chromium.org/5322001/ >>> >> >> >>> >> > >>> > >>> > >>> > >>> > http://codereview.chromium.org/5322001/ >>> > >> >> >> >> http://codereview.chromium.org/5322001/ > >
I don't understand your reply. I'm suggesting you mock out the translate process so it doesn't require a network connection. On Tue, Nov 30, 2010 at 11:15 AM, Naoki Takano <takano.naoki@gmail.com>wrote: > Yes, > > And the Translate Tool Bar shows the network error. > > On Tue, Nov 30, 2010 at 10:55 AM, James Hawkins <jhawkins@chromium.org> > wrote: > > Mock the translation process? > > > > On Mon, Nov 29, 2010 at 11:10 PM, <Takano.Naoki@gmail.com> wrote: > >> > >> I tried to add translate trigger. > >> It looks triggered but fails due to network error. > >> > >> The error message says. > >> "The translation failed because of a problem with the network > connection." > >> > >> I guess InProcessBrowserTest does not support any network connection. > >> As you know, translation needs network connection to translate the text. > >> > >> For example, if there is a simple code to navigate to google in test > class > >> derived from InProcessBrowserTest as following, > >> ui_test_utils::NavigateToURL(browser(), GURL("http://www.google.com/ > ")); > >> ui_test_utils::WaitForNavigationInCurrentTab(browser()); > >> ui_test_utils::WaitForNewTab(browser()); // just wait to operate mouse. > >> > >> Once we run browser test, we can see the browser. I cannot see goole web > >> page, > >> but the location bar shows "www.google.com". > >> > >> Even if I try to reload, I cannot get any real web page. > >> > >> Is this correct, or is there any other trick I have to do? > >> > >> Thanks, > >> > >> > >> > >> On 2010/11/29 19:00:31, takano.naoki_gmail.com wrote: > >>> > >>> Thank you for you suggestion!! > >> > >>> I'll try it. > >> > >>> On Mon, Nov 29, 2010 at 9:32 AM, <mailto:dhollowa@chromium.org> > wrote: > >>> > autofill_browsertest.cc may provide some insight. You may be > able > >>> > to > >> > >> take > >>> > >>> > the > >>> > AutoFillTest.BasicFormFill test and modify it to add a navigation > that > >>> > triggers > >>> > translate. > >>> > > >>> > On 2010/11/25 06:41:19, http://takano.naoki_gmail.com wrote: > >>> >> > >>> >> James, Ilya > >>> > > >>> >> I'm trying to write test code, but I have a couple of questions.. > >>> >> It looks like very hard for me to implement this case for now. > >>> > > >>> >> The reason is following, > >>> >> 1, At first, as I mentioned, I tried to use translate.py, but I > cannot > >>> >> find any form related API in PYAUTO. Of course I looked into > >>> >> autofill.py, but it actually fill the profile directly with > >>> >> Javascript, but it doesn't check anything in real html pages. > >>> >> 2, After that, I referred to translate_manager_unittest.cc, but it > >>> >> does not looks to trigger any rendering. I found how to get > >>> >> AutoFillManager() instance to handle forms, but the instance is not > >>> >> called even if translation happens. > >>> >> 3, Right now, I start to refer to form_structure_browsertest.cc, but > I > >>> >> don't know this is right approach or not. I found the related mail > >>> >> thread as following > >>> > > >>> > > >> > >> > >> > http://groups.google.com/a/chromium.org/group/chromium-reviews/browse_thread/... > >>> > >>> >> > >>> >> I found it, but I cannot confirm it. Because my local source is a > >>> >> little bit old. After I get it, I'll confirm it. > >>> > > >>> >> Do you have any other idea or any test pattern I should refer to? > >>> > > >>> >> Thanks, > >>> > > >>> >> On Tue, Nov 23, 2010 at 1:33 PM, Naoki Takano > >>> >> <mailto:takano.naoki@gmail.com> > >>> > > >>> > wrote: > >>> >> > >>> >> > Ok, I try. > >>> >> > > >>> >> > Thanks!! > >>> >> > > >>> >> > On Tue, Nov 23, 2010 at 1:31 PM, > >>> >> > mailto: <jhawkins@chromium.org> > >> > >> wrote: > >>> > >>> >> >> On 2010/11/23 21:27:10, http://takano.naoki_gmail.com wrote: > >>> >> >>> > >>> >> >>> James, > >>> >> >> > >>> >> >>> I mean "you're satisfied with functional test only, not writing > >>> >> >>> unit > >>> >> >>> test." > >>> >> >> > >>> >> >>> Last time when I fixed a bug in chrome project, I wrote unit > >>> >> >>> tests, so > >>> >> >>> I wonder if it's sufficient to write only functional tests. > >>> >> >> > >>> >> >> > >>> >> >> Yes it's fine. > >>> >> >> > >>> >> >> http://codereview.chromium.org/5322001/ > >>> >> >> > >>> >> > > >>> > > >>> > > >>> > > >>> > http://codereview.chromium.org/5322001/ > >>> > > >> > >> > >> > >> http://codereview.chromium.org/5322001/ > > > > >
Sorry for misunderstanding. I see. I'll try it. On Tue, Nov 30, 2010 at 11:33 AM, James Hawkins <jhawkins@chromium.org> wrote: > I don't understand your reply. I'm suggesting you mock out the translate > process so it doesn't require a network connection. > > On Tue, Nov 30, 2010 at 11:15 AM, Naoki Takano <takano.naoki@gmail.com> > wrote: >> >> Yes, >> >> And the Translate Tool Bar shows the network error. >> >> On Tue, Nov 30, 2010 at 10:55 AM, James Hawkins <jhawkins@chromium.org> >> wrote: >> > Mock the translation process? >> > >> > On Mon, Nov 29, 2010 at 11:10 PM, <Takano.Naoki@gmail.com> wrote: >> >> >> >> I tried to add translate trigger. >> >> It looks triggered but fails due to network error. >> >> >> >> The error message says. >> >> "The translation failed because of a problem with the network >> >> connection." >> >> >> >> I guess InProcessBrowserTest does not support any network connection. >> >> As you know, translation needs network connection to translate the >> >> text. >> >> >> >> For example, if there is a simple code to navigate to google in test >> >> class >> >> derived from InProcessBrowserTest as following, >> >> ui_test_utils::NavigateToURL(browser(), >> >> GURL("http://www.google.com/")); >> >> ui_test_utils::WaitForNavigationInCurrentTab(browser()); >> >> ui_test_utils::WaitForNewTab(browser()); // just wait to operate >> >> mouse. >> >> >> >> Once we run browser test, we can see the browser. I cannot see goole >> >> web >> >> page, >> >> but the location bar shows "www.google.com". >> >> >> >> Even if I try to reload, I cannot get any real web page. >> >> >> >> Is this correct, or is there any other trick I have to do? >> >> >> >> Thanks, >> >> >> >> >> >> >> >> On 2010/11/29 19:00:31, takano.naoki_gmail.com wrote: >> >>> >> >>> Thank you for you suggestion!! >> >> >> >>> I'll try it. >> >> >> >>> On Mon, Nov 29, 2010 at 9:32 AM, <mailto:dhollowa@chromium.org> >> >>> wrote: >> >>> > autofill_browsertest.cc may provide some insight. You may be >> >>> > able >> >>> > to >> >> >> >> take >> >>> >> >>> > the >> >>> > AutoFillTest.BasicFormFill test and modify it to add a navigation >> >>> > that >> >>> > triggers >> >>> > translate. >> >>> > >> >>> > On 2010/11/25 06:41:19, http://takano.naoki_gmail.com wrote: >> >>> >> >> >>> >> James, Ilya >> >>> > >> >>> >> I'm trying to write test code, but I have a couple of questions.. >> >>> >> It looks like very hard for me to implement this case for now. >> >>> > >> >>> >> The reason is following, >> >>> >> 1, At first, as I mentioned, I tried to use translate.py, but I >> >>> >> cannot >> >>> >> find any form related API in PYAUTO. Of course I looked into >> >>> >> autofill.py, but it actually fill the profile directly with >> >>> >> Javascript, but it doesn't check anything in real html pages. >> >>> >> 2, After that, I referred to translate_manager_unittest.cc, but it >> >>> >> does not looks to trigger any rendering. I found how to get >> >>> >> AutoFillManager() instance to handle forms, but the instance is not >> >>> >> called even if translation happens. >> >>> >> 3, Right now, I start to refer to form_structure_browsertest.cc, >> >>> >> but I >> >>> >> don't know this is right approach or not. I found the related mail >> >>> >> thread as following >> >>> > >> >>> > >> >> >> >> >> >> >> >> http://groups.google.com/a/chromium.org/group/chromium-reviews/browse_thread/... >> >>> >> >>> >> >> >>> >> I found it, but I cannot confirm it. Because my local source is a >> >>> >> little bit old. After I get it, I'll confirm it. >> >>> > >> >>> >> Do you have any other idea or any test pattern I should refer to? >> >>> > >> >>> >> Thanks, >> >>> > >> >>> >> On Tue, Nov 23, 2010 at 1:33 PM, Naoki Takano >> >>> >> <mailto:takano.naoki@gmail.com> >> >>> > >> >>> > wrote: >> >>> >> >> >>> >> > Ok, I try. >> >>> >> > >> >>> >> > Thanks!! >> >>> >> > >> >>> >> > On Tue, Nov 23, 2010 at 1:31 PM, >> >>> >> > mailto: <jhawkins@chromium.org> >> >> >> >> wrote: >> >>> >> >>> >> >> On 2010/11/23 21:27:10, http://takano.naoki_gmail.com wrote: >> >>> >> >>> >> >>> >> >>> James, >> >>> >> >> >> >>> >> >>> I mean "you're satisfied with functional test only, not writing >> >>> >> >>> unit >> >>> >> >>> test." >> >>> >> >> >> >>> >> >>> Last time when I fixed a bug in chrome project, I wrote unit >> >>> >> >>> tests, so >> >>> >> >>> I wonder if it's sufficient to write only functional tests. >> >>> >> >> >> >>> >> >> >> >>> >> >> Yes it's fine. >> >>> >> >> >> >>> >> >> http://codereview.chromium.org/5322001/ >> >>> >> >> >> >>> >> > >> >>> > >> >>> > >> >>> > >> >>> > http://codereview.chromium.org/5322001/ >> >>> > >> >> >> >> >> >> >> >> http://codereview.chromium.org/5322001/ >> > >> > > >
James, Finally I wrote test code, but still have a couple of questions and have a bug. Could you help me? At first, I made another test entry in autofill_browsertest.cc. Is this OK? The test pattern simulates following case. 1, Assume already have profiles to fill automatically. 2, Navigate Janapese page. 3, Check translation bar and simulate press Translate button. 4, Simulate translate script fetch with TestURLFetcher. 5, Kick the translation. 6, Simulate the translation with replacement of inner html. 7, Try autofill starting with pressing M key. As I already inserted into the patch, the test suspends in javascript execution. I don't know why it suspends here. Do you know any tricks? Also I understand the source code is not well polished and needs refactoring. But I uploaded this patch because I want to know if this is the right direction and I want to ask the javascript execution tricks. Also this patch build fails because the link error. Thanks, http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_browsertest.cc (right): http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_browsertest.cc:139: Is this Ok to include javascript code as text string here? Or should I separate it into test resource dir? http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_browsertest.cc:254: " <input type=\"text\" id=\"lastname\" /><br />" Is is OK to use Japanese character here? For now, I use UTF-8. http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_browsertest.cc:304: bool result; As I wrote as comment, this test stops at ExecuteJavaScriptAndExtractBool(). I dig into the source code and I found the stop location at loop->Run(NULL); So it looks like waiting for event. But the program itself is working, so if I press click the text field, it restarts and the test passes. To simulate this, I inserted document.getElementById('firstname').focus() after cr.googleTranslate.onTranslateElementLoad() but still it stops. Do you have any idea not to suspend here?
http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_browsertest.cc (right): http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_browsertest.cc:139: On 2010/12/05 01:34:34, honten wrote: > Is this Ok to include javascript code as text string here? > Or should I separate it into test resource dir? Should be fine to inline it. http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_browsertest.cc:254: " <input type=\"text\" id=\"lastname\" /><br />" On 2010/12/05 01:34:34, honten wrote: > Is is OK to use Japanese character here? > For now, I use UTF-8. UTF8 http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_browsertest.cc:304: bool result; On 2010/12/05 01:34:34, honten wrote: > As I wrote as comment, this test stops at ExecuteJavaScriptAndExtractBool(). I > dig into the source code and I found the stop location at loop->Run(NULL); So it > looks like waiting for event. But the program itself is working, so if I press > click the text field, it restarts and the test passes. > > To simulate this, I inserted document.getElementById('firstname').focus() after > cr.googleTranslate.onTranslateElementLoad() but still it stops. > > Do you have any idea not to suspend here? I don't have any ideas.
James, Thank you for your comment. I'm relieved the direction looks right. But still I'm stack at the javascript execution. > dhollowa, Do you have any idea about that? I cannot understand why the message loop pumping stops if the test calls Javascript. I suspect the focus might be gone from render view... Thanks, On Mon, Dec 6, 2010 at 11:02 AM, <jhawkins@chromium.org> wrote: > > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > File chrome/browser/autofill/autofill_browsertest.cc (right): > > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > chrome/browser/autofill/autofill_browsertest.cc:139: > On 2010/12/05 01:34:34, honten wrote: >> >> Is this Ok to include javascript code as text string here? >> Or should I separate it into test resource dir? > > Should be fine to inline it. > > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > chrome/browser/autofill/autofill_browsertest.cc:254: " <input > type=\"text\" id=\"lastname\" /><br />" > On 2010/12/05 01:34:34, honten wrote: >> >> Is is OK to use Japanese character here? >> For now, I use UTF-8. > > UTF8 > > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > chrome/browser/autofill/autofill_browsertest.cc:304: bool result; > On 2010/12/05 01:34:34, honten wrote: >> >> As I wrote as comment, this test stops at > > ExecuteJavaScriptAndExtractBool(). I >> >> dig into the source code and I found the stop location at > > loop->Run(NULL); So it >> >> looks like waiting for event. But the program itself is working, so if > > I press >> >> click the text field, it restarts and the test passes. > >> To simulate this, I inserted > > document.getElementById('firstname').focus() after >> >> cr.googleTranslate.onTranslateElementLoad() but still it stops. > >> Do you have any idea not to suspend here? > > I don't have any ideas. > > http://codereview.chromium.org/5322001/ >
I added jcampan. Sorry for bothering you, jcampan. As I wrote in this thread, when I execute javascript with ExecuteJavaScriptHelper() in my test code, it suspends. Do you have any idea about this issue? Thanks, On Mon, Dec 6, 2010 at 11:34 AM, Naoki Takano <takano.naoki@gmail.com> wrote: > James, > > Thank you for your comment. I'm relieved the direction looks right. > But still I'm stack at the javascript execution. > >> dhollowa, > Do you have any idea about that? > I cannot understand why the message loop pumping stops if the test > calls Javascript. > I suspect the focus might be gone from render view... > > Thanks, > > On Mon, Dec 6, 2010 at 11:02 AM, <jhawkins@chromium.org> wrote: >> >> http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... >> File chrome/browser/autofill/autofill_browsertest.cc (right): >> >> http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... >> chrome/browser/autofill/autofill_browsertest.cc:139: >> On 2010/12/05 01:34:34, honten wrote: >>> >>> Is this Ok to include javascript code as text string here? >>> Or should I separate it into test resource dir? >> >> Should be fine to inline it. >> >> http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... >> chrome/browser/autofill/autofill_browsertest.cc:254: " <input >> type=\"text\" id=\"lastname\" /><br />" >> On 2010/12/05 01:34:34, honten wrote: >>> >>> Is is OK to use Japanese character here? >>> For now, I use UTF-8. >> >> UTF8 >> >> http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... >> chrome/browser/autofill/autofill_browsertest.cc:304: bool result; >> On 2010/12/05 01:34:34, honten wrote: >>> >>> As I wrote as comment, this test stops at >> >> ExecuteJavaScriptAndExtractBool(). I >>> >>> dig into the source code and I found the stop location at >> >> loop->Run(NULL); So it >>> >>> looks like waiting for event. But the program itself is working, so if >> >> I press >>> >>> click the text field, it restarts and the test passes. >> >>> To simulate this, I inserted >> >> document.getElementById('firstname').focus() after >>> >>> cr.googleTranslate.onTranslateElementLoad() but still it stops. >> >>> Do you have any idea not to suspend here? >> >> I don't have any ideas. >> >> http://codereview.chromium.org/5322001/ >> >
[side note to avoid confusion, I recently changed my name, my new ldap is jcivelli, but jcampan == jcivelli] Looks like the JS function you are calling is not returning anything. That's why it is hanging. Use ExecuteJavaScript instead. Jay On Mon, Dec 6, 2010 at 11:47 AM, Naoki Takano <takano.naoki@gmail.com> wrote: > I added jcampan. > Sorry for bothering you, jcampan. > > As I wrote in this thread, when I execute javascript with > ExecuteJavaScriptHelper() in my test code, it suspends. > > Do you have any idea about this issue? > > Thanks, > > On Mon, Dec 6, 2010 at 11:34 AM, Naoki Takano <takano.naoki@gmail.com> wrote: >> James, >> >> Thank you for your comment. I'm relieved the direction looks right. >> But still I'm stack at the javascript execution. >> >>> dhollowa, >> Do you have any idea about that? >> I cannot understand why the message loop pumping stops if the test >> calls Javascript. >> I suspect the focus might be gone from render view... >> >> Thanks, >> >> On Mon, Dec 6, 2010 at 11:02 AM, <jhawkins@chromium.org> wrote: >>> >>> http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... >>> File chrome/browser/autofill/autofill_browsertest.cc (right): >>> >>> http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... >>> chrome/browser/autofill/autofill_browsertest.cc:139: >>> On 2010/12/05 01:34:34, honten wrote: >>>> >>>> Is this Ok to include javascript code as text string here? >>>> Or should I separate it into test resource dir? >>> >>> Should be fine to inline it. >>> >>> http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... >>> chrome/browser/autofill/autofill_browsertest.cc:254: " <input >>> type=\"text\" id=\"lastname\" /><br />" >>> On 2010/12/05 01:34:34, honten wrote: >>>> >>>> Is is OK to use Japanese character here? >>>> For now, I use UTF-8. >>> >>> UTF8 >>> >>> http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... >>> chrome/browser/autofill/autofill_browsertest.cc:304: bool result; >>> On 2010/12/05 01:34:34, honten wrote: >>>> >>>> As I wrote as comment, this test stops at >>> >>> ExecuteJavaScriptAndExtractBool(). I >>>> >>>> dig into the source code and I found the stop location at >>> >>> loop->Run(NULL); So it >>>> >>>> looks like waiting for event. But the program itself is working, so if >>> >>> I press >>>> >>>> click the text field, it restarts and the test passes. >>> >>>> To simulate this, I inserted >>> >>> document.getElementById('firstname').focus() after >>>> >>>> cr.googleTranslate.onTranslateElementLoad() but still it stops. >>> >>>> Do you have any idea not to suspend here? >>> >>> I don't have any ideas. >>> >>> http://codereview.chromium.org/5322001/ >>> >> >
Jay, Thank you for your reply. I'll try it tonight. Thanks a lot!! On Mon, Dec 6, 2010 at 2:03 PM, Jay Civelli <jcivelli@google.com> wrote: > [side note to avoid confusion, I recently changed my name, my new ldap > is jcivelli, but jcampan == jcivelli] > > Looks like the JS function you are calling is not returning anything. > That's why it is hanging. > Use ExecuteJavaScript instead. > > Jay > > On Mon, Dec 6, 2010 at 11:47 AM, Naoki Takano <takano.naoki@gmail.com> wrote: >> I added jcampan. >> Sorry for bothering you, jcampan. >> >> As I wrote in this thread, when I execute javascript with >> ExecuteJavaScriptHelper() in my test code, it suspends. >> >> Do you have any idea about this issue? >> >> Thanks, >> >> On Mon, Dec 6, 2010 at 11:34 AM, Naoki Takano <takano.naoki@gmail.com> wrote: >>> James, >>> >>> Thank you for your comment. I'm relieved the direction looks right. >>> But still I'm stack at the javascript execution. >>> >>>> dhollowa, >>> Do you have any idea about that? >>> I cannot understand why the message loop pumping stops if the test >>> calls Javascript. >>> I suspect the focus might be gone from render view... >>> >>> Thanks, >>> >>> On Mon, Dec 6, 2010 at 11:02 AM, <jhawkins@chromium.org> wrote: >>>> >>>> http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... >>>> File chrome/browser/autofill/autofill_browsertest.cc (right): >>>> >>>> http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... >>>> chrome/browser/autofill/autofill_browsertest.cc:139: >>>> On 2010/12/05 01:34:34, honten wrote: >>>>> >>>>> Is this Ok to include javascript code as text string here? >>>>> Or should I separate it into test resource dir? >>>> >>>> Should be fine to inline it. >>>> >>>> http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... >>>> chrome/browser/autofill/autofill_browsertest.cc:254: " <input >>>> type=\"text\" id=\"lastname\" /><br />" >>>> On 2010/12/05 01:34:34, honten wrote: >>>>> >>>>> Is is OK to use Japanese character here? >>>>> For now, I use UTF-8. >>>> >>>> UTF8 >>>> >>>> http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... >>>> chrome/browser/autofill/autofill_browsertest.cc:304: bool result; >>>> On 2010/12/05 01:34:34, honten wrote: >>>>> >>>>> As I wrote as comment, this test stops at >>>> >>>> ExecuteJavaScriptAndExtractBool(). I >>>>> >>>>> dig into the source code and I found the stop location at >>>> >>>> loop->Run(NULL); So it >>>>> >>>>> looks like waiting for event. But the program itself is working, so if >>>> >>>> I press >>>>> >>>>> click the text field, it restarts and the test passes. >>>> >>>>> To simulate this, I inserted >>>> >>>> document.getElementById('firstname').focus() after >>>>> >>>>> cr.googleTranslate.onTranslateElementLoad() but still it stops. >>>> >>>>> Do you have any idea not to suspend here? >>>> >>>> I don't have any ideas. >>>> >>>> http://codereview.chromium.org/5322001/ >>>> >>> >> >
Jay, Thanks to your advice, I resolved the javascript execution problem. But after that, still it stops at the point where simulating to press key 'M' like following. ASSERT_TRUE(ui_test_utils::SendKeyPressAndWait( browser(), app::VKEY_M, false, true, false, false, NotificationType::AUTOFILL_DID_SHOW_SUGGESTIONS, Source<RenderViewHost>(rvh()))); This part is completely the same call as the AutoFillTest.TranslateAndFormFill. In this function, it stops at RunMessageLoop(). Other interesting thing is that the test fails if I click the text field when the stop. The error says chrome/browser/autofill/autofill_browsertest.cc:66: Failure Value of: value Actual: "" Expected: expected_value Which is: "M" It looks like key press M is absorbed to somewhere... I suspect there is focus issue but I already call ClickOnView. Do you have any other idea? Sorry for bothering you so often. Thanks, On 2010/12/06 22:15:06, takano.naoki_gmail.com wrote: > Jay, > > Thank you for your reply. > > I'll try it tonight. > > Thanks a lot!! > > On Mon, Dec 6, 2010 at 2:03 PM, Jay Civelli <mailto:jcivelli@google.com> wrote: > > [side note to avoid confusion, I recently changed my name, my new ldap > > is jcivelli, but jcampan == jcivelli] > > > > Looks like the JS function you are calling is not returning anything. > > That's why it is hanging. > > Use ExecuteJavaScript instead. > > > > Jay > > > > On Mon, Dec 6, 2010 at 11:47 AM, Naoki Takano <mailto:takano.naoki@gmail.com> wrote: > >> I added jcampan. > >> Sorry for bothering you, jcampan. > >> > >> As I wrote in this thread, when I execute javascript with > >> ExecuteJavaScriptHelper() in my test code, it suspends. > >> > >> Do you have any idea about this issue? > >> > >> Thanks, > >> > >> On Mon, Dec 6, 2010 at 11:34 AM, Naoki Takano <mailto:takano.naoki@gmail.com> wrote: > >>> James, > >>> > >>> Thank you for your comment. I'm relieved the direction looks right. > >>> But still I'm stack at the javascript execution. > >>> > >>>> dhollowa, > >>> Do you have any idea about that? > >>> I cannot understand why the message loop pumping stops if the test > >>> calls Javascript. > >>> I suspect the focus might be gone from render view... > >>> > >>> Thanks, > >>> > >>> On Mon, Dec 6, 2010 at 11:02 AM, mailto: <jhawkins@chromium.org> wrote: > >>>> > >>>> > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > >>>> File chrome/browser/autofill/autofill_browsertest.cc (right): > >>>> > >>>> > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > >>>> chrome/browser/autofill/autofill_browsertest.cc:139: > >>>> On 2010/12/05 01:34:34, honten wrote: > >>>>> > >>>>> Is this Ok to include javascript code as text string here? > >>>>> Or should I separate it into test resource dir? > >>>> > >>>> Should be fine to inline it. > >>>> > >>>> > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > >>>> chrome/browser/autofill/autofill_browsertest.cc:254: " <input > >>>> type=\"text\" id=\"lastname\" /><br />" > >>>> On 2010/12/05 01:34:34, honten wrote: > >>>>> > >>>>> Is is OK to use Japanese character here? > >>>>> For now, I use UTF-8. > >>>> > >>>> UTF8 > >>>> > >>>> > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > >>>> chrome/browser/autofill/autofill_browsertest.cc:304: bool result; > >>>> On 2010/12/05 01:34:34, honten wrote: > >>>>> > >>>>> As I wrote as comment, this test stops at > >>>> > >>>> ExecuteJavaScriptAndExtractBool(). I > >>>>> > >>>>> dig into the source code and I found the stop location at > >>>> > >>>> loop->Run(NULL); So it > >>>>> > >>>>> looks like waiting for event. But the program itself is working, so if > >>>> > >>>> I press > >>>>> > >>>>> click the text field, it restarts and the test passes. > >>>> > >>>>> To simulate this, I inserted > >>>> > >>>> document.getElementById('firstname').focus() after > >>>>> > >>>>> cr.googleTranslate.onTranslateElementLoad() but still it stops. > >>>> > >>>>> Do you have any idea not to suspend here? > >>>> > >>>> I don't have any ideas. > >>>> > >>>> http://codereview.chromium.org/5322001/ > >>>> > >>> > >> > >
Jay, I guess focus() javascript call looks fail before the 'M' key press simulation. ui_test_utils::ExecuteJavaScriptAndExtractBool returns true, but actually focus() points to wrong element. As you know, this test simulates translation with replacing the html text. And the focus points to the original html file and does not point to the new translated html text. Do we have any good way to wait till the whole javascript is done completely? Or is there any way to wait for a while like 1sec? Thanks, On 2010/12/07 05:29:43, honten wrote: > Jay, > > Thanks to your advice, I resolved the javascript execution problem. But after > that, still it stops at the point where simulating to press key 'M' like > following. > > ASSERT_TRUE(ui_test_utils::SendKeyPressAndWait( > browser(), app::VKEY_M, false, true, false, false, > NotificationType::AUTOFILL_DID_SHOW_SUGGESTIONS, > Source<RenderViewHost>(rvh()))); > > This part is completely the same call as the AutoFillTest.TranslateAndFormFill. > In this function, it stops at RunMessageLoop(). > > Other interesting thing is that the test fails if I click the text field when > the stop. The error says > chrome/browser/autofill/autofill_browsertest.cc:66: Failure > Value of: value > Actual: "" > Expected: expected_value > Which is: "M" > > It looks like key press M is absorbed to somewhere... I suspect there is focus > issue but I already call ClickOnView. > > Do you have any other idea? > > Sorry for bothering you so often. > > Thanks, > On 2010/12/06 22:15:06, http://takano.naoki_gmail.com wrote: > > Jay, > > > > Thank you for your reply. > > > > I'll try it tonight. > > > > Thanks a lot!! > > > > On Mon, Dec 6, 2010 at 2:03 PM, Jay Civelli <mailto:jcivelli@google.com> > wrote: > > > [side note to avoid confusion, I recently changed my name, my new ldap > > > is jcivelli, but jcampan == jcivelli] > > > > > > Looks like the JS function you are calling is not returning anything. > > > That's why it is hanging. > > > Use ExecuteJavaScript instead. > > > > > > Jay > > > > > > On Mon, Dec 6, 2010 at 11:47 AM, Naoki Takano > <mailto:takano.naoki@gmail.com> wrote: > > >> I added jcampan. > > >> Sorry for bothering you, jcampan. > > >> > > >> As I wrote in this thread, when I execute javascript with > > >> ExecuteJavaScriptHelper() in my test code, it suspends. > > >> > > >> Do you have any idea about this issue? > > >> > > >> Thanks, > > >> > > >> On Mon, Dec 6, 2010 at 11:34 AM, Naoki Takano > <mailto:takano.naoki@gmail.com> wrote: > > >>> James, > > >>> > > >>> Thank you for your comment. I'm relieved the direction looks right. > > >>> But still I'm stack at the javascript execution. > > >>> > > >>>> dhollowa, > > >>> Do you have any idea about that? > > >>> I cannot understand why the message loop pumping stops if the test > > >>> calls Javascript. > > >>> I suspect the focus might be gone from render view... > > >>> > > >>> Thanks, > > >>> > > >>> On Mon, Dec 6, 2010 at 11:02 AM, mailto: <jhawkins@chromium.org> wrote: > > >>>> > > >>>> > > > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > > >>>> File chrome/browser/autofill/autofill_browsertest.cc (right): > > >>>> > > >>>> > > > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > > >>>> chrome/browser/autofill/autofill_browsertest.cc:139: > > >>>> On 2010/12/05 01:34:34, honten wrote: > > >>>>> > > >>>>> Is this Ok to include javascript code as text string here? > > >>>>> Or should I separate it into test resource dir? > > >>>> > > >>>> Should be fine to inline it. > > >>>> > > >>>> > > > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > > >>>> chrome/browser/autofill/autofill_browsertest.cc:254: " <input > > >>>> type=\"text\" id=\"lastname\" /><br />" > > >>>> On 2010/12/05 01:34:34, honten wrote: > > >>>>> > > >>>>> Is is OK to use Japanese character here? > > >>>>> For now, I use UTF-8. > > >>>> > > >>>> UTF8 > > >>>> > > >>>> > > > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > > >>>> chrome/browser/autofill/autofill_browsertest.cc:304: bool result; > > >>>> On 2010/12/05 01:34:34, honten wrote: > > >>>>> > > >>>>> As I wrote as comment, this test stops at > > >>>> > > >>>> ExecuteJavaScriptAndExtractBool(). I > > >>>>> > > >>>>> dig into the source code and I found the stop location at > > >>>> > > >>>> loop->Run(NULL); So it > > >>>>> > > >>>>> looks like waiting for event. But the program itself is working, so if > > >>>> > > >>>> I press > > >>>>> > > >>>>> click the text field, it restarts and the test passes. > > >>>> > > >>>>> To simulate this, I inserted > > >>>> > > >>>> document.getElementById('firstname').focus() after > > >>>>> > > >>>>> cr.googleTranslate.onTranslateElementLoad() but still it stops. > > >>>> > > >>>>> Do you have any idea not to suspend here? > > >>>> > > >>>> I don't have any ideas. > > >>>> > > >>>> http://codereview.chromium.org/5322001/ > > >>>> > > >>> > > >> > > >
Jay,
Sorry for bothering you again, but finally I figure it out!!
The problem is here
rvh()->OnMessageReceived(ViewHostMsg_PageTranslated(0, 0, "ja", "en",
TranslateErrors::NONE));
Instead of this, I should use
ui_test_utils::WaitForNotification(NotificationType::PAGE_TRANSLATED);
Today is time up, so I will upload it again after revise it.
Thanks!!
On 2010/12/07 07:05:15, honten wrote:
> Jay,
>
> I guess focus() javascript call looks fail before the 'M' key press
simulation.
> ui_test_utils::ExecuteJavaScriptAndExtractBool returns true, but actually
> focus() points to wrong element.
>
> As you know, this test simulates translation with replacing the html text. And
> the focus points to the original html file and does not point to the new
> translated html text.
>
> Do we have any good way to wait till the whole javascript is done completely?
Or
> is there any way to wait for a while like 1sec?
>
> Thanks,
>
> On 2010/12/07 05:29:43, honten wrote:
> > Jay,
> >
> > Thanks to your advice, I resolved the javascript execution problem. But
after
> > that, still it stops at the point where simulating to press key 'M' like
> > following.
> >
> > ASSERT_TRUE(ui_test_utils::SendKeyPressAndWait(
> > browser(), app::VKEY_M, false, true, false, false,
> > NotificationType::AUTOFILL_DID_SHOW_SUGGESTIONS,
> > Source<RenderViewHost>(rvh())));
> >
> > This part is completely the same call as the
> AutoFillTest.TranslateAndFormFill.
> > In this function, it stops at RunMessageLoop().
> >
> > Other interesting thing is that the test fails if I click the text field
when
> > the stop. The error says
> > chrome/browser/autofill/autofill_browsertest.cc:66: Failure
> > Value of: value
> > Actual: ""
> > Expected: expected_value
> > Which is: "M"
> >
> > It looks like key press M is absorbed to somewhere... I suspect there is
> focus
> > issue but I already call ClickOnView.
> >
> > Do you have any other idea?
> >
> > Sorry for bothering you so often.
> >
> > Thanks,
> > On 2010/12/06 22:15:06, http://takano.naoki_gmail.com wrote:
> > > Jay,
> > >
> > > Thank you for your reply.
> > >
> > > I'll try it tonight.
> > >
> > > Thanks a lot!!
> > >
> > > On Mon, Dec 6, 2010 at 2:03 PM, Jay Civelli <mailto:jcivelli@google.com>
> > wrote:
> > > > [side note to avoid confusion, I recently changed my name, my new ldap
> > > > is jcivelli, but jcampan == jcivelli]
> > > >
> > > > Looks like the JS function you are calling is not returning anything.
> > > > That's why it is hanging.
> > > > Use ExecuteJavaScript instead.
> > > >
> > > > Jay
> > > >
> > > > On Mon, Dec 6, 2010 at 11:47 AM, Naoki Takano
> > <mailto:takano.naoki@gmail.com> wrote:
> > > >> I added jcampan.
> > > >> Sorry for bothering you, jcampan.
> > > >>
> > > >> As I wrote in this thread, when I execute javascript with
> > > >> ExecuteJavaScriptHelper() in my test code, it suspends.
> > > >>
> > > >> Do you have any idea about this issue?
> > > >>
> > > >> Thanks,
> > > >>
> > > >> On Mon, Dec 6, 2010 at 11:34 AM, Naoki Takano
> > <mailto:takano.naoki@gmail.com> wrote:
> > > >>> James,
> > > >>>
> > > >>> Thank you for your comment. I'm relieved the direction looks right.
> > > >>> But still I'm stack at the javascript execution.
> > > >>>
> > > >>>> dhollowa,
> > > >>> Do you have any idea about that?
> > > >>> I cannot understand why the message loop pumping stops if the test
> > > >>> calls Javascript.
> > > >>> I suspect the focus might be gone from render view...
> > > >>>
> > > >>> Thanks,
> > > >>>
> > > >>> On Mon, Dec 6, 2010 at 11:02 AM, mailto: <jhawkins@chromium.org>
> wrote:
> > > >>>>
> > > >>>>
> > >
> >
>
http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut...
> > > >>>> File chrome/browser/autofill/autofill_browsertest.cc (right):
> > > >>>>
> > > >>>>
> > >
> >
>
http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut...
> > > >>>> chrome/browser/autofill/autofill_browsertest.cc:139:
> > > >>>> On 2010/12/05 01:34:34, honten wrote:
> > > >>>>>
> > > >>>>> Is this Ok to include javascript code as text string here?
> > > >>>>> Or should I separate it into test resource dir?
> > > >>>>
> > > >>>> Should be fine to inline it.
> > > >>>>
> > > >>>>
> > >
> >
>
http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut...
> > > >>>> chrome/browser/autofill/autofill_browsertest.cc:254: " <input
> > > >>>> type=\"text\" id=\"lastname\" /><br />"
> > > >>>> On 2010/12/05 01:34:34, honten wrote:
> > > >>>>>
> > > >>>>> Is is OK to use Japanese character here?
> > > >>>>> For now, I use UTF-8.
> > > >>>>
> > > >>>> UTF8
> > > >>>>
> > > >>>>
> > >
> >
>
http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut...
> > > >>>> chrome/browser/autofill/autofill_browsertest.cc:304: bool result;
> > > >>>> On 2010/12/05 01:34:34, honten wrote:
> > > >>>>>
> > > >>>>> As I wrote as comment, this test stops at
> > > >>>>
> > > >>>> ExecuteJavaScriptAndExtractBool(). I
> > > >>>>>
> > > >>>>> dig into the source code and I found the stop location at
> > > >>>>
> > > >>>> loop->Run(NULL); So it
> > > >>>>>
> > > >>>>> looks like waiting for event. But the program itself is working, so
if
> > > >>>>
> > > >>>> I press
> > > >>>>>
> > > >>>>> click the text field, it restarts and the test passes.
> > > >>>>
> > > >>>>> To simulate this, I inserted
> > > >>>>
> > > >>>> document.getElementById('firstname').focus() after
> > > >>>>>
> > > >>>>> cr.googleTranslate.onTranslateElementLoad() but still it stops.
> > > >>>>
> > > >>>>> Do you have any idea not to suspend here?
> > > >>>>
> > > >>>> I don't have any ideas.
> > > >>>>
> > > >>>> http://codereview.chromium.org/5322001/
> > > >>>>
> > > >>>
> > > >>
> > > >
James, dhollowa, jcivelli Could you review my change? Thanks, On 2010/12/07 07:22:59, honten wrote: > Jay, > > Sorry for bothering you again, but finally I figure it out!! > > The problem is here > rvh()->OnMessageReceived(ViewHostMsg_PageTranslated(0, 0, "ja", "en", > TranslateErrors::NONE)); > > Instead of this, I should use > ui_test_utils::WaitForNotification(NotificationType::PAGE_TRANSLATED); > > Today is time up, so I will upload it again after revise it. > > Thanks!! > > On 2010/12/07 07:05:15, honten wrote: > > Jay, > > > > I guess focus() javascript call looks fail before the 'M' key press > simulation. > > ui_test_utils::ExecuteJavaScriptAndExtractBool returns true, but actually > > focus() points to wrong element. > > > > As you know, this test simulates translation with replacing the html text. And > > the focus points to the original html file and does not point to the new > > translated html text. > > > > Do we have any good way to wait till the whole javascript is done completely? > Or > > is there any way to wait for a while like 1sec? > > > > Thanks, > > > > On 2010/12/07 05:29:43, honten wrote: > > > Jay, > > > > > > Thanks to your advice, I resolved the javascript execution problem. But > after > > > that, still it stops at the point where simulating to press key 'M' like > > > following. > > > > > > ASSERT_TRUE(ui_test_utils::SendKeyPressAndWait( > > > browser(), app::VKEY_M, false, true, false, false, > > > NotificationType::AUTOFILL_DID_SHOW_SUGGESTIONS, > > > Source<RenderViewHost>(rvh()))); > > > > > > This part is completely the same call as the > > AutoFillTest.TranslateAndFormFill. > > > In this function, it stops at RunMessageLoop(). > > > > > > Other interesting thing is that the test fails if I click the text field > when > > > the stop. The error says > > > chrome/browser/autofill/autofill_browsertest.cc:66: Failure > > > Value of: value > > > Actual: "" > > > Expected: expected_value > > > Which is: "M" > > > > > > It looks like key press M is absorbed to somewhere... I suspect there is > > focus > > > issue but I already call ClickOnView. > > > > > > Do you have any other idea? > > > > > > Sorry for bothering you so often. > > > > > > Thanks, > > > On 2010/12/06 22:15:06, http://takano.naoki_gmail.com wrote: > > > > Jay, > > > > > > > > Thank you for your reply. > > > > > > > > I'll try it tonight. > > > > > > > > Thanks a lot!! > > > > > > > > On Mon, Dec 6, 2010 at 2:03 PM, Jay Civelli <mailto:jcivelli@google.com> > > > wrote: > > > > > [side note to avoid confusion, I recently changed my name, my new ldap > > > > > is jcivelli, but jcampan == jcivelli] > > > > > > > > > > Looks like the JS function you are calling is not returning anything. > > > > > That's why it is hanging. > > > > > Use ExecuteJavaScript instead. > > > > > > > > > > Jay > > > > > > > > > > On Mon, Dec 6, 2010 at 11:47 AM, Naoki Takano > > > <mailto:takano.naoki@gmail.com> wrote: > > > > >> I added jcampan. > > > > >> Sorry for bothering you, jcampan. > > > > >> > > > > >> As I wrote in this thread, when I execute javascript with > > > > >> ExecuteJavaScriptHelper() in my test code, it suspends. > > > > >> > > > > >> Do you have any idea about this issue? > > > > >> > > > > >> Thanks, > > > > >> > > > > >> On Mon, Dec 6, 2010 at 11:34 AM, Naoki Takano > > > <mailto:takano.naoki@gmail.com> wrote: > > > > >>> James, > > > > >>> > > > > >>> Thank you for your comment. I'm relieved the direction looks right. > > > > >>> But still I'm stack at the javascript execution. > > > > >>> > > > > >>>> dhollowa, > > > > >>> Do you have any idea about that? > > > > >>> I cannot understand why the message loop pumping stops if the test > > > > >>> calls Javascript. > > > > >>> I suspect the focus might be gone from render view... > > > > >>> > > > > >>> Thanks, > > > > >>> > > > > >>> On Mon, Dec 6, 2010 at 11:02 AM, mailto: <jhawkins@chromium.org> > > wrote: > > > > >>>> > > > > >>>> > > > > > > > > > > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > > > > >>>> File chrome/browser/autofill/autofill_browsertest.cc (right): > > > > >>>> > > > > >>>> > > > > > > > > > > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > > > > >>>> chrome/browser/autofill/autofill_browsertest.cc:139: > > > > >>>> On 2010/12/05 01:34:34, honten wrote: > > > > >>>>> > > > > >>>>> Is this Ok to include javascript code as text string here? > > > > >>>>> Or should I separate it into test resource dir? > > > > >>>> > > > > >>>> Should be fine to inline it. > > > > >>>> > > > > >>>> > > > > > > > > > > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > > > > >>>> chrome/browser/autofill/autofill_browsertest.cc:254: " <input > > > > >>>> type=\"text\" id=\"lastname\" /><br />" > > > > >>>> On 2010/12/05 01:34:34, honten wrote: > > > > >>>>> > > > > >>>>> Is is OK to use Japanese character here? > > > > >>>>> For now, I use UTF-8. > > > > >>>> > > > > >>>> UTF8 > > > > >>>> > > > > >>>> > > > > > > > > > > http://codereview.chromium.org/5322001/diff/29001/chrome/browser/autofill/aut... > > > > >>>> chrome/browser/autofill/autofill_browsertest.cc:304: bool result; > > > > >>>> On 2010/12/05 01:34:34, honten wrote: > > > > >>>>> > > > > >>>>> As I wrote as comment, this test stops at > > > > >>>> > > > > >>>> ExecuteJavaScriptAndExtractBool(). I > > > > >>>>> > > > > >>>>> dig into the source code and I found the stop location at > > > > >>>> > > > > >>>> loop->Run(NULL); So it > > > > >>>>> > > > > >>>>> looks like waiting for event. But the program itself is working, so > if > > > > >>>> > > > > >>>> I press > > > > >>>>> > > > > >>>>> click the text field, it restarts and the test passes. > > > > >>>> > > > > >>>>> To simulate this, I inserted > > > > >>>> > > > > >>>> document.getElementById('firstname').focus() after > > > > >>>>> > > > > >>>>> cr.googleTranslate.onTranslateElementLoad() but still it stops. > > > > >>>> > > > > >>>>> Do you have any idea not to suspend here? > > > > >>>> > > > > >>>> I don't have any ideas. > > > > >>>> > > > > >>>> http://codereview.chromium.org/5322001/ > > > > >>>> > > > > >>> > > > > >> > > > > >
http://codereview.chromium.org/5322001/diff/45001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_browsertest.cc (right): http://codereview.chromium.org/5322001/diff/45001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_browsertest.cc:24: #include "chrome/browser/renderer_host/render_view_host.h" This needs to be moved up. http://codereview.chromium.org/5322001/diff/45001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_browsertest.cc:33: static const std::string kTestFormString = Static globals must not be of class-type. POD (plain old data) only. So this should be converted to const char* const KTestFormString = ... http://codereview.chromium.org/5322001/diff/45001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_browsertest.cc:66: TestURLFetcherFactory url_fetcher_factory_; Data members should come last in declaration order. http://codereview.chromium.org/5322001/diff/45001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_browsertest.cc:284: nit: empty newline is not needed. http://codereview.chromium.org/5322001/diff/45001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/5322001/diff/45001/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:317: 'common/net/test_url_fetcher_factory.cc', These files should be added to the review, no?
dhollowa, Thank you for your response. But I have a question. Could you tell me? Thanks, http://codereview.chromium.org/5322001/diff/45001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/5322001/diff/45001/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:317: 'common/net/test_url_fetcher_factory.cc', Sorry, I cannot understand what you are saying. Do you mean I have to get any other permission to add these files here? The reason why I have to add these files is interactive_ui_tests exe binary fails because of link error. Or any other reasons? On 2010/12/08 17:33:41, dhollowa wrote: > These files should be added to the review, no?
http://codereview.chromium.org/5322001/diff/45001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/5322001/diff/45001/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:317: 'common/net/test_url_fetcher_factory.cc', Got it, ok. No problem then. On 2010/12/08 17:58:20, honten wrote: > Sorry, I cannot understand what you are saying. > > Do you mean I have to get any other permission to add these files here? > > The reason why I have to add these files is interactive_ui_tests exe binary > fails because of link error. > > Or any other reasons? > > On 2010/12/08 17:33:41, dhollowa wrote: > > These files should be added to the review, no? >
dhollowa, Could you review again? Thanks, On 2010/12/08 18:21:33, dhollowa wrote: > http://codereview.chromium.org/5322001/diff/45001/chrome/chrome_tests.gypi > File chrome/chrome_tests.gypi (right): > > http://codereview.chromium.org/5322001/diff/45001/chrome/chrome_tests.gypi#ne... > chrome/chrome_tests.gypi:317: 'common/net/test_url_fetcher_factory.cc', > > Got it, ok. No problem then. > > On 2010/12/08 17:58:20, honten wrote: > > Sorry, I cannot understand what you are saying. > > > > Do you mean I have to get any other permission to add these files here? > > > > The reason why I have to add these files is interactive_ui_tests exe binary > > fails because of link error. > > > > Or any other reasons? > > > > On 2010/12/08 17:33:41, dhollowa wrote: > > > These files should be added to the review, no? > >
LGTM http://codereview.chromium.org/5322001/diff/54001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/5322001/diff/54001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.cc:2019: // current form and re-parse it in AutoFillManter first to get the new AutoFillManager
Thank you for finding typo;-) On Thu, Dec 9, 2010 at 10:43 AM, <jhawkins@chromium.org> wrote: > LGTM > > > http://codereview.chromium.org/5322001/diff/54001/chrome/browser/renderer_hos... > File chrome/browser/renderer_host/render_view_host.cc (right): > > http://codereview.chromium.org/5322001/diff/54001/chrome/browser/renderer_hos... > chrome/browser/renderer_host/render_view_host.cc:2019: // current form > and re-parse it in AutoFillManter first to get the new > AutoFillManager > > http://codereview.chromium.org/5322001/ >
I fix it, please review it. On 2010/12/09 18:48:50, takano.naoki_gmail.com wrote: > Thank you for finding typo;-) > > On Thu, Dec 9, 2010 at 10:43 AM, <mailto:jhawkins@chromium.org> wrote: > > LGTM > > > > > > > http://codereview.chromium.org/5322001/diff/54001/chrome/browser/renderer_hos... > > File chrome/browser/renderer_host/render_view_host.cc (right): > > > > > http://codereview.chromium.org/5322001/diff/54001/chrome/browser/renderer_hos... > > chrome/browser/renderer_host/render_view_host.cc:2019: // current form > > and re-parse it in AutoFillManter first to get the new > > AutoFillManager > > > > http://codereview.chromium.org/5322001/ > >
http://codereview.chromium.org/5322001/diff/60001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_browsertest.cc (right): http://codereview.chromium.org/5322001/diff/60001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_browsertest.cc:24: #include "chrome/browser/renderer_host/render_view_host.h" It looks like these previous fixes got reverted somehow. Yes? I.e. Patch Set 7 seems to undo the good work in Patch Set 6.
I'll fix it tonight. Sorry for bothering you... http://codereview.chromium.org/5322001/diff/60001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_browsertest.cc (right): http://codereview.chromium.org/5322001/diff/60001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_browsertest.cc:24: #include "chrome/browser/renderer_host/render_view_host.h" Yes, Accidentally this file is roll backed. I started using git, and that might cause the problem when I migrated from svn to git. On 2010/12/10 18:56:50, dhollowa wrote: > It looks like these previous fixes got reverted somehow. Yes? I.e. Patch Set 7 > seems to undo the good work in Patch Set 6.
Please review it. Thanks, On 2010/12/10 19:40:02, honten wrote: > I'll fix it tonight. > > Sorry for bothering you... > > http://codereview.chromium.org/5322001/diff/60001/chrome/browser/autofill/aut... > File chrome/browser/autofill/autofill_browsertest.cc (right): > > http://codereview.chromium.org/5322001/diff/60001/chrome/browser/autofill/aut... > chrome/browser/autofill/autofill_browsertest.cc:24: #include > "chrome/browser/renderer_host/render_view_host.h" > Yes, > > Accidentally this file is roll backed. I started using git, and that might cause > the problem when I migrated from svn to git. > > On 2010/12/10 18:56:50, dhollowa wrote: > > It looks like these previous fixes got reverted somehow. Yes? I.e. Patch Set > 7 > > seems to undo the good work in Patch Set 6.
LGTM, thanks!
dhollowa, Could you commit it? On Mon, Dec 13, 2010 at 12:45 PM, <dhollowa@chromium.org> wrote: > LGTM, thanks! > > http://codereview.chromium.org/5322001/ >
Sure. Did you run it through the try bots? On 2010/12/13 21:07:03, takano.naoki_gmail.com wrote: > dhollowa, > > Could you commit it? > > On Mon, Dec 13, 2010 at 12:45 PM, <mailto:dhollowa@chromium.org> wrote: > > LGTM, thanks! > > > > http://codereview.chromium.org/5322001/ > >
No, As you know, I don't have commit permission, so I suppose I cannot try the bots. Could you run it? On Mon, Dec 13, 2010 at 1:51 PM, <dhollowa@chromium.org> wrote: > > Sure. Did you run it through the try bots? > > On 2010/12/13 21:07:03, takano.naoki_gmail.com wrote: >> >> dhollowa, > >> Could you commit it? > >> On Mon, Dec 13, 2010 at 12:45 PM, <mailto:dhollowa@chromium.org> wrote: >> > LGTM, thanks! >> > >> > http://codereview.chromium.org/5322001/ >> > > > > > http://codereview.chromium.org/5322001/ >
Ok, I can do it. On 2010/12/13 22:05:44, takano.naoki_gmail.com wrote: > No, > > As you know, I don't have commit permission, so I suppose I cannot try the bots. > > Could you run it? > > On Mon, Dec 13, 2010 at 1:51 PM, <mailto:dhollowa@chromium.org> wrote: > > > > Sure. Did you run it through the try bots? > > > > On 2010/12/13 21:07:03, http://takano.naoki_gmail.com wrote: > >> > >> dhollowa, > > > >> Could you commit it? > > > >> On Mon, Dec 13, 2010 at 12:45 PM, <mailto:dhollowa@chromium.org> wrote: > >> > LGTM, thanks! > >> > > >> > http://codereview.chromium.org/5322001/ > >> > > > > > > > > > http://codereview.chromium.org/5322001/ > >
Passed try bots. Committing.
On 2010/12/14 00:55:06, dhollowa wrote: > Passed try bots. Committing. Had to revert due to ChromeOS issue: chrome/browser/chromeos/login/account_creation_view.cc: In member function 'virtual TabContents* chromeos::AccountCreationDomView::CreateTabContents(Profile*, SiteInstance*)': chrome/browser/chromeos/login/account_creation_view.cc:99:error: cannot allocate an object of abstract type 'chromeos::AccountCreationTabContents' chrome/browser/chromeos/login/account_creation_view.cc:19: note: because the following virtual functions are pure within 'chromeos::AccountCreationTabContents': ./chrome/browser/renderer_host/render_view_host_delegate.h:593: note: virtual void RenderViewHostDelegate::AutoFill::Reset() make: *** [out/Release/obj.target/browser/chrome/browser/chromeos/login/account_creation_view.o] Error 1
Ok... How should I treat this? Can I make Chrome OS on Linux env? On 2010/12/14 01:31:21, dhollowa wrote: > On 2010/12/14 00:55:06, dhollowa wrote: > > Passed try bots. Committing. > > Had to revert due to ChromeOS issue: > > > chrome/browser/chromeos/login/account_creation_view.cc: In member function > 'virtual TabContents* > chromeos::AccountCreationDomView::CreateTabContents(Profile*, SiteInstance*)': > chrome/browser/chromeos/login/account_creation_view.cc:99:error: cannot allocate > an object of abstract type 'chromeos::AccountCreationTabContents' > chrome/browser/chromeos/login/account_creation_view.cc:19: note: because the > following virtual functions are pure within > 'chromeos::AccountCreationTabContents': > ./chrome/browser/renderer_host/render_view_host_delegate.h:593: note: virtual > void RenderViewHostDelegate::AutoFill::Reset() > make: *** > [out/Release/obj.target/browser/chrome/browser/chromeos/login/account_creation_view.o] > Error 1
Your best bet is to make the fix and then use the try bots to test. On 2010/12/14 01:33:26, honten wrote: > Ok... > > How should I treat this? > > Can I make Chrome OS on Linux env? > > On 2010/12/14 01:31:21, dhollowa wrote: > > On 2010/12/14 00:55:06, dhollowa wrote: > > > Passed try bots. Committing. > > > > Had to revert due to ChromeOS issue: > > > > > > chrome/browser/chromeos/login/account_creation_view.cc: In member function > > 'virtual TabContents* > > chromeos::AccountCreationDomView::CreateTabContents(Profile*, SiteInstance*)': > > chrome/browser/chromeos/login/account_creation_view.cc:99:error: cannot > allocate > > an object of abstract type 'chromeos::AccountCreationTabContents' > > chrome/browser/chromeos/login/account_creation_view.cc:19: note: because the > > following virtual functions are pure within > > 'chromeos::AccountCreationTabContents': > > ./chrome/browser/renderer_host/render_view_host_delegate.h:593: note: virtual > > void RenderViewHostDelegate::AutoFill::Reset() > > make: *** > > > [out/Release/obj.target/browser/chrome/browser/chromeos/login/account_creation_view.o] > > Error 1
I see, But as you know, I cannot run try bots. Could you try it again once I send my patch to you? Thanks, On Mon, Dec 13, 2010 at 5:45 PM, <dhollowa@chromium.org> wrote: > > Your best bet is to make the fix and then use the try bots to test. > > On 2010/12/14 01:33:26, honten wrote: >> >> Ok... > >> How should I treat this? > >> Can I make Chrome OS on Linux env? > >> On 2010/12/14 01:31:21, dhollowa wrote: >> > On 2010/12/14 00:55:06, dhollowa wrote: >> > > Passed try bots. Committing. >> > >> > Had to revert due to ChromeOS issue: >> > >> > >> > chrome/browser/chromeos/login/account_creation_view.cc: In member >> > function >> > 'virtual TabContents* >> > chromeos::AccountCreationDomView::CreateTabContents(Profile*, > > SiteInstance*)': >> >> > chrome/browser/chromeos/login/account_creation_view.cc:99:error: cannot >> allocate >> > an object of abstract type 'chromeos::AccountCreationTabContents' >> > chrome/browser/chromeos/login/account_creation_view.cc:19: note: >> > because > > the >> >> > following virtual functions are pure within >> > 'chromeos::AccountCreationTabContents': >> > ./chrome/browser/renderer_host/render_view_host_delegate.h:593: note: > > virtual >> >> > void RenderViewHostDelegate::AutoFill::Reset() >> > make: *** >> > > > [out/Release/obj.target/browser/chrome/browser/chromeos/login/account_creation_view.o] >> >> > Error 1 > > > > http://codereview.chromium.org/5322001/ >
Yes, that's fine. On 2010/12/14 01:48:53, takano.naoki_gmail.com wrote: > I see, > > But as you know, I cannot run try bots. > > Could you try it again once I send my patch to you? > > Thanks, > > On Mon, Dec 13, 2010 at 5:45 PM, <mailto:dhollowa@chromium.org> wrote: > > > > Your best bet is to make the fix and then use the try bots to test. > > > > On 2010/12/14 01:33:26, honten wrote: > >> > >> Ok... > > > >> How should I treat this? > > > >> Can I make Chrome OS on Linux env? > > > >> On 2010/12/14 01:31:21, dhollowa wrote: > >> > On 2010/12/14 00:55:06, dhollowa wrote: > >> > > Passed try bots. Committing. > >> > > >> > Had to revert due to ChromeOS issue: > >> > > >> > > >> > chrome/browser/chromeos/login/account_creation_view.cc: In member > >> > function > >> > 'virtual TabContents* > >> > chromeos::AccountCreationDomView::CreateTabContents(Profile*, > > > > SiteInstance*)': > >> > >> > chrome/browser/chromeos/login/account_creation_view.cc:99:error: cannot > >> allocate > >> > an object of abstract type 'chromeos::AccountCreationTabContents' > >> > chrome/browser/chromeos/login/account_creation_view.cc:19: note: > >> > because > > > > the > >> > >> > following virtual functions are pure within > >> > 'chromeos::AccountCreationTabContents': > >> > ./chrome/browser/renderer_host/render_view_host_delegate.h:593: note: > > > > virtual > >> > >> > void RenderViewHostDelegate::AutoFill::Reset() > >> > make: *** > >> > > > > > > [out/Release/obj.target/browser/chrome/browser/chromeos/login/account_creation_view.o] > >> > >> > Error 1 > > > > > > > > http://codereview.chromium.org/5322001/ > >
Now, I'm looking into AccountCreationTabContents.cpp and I guess I just add empty method implementation... Anyway I'll do it tonight. Thanks, On Mon, Dec 13, 2010 at 5:52 PM, <dhollowa@chromium.org> wrote: > > Yes, that's fine. > > On 2010/12/14 01:48:53, takano.naoki_gmail.com wrote: >> >> I see, > >> But as you know, I cannot run try bots. > >> Could you try it again once I send my patch to you? > >> Thanks, > >> On Mon, Dec 13, 2010 at 5:45 PM, <mailto:dhollowa@chromium.org> wrote: >> > >> > Your best bet is to make the fix and then use the try bots to test. >> > >> > On 2010/12/14 01:33:26, honten wrote: >> >> >> >> Ok... >> > >> >> How should I treat this? >> > >> >> Can I make Chrome OS on Linux env? >> > >> >> On 2010/12/14 01:31:21, dhollowa wrote: >> >> > On 2010/12/14 00:55:06, dhollowa wrote: >> >> > > Passed try bots. Committing. >> >> > >> >> > Had to revert due to ChromeOS issue: >> >> > >> >> > >> >> > chrome/browser/chromeos/login/account_creation_view.cc: In member >> >> > function >> >> > 'virtual TabContents* >> >> > chromeos::AccountCreationDomView::CreateTabContents(Profile*, >> > >> > SiteInstance*)': >> >> >> >> > chrome/browser/chromeos/login/account_creation_view.cc:99:error: >> >> > cannot >> >> allocate >> >> > an object of abstract type 'chromeos::AccountCreationTabContents' >> >> > chrome/browser/chromeos/login/account_creation_view.cc:19: note: >> >> > because >> > >> > the >> >> >> >> > following virtual functions are pure within >> >> > 'chromeos::AccountCreationTabContents': >> >> > ./chrome/browser/renderer_host/render_view_host_delegate.h:593: note: >> > >> > virtual >> >> >> >> > void RenderViewHostDelegate::AutoFill::Reset() >> >> > make: *** >> >> > >> > >> > > > [out/Release/obj.target/browser/chrome/browser/chromeos/login/account_creation_view.o] >> >> >> >> >> > Error 1 >> > >> > >> > >> > http://codereview.chromium.org/5322001/ >> > > > > > http://codereview.chromium.org/5322001/ >
Uploaded. Could you review it? Thanks, On 2010/12/14 01:56:35, takano.naoki_gmail.com wrote: > Now, I'm looking into AccountCreationTabContents.cpp and I guess I > just add empty method implementation... > > Anyway I'll do it tonight. > > Thanks, > > On Mon, Dec 13, 2010 at 5:52 PM, <mailto:dhollowa@chromium.org> wrote: > > > > Yes, that's fine. > > > > On 2010/12/14 01:48:53, http://takano.naoki_gmail.com wrote: > >> > >> I see, > > > >> But as you know, I cannot run try bots. > > > >> Could you try it again once I send my patch to you? > > > >> Thanks, > > > >> On Mon, Dec 13, 2010 at 5:45 PM, <mailto:dhollowa@chromium.org> wrote: > >> > > >> > Your best bet is to make the fix and then use the try bots to test. > >> > > >> > On 2010/12/14 01:33:26, honten wrote: > >> >> > >> >> Ok... > >> > > >> >> How should I treat this? > >> > > >> >> Can I make Chrome OS on Linux env? > >> > > >> >> On 2010/12/14 01:31:21, dhollowa wrote: > >> >> > On 2010/12/14 00:55:06, dhollowa wrote: > >> >> > > Passed try bots. Committing. > >> >> > > >> >> > Had to revert due to ChromeOS issue: > >> >> > > >> >> > > >> >> > chrome/browser/chromeos/login/account_creation_view.cc: In member > >> >> > function > >> >> > 'virtual TabContents* > >> >> > chromeos::AccountCreationDomView::CreateTabContents(Profile*, > >> > > >> > SiteInstance*)': > >> >> > >> >> > chrome/browser/chromeos/login/account_creation_view.cc:99:error: > >> >> > cannot > >> >> allocate > >> >> > an object of abstract type 'chromeos::AccountCreationTabContents' > >> >> > chrome/browser/chromeos/login/account_creation_view.cc:19: note: > >> >> > because > >> > > >> > the > >> >> > >> >> > following virtual functions are pure within > >> >> > 'chromeos::AccountCreationTabContents': > >> >> > ./chrome/browser/renderer_host/render_view_host_delegate.h:593: note: > >> > > >> > virtual > >> >> > >> >> > void RenderViewHostDelegate::AutoFill::Reset() > >> >> > make: *** > >> >> > > >> > > >> > > > > > > [out/Release/obj.target/browser/chrome/browser/chromeos/login/account_creation_view.o] > >> > >> >> > >> >> > Error 1 > >> > > >> > > >> > > >> > http://codereview.chromium.org/5322001/ > >> > > > > > > > > > http://codereview.chromium.org/5322001/ > >
Just in case, I want to make sure you committed. Did you commit my change? Thanks, On 2010/12/14 03:36:17, honten wrote: > Uploaded. > > Could you review it? > > Thanks, > > On 2010/12/14 01:56:35, http://takano.naoki_gmail.com wrote: > > Now, I'm looking into AccountCreationTabContents.cpp and I guess I > > just add empty method implementation... > > > > Anyway I'll do it tonight. > > > > Thanks, > > > > On Mon, Dec 13, 2010 at 5:52 PM, <mailto:dhollowa@chromium.org> wrote: > > > > > > Yes, that's fine. > > > > > > On 2010/12/14 01:48:53, http://takano.naoki_gmail.com wrote: > > >> > > >> I see, > > > > > >> But as you know, I cannot run try bots. > > > > > >> Could you try it again once I send my patch to you? > > > > > >> Thanks, > > > > > >> On Mon, Dec 13, 2010 at 5:45 PM, <mailto:dhollowa@chromium.org> > wrote: > > >> > > > >> > Your best bet is to make the fix and then use the try bots to test. > > >> > > > >> > On 2010/12/14 01:33:26, honten wrote: > > >> >> > > >> >> Ok... > > >> > > > >> >> How should I treat this? > > >> > > > >> >> Can I make Chrome OS on Linux env? > > >> > > > >> >> On 2010/12/14 01:31:21, dhollowa wrote: > > >> >> > On 2010/12/14 00:55:06, dhollowa wrote: > > >> >> > > Passed try bots. Committing. > > >> >> > > > >> >> > Had to revert due to ChromeOS issue: > > >> >> > > > >> >> > > > >> >> > chrome/browser/chromeos/login/account_creation_view.cc: In member > > >> >> > function > > >> >> > 'virtual TabContents* > > >> >> > chromeos::AccountCreationDomView::CreateTabContents(Profile*, > > >> > > > >> > SiteInstance*)': > > >> >> > > >> >> > chrome/browser/chromeos/login/account_creation_view.cc:99:error: > > >> >> > cannot > > >> >> allocate > > >> >> > an object of abstract type 'chromeos::AccountCreationTabContents' > > >> >> > chrome/browser/chromeos/login/account_creation_view.cc:19: note: > > >> >> > because > > >> > > > >> > the > > >> >> > > >> >> > following virtual functions are pure within > > >> >> > 'chromeos::AccountCreationTabContents': > > >> >> > ./chrome/browser/renderer_host/render_view_host_delegate.h:593: note: > > >> > > > >> > virtual > > >> >> > > >> >> > void RenderViewHostDelegate::AutoFill::Reset() > > >> >> > make: *** > > >> >> > > > >> > > > >> > > > > > > > > > > [out/Release/obj.target/browser/chrome/browser/chromeos/login/account_creation_view.o] > > >> > > >> >> > > >> >> > Error 1 > > >> > > > >> > > > >> > > > >> > http://codereview.chromium.org/5322001/ > > >> > > > > > > > > > > > > > http://codereview.chromium.org/5322001/ > > >
On 2010/12/15 06:45:04, honten wrote: > Just in case, I want to make sure you committed. > > Did you commit my change? > Committed. Yes. Revision 69199.
Great!! Thanks a lot!! On Wed, Dec 15, 2010 at 2:04 PM, <dhollowa@chromium.org> wrote: > On 2010/12/15 06:45:04, honten wrote: >> >> Just in case, I want to make sure you committed. > >> Did you commit my change? > > > Committed. Yes. Revision 69199. > > http://codereview.chromium.org/5322001/ >
Thank you! On 2010/12/16 01:08:34, takano.naoki_gmail.com wrote: > Great!! > > Thanks a lot!! > > On Wed, Dec 15, 2010 at 2:04 PM, <mailto:dhollowa@chromium.org> wrote: > > On 2010/12/15 06:45:04, honten wrote: > >> > >> Just in case, I want to make sure you committed. > > > >> Did you commit my change? > > > > > > Committed. Yes. Revision 69199. > > > > http://codereview.chromium.org/5322001/ > > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
