Fix Firefox bookmarks import.

Fix Firefox bookmarks import.

Firefox abandoned usage of the moz_bookmarks_roots table since v. 30 and
removed the table in v. 31 in favor of storing relevant info in the
'guid' column of the moz_bookmarks table.

BUG=638977
Committed: https://crrev.com/8a1ed3436f171ff0666d15d996ee033928c7dbf0
Cr-Commit-Position: refs/heads/master@{#436262}

[kszatan, 2016-08-30 08:31:25]

Description was changed from

==========
Fix Firefox bookmarks import.

Firefox abandoned usage of the moz_bookmarks_roots table since v. 30 and
removed the table in v. 31 in favor of storing relevant info in the
'guid' column of the moz_bookmarks table.

BUG=638977
==========

to

==========
Fix Firefox bookmarks import.

Firefox abandoned usage of the moz_bookmarks_roots table since v. 30 and
removed the table in v. 31 in favor of storing relevant info in the
'guid' column of the moz_bookmarks table.

BUG=638977
==========

[kszatan, 2016-08-30 08:31:25]

kszatan@opera.com changed reviewers:
+ gab@chromium.org

[kszatan, 2016-08-30 08:32:17]

kszatan@opera.com changed reviewers:
+ isherman@chromium.org

[kszatan, 2016-08-30 08:32:45]

ptal

[Ilya Sherman, 2016-08-30 20:52:33]

Hi, thanks for the contribution!  Could you please add test coverage for your
changes?  Otherwise this looks great =)

[kszatan, 2016-09-08 13:21:38]

On 2016/08/30 20:52:33, Ilya Sherman (Away Sep 9-20) wrote:
> Hi, thanks for the contribution!  Could you please add test coverage for your
> changes?  Otherwise this looks great =)

It seems I was too quick with those changes. To make it work robustly I have to
take places.sqlite schema version into account. The changes in patch set 1 will
work with schema version 25, which means Firefox 36 and later. I'll try to make
it work also with earlier schema versions and add appropriate unit tests.

[Ilya Sherman, 2016-09-08 20:33:18]

On 2016/09/08 13:21:38, kszatan wrote:
> On 2016/08/30 20:52:33, Ilya Sherman (Away Sep 9-20) wrote:
> > Hi, thanks for the contribution!  Could you please add test coverage for
your
> > changes?  Otherwise this looks great =)
> 
> It seems I was too quick with those changes. To make it work robustly I have
to
> take places.sqlite schema version into account. The changes in patch set 1
will
> work with schema version 25, which means Firefox 36 and later. I'll try to
make
> it work also with earlier schema versions and add appropriate unit tests.

Okay.  Do we know whether there are many users who use a version of Firefox
earlier than 36?  It's probably worth dropping support for older versions at
some point.

[kszatan, 2016-09-09 09:00:58]

On 2016/09/08 20:33:18, Ilya Sherman (Away Sep 9-20) wrote:
> Okay.  Do we know whether there are many users who use a version of Firefox
> earlier than 36?  It's probably worth dropping support for older versions at
> some point.
I don't know how many such users are out there. Firefox imports bookmarks
starting from schema version 11 of places.sqlite. I think I'll do the same
because probably it's only a matter of merging my changes with a previous method
of importing bookmarks, so it's not that much work.

[kszatan, 2016-09-14 09:58:35]

On 2016/09/09 09:00:58, kszatan wrote:
> On 2016/09/08 20:33:18, Ilya Sherman (Away Sep 9-20) wrote:
> > Okay.  Do we know whether there are many users who use a version of Firefox
> > earlier than 36?  It's probably worth dropping support for older versions at
> > some point.
> I don't know how many such users are out there. Firefox imports bookmarks
> starting from schema version 11 of places.sqlite. I think I'll do the same
> because probably it's only a matter of merging my changes with a previous
method
> of importing bookmarks, so it's not that much work.

There's a bunch of files in the new patch set but most of them are part of FF
profiles. Most probably not all of them are needed for the testing of import but
they don't weigh much so I kept them.
git cl upload warns about files not being included in GN/GYPI files but both
unit_tests & chrome targets are building correctly.
I added unit tests only for my changes. I would further refactor the code and
move all FF profile related code from firefox_importer.cc to its own class. 
Please take a look and tell me if there's anything more to change.

[kszatan, 2016-09-23 12:24:07]

Any update on this?

[Ilya Sherman, 2016-09-27 01:39:34]

Wow, this CL got a lot more complicated!  Apologies for the delay -- I've been
catching up from being away for some time.

I think this is probably a reasonable approach, but I have a couple of requests
to ease the review burden:
(1) A lot of the files that you're adding as test data do not seem strictly
necessary.  Could you please trim the list down to just the needed files?
(2) It looks like you're moving some existing code into places_factory, and then
also adding new code.  Could you please split this up into two (or more) CLs --
separating the work of moving existing code (without making any other changes to
it), vs. adding new code?  I wish our code review tool were better, such that it
made this easier to follow in a single CL... but unfortunately I'm not aware of
such functionality in the Chromium code review tool today.

Thanks!

[kszatan, 2016-09-29 06:26:34]

I'm not really familiar with Chromium code review either but will try to split
it into 2 CLs in a few days.

[gab, 2016-10-21 15:03:36]

gab@chromium.org changed reviewers:
- gab@chromium.org

[kszatan, 2016-10-26 10:09:05]

This is the minimal version fixing the bug with tests. I'll make refactoring in
another CL. PTAL.

[Ilya Sherman, 2016-10-26 22:55:59]

Thanks!  Looks pretty good -- just a few nits:

https://codereview.chromium.org/2296633002/diff/100001/chrome/common/importer...
File chrome/common/importer/mock_importer_bridge.h (right):

https://codereview.chromium.org/2296633002/diff/100001/chrome/common/importer...
chrome/common/importer/mock_importer_bridge.h:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
nit: Please omit "(c)" from this line.

https://codereview.chromium.org/2296633002/diff/100001/chrome/common/importer...
chrome/common/importer/mock_importer_bridge.h:45: std::vector<ImporterURLRow>
history;
Hmm, where are these three lines used?  It looks like these could easily be
local variables in tests, rather than part of the mock header's interface.

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
File chrome/utility/importer/firefox_importer_unittest.cc (right):

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:130:
scoped_refptr<FirefoxImporter> ff_importer = new FirefoxImporter;
nit: s/ff_importer/importer

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:141: EXPECT_CALL(*bridge,
NotifyEnded());
Could you use a NiceMock to omit most of these EXPECT_CALLs?

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:143: EXPECT_EQ(6,
bridge->bookmarks.size());
nit: Please make this an ASSERT_EQ, since the lines below could read
out-of-bounds if this fails.

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:145:
bridge->bookmarks[0].url.spec().c_str());
nit: Could you use EXPECT_EQ and drop the ".c_str()" call?

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:156: EXPECT_EQ(5,
bridge->favicons.size());
nit: Please make this an ASSERT_EQ, since the lines below could read
out-of-bounds if this fails.

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:194: }
Same nits apply to this test as well =)

[kszatan, 2016-10-27 08:43:38]

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
File chrome/utility/importer/firefox_importer_unittest.cc (right):

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:141: EXPECT_CALL(*bridge,
NotifyEnded());
On 2016/10/26 22:55:59, Ilya Sherman wrote:
> Could you use a NiceMock to omit most of these EXPECT_CALLs?

I don't think so because I really want to check that these calls are executed
and with correct arguments.

[Ilya Sherman, 2016-10-27 22:00:26]

Thanks!  LGTM % a final couple of nits.  Also, for future reference, please mark
comments that you've addressed as "Done" -- this is helpful for me keeping track
of the code review progress.

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
File chrome/utility/importer/firefox_importer_unittest.cc (right):

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:141: EXPECT_CALL(*bridge,
NotifyEnded());
On 2016/10/27 08:43:36, kszatan wrote:
> On 2016/10/26 22:55:59, Ilya Sherman wrote:
> > Could you use a NiceMock to omit most of these EXPECT_CALLs?
> 
> I don't think so because I really want to check that these calls are executed
> and with correct arguments.

Okay, fair enough.

https://codereview.chromium.org/2296633002/diff/120001/chrome/utility/importe...
File chrome/utility/importer/firefox_importer_unittest.cc (right):

https://codereview.chromium.org/2296633002/diff/120001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:135:
favicon_base::FaviconUsageDataList favicons;
nit: Please #include the appropriate headers for these in this file.

https://codereview.chromium.org/2296633002/diff/120001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:139:
.WillOnce(::testing::SaveArg<0>(&(bookmarks)));
nit: No need for the parens around bookmarks; ditto below.

[kszatan, 2016-11-02 08:17:34]

Fixed. Sorry for the delays but I'm working actively on other things also.

https://codereview.chromium.org/2296633002/diff/100001/chrome/common/importer...
File chrome/common/importer/mock_importer_bridge.h (right):

https://codereview.chromium.org/2296633002/diff/100001/chrome/common/importer...
chrome/common/importer/mock_importer_bridge.h:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
On 2016/10/26 22:55:59, Ilya Sherman wrote:
> nit: Please omit "(c)" from this line.

Done.

https://codereview.chromium.org/2296633002/diff/100001/chrome/common/importer...
chrome/common/importer/mock_importer_bridge.h:45: std::vector<ImporterURLRow>
history;
On 2016/10/26 22:55:59, Ilya Sherman wrote:
> Hmm, where are these three lines used?  It looks like these could easily be
> local variables in tests, rather than part of the mock header's interface.

Done.

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
File chrome/utility/importer/firefox_importer_unittest.cc (right):

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:130:
scoped_refptr<FirefoxImporter> ff_importer = new FirefoxImporter;
On 2016/10/26 22:55:59, Ilya Sherman wrote:
> nit: s/ff_importer/importer

Done.

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:143: EXPECT_EQ(6,
bridge->bookmarks.size());
On 2016/10/26 22:55:59, Ilya Sherman wrote:
> nit: Please make this an ASSERT_EQ, since the lines below could read
> out-of-bounds if this fails.

Done.

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:145:
bridge->bookmarks[0].url.spec().c_str());
On 2016/10/26 22:55:59, Ilya Sherman wrote:
> nit: Could you use EXPECT_EQ and drop the ".c_str()" call?

Done.

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:156: EXPECT_EQ(5,
bridge->favicons.size());
On 2016/10/26 22:55:59, Ilya Sherman wrote:
> nit: Please make this an ASSERT_EQ, since the lines below could read
> out-of-bounds if this fails.

Done.

https://codereview.chromium.org/2296633002/diff/100001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:194: }
On 2016/10/26 22:55:59, Ilya Sherman wrote:
> Same nits apply to this test as well =)

Done.

https://codereview.chromium.org/2296633002/diff/120001/chrome/utility/importe...
File chrome/utility/importer/firefox_importer_unittest.cc (right):

https://codereview.chromium.org/2296633002/diff/120001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:135:
favicon_base::FaviconUsageDataList favicons;
On 2016/10/27 22:00:25, Ilya Sherman wrote:
> nit: Please #include the appropriate headers for these in this file.

Done.

https://codereview.chromium.org/2296633002/diff/120001/chrome/utility/importe...
chrome/utility/importer/firefox_importer_unittest.cc:139:
.WillOnce(::testing::SaveArg<0>(&(bookmarks)));
On 2016/10/27 22:00:25, Ilya Sherman wrote:
> nit: No need for the parens around bookmarks; ditto below.

Done.

[kszatan, 2016-11-02 11:08:12]

The CQ bit was checked by kszatan@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 11:08:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 11:08:23]

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

[commit-bot: I haz the power, 2016-11-02 11:08:25]

Dry run: The author kszatan@opera.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[kszatan, 2016-11-02 13:15:20]

The CQ bit was checked by kszatan@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 13:15:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 13:17:32]

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

[commit-bot: I haz the power, 2016-11-02 13:17:33]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[kszatan, 2016-11-02 13:29:26]

I have problems with uploading places.sqlite file. It weighs 1.1M and git
complains:
Not uploading the base file for
chrome/test/data/import/firefox/48.0.2/places.sqlite because it's too large.
Not uploading the current file for
chrome/test/data/import/firefox/48.0.2/places.sqlite because it's too large.

I can gzip the whole profile but I don't know if there's a portable way I can
unzip it automatically for the tests.

[Ilya Sherman, 2016-11-02 20:29:00]

(Still LGTM, thanks!)

On 2016/11/02 13:29:26, kszatan wrote:
> I have problems with uploading places.sqlite file. It weighs 1.1M and git
> complains:
> Not uploading the base file for
> chrome/test/data/import/firefox/48.0.2/places.sqlite because it's too large.
> Not uploading the current file for
> chrome/test/data/import/firefox/48.0.2/places.sqlite because it's too large.
> 
> I can gzip the whole profile but I don't know if there's a portable way I can
> unzip it automatically for the tests.

Hmm, it might require a manual commit rather than using the commit queue.  But,
before we take that route: Is it possible to trim the contents of places.sqlite
to not be so large, without affecting the test results?  1.1M seems excessive
for a test data file.

[kszatan, 2016-11-03 07:53:05]

On 2016/11/02 20:29:00, Ilya Sherman wrote:
> (Still LGTM, thanks!)
> 
> On 2016/11/02 13:29:26, kszatan wrote:
> > I have problems with uploading places.sqlite file. It weighs 1.1M and git
> > complains:
> > Not uploading the base file for
> > chrome/test/data/import/firefox/48.0.2/places.sqlite because it's too large.
> > Not uploading the current file for
> > chrome/test/data/import/firefox/48.0.2/places.sqlite because it's too large.
> > 
> > I can gzip the whole profile but I don't know if there's a portable way I
can
> > unzip it automatically for the tests.
> 
> Hmm, it might require a manual commit rather than using the commit queue. 
But,
> before we take that route: Is it possible to trim the contents of
places.sqlite
> to not be so large, without affecting the test results?  1.1M seems excessive
> for a test data file.

I have already run VACUUM on the database to reduce it from 10M to 1.1M.
Compressed places.sqlite - 12K.

[Ilya Sherman, 2016-11-03 21:19:15]

On 2016/11/03 07:53:05, kszatan wrote:
> On 2016/11/02 20:29:00, Ilya Sherman wrote:
> > (Still LGTM, thanks!)
> > 
> > On 2016/11/02 13:29:26, kszatan wrote:
> > > I have problems with uploading places.sqlite file. It weighs 1.1M and git
> > > complains:
> > > Not uploading the base file for
> > > chrome/test/data/import/firefox/48.0.2/places.sqlite because it's too
large.
> > > Not uploading the current file for
> > > chrome/test/data/import/firefox/48.0.2/places.sqlite because it's too
large.
> > > 
> > > I can gzip the whole profile but I don't know if there's a portable way I
> can
> > > unzip it automatically for the tests.
> > 
> > Hmm, it might require a manual commit rather than using the commit queue. 
> But,
> > before we take that route: Is it possible to trim the contents of
> places.sqlite
> > to not be so large, without affecting the test results?  1.1M seems
excessive
> > for a test data file.
> 
> I have already run VACUUM on the database to reduce it from 10M to 1.1M.
> Compressed places.sqlite - 12K.

Wow, that's an impressive compression ratio!  We do have gzip checked into the
Chromium repo, e.g. under //third_party/zlib.  Would it be possible to use that
to unzip the compressed file into a temp directory, and then run the test within
the context of that decompressed file?

[kszatan, 2016-11-10 11:16:01]

On 2016/11/03 21:19:15, Ilya Sherman wrote:
> On 2016/11/03 07:53:05, kszatan wrote:
> > On 2016/11/02 20:29:00, Ilya Sherman wrote:
> > > (Still LGTM, thanks!)
> > > 
> > > On 2016/11/02 13:29:26, kszatan wrote:
> > > > I have problems with uploading places.sqlite file. It weighs 1.1M and
git
> > > > complains:
> > > > Not uploading the base file for
> > > > chrome/test/data/import/firefox/48.0.2/places.sqlite because it's too
> large.
> > > > Not uploading the current file for
> > > > chrome/test/data/import/firefox/48.0.2/places.sqlite because it's too
> large.
> > > > 
> > > > I can gzip the whole profile but I don't know if there's a portable way
I
> > can
> > > > unzip it automatically for the tests.
> > > 
> > > Hmm, it might require a manual commit rather than using the commit queue. 
> > But,
> > > before we take that route: Is it possible to trim the contents of
> > places.sqlite
> > > to not be so large, without affecting the test results?  1.1M seems
> excessive
> > > for a test data file.
> > 
> > I have already run VACUUM on the database to reduce it from 10M to 1.1M.
> > Compressed places.sqlite - 12K.
> 
> Wow, that's an impressive compression ratio!  We do have gzip checked into the
> Chromium repo, e.g. under //third_party/zlib.  Would it be possible to use
that
> to unzip the compressed file into a temp directory, and then run the test
within
> the context of that decompressed file?


I can see at least two utilities that could help me in this case. In my
depot_tools there are:
* git-2.8.3-64_bin/usr/bin/gunzip
* git-2.8.3-64_bin/usr/bin/unzip.exe
* python276_bin/Lib/zipfile.py

There's also Unzip function in build/util/lib/common/util.py but I don't like
that it relies on a fixed path on Windows.
In build/config/zip.gni there's a template("zip") which can be used to zip files
from GN scripts. I could add build/config/unzip.gni (based on python's
zipfily.py) and then use it in my target. I can see that there are
"copy_test_data" targets in some GN files, that copy test files to the build
directory. The way it would work is this:
1. 'gn gen' is executed.
2. GN encounters "copy_test_data" target/action and unzips profiles to
appropriate directories.
3. Unit tests work on unzipped profiles in the build directory.
Does it sound OK?

[kszatan, 2016-11-14 13:49:01]

The CQ bit was checked by kszatan@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 13:49:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 13:51:11]

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

[commit-bot: I haz the power, 2016-11-14 13:51:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kszatan, 2016-11-16 10:23:04]

The CQ bit was checked by kszatan@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 10:23:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 10:25:54]

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

[commit-bot: I haz the power, 2016-11-16 10:25:55]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kszatan, 2016-11-16 10:43:43]

OK, so I've talked with a few people on #chromium and the say that 1.1M binary
file is fine. But I still have problem with failing try bots. I have no idea
what causes the problem. What now? Should I run 'git cl land" as recommended in
this issue: https://bugs.chromium.org/p/chromium/issues/detail?id=579778?

[Ilya Sherman, 2016-11-21 20:22:42]

On 2016/11/16 10:43:43, kszatan wrote:
> OK, so I've talked with a few people on #chromium and the say that 1.1M binary
> file is fine. But I still have problem with failing try bots. I have no idea
> what causes the problem. What now? Should I run 'git cl land" as recommended
in
> this issue: https://bugs.chromium.org/p/chromium/issues/detail?id=579778?

Sorry, I'm not really any more familiar with this than you are.  If you're
confident that the CL would pass checks other than the file size issue, I'd
recommend (1) first, alerting the sheriffs (on IRC) that you're planning to land
the CL without going through the Commit Queue, (2) then trying 'git cl land',
and (3) being available on email and IRC for an hour or two after the CL lands,
in case there are any issues with it.

[kszatan, 2016-11-22 15:02:01]

From another IRC conversation:

<Torne>you should land *just* the new binary file in a seprate CL by itself
<Torne>first
<Torne>and then use the CQ as normal to land the CL with the acutal code changes

So, I'll prepare another CL just for the binary file and then rebase this CL.

[kszatan, 2016-12-01 09:07:03]

The CQ bit was checked by kszatan@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 09:07:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kszatan, 2016-12-01 09:30:54]

The CQ bit was unchecked by kszatan@opera.com

[kszatan, 2016-12-02 08:36:16]

The CQ bit was checked by kszatan@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 08:36:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 08:51:57]

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

[commit-bot: I haz the power, 2016-12-02 08:52:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kszatan, 2016-12-02 08:56:39]

The CQ bit was checked by kszatan@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 08:56:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 10:04:19]

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

[commit-bot: I haz the power, 2016-12-02 10:04:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kszatan, 2016-12-05 09:39:26]

The CQ bit was checked by kszatan@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 09:39:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 09:57:53]

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

[commit-bot: I haz the power, 2016-12-05 09:57:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kszatan, 2016-12-05 10:11:28]

The CQ bit was checked by kszatan@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 10:11:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 11:12:53]

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

[commit-bot: I haz the power, 2016-12-05 11:12:54]

Dry run: This issue passed the CQ dry run.

[kszatan, 2016-12-05 11:13:13]

The CQ bit was checked by kszatan@opera.com

[kszatan, 2016-12-05 11:13:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2296633002/#ps250001
(title: "build fix")

[commit-bot: I haz the power, 2016-12-05 11:13:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kszatan, 2016-12-05 11:14:24]

The CQ bit was unchecked by kszatan@opera.com

[kszatan, 2016-12-05 11:28:00]

The CQ bit was checked by kszatan@opera.com

[commit-bot: I haz the power, 2016-12-05 11:28:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 11:32:06]

CQ is committing da patch.

Bot data: {"patchset_id": 250001, "attempt_start_ts": 1480937280111010,
"parent_rev": "2cbaabd3b05388a83406962140282a8a46567619", "commit_rev":
"6c52cb2154624285a0ce6c7fa0d3faeba92e296c"}

[commit-bot: I haz the power, 2016-12-05 11:33:09]

Description was changed from

==========
Fix Firefox bookmarks import.

Firefox abandoned usage of the moz_bookmarks_roots table since v. 30 and
removed the table in v. 31 in favor of storing relevant info in the
'guid' column of the moz_bookmarks table.

BUG=638977
==========

to

==========
Fix Firefox bookmarks import.

Firefox abandoned usage of the moz_bookmarks_roots table since v. 30 and
removed the table in v. 31 in favor of storing relevant info in the
'guid' column of the moz_bookmarks table.

BUG=638977
==========

[commit-bot: I haz the power, 2016-12-05 11:33:11]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2016-12-05 11:34:30]

Description was changed from

==========
Fix Firefox bookmarks import.

Firefox abandoned usage of the moz_bookmarks_roots table since v. 30 and
removed the table in v. 31 in favor of storing relevant info in the
'guid' column of the moz_bookmarks table.

BUG=638977
==========

to

==========
Fix Firefox bookmarks import.

Firefox abandoned usage of the moz_bookmarks_roots table since v. 30 and
removed the table in v. 31 in favor of storing relevant info in the
'guid' column of the moz_bookmarks table.

BUG=638977

Committed: https://crrev.com/8a1ed3436f171ff0666d15d996ee033928c7dbf0
Cr-Commit-Position: refs/heads/master@{#436262}
==========

[commit-bot: I haz the power, 2016-12-05 11:34:32]

Patchset 14 (id:??) landed as
https://crrev.com/8a1ed3436f171ff0666d15d996ee033928c7dbf0
Cr-Commit-Position: refs/heads/master@{#436262}

[kszatan, 2016-12-05 11:48:41]

I had to disable existing firefox importer browsertests, which are based on
older profiles. They should be reenabled in
https://codereview.chromium.org/2451223004/.