Parsed search.json for importing search engine settings from FireFox.

Parsed search.json for importing search engine settings from FireFox.
Since FireFox 3.5, search.json (instead of search.sqlite) is used for
storing search engines data.

BUG=329175
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280870

[Tapu Ghose, 2014-04-22 23:25:03]

Hello,

I have submitted this patch to fix Issue 329175.

For your reference, I have updated "GetSearchEnginesXMLData" function only. I
have not touched any other code.

I will appreciate if you please review my codes.

Thank you.

Regards,
--Tapu.

[tfarina, 2014-04-22 23:49:11]

https://codereview.chromium.org/247223003/diff/1/chrome/utility/importer/fire...
File chrome/utility/importer/firefox_importer.cc (right):

https://codereview.chromium.org/247223003/diff/1/chrome/utility/importer/fire...
chrome/utility/importer/firefox_importer.cc:115: ImportHomepage();  // Doesn't
have a UI item.
I don't think you meant to call ImportHomepage() twice in a row. Please, remove
this duplicated code.

[Tapu Ghose, 2014-04-23 00:04:00]

I think something went wrong. I have not touched anything other than
"GetSearchEnginesXMLData". I literally added line 390 through line 437.
Other than that I have not touched anything. I think I messed up while
updating code from git before committing my changes. I will look into it
soon.

Thanks.

--Tapu.


On Tue, Apr 22, 2014 at 7:49 PM, <tfarina@chromium.org> wrote:

>
> https://codereview.chromium.org/247223003/diff/1/chrome/
> utility/importer/firefox_importer.cc
> File chrome/utility/importer/firefox_importer.cc (right):
>
> https://codereview.chromium.org/247223003/diff/1/chrome/
> utility/importer/firefox_importer.cc#newcode115
> chrome/utility/importer/firefox_importer.cc:115: ImportHomepage();  //
> Doesn't have a UI item.
> I don't think you meant to call ImportHomepage() twice in a row. Please,
> remove this duplicated code.
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tapu Ghose, 2014-04-23 02:13:49]

The CQ bit was checked by ghose.tapu@gmail.com

[Tapu Ghose, 2014-04-23 02:13:49]

The CQ bit was unchecked by ghose.tapu@gmail.com

[Tapu Ghose, 2014-04-23 03:06:13]

PTAL

[tfarina, 2014-04-23 03:14:52]

I'm adding Gabriel for review, just in case.

And copying Ilya, in case he wants to help you in this review.

[Tapu Ghose, 2014-04-23 03:16:02]

Thanks Thiago.


On Tue, Apr 22, 2014 at 11:14 PM, <tfarina@chromium.org> wrote:

> I'm adding Gabriel for review, just in case.
>
> And copying Ilya, in case he wants to help you in this review.
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2014-04-23 04:56:01]

Thanks for working on this!  In addition to addressing my inline comments,
please add some test coverage for these changes.

https://codereview.chromium.org/247223003/diff/20001/chrome/utility/importer/...
File chrome/utility/importer/firefox_importer.cc (right):

https://codereview.chromium.org/247223003/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:389: // since Firefox 3.5, search
engines are no longer stored in search.sqlite.
nit: Please start the sentence with a capital letter.

https://codereview.chromium.org/247223003/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:399: if (!root.get())
nit: No need for the ".get()".

https://codereview.chromium.org/247223003/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:403:
static_cast<base::DictionaryValue*>(root.release());
Please use GetAsDictionary() for a type-safe conversion.

https://codereview.chromium.org/247223003/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:403:
static_cast<base::DictionaryValue*>(root.release());
Releasing the scoped_ptr here leaks memory.  It's quite rare to need to call
release().  If you ever need to transfer ownership, you should instead call
Pass().

https://codereview.chromium.org/247223003/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:410: // search engine list can be
found from key <engines> of the dictionary.
nit: Please write this as a complete sentence, including capitalizing the first
letter.

https://codereview.chromium.org/247223003/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:419: const base::ListValue*
search_engines = NULL;
nit: Please move this down even closer to where it's first used.

https://codereview.chromium.org/247223003/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:438: }
Please factor this code out into a separate method.

[Tapu Ghose, 2014-05-01 03:23:17]

Hello Ilya,

Thank you very much for reviewing my codes and providing feedback. I tried to
address your comments and added test case as suggested by you. I would like to
thank you for guiding me in last few days regarding adding and testing new test
case. Please review my updates.

Thank you.

--Tapu.

[Ilya Sherman, 2014-05-01 21:37:02]

Thanks!  This is getting pretty close -- just a few issues left to hammer out
with the test code.

https://codereview.chromium.org/247223003/diff/40001/chrome/browser/importer/...
File chrome/browser/importer/firefox_importer_browsertest.cc (right):

https://codereview.chromium.org/247223003/diff/40001/chrome/browser/importer/...
chrome/browser/importer/firefox_importer_browsertest.cc:88: {L"\x4E2D\x6587",
"http://www.google.com/"},
Hmm, it seems strange to need so many changes to this list.  For example, why is
the "&tag=wwwcanoniccom-20" URL parameter needed for Amazon?

It would be most convenient to simply have identical lists of search engines in
the 3.x profile and in the newest profile.  But, if that's not possible, then
let's preserve the original kFirefoxKeywords list as something like
kFirefox3xKeywords, and update the test code expectations to read from the
appropriate list, depending on which test case is being run.

https://codereview.chromium.org/247223003/diff/40001/chrome/browser/importer/...
chrome/browser/importer/firefox_importer_browsertest.cc:245: }
Can we get rid of lines 242-245 entirely?  Also possibly line 240 and 241, given
that line 246 is the same as 240?

Alternately, I'm noticing that the name of this test is
"Firefox3xImporterBrowserTest".  Perhaps it would be best to add a new method
for testing newer versions of Firefox, and leave this one as it was.

https://codereview.chromium.org/247223003/diff/40001/chrome/browser/importer/...
chrome/browser/importer/firefox_importer_browsertest.cc:295: }
Please include a README file in the new test directory describing how the
directory was generated.

https://codereview.chromium.org/247223003/diff/40001/chrome/utility/importer/...
File chrome/utility/importer/firefox_importer.cc (right):

https://codereview.chromium.org/247223003/diff/40001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:490: 
nit: I'd omit this blank line.

https://codereview.chromium.org/247223003/diff/40001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:496: 
nit: I'd omit this blank line.

https://codereview.chromium.org/247223003/diff/40001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:500: // Dictionary
"search_directories" contains a list of search engines.
nit: Rather than using quotation marks to denote variable names, Chromium code
generally uses pipes, i.e. we'd write """Dictionary |search_directories|
contains ..."""

https://codereview.chromium.org/247223003/diff/40001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:507: // Therefore, it needs to be
retrieved by using iterator
nit: "by using iterator" -> "by searching."

https://codereview.chromium.org/247223003/diff/40001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer.cc:513: if
(search_directories->GetList(kEngines, &search_engines)) {
nit: I'd suggest reversing this condition and using an early return, as you do
above, to reduce the level of indentation.

[Tapu Ghose, 2014-05-01 21:52:58]

Hi Ilya,

Thanks for the feedback. I definitely agree that we should play with
original kFirefoxKeywords. I made the changes in order to match with my
local machine's xml files content. I would definitely do another round of
test by modifying my local xml files such that they respect original
kFirefoxKeywords.

Cheers,
--Tapu.


On Thu, May 1, 2014 at 5:37 PM, <isherman@chromium.org> wrote:

> Thanks!  This is getting pretty close -- just a few issues left to hammer
> out
> with the test code.
>
>
> https://codereview.chromium.org/247223003/diff/40001/
> chrome/browser/importer/firefox_importer_browsertest.cc
> File chrome/browser/importer/firefox_importer_browsertest.cc (right):
>
> https://codereview.chromium.org/247223003/diff/40001/
> chrome/browser/importer/firefox_importer_browsertest.cc#newcode88
> chrome/browser/importer/firefox_importer_browsertest.cc:88:
> {L"\x4E2D\x6587", "http://www.google.com/"},
> Hmm, it seems strange to need so many changes to this list.  For
> example, why is the "&tag=wwwcanoniccom-20" URL parameter needed for
> Amazon?
>
> It would be most convenient to simply have identical lists of search
> engines in the 3.x profile and in the newest profile.  But, if that's
> not possible, then let's preserve the original kFirefoxKeywords list as
> something like kFirefox3xKeywords, and update the test code expectations
> to read from the appropriate list, depending on which test case is being
> run.
>
> https://codereview.chromium.org/247223003/diff/40001/
> chrome/browser/importer/firefox_importer_browsertest.cc#newcode245
> chrome/browser/importer/firefox_importer_browsertest.cc:245: }
> Can we get rid of lines 242-245 entirely?  Also possibly line 240 and
> 241, given that line 246 is the same as 240?
>
> Alternately, I'm noticing that the name of this test is
> "Firefox3xImporterBrowserTest".  Perhaps it would be best to add a new
> method for testing newer versions of Firefox, and leave this one as it
> was.
>
> https://codereview.chromium.org/247223003/diff/40001/
> chrome/browser/importer/firefox_importer_browsertest.cc#newcode295
> chrome/browser/importer/firefox_importer_browsertest.cc:295: }
> Please include a README file in the new test directory describing how
> the directory was generated.
>
> https://codereview.chromium.org/247223003/diff/40001/
> chrome/utility/importer/firefox_importer.cc
> File chrome/utility/importer/firefox_importer.cc (right):
>
> https://codereview.chromium.org/247223003/diff/40001/
> chrome/utility/importer/firefox_importer.cc#newcode490
> chrome/utility/importer/firefox_importer.cc:490:
> nit: I'd omit this blank line.
>
> https://codereview.chromium.org/247223003/diff/40001/
> chrome/utility/importer/firefox_importer.cc#newcode496
> chrome/utility/importer/firefox_importer.cc:496:
> nit: I'd omit this blank line.
>
> https://codereview.chromium.org/247223003/diff/40001/
> chrome/utility/importer/firefox_importer.cc#newcode500
> chrome/utility/importer/firefox_importer.cc:500: // Dictionary
> "search_directories" contains a list of search engines.
> nit: Rather than using quotation marks to denote variable names,
> Chromium code generally uses pipes, i.e. we'd write """Dictionary
> |search_directories| contains ..."""
>
> https://codereview.chromium.org/247223003/diff/40001/
> chrome/utility/importer/firefox_importer.cc#newcode507
> chrome/utility/importer/firefox_importer.cc:507: // Therefore, it needs
> to be retrieved by using iterator
> nit: "by using iterator" -> "by searching."
>
> https://codereview.chromium.org/247223003/diff/40001/
> chrome/utility/importer/firefox_importer.cc#newcode513
> chrome/utility/importer/firefox_importer.cc:513: if
> (search_directories->GetList(kEngines, &search_engines)) {
> nit: I'd suggest reversing this condition and using an early return, as
> you do above, to reduce the level of indentation.
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tapu Ghose, 2014-05-03 05:10:09]

PTAL

[Ilya Sherman, 2014-05-06 04:21:48]

Thanks!  I have a few more nits for you, and I'm going to try running this CL
through the try bots to see if the new test works across all platforms.

https://codereview.chromium.org/247223003/diff/60001/chrome/test/data/firefox...
File chrome/test/data/firefox_profile/README (right):

https://codereview.chromium.org/247223003/diff/60001/chrome/test/data/firefox...
chrome/test/data/firefox_profile/README:10: Moreover, each list of |engines| is
updated to respect existing |kFirefoxKeywords|.
nit: Please wrap to 80 columns per line.

https://codereview.chromium.org/247223003/diff/60001/chrome/test/data/firefox...
chrome/test/data/firefox_profile/README:11: Please NOTE that values of
|filePath| is dependant on workstation. They NEED to be
nit: Ditto here.

https://codereview.chromium.org/247223003/diff/60001/chrome/test/data/firefox...
chrome/test/data/firefox_profile/README:12: updated in order to pass the test in
corresponding workstation.
Hmm.  Do you mean that the test you're adding will only run correctly on a
single platform?  If so, it would be good to brainstorm how to make the test
pass on all platforms.

[Tapu Ghose, 2014-05-06 05:18:52]

PTAL

https://codereview.chromium.org/247223003/diff/60001/chrome/test/data/firefox...
File chrome/test/data/firefox_profile/README (right):

https://codereview.chromium.org/247223003/diff/60001/chrome/test/data/firefox...
chrome/test/data/firefox_profile/README:10: Moreover, each list of |engines| is
updated to respect existing |kFirefoxKeywords|.
On 2014/05/06 04:21:49, Ilya Sherman wrote:
> nit: Please wrap to 80 columns per line.

I should be more careful. Usually, I run "git cl format" before committing. But
occasionally it fails to format the codes.

https://codereview.chromium.org/247223003/diff/60001/chrome/test/data/firefox...
chrome/test/data/firefox_profile/README:12: updated in order to pass the test in
corresponding workstation.
On 2014/05/06 04:21:49, Ilya Sherman wrote:
> Hmm.  Do you mean that the test you're adding will only run correctly on a
> single platform?  If so, it would be good to brainstorm how to make the test
> pass on all platforms.

No. The test should run for all platforms. Here, I meant to say that if you run
the test in a try bot, then values of |filePath| in test search.json should be
updated.
In the test search.json, you can see that values of |filePath| is pointing to
some of my local machine's directories. The values of |filePath| is the location
of xml files for Firefox's search engines.

[Ilya Sherman, 2014-05-06 19:00:11]

https://codereview.chromium.org/247223003/diff/60001/chrome/test/data/firefox...
File chrome/test/data/firefox_profile/README (right):

https://codereview.chromium.org/247223003/diff/60001/chrome/test/data/firefox...
chrome/test/data/firefox_profile/README:10: Moreover, each list of |engines| is
updated to respect existing |kFirefoxKeywords|.
On 2014/05/06 05:18:53, Tapu wrote:
> On 2014/05/06 04:21:49, Ilya Sherman wrote:
> > nit: Please wrap to 80 columns per line.
> 
> I should be more careful. Usually, I run "git cl format" before committing.
But
> occasionally it fails to format the codes.

Yeah, I think "git cl format" only formats C++ files.  Thanks for fixing the
wrapping here :)

https://codereview.chromium.org/247223003/diff/60001/chrome/test/data/firefox...
chrome/test/data/firefox_profile/README:12: updated in order to pass the test in
corresponding workstation.
On 2014/05/06 05:18:53, Tapu wrote:
> On 2014/05/06 04:21:49, Ilya Sherman wrote:
> > Hmm.  Do you mean that the test you're adding will only run correctly on a
> > single platform?  If so, it would be good to brainstorm how to make the test
> > pass on all platforms.
> 
> No. The test should run for all platforms. Here, I meant to say that if you
run
> the test in a try bot, then values of |filePath| in test search.json should be
> updated.
> In the test search.json, you can see that values of |filePath| is pointing to
> some of my local machine's directories. The values of |filePath| is the
location
> of xml files for Firefox's search engines.

Is it possible to include the XML files in the test directory, and to use
relative file paths to those XML files?  The test should be able to run
successfully on all platforms without any changes to the data files.  Once the
test code is checked in to the tree, it will be run automatically (so that it
can catch regressions :)), so we need to make sure that it works everywhere.

Currently, the try bots from patch set 4 are showing failures on all platforms. 
(There are also many unrelated failures that you can safely ignore.  Our
infrastructure has lots of sources of flaky failures, which folks are gradually
working on fixing.)  Do those results give you enough information to debug and
implement fixes?

[Tapu Ghose, 2014-05-08 04:05:09]

On 2014/05/06 19:00:11, Ilya Sherman wrote:
>
https://codereview.chromium.org/247223003/diff/60001/chrome/test/data/firefox...
> File chrome/test/data/firefox_profile/README (right):
> 
>
https://codereview.chromium.org/247223003/diff/60001/chrome/test/data/firefox...
> chrome/test/data/firefox_profile/README:10: Moreover, each list of |engines|
is
> updated to respect existing |kFirefoxKeywords|.
> On 2014/05/06 05:18:53, Tapu wrote:
> > On 2014/05/06 04:21:49, Ilya Sherman wrote:
> > > nit: Please wrap to 80 columns per line.
> > 
> > I should be more careful. Usually, I run "git cl format" before committing.
> But
> > occasionally it fails to format the codes.
> 
> Yeah, I think "git cl format" only formats C++ files.  Thanks for fixing the
> wrapping here :)
> 
>
https://codereview.chromium.org/247223003/diff/60001/chrome/test/data/firefox...
> chrome/test/data/firefox_profile/README:12: updated in order to pass the test
in
> corresponding workstation.
> On 2014/05/06 05:18:53, Tapu wrote:
> > On 2014/05/06 04:21:49, Ilya Sherman wrote:
> > > Hmm.  Do you mean that the test you're adding will only run correctly on a
> > > single platform?  If so, it would be good to brainstorm how to make the
test
> > > pass on all platforms.
> > 
> > No. The test should run for all platforms. Here, I meant to say that if you
> run
> > the test in a try bot, then values of |filePath| in test search.json should
be
> > updated.
> > In the test search.json, you can see that values of |filePath| is pointing
to
> > some of my local machine's directories. The values of |filePath| is the
> location
> > of xml files for Firefox's search engines.
> 
> Is it possible to include the XML files in the test directory, and to use
> relative file paths to those XML files?  The test should be able to run
> successfully on all platforms without any changes to the data files.  Once the
> test code is checked in to the tree, it will be run automatically (so that it
> can catch regressions :)), so we need to make sure that it works everywhere.
> 
> Currently, the try bots from patch set 4 are showing failures on all
platforms. 
> (There are also many unrelated failures that you can safely ignore.  Our
> infrastructure has lots of sources of flaky failures, which folks are
gradually
> working on fixing.)  Do those results give you enough information to debug and
> implement fixes?

I have included the XML files in the test directory and used relative path in
search.json. Tested in my Ubuntu machine and passed with no error (not tested
in any other platform though).

[Tapu Ghose, 2014-05-08 04:05:43]

PTAL

[Ilya Sherman, 2014-05-09 23:54:45]

On 2014/05/08 04:05:43, Tapu wrote:
> PTAL

Hi, apologies for being slow to respond.  I haven't had a chance to review this
yet, but it's definitely on my radar.  I'm aiming to get to it early next week.
Thanks for your patience!

[Ilya Sherman, 2014-05-13 03:23:10]

Thanks for uploading the XML files :)

I'm going to try running this past the try bots again.  However, it looks like
you might need to re-upload your changes -- the file
"chrome/test/data/firefox_profile/searchplugins/default/creativecommons.xml"
might not have made it.

[Ilya Sherman, 2014-05-13 03:23:47]

On 2014/05/13 03:23:10, Ilya Sherman wrote:
> Thanks for uploading the XML files :)
> 
> I'm going to try running this past the try bots again.  However, it looks like
> you might need to re-upload your changes -- the file
> "chrome/test/data/firefox_profile/searchplugins/default/creativecommons.xml"
> might not have made it.

(Specifically, there isn't an "A" next to the filename, which often means that
something went awry during the upload.)

[Tapu Ghose, 2014-05-13 05:10:27]

On 2014/05/13 03:23:47, Ilya Sherman wrote:
> On 2014/05/13 03:23:10, Ilya Sherman wrote:
> > Thanks for uploading the XML files :)
> > 
> > I'm going to try running this past the try bots again.  However, it looks
like
> > you might need to re-upload your changes -- the file
> > "chrome/test/data/firefox_profile/searchplugins/default/creativecommons.xml"
> > might not have made it.
> 
> (Specifically, there isn't an "A" next to the filename, which often means that
> something went awry during the upload.)

Thank you for reviewing. I have re-uploaded.

[Tapu Ghose, 2014-05-13 05:10:42]

PTAL

[Ilya Sherman, 2014-05-13 18:43:17]

On 2014/05/13 05:10:27, Tapu wrote:
> On 2014/05/13 03:23:47, Ilya Sherman wrote:
> > On 2014/05/13 03:23:10, Ilya Sherman wrote:
> > > Thanks for uploading the XML files :)
> > > 
> > > I'm going to try running this past the try bots again.  However, it looks
> like
> > > you might need to re-upload your changes -- the file
> > >
"chrome/test/data/firefox_profile/searchplugins/default/creativecommons.xml"
> > > might not have made it.
> > 
> > (Specifically, there isn't an "A" next to the filename, which often means
that
> > something went awry during the upload.)
> 
> Thank you for reviewing. I have re-uploaded.

Thanks.  I've kicked off another set of try bot runs.  Let's see what the
results look like :)

[Tapu Ghose, 2014-05-13 19:41:04]

Hello Ilya,

Thank you for putting your valuable time behind reviewing. Please note that
prior to Firefox 3.5 |search.sqlite| was used for search engine info. As we
noticed before, |search.sqlite| was never used for testing. If you
remember, there was a |if| block in firefox_importer_browsertest which used
to make the test exit without testing anything. I have a feeling that the
tests might fail for sqlite version. May be we need to add similar xml
files in firefox35_profile diretory too. Also, we may need to update the
search.sqilte accordingly to make the tests pass.

Cheers,
--Tapu.


On Tue, May 13, 2014 at 2:43 PM, <isherman@chromium.org> wrote:

> On 2014/05/13 05:10:27, Tapu wrote:
>
>> On 2014/05/13 03:23:47, Ilya Sherman wrote:
>> > On 2014/05/13 03:23:10, Ilya Sherman wrote:
>> > > Thanks for uploading the XML files :)
>> > >
>> > > I'm going to try running this past the try bots again.  However, it
>> looks
>> like
>> > > you might need to re-upload your changes -- the file
>> > >
>>
> "chrome/test/data/firefox_profile/searchplugins/default/
> creativecommons.xml"
>
>> > > might not have made it.
>> >
>> > (Specifically, there isn't an "A" next to the filename, which often
>> means
>>
> that
>
>> > something went awry during the upload.)
>>
>
>  Thank you for reviewing. I have re-uploaded.
>>
>
> Thanks.  I've kicked off another set of try bot runs.  Let's see what the
> results look like :)
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2014-05-13 21:08:07]

It looks like the try runs are failing because your patch needs to be rebased,
so that it applies cleanly on top of the HEAD revision.

On 2014/05/13 19:41:04, Tapu wrote:
> Hello Ilya,
> 
> Thank you for putting your valuable time behind reviewing. Please note that
> prior to Firefox 3.5 |search.sqlite| was used for search engine info. As we
> noticed before, |search.sqlite| was never used for testing. If you
> remember, there was a |if| block in firefox_importer_browsertest which used
> to make the test exit without testing anything. I have a feeling that the
> tests might fail for sqlite version. May be we need to add similar xml
> files in firefox35_profile diretory too. Also, we may need to update the
> search.sqilte accordingly to make the tests pass.

Thanks for continuing to work on this CL!  I appreciate your patience and
understanding that automated test coverage is important.

The Firefox35Importer test disables importing search engines.  If the
Firefox30Importer test currently fails, we can either (a) fix the test, or (b)
similarly disable testing search engine import.  Which option is better depends
on how hard it would be to get the test working correctly.  You should be able
to verify locally whether the test passes on the platform you're developing on. 
The advantage of the try bots is that they let you test on all the other
platforms as well, in case there are any platform-specific differences.

[Tapu Ghose, 2014-05-17 03:53:52]

On 2014/05/13 21:08:07, Ilya Sherman wrote:
> It looks like the try runs are failing because your patch needs to be rebased,
> so that it applies cleanly on top of the HEAD revision.

Patch rebased.

> 
> On 2014/05/13 19:41:04, Tapu wrote:
> > Hello Ilya,
> > 
> > Thank you for putting your valuable time behind reviewing. Please note that
> > prior to Firefox 3.5 |search.sqlite| was used for search engine info. As we
> > noticed before, |search.sqlite| was never used for testing. If you
> > remember, there was a |if| block in firefox_importer_browsertest which used
> > to make the test exit without testing anything. I have a feeling that the
> > tests might fail for sqlite version. May be we need to add similar xml
> > files in firefox35_profile diretory too. Also, we may need to update the
> > search.sqilte accordingly to make the tests pass.
> 
> Thanks for continuing to work on this CL!  I appreciate your patience and
> understanding that automated test coverage is important.
> 
> The Firefox35Importer test disables importing search engines.  If the
> Firefox30Importer test currently fails, we can either (a) fix the test, or (b)
> similarly disable testing search engine import.  Which option is better
depends
> on how hard it would be to get the test working correctly.  You should be able
> to verify locally whether the test passes on the platform you're developing
on. 
> The advantage of the try bots is that they let you test on all the other
> platforms as well, in case there are any platform-specific differences.

Added test data for Firefox30Importer. Details of how test data is created can
be
found in ../../chrome/test/data/firefox3_profile/README.

[Tapu Ghose, 2014-05-17 03:54:28]

PTAL

[Tapu Ghose, 2014-05-17 04:06:54]

PTAL

[Tapu Ghose, 2014-05-17 17:45:00]

Removed unwanted dot (.) from relative path in search.json test data. I ran the
browser tests in my local machine (ubuntu) and all three tests for firefox
profile importer was passed (log appended below).

For the first time I tried to run "git cl try" and I think I should not have
done so.

Log for the browser test:
--------------------------------------------------
tapughose@tg-ubuntu:~/chromium/src$ ./out/Debug/browser_tests
--gtest_filter=FirefoxProfileImporterBrowserTest.*
IMPORTANT DEBUGGING NOTE: each test is run inside its own process.
For debugging a test inside a debugger, use the
--gtest_filter=<your_test_name> flag along with either
--single_process (to run the test in one launcher/browser process) or
--single-process (to do the above, and also run Chrome in single-process mode).
Using sharding settings from environment. This is shard 0/1
Using 1 parallel jobs.
Note: Google Test filter = FirefoxProfileImporterBrowserTest.Firefox30Importer
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from FirefoxProfileImporterBrowserTest, where TypeParam = 
[ RUN      ] FirefoxProfileImporterBrowserTest.Firefox30Importer
[7192:7192:0517/134216:WARNING:password_store_factory.cc(213)] Using basic
(unencrypted) store for password storage. See
http://code.google.com/p/chromium/wiki/LinuxPasswordStorage for more information
about password storage options.
[7192:7215:0517/134219:ERROR:raw_channel_posix.cc(148)] recvmsg: Connection
reset by peer
[7192:7215:0517/134219:ERROR:channel.cc(293)] RawChannel fatal error (type 1)
[       OK ] FirefoxProfileImporterBrowserTest.Firefox30Importer (6535 ms)
[----------] 1 test from FirefoxProfileImporterBrowserTest (6535 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (6539 ms total)
[  PASSED  ] 1 test.
[1/3] FirefoxProfileImporterBrowserTest.Firefox30Importer (8129 ms)
Note: Google Test filter = FirefoxProfileImporterBrowserTest.Firefox35Importer
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from FirefoxProfileImporterBrowserTest, where TypeParam = 
[ RUN      ] FirefoxProfileImporterBrowserTest.Firefox35Importer
[7253:7253:0517/134224:WARNING:password_store_factory.cc(213)] Using basic
(unencrypted) store for password storage. See
http://code.google.com/p/chromium/wiki/LinuxPasswordStorage for more information
about password storage options.
[       OK ] FirefoxProfileImporterBrowserTest.Firefox35Importer (6384 ms)
[----------] 1 test from FirefoxProfileImporterBrowserTest (6384 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (6387 ms total)
[  PASSED  ] 1 test.
[2/3] FirefoxProfileImporterBrowserTest.Firefox35Importer (8130 ms)
Note: Google Test filter = FirefoxProfileImporterBrowserTest.FirefoxImporter
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from FirefoxProfileImporterBrowserTest, where TypeParam = 
[ RUN      ] FirefoxProfileImporterBrowserTest.FirefoxImporter
[7314:7314:0517/134232:WARNING:password_store_factory.cc(213)] Using basic
(unencrypted) store for password storage. See
http://code.google.com/p/chromium/wiki/LinuxPasswordStorage for more information
about password storage options.
[7314:7337:0517/134235:ERROR:raw_channel_posix.cc(148)] recvmsg: Connection
reset by peer
[7314:7337:0517/134235:ERROR:channel.cc(293)] RawChannel fatal error (type 1)
[       OK ] FirefoxProfileImporterBrowserTest.FirefoxImporter (6530 ms)
[----------] 1 test from FirefoxProfileImporterBrowserTest (6530 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (6532 ms total)
[  PASSED  ] 1 test.
[3/3] FirefoxProfileImporterBrowserTest.FirefoxImporter (8136 ms)
SUCCESS: all tests passed.

[Ilya Sherman, 2014-05-29 07:21:58]

Thanks!  I've launched some fresh try jobs on all three platforms.

https://codereview.chromium.org/247223003/diff/170001/chrome/browser/importer...
File chrome/browser/importer/firefox_importer_browsertest.cc (right):

https://codereview.chromium.org/247223003/diff/170001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:278: observer.get(),
true);
nit: Please fix the alignment of the text on this line.  Ditto for the other two
tests.

[Ilya Sherman, 2014-05-29 20:31:41]

On 2014/05/29 07:21:58, Ilya Sherman wrote:
> Thanks!  I've launched some fresh try jobs on all three platforms.

Looks like the tests failed on various bots.  Please take a look at the failures
and try to address them.  Thanks.

[Tapu Ghose, 2014-06-01 02:36:51]

Hi Ilya,

Sorry for not getting in touch earlier. I was away for few days.

I was looking at the log for test failure in linux_rel bot. I used to get
the similar errors in my local machine when the program was failing to find
|search.json| file
(/chromium/src/chrome/test/data/firefox_profile/search.json). And most
importantly those errors encountered when the program was unable to find
xml files for search engines which are mentioned by |filePath| attribute in
search.json. I have used relative path for them. I was wondering if the
relative paths are breaking the tests in bots. Here is an example of
|filePath| value I have used in search.json:

"filePath":"./chrome/test/data/firefox_profile/searchplugins/default/google.xml"

Please let me know if you have any thoughts.

Cheers,
~Tapu.


On Thu, May 29, 2014 at 4:31 PM, <isherman@chromium.org> wrote:

> On 2014/05/29 07:21:58, Ilya Sherman wrote:
>
>> Thanks!  I've launched some fresh try jobs on all three platforms.
>>
>
> Looks like the tests failed on various bots.  Please take a look at the
> failures
> and try to address them.  Thanks.
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2014-06-04 00:36:13]

On 2014/06/01 02:36:51, Tapu wrote:
> Hi Ilya,
> 
> Sorry for not getting in touch earlier. I was away for few days.
> 
> I was looking at the log for test failure in linux_rel bot. I used to get
> the similar errors in my local machine when the program was failing to find
> |search.json| file
> (/chromium/src/chrome/test/data/firefox_profile/search.json). And most
> importantly those errors encountered when the program was unable to find
> xml files for search engines which are mentioned by |filePath| attribute in
> search.json. I have used relative path for them. I was wondering if the
> relative paths are breaking the tests in bots. Here is an example of
> |filePath| value I have used in search.json:
> 
>
"filePath":"./chrome/test/data/firefox_profile/searchplugins/default/google.xml"
> 
> Please let me know if you have any thoughts.
> 
> Cheers,
> ~Tapu.

Hi Tapu,

Sorry for being slow to reply here.  I'm going to try to patch this in locally
on my Mac and help you debug, but I haven't found time for it yet.  I'll try to
make some time tomorrow.

[Tapu Ghose, 2014-06-04 00:37:48]

Thanks Ilya. I appreciate your patience and guidance.

Cheers,
~Tapu.


On Tue, Jun 3, 2014 at 8:36 PM, <isherman@chromium.org> wrote:

> On 2014/06/01 02:36:51, Tapu wrote:
>
>> Hi Ilya,
>>
>
>  Sorry for not getting in touch earlier. I was away for few days.
>>
>
>  I was looking at the log for test failure in linux_rel bot. I used to get
>> the similar errors in my local machine when the program was failing to
>> find
>> |search.json| file
>> (/chromium/src/chrome/test/data/firefox_profile/search.json). And most
>> importantly those errors encountered when the program was unable to find
>> xml files for search engines which are mentioned by |filePath| attribute
>> in
>> search.json. I have used relative path for them. I was wondering if the
>> relative paths are breaking the tests in bots. Here is an example of
>> |filePath| value I have used in search.json:
>>
>
>
> "filePath":"./chrome/test/data/firefox_profile/
> searchplugins/default/google.xml"
>
>  Please let me know if you have any thoughts.
>>
>
>  Cheers,
>> ~Tapu.
>>
>
> Hi Tapu,
>
> Sorry for being slow to reply here.  I'm going to try to patch this in
> locally
> on my Mac and help you debug, but I haven't found time for it yet.  I'll
> try to
> make some time tomorrow.
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2014-06-05 06:44:51]

Hi Tapu,

I tried to patch in your changes, but it looks like our system doesn't support
patching binary files very well.  I suspect that this might be the issue that
we're seeing with the trybots as well.  Could you please email me your modified
copy of the search.sqlite file?  I'll then try patching that in, and see if
everything runs smoothly.  If so, I'll go ahead and check in that file as a
separate CL; then you'll hopefully be unblocked on this CL.  Thanks for your
patience!

Cheers,
~ilya

[Tapu Ghose, 2014-06-05 11:39:28]

Hello Ilya,

Thank you for providing your valuable time behind tracing the issue. Please
find the attached

search.sqlite (placed under chrome/test/data/firefox3_profile directory)
search.json   (placed under chrome/test/data/firefox_profile directory)

that I have used.

Cheers,
~Tapu.


On Thu, Jun 5, 2014 at 2:44 AM, <isherman@chromium.org> wrote:

> Hi Tapu,
>
> I tried to patch in your changes, but it looks like our system doesn't
> support
> patching binary files very well.  I suspect that this might be the issue
> that
> we're seeing with the trybots as well.  Could you please email me your
> modified
> copy of the search.sqlite file?  I'll then try patching that in, and see if
> everything runs smoothly.  If so, I'll go ahead and check in that file as a
> separate CL; then you'll hopefully be unblocked on this CL.  Thanks for
> your
> patience!
>
> Cheers,
> ~ilya
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2014-06-06 04:15:42]

Hi Tapu,

I patched in your changes locally, but I wasn't able to get the tests to pass on
Linux with the changes patched in.  However, I now have a better understanding
of what the test code is doing, and consequently have some questions (see
below).

I think a useful next step might be to split off a separate CL just to make the
import_search_engines case work smoothly for the existing Firefox3 tests.  Once
you have a CL for that working, I can patch it in, verify that it works for me
as well, and manually commit the changes, including the modification to the
binary sqlite file.  Once that's landed, it should unblock your work on this CL.

https://codereview.chromium.org/247223003/diff/190001/chrome/browser/importer...
File chrome/browser/importer/firefox_importer_browsertest.cc (right):

https://codereview.chromium.org/247223003/diff/190001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:245:
ASSERT_TRUE(base::CopyDirectory(data_path, search_engine_path, false));
I'm noticing that this code currently tries to copy the directory
non-recursively.  That's doesn't seem right -- it seems like that would be a
no-op, since the "searchplugins" directory only contains subdirectories.  Are
you sure this is actually copying the files as you'd expect?

https://codereview.chromium.org/247223003/diff/190001/chrome/test/data/firefo...
File chrome/test/data/firefox3_profile/README (right):

https://codereview.chromium.org/247223003/diff/190001/chrome/test/data/firefo...
chrome/test/data/firefox3_profile/README:20: path. The actual app or profile
path varies with operating system. To make
Do the app and profile paths vary by operating system, even in the test setup? 
Looking more closely at the test code, it looks like SetUp() creates a temporary
directory, and profile and app directories within it.  Do these app and profile
paths not get used?

https://codereview.chromium.org/247223003/diff/190001/chrome/test/data/firefo...
chrome/test/data/firefox3_profile/README:25: [profile] =>
./chrome/test/data/firefox3_profile/searchplugins/installed
What is this path relative to?  Since the test code copies all of the files over
to a temporary directory, it seems unnecessary to have the
"chrome/test/data/firefox3_profile" prefix.

https://codereview.chromium.org/247223003/diff/190001/chrome/test/data/firefo...
chrome/test/data/firefox3_profile/README:25: [profile] =>
./chrome/test/data/firefox3_profile/searchplugins/installed
Also, I've realized that the test code expects the search engine XML files to
all live in a common location.  Rather than adding them once to
//chrome/test/data/firefox3_profile/searchplugins and again to
//chrome/test/data/firefox_profile/searchplugins, you could add them to
//chrome/test/data/firefox_searchplugins.  You'd then have the test code copy
over the contents of this directory to {tempdir}/app/searchplugins.  Does that
make sense?

[Tapu Ghose, 2014-06-15 04:18:57]

Hi Ilya,

I have addressed your feedback. I also attached the update search.sqlite
(binary file) for your convenience.

Cheers,
~Tapu.


On Fri, Jun 6, 2014 at 12:15 AM, <isherman@chromium.org> wrote:

> Hi Tapu,
>
> I patched in your changes locally, but I wasn't able to get the tests to
> pass on
> Linux with the changes patched in.  However, I now have a better
> understanding
> of what the test code is doing, and consequently have some questions (see
> below).
>
> I think a useful next step might be to split off a separate CL just to
> make the
> import_search_engines case work smoothly for the existing Firefox3 tests.
>  Once
> you have a CL for that working, I can patch it in, verify that it works
> for me
> as well, and manually commit the changes, including the modification to the
> binary sqlite file.  Once that's landed, it should unblock your work on
> this CL.
>
>
> https://codereview.chromium.org/247223003/diff/190001/
> chrome/browser/importer/firefox_importer_browsertest.cc
> File chrome/browser/importer/firefox_importer_browsertest.cc (right):
>
> https://codereview.chromium.org/247223003/diff/190001/
> chrome/browser/importer/firefox_importer_browsertest.cc#newcode245
> chrome/browser/importer/firefox_importer_browsertest.cc:245:
> ASSERT_TRUE(base::CopyDirectory(data_path, search_engine_path, false));
> I'm noticing that this code currently tries to copy the directory
> non-recursively.  That's doesn't seem right -- it seems like that would
> be a no-op, since the "searchplugins" directory only contains
> subdirectories.  Are you sure this is actually copying the files as
> you'd expect?
>
> https://codereview.chromium.org/247223003/diff/190001/
> chrome/test/data/firefox3_profile/README
> File chrome/test/data/firefox3_profile/README (right):
>
> https://codereview.chromium.org/247223003/diff/190001/
> chrome/test/data/firefox3_profile/README#newcode20
> chrome/test/data/firefox3_profile/README:20: path. The actual app or
> profile path varies with operating system. To make
> Do the app and profile paths vary by operating system, even in the test
> setup?  Looking more closely at the test code, it looks like SetUp()
> creates a temporary directory, and profile and app directories within
> it.  Do these app and profile paths not get used?
>
> https://codereview.chromium.org/247223003/diff/190001/
> chrome/test/data/firefox3_profile/README#newcode25
> chrome/test/data/firefox3_profile/README:25: [profile] =>
> ./chrome/test/data/firefox3_profile/searchplugins/installed
> What is this path relative to?  Since the test code copies all of the
> files over to a temporary directory, it seems unnecessary to have the
> "chrome/test/data/firefox3_profile" prefix.
>
> https://codereview.chromium.org/247223003/diff/190001/
> chrome/test/data/firefox3_profile/README#newcode25
> chrome/test/data/firefox3_profile/README:25: [profile] =>
> ./chrome/test/data/firefox3_profile/searchplugins/installed
> Also, I've realized that the test code expects the search engine XML
> files to all live in a common location.  Rather than adding them once to
> //chrome/test/data/firefox3_profile/searchplugins and again to
> //chrome/test/data/firefox_profile/searchplugins, you could add them to
> //chrome/test/data/firefox_searchplugins.  You'd then have the test code
> copy over the contents of this directory to {tempdir}/app/searchplugins.
>  Does that make sense?
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tapu Ghose, 2014-06-15 04:19:49]

PTAL

https://codereview.chromium.org/247223003/diff/190001/chrome/browser/importer...
File chrome/browser/importer/firefox_importer_browsertest.cc (right):

https://codereview.chromium.org/247223003/diff/190001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:245:
ASSERT_TRUE(base::CopyDirectory(data_path, search_engine_path, false));
On 2014/06/06 04:15:42, Ilya Sherman wrote:
> I'm noticing that this code currently tries to copy the directory
> non-recursively.  That's doesn't seem right -- it seems like that would be a
> no-op, since the "searchplugins" directory only contains subdirectories.  Are
> you sure this is actually copying the files as you'd expect?

Updated test code for copying the files properly.

https://codereview.chromium.org/247223003/diff/190001/chrome/test/data/firefo...
File chrome/test/data/firefox3_profile/README (right):

https://codereview.chromium.org/247223003/diff/190001/chrome/test/data/firefo...
chrome/test/data/firefox3_profile/README:20: path. The actual app or profile
path varies with operating system. To make
On 2014/06/06 04:15:42, Ilya Sherman wrote:
> Do the app and profile paths vary by operating system, even in the test setup?

> Looking more closely at the test code, it looks like SetUp() creates a
temporary
> directory, and profile and app directories within it.  Do these app and
profile
> paths not get used?

You are right. [app] and [profle] should not vary by operating system in test
setup.
Update test code to make use of app and profile paths.

https://codereview.chromium.org/247223003/diff/190001/chrome/test/data/firefo...
chrome/test/data/firefox3_profile/README:25: [profile] =>
./chrome/test/data/firefox3_profile/searchplugins/installed
On 2014/06/06 04:15:42, Ilya Sherman wrote:
> What is this path relative to?  Since the test code copies all of the files
over
> to a temporary directory, it seems unnecessary to have the
> "chrome/test/data/firefox3_profile" prefix.

The path was relative to /src. But now I made use of temporary directory.

https://codereview.chromium.org/247223003/diff/190001/chrome/test/data/firefo...
chrome/test/data/firefox3_profile/README:25: [profile] =>
./chrome/test/data/firefox3_profile/searchplugins/installed
On 2014/06/06 04:15:42, Ilya Sherman wrote:
> Also, I've realized that the test code expects the search engine XML files to
> all live in a common location.  Rather than adding them once to
> //chrome/test/data/firefox3_profile/searchplugins and again to
> //chrome/test/data/firefox_profile/searchplugins, you could add them to
> //chrome/test/data/firefox_searchplugins.  You'd then have the test code copy
> over the contents of this directory to {tempdir}/app/searchplugins.  Does that
> make sense?

Completely agree with you. All search engine XML files are placed in
//chrome/test/data/firefox_searchplugins and updated test code
accordingly.

[Ilya Sherman, 2014-06-17 23:26:42]

Thanks, Tapu!  The tests now pass on my local machine (except that they
sometimes time out instead... I'm not sure whether that's an issue with the
tests, or with my testing over ssh).

Since you're not modifying any binary files anymore, I think it might actually
be possible to land this CL via the commit queue.  I've kicked off another set
of try jobs, and have what should hopefully be a final round of comments for you
to address.  Then, if the try runs look good, we should be good to go! :)

https://codereview.chromium.org/247223003/diff/220001/chrome/browser/importer...
File chrome/browser/importer/firefox_importer_browsertest.cc (right):

https://codereview.chromium.org/247223003/diff/220001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:204: bool
import_search_engines_;
With your improvements to the tests, is it possible to now simply always test
search engine import, and remove this variable?

https://codereview.chromium.org/247223003/diff/220001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:231: bool
import_search_plugins) {
Ditto for this variable.

https://codereview.chromium.org/247223003/diff/220001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:245:
base::CreateDirectory(default_search_engine_path);
nit: I'd condense these five lines a bit, like so:

    // Create a directory to house default search engines.
    base::FilePath default_search_engines_path =
        app_path_.AppendASCII("searchplugins");
    base::CreateDirectory(default_search_engines_path);

https://codereview.chromium.org/247223003/diff/220001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:250:
custom_search_engine_path.AppendASCII("searchplugins");
Are you intentionally not calling base::CreateDirectory() for this path?

https://codereview.chromium.org/247223003/diff/220001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:260:
custom_search_engine_source_path.AppendASCII("custom");
I'd similarly condense these six lines down to four.

https://codereview.chromium.org/247223003/diff/220001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:284:
make_scoped_refptr(writer).get());
The more I look at the pre-existing test code, the more mystified I am.  This
"make_scoped_refptr(writer).get()" bit should almost certainly be replaced just
with "writer".  Mind making that change while you're here?

[Tapu Ghose, 2014-06-18 12:16:42]

PTAL

https://codereview.chromium.org/247223003/diff/220001/chrome/browser/importer...
File chrome/browser/importer/firefox_importer_browsertest.cc (right):

https://codereview.chromium.org/247223003/diff/220001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:204: bool
import_search_engines_;
On 2014/06/17 23:26:42, Ilya Sherman wrote:
> With your improvements to the tests, is it possible to now simply always test
> search engine import, and remove this variable?

I agree with you.

https://codereview.chromium.org/247223003/diff/220001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:250:
custom_search_engine_path.AppendASCII("searchplugins");
On 2014/06/17 23:26:42, Ilya Sherman wrote:
> Are you intentionally not calling base::CreateDirectory() for this path?

It was unintentional.

https://codereview.chromium.org/247223003/diff/220001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:284:
make_scoped_refptr(writer).get());
On 2014/06/17 23:26:42, Ilya Sherman wrote:
> The more I look at the pre-existing test code, the more mystified I am.  This
> "make_scoped_refptr(writer).get()" bit should almost certainly be replaced
just
> with "writer".  Mind making that change while you're here?

Not a problem...I will make the change as you suggested.

[Tapu Ghose, 2014-06-18 12:53:51]

Hi Ilya,

//chrome/test/data/firefox_profile holds 6 binary files (*.db, *.sqlite). I
was wondering if they are causing test failures
(FirefoxProfileImporterBrowserTest.FirefoxImporter) in try jobs.

Cheers,
~Tapu.


On Wed, Jun 18, 2014 at 8:16 AM, <ghose.tapu@gmail.com> wrote:

> PTAL
>
>
>
> https://codereview.chromium.org/247223003/diff/220001/
> chrome/browser/importer/firefox_importer_browsertest.cc
> File chrome/browser/importer/firefox_importer_browsertest.cc (right):
>
> https://codereview.chromium.org/247223003/diff/220001/
> chrome/browser/importer/firefox_importer_browsertest.cc#newcode204
> chrome/browser/importer/firefox_importer_browsertest.cc:204: bool
> import_search_engines_;
> On 2014/06/17 23:26:42, Ilya Sherman wrote:
>
>> With your improvements to the tests, is it possible to now simply
>>
> always test
>
>> search engine import, and remove this variable?
>>
>
> I agree with you.
>
>
> https://codereview.chromium.org/247223003/diff/220001/
> chrome/browser/importer/firefox_importer_browsertest.cc#newcode250
> chrome/browser/importer/firefox_importer_browsertest.cc:250:
> custom_search_engine_path.AppendASCII("searchplugins");
> On 2014/06/17 23:26:42, Ilya Sherman wrote:
>
>> Are you intentionally not calling base::CreateDirectory() for this
>>
> path?
>
> It was unintentional.
>
>
> https://codereview.chromium.org/247223003/diff/220001/
> chrome/browser/importer/firefox_importer_browsertest.cc#newcode284
> chrome/browser/importer/firefox_importer_browsertest.cc:284:
> make_scoped_refptr(writer).get());
> On 2014/06/17 23:26:42, Ilya Sherman wrote:
>
>> The more I look at the pre-existing test code, the more mystified I
>>
> am.  This
>
>> "make_scoped_refptr(writer).get()" bit should almost certainly be
>>
> replaced just
>
>> with "writer".  Mind making that change while you're here?
>>
>
> Not a problem...I will make the change as you suggested.
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2014-06-19 00:49:16]

On 2014/06/18 12:53:51, Tapu wrote:
> //chrome/test/data/firefox_profile holds 6 binary files (*.db, *.sqlite). I
> was wondering if they are causing test failures
> (FirefoxProfileImporterBrowserTest.FirefoxImporter) in try jobs.

Yes, that seems likely to be the case, since that's the only test that's failing
now.  Could you please email me the binary files?  I can then manually commit
those as a separate CL.

https://codereview.chromium.org/247223003/diff/240001/chrome/browser/importer...
File chrome/browser/importer/firefox_importer_browsertest.cc (right):

https://codereview.chromium.org/247223003/diff/240001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:108: }
Please remove this constructor entirely.

https://codereview.chromium.org/247223003/diff/240001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:247: // Import search
engines
nit: "Import" -> "Copy over"

https://codereview.chromium.org/247223003/diff/240001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:247: // Import search
engines
nit: Please end the sentence with a period.

https://codereview.chromium.org/247223003/diff/240001/chrome/browser/importer...
chrome/browser/importer/firefox_importer_browsertest.cc:297:
IN_PROC_BROWSER_TEST_F(FirefoxProfileImporterBrowserTest, FirefoxImporter) {
After looking at the tests more closely, I think you will need the
MAYBE_IMPORTER macro for this test after all.  The search engine importing logic
might work correctly across all platforms now, but other importing logic still
does not, which is likely to make this test fail where it shouldn't.

[Tapu Ghose, 2014-06-19 01:20:31]

Hi Ilya,

Please find the attached binary files.

Cheers,
~Tapu.


On Wed, Jun 18, 2014 at 8:49 PM, <isherman@chromium.org> wrote:

> On 2014/06/18 12:53:51, Tapu wrote:
>
>> //chrome/test/data/firefox_profile holds 6 binary files (*.db,
>> *.sqlite). I
>> was wondering if they are causing test failures
>> (FirefoxProfileImporterBrowserTest.FirefoxImporter) in try jobs.
>>
>
> Yes, that seems likely to be the case, since that's the only test that's
> failing
> now.  Could you please email me the binary files?  I can then manually
> commit
> those as a separate CL.
>
>
> https://codereview.chromium.org/247223003/diff/240001/
> chrome/browser/importer/firefox_importer_browsertest.cc
> File chrome/browser/importer/firefox_importer_browsertest.cc (right):
>
> https://codereview.chromium.org/247223003/diff/240001/
> chrome/browser/importer/firefox_importer_browsertest.cc#newcode108
> chrome/browser/importer/firefox_importer_browsertest.cc:108: }
> Please remove this constructor entirely.
>
> https://codereview.chromium.org/247223003/diff/240001/
> chrome/browser/importer/firefox_importer_browsertest.cc#newcode247
> chrome/browser/importer/firefox_importer_browsertest.cc:247: // Import
> search engines
> nit: "Import" -> "Copy over"
>
> https://codereview.chromium.org/247223003/diff/240001/
> chrome/browser/importer/firefox_importer_browsertest.cc#newcode247
> chrome/browser/importer/firefox_importer_browsertest.cc:247: // Import
> search engines
> nit: Please end the sentence with a period.
>
> https://codereview.chromium.org/247223003/diff/240001/
> chrome/browser/importer/firefox_importer_browsertest.cc#newcode297
> chrome/browser/importer/firefox_importer_browsertest.cc:297:
> IN_PROC_BROWSER_TEST_F(FirefoxProfileImporterBrowserTest,
> FirefoxImporter) {
> After looking at the tests more closely, I think you will need the
> MAYBE_IMPORTER macro for this test after all.  The search engine
> importing logic might work correctly across all platforms now, but other
> importing logic still does not, which is likely to make this test fail
> where it shouldn't.
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2014-06-19 22:58:19]

Thanks, Tapu.  I've landed the binary files as
http://src.chromium.org/viewvc/chrome?revision=278513&view=revision

[Tapu Ghose, 2014-06-21 01:28:44]

Thanks Ilya. I am looking forward to see how the test goes now.

Cheers,
~Tapu.


On Thu, Jun 19, 2014 at 6:58 PM, <isherman@chromium.org> wrote:

> Thanks, Tapu.  I've landed the binary files as
> http://src.chromium.org/viewvc/chrome?revision=278513&view=revision
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2014-06-24 07:02:05]

I've kicked off another set of try jobs, but I think there's a good chance that
you'll need to rebase and upload a new patch set in order for the try bots to
pick up the binary files that I landed.

[Tapu Ghose, 2014-06-24 12:25:41]

Hello Ilya,

I will rebase and upload a new patch this evening.

Cheers,

~Tapu.


On Tue, Jun 24, 2014 at 3:02 AM, <isherman@chromium.org> wrote:

> I've kicked off another set of try jobs, but I think there's a good chance
> that
> you'll need to rebase and upload a new patch set in order for the try bots
> to
> pick up the binary files that I landed.
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tapu Ghose, 2014-06-26 04:40:46]

On 2014/06/24 12:25:41, Tapu wrote:
> Hello Ilya,
> 
> I will rebase and upload a new patch this evening.
> 
> Cheers,
> 
> ~Tapu.
> 
> 
> On Tue, Jun 24, 2014 at 3:02 AM, <mailto:isherman@chromium.org> wrote:
> 
> > I've kicked off another set of try jobs, but I think there's a good chance
> > that
> > you'll need to rebase and upload a new patch set in order for the try bots
> > to
> > pick up the binary files that I landed.
> >
> > https://codereview.chromium.org/247223003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Hi Ilya,

I have rebased my working branch and uploaded a new patch. I had few issues
after rebasing
yesterday. My local build was failing due to some error introduced by another
patch.
However, the author or reviewer of the patch fixed it lately. Finally, I got my
build working.
I am expecting firefox browser tests to be passed in the try bots.

Thank you.

~Tapu.

[Ilya Sherman, 2014-06-27 06:01:39]

Thanks, Tapu.  I've kicked off another set of try jobs.

[Tapu Ghose, 2014-06-27 21:45:05]

Hi Illya,

Looks like try jobs failed for firefox importer test :(

The tests pass in my local machine. To run the tests I ran the following
command:

./browser_tests --gtest_filter=FirefoxProfileImporterBrowserTest.*

I was wondering if I need to run some other tests too. Any other
suggestions will be appreciated. I would love to get it done.

Thanks.

~ Tapu.


On Fri, Jun 27, 2014 at 2:01 AM, <isherman@chromium.org> wrote:

> Thanks, Tapu.  I've kicked off another set of try jobs.
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2014-06-27 22:13:54]

On 2014/06/27 21:45:05, Tapu wrote:
> Hi Illya,
> 
> Looks like try jobs failed for firefox importer test :(
> 
> The tests pass in my local machine. To run the tests I ran the following
> command:
> 
> ./browser_tests --gtest_filter=FirefoxProfileImporterBrowserTest.*

Hmm, that seems like the correct command.  You're developing on Linux, right? 
It's surprising that the tests pass on your local machine, but fail on the
trybots for the same OS...

It's possible that there is a race condition in the tests -- I think I remember
seeing the test sometimes pass and sometimes fail when I ran it locally.  What
do you see if you run the following command, which runs the tests repeatedly
until a failure is seen?

./browser_tests --gtest_filter=FirefoxProfileImporterBrowserTest.*
--gtest_repeat=-1 --gtest_break_on_failure

I'll try to find some time soon to take a look to see whether the tests pass on
my local machine.  In the meantime, you might want to try sprinkling in some
LOG(ERROR) statements, so that you can see what's causing the test to fail on
the bots.

Thanks for your patience on this CL, and apologies that it's taking so long to
get your changes landed.  Test coverage is very important to ensure that your
fixes remain valid -- because there's a lot of moving parts in the Chromium code
base, untested code tends to pretty quickly accidentally become broken. 
However, it's very unusual to hit so many problems with getting the tests to
work, especially when they seem to run fine on your local computer.  I really
appreciate you continuing to chip away at this!

[Tapu Ghose, 2014-06-27 22:46:10]

Yes, I am developing on linux. In the search.json
(//src/chrome/test/data/firefox_profile/search.json) I have used relative
path for "filePath" attribute. The paths are relative to the //src
directory. I am not sure if the relative path is failing to find xml files
for firefox searchplugins. I will play with it in the mean time.


On Fri, Jun 27, 2014 at 6:13 PM, <isherman@chromium.org> wrote:

> On 2014/06/27 21:45:05, Tapu wrote:
>
>> Hi Illya,
>>
>
>  Looks like try jobs failed for firefox importer test :(
>>
>
>  The tests pass in my local machine. To run the tests I ran the following
>> command:
>>
>
>  ./browser_tests --gtest_filter=FirefoxProfileImporterBrowserTest.*
>>
>
> Hmm, that seems like the correct command.  You're developing on Linux,
> right?
> It's surprising that the tests pass on your local machine, but fail on the
> trybots for the same OS...
>
> It's possible that there is a race condition in the tests -- I think I
> remember
> seeing the test sometimes pass and sometimes fail when I ran it locally.
>  What
> do you see if you run the following command, which runs the tests
> repeatedly
> until a failure is seen?
>
> ./browser_tests --gtest_filter=FirefoxProfileImporterBrowserTest.*
> --gtest_repeat=-1 --gtest_break_on_failure
>
> I'll try to find some time soon to take a look to see whether the tests
> pass on
> my local machine.  In the meantime, you might want to try sprinkling in
> some
> LOG(ERROR) statements, so that you can see what's causing the test to fail
> on
> the bots.
>
> Thanks for your patience on this CL, and apologies that it's taking so
> long to
> get your changes landed.  Test coverage is very important to ensure that
> your
> fixes remain valid -- because there's a lot of moving parts in the
> Chromium code
> base, untested code tends to pretty quickly accidentally become broken.
> However, it's very unusual to hit so many problems with getting the tests
> to
> work, especially when they seem to run fine on your local computer.  I
> really
> appreciate you continuing to chip away at this!
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tapu Ghose, 2014-06-28 01:42:35]

Hi Ilya,

Since tests are passing in my linux machine but failing in try bots, I have
used, instead of relative paths, [app] and [profile] for |filePath| attribute in
search.json. Accordingly, I have updated
//src/chrome/utility/importer/firefox_importer.cc.

In reality, search.json does not use [app] or [profile] as part of |filePath|
attribute. I have used them for sake of test cases to be run properly. As you
know, a temporary directory gets created during test setup, which in turns holds
searchplugins. Therefore, I thought to make use of these searchplugins instead
of using plugins from |firefox_searchplugins| directory.

Please let me know if I have gone with wrong approach.

Thank you.

~ Tapu.

[Tapu Ghose, 2014-06-28 01:43:28]

PTAL

[Ilya Sherman, 2014-07-01 04:24:18]

Hooray, the try jobs are passing with your latest changes!  Please address the
outstanding comments on Patch Set 14, and then this CL should be good to go :D
:D

[Tapu Ghose, 2014-07-01 12:36:38]

On 2014/07/01 04:24:18, Ilya Sherman wrote:
> Hooray, the try jobs are passing with your latest changes!  Please address the
> outstanding comments on Patch Set 14, and then this CL should be good to go :D
> :D

Hi Ilya,

It was great to see that try jobs were passing :)

I have uploaded another patch (17) by addressing overlooked comments that you
made
on patch set 14.

Cheers,

~ Tapu.

[Ilya Sherman, 2014-07-01 17:12:01]

LGTM.  Thanks very much for your patience and persistence working on this issue,
Tapu!

[Ilya Sherman, 2014-07-01 17:12:09]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-07-01 17:13:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ghose.tapu@gmail.com/247223003/300001

[Tapu Ghose, 2014-07-01 17:15:55]

Thanks Ilya. I appreciate all your co-operations and guidance through out
this issue.


On Tue, Jul 1, 2014 at 1:12 PM, <isherman@chromium.org> wrote:

> LGTM.  Thanks very much for your patience and persistence working on this
> issue,
> Tapu!
>
> https://codereview.chromium.org/247223003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-07-01 19:33:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-01 19:36:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-01 19:36:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Ilya Sherman, 2014-07-01 19:40:14]

Committing with NOTRY=true because the presubmit check is incorrectly triggering
for test data files.

[Ilya Sherman, 2014-07-01 19:40:21]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-07-01 19:41:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ghose.tapu@gmail.com/247223003/300001

[commit-bot: I haz the power, 2014-07-01 19:47:40]

Change committed as 280870