Replace xmllite with libxml in autofill

Replace xmllite with libxml in autofill

This CL switches the XML parsing and writing code from using xmllite to libxml.
Other mentionable changes:
 * UTF-8 encoding is dropped from the generated XML prolog. UTF8 is the default [1].
 * Tests are changed to drop any expectations about the parsed data if the parser
   signals an error. This conforms with how parsing is and should be used in production.
 * Some const string references in arguments were changed to value passing, to make
   use of the now allowed std::move.

[1] http://www.w3schools.com/xml/xml_syntax.asp

BUG=556433
TBR=isherman@chromium.org

Committed: https://crrev.com/efef7d0416f2797495e381a20358314b33b34eb0
Cr-Commit-Position: refs/heads/master@{#361862}

[vabr (Chromium), 2015-11-25 16:57:18]

vabr@chromium.org changed reviewers:
+ mathp@chromium.org

[vabr (Chromium), 2015-11-25 16:57:19]

Hi Mathieu,

Could I bother you with yet another CL today?

Thanks!
Vaclav

[vabr (Chromium), 2015-11-25 16:58:47]

vabr@chromium.org changed reviewers:
+ scottmg@chromium.org

[vabr (Chromium), 2015-11-25 16:58:48]

Scott,
Could you review adding "+third_party/libxml" to
components/autofill/core/browser/DEPS?

Thanks!
Vaclav

[scottmg, 2015-11-25 17:55:41]

+deps lgtm

[Mathieu, 2015-11-25 17:59:10]

lgtm with a few nits. 

Thanks for doing this work!

https://codereview.chromium.org/1477733003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_xml_parser.cc (right):

https://codereview.chromium.org/1477733003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_xml_parser.cc:7: #include <string.h>
Is it advised to #include <utility> for std::move?

https://codereview.chromium.org/1477733003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_xml_parser.cc:102: if
(!reader.NodeAttribute("positiveuploadrate", &attribute_value))
Can you put a TODO in my name to update this to be non-required? Both are set to
1 on the server side and have been for a while. We should update the default in
client code to 1 and only include it in the response if it's different than 1.
crbug.com/561619

https://codereview.chromium.org/1477733003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/1477733003/diff/40001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:584: return false;
nit: please add new line

[vabr (Chromium), 2015-11-26 09:36:12]

Thanks, Mathieu, for the review!

I addressed your comments, and will be landing soon.

Cheers,
Vaclav

https://codereview.chromium.org/1477733003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_xml_parser.cc (right):

https://codereview.chromium.org/1477733003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_xml_parser.cc:7: #include <string.h>
On 2015/11/25 17:59:10, Mathieu Perreault wrote:
> Is it advised to #include <utility> for std::move?

Good point, thanks!
Also, removed <string.h>, because that is the C library header.
autofill_xml_parser.h already #includes <string> for std::string, and I don't
think <cstring> (the C++ equivalent of <string.h> is needed here, any more).

https://codereview.chromium.org/1477733003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_xml_parser.cc:102: if
(!reader.NodeAttribute("positiveuploadrate", &attribute_value))
On 2015/11/25 17:59:10, Mathieu Perreault wrote:
> Can you put a TODO in my name to update this to be non-required? Both are set
to
> 1 on the server side and have been for a while. We should update the default
in
> client code to 1 and only include it in the response if it's different than 1.
> crbug.com/561619

Sure, thanks for pointing this out!
Instead of TODO(mathp) I used TODO(crbug.com/561619) which has recently been set
the only encouraged format on iOS and supported in the rest of Chromium as well.
It prevents stale references when people change projects. Also, while
crbug.com/561619 is not clickable, TODO(http://crbug.com/561619) would create an
invalid link to "http://crbug.com/561619)" in XCode, so the preference was made
towards non-clickable.

https://codereview.chromium.org/1477733003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/1477733003/diff/40001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:584: return false;
On 2015/11/25 17:59:10, Mathieu Perreault wrote:
> nit: please add new line 

Done.

[vabr (Chromium), 2015-11-26 09:45:22]

vabr@chromium.org changed reviewers:
+ isherman@chromium.org

[vabr (Chromium), 2015-11-26 09:45:23]

Ilya,

I'm going to TBR you for chrome/browser/autofill/autofill_server_browsertest.cc,
based on the following:

* The change is trivial, adjusting expectations in tests.
* the change was reviewed.
* The same change happens in other tests, which were reviewed (by Mathieu), and
within my ownership.
* The change in the production code requiring the expectation change was
properly reviewed as well.
* The current U.S. holidays would make this CL is block making GN work on iOS
and removing big libraries from iOS build for several days.

If you tell me this is not acceptable, I won't do it in the future. I was trying
to prepare in advance with https://codereview.chromium.org/1474993002/, but
missed that the browsertest here is in a yet different directory. Will send you
and Evan an e-mail about this.

Thanks!
Vaclav

[vabr (Chromium), 2015-11-26 09:46:04]

Description was changed from

==========
Replace xmllite with libxml in autofill

This CL switches the XML parsing and writing code from using xmllite to libxml.
Other mentionable changes:
 * UTF-8 encoding is dropped from the generated XML prolog. UTF8 is the default
[1].
 * Tests are changed to drop any expectations about the parsed data if the
parser
   signals an error. This conforms with how parsing is and should be used in
production.
 * Some const string references in arguments were changed to value passing, to
make
   use of the now allowed std::move.

[1] http://www.w3schools.com/xml/xml_syntax.asp

BUG=556433
==========

to

==========
Replace xmllite with libxml in autofill

This CL switches the XML parsing and writing code from using xmllite to libxml.
Other mentionable changes:
 * UTF-8 encoding is dropped from the generated XML prolog. UTF8 is the default
[1].
 * Tests are changed to drop any expectations about the parsed data if the
parser
   signals an error. This conforms with how parsing is and should be used in
production.
 * Some const string references in arguments were changed to value passing, to
make
   use of the now allowed std::move.

[1] http://www.w3schools.com/xml/xml_syntax.asp

BUG=556433
TBR=isherman@chromium.org
==========

[vabr (Chromium), 2015-11-26 09:46:29]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2015-11-26 09:46:30]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1477733003/#ps80001
(title: "Just rebased")

[commit-bot: I haz the power, 2015-11-26 09:46:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477733003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477733003/80001

[commit-bot: I haz the power, 2015-11-26 10:12:53]

Description was changed from

==========
Replace xmllite with libxml in autofill

This CL switches the XML parsing and writing code from using xmllite to libxml.
Other mentionable changes:
 * UTF-8 encoding is dropped from the generated XML prolog. UTF8 is the default
[1].
 * Tests are changed to drop any expectations about the parsed data if the
parser
   signals an error. This conforms with how parsing is and should be used in
production.
 * Some const string references in arguments were changed to value passing, to
make
   use of the now allowed std::move.

[1] http://www.w3schools.com/xml/xml_syntax.asp

BUG=556433
TBR=isherman@chromium.org
==========

to

==========
Replace xmllite with libxml in autofill

This CL switches the XML parsing and writing code from using xmllite to libxml.
Other mentionable changes:
 * UTF-8 encoding is dropped from the generated XML prolog. UTF8 is the default
[1].
 * Tests are changed to drop any expectations about the parsed data if the
parser
   signals an error. This conforms with how parsing is and should be used in
production.
 * Some const string references in arguments were changed to value passing, to
make
   use of the now allowed std::move.

[1] http://www.w3schools.com/xml/xml_syntax.asp

BUG=556433
TBR=isherman@chromium.org
==========

[commit-bot: I haz the power, 2015-11-26 10:12:54]

Committed patchset #5 (id:80001)

[sdefresne, 2015-11-26 10:13:30]

On 2015/11/26 at 10:12:54, commit-bot wrote:
> Committed patchset #5 (id:80001)

Thanks a lot for taking care of that migration!

[commit-bot: I haz the power, 2015-11-26 10:13:57]

Description was changed from

==========
Replace xmllite with libxml in autofill

This CL switches the XML parsing and writing code from using xmllite to libxml.
Other mentionable changes:
 * UTF-8 encoding is dropped from the generated XML prolog. UTF8 is the default
[1].
 * Tests are changed to drop any expectations about the parsed data if the
parser
   signals an error. This conforms with how parsing is and should be used in
production.
 * Some const string references in arguments were changed to value passing, to
make
   use of the now allowed std::move.

[1] http://www.w3schools.com/xml/xml_syntax.asp

BUG=556433
TBR=isherman@chromium.org
==========

to

==========
Replace xmllite with libxml in autofill

This CL switches the XML parsing and writing code from using xmllite to libxml.
Other mentionable changes:
 * UTF-8 encoding is dropped from the generated XML prolog. UTF8 is the default
[1].
 * Tests are changed to drop any expectations about the parsed data if the
parser
   signals an error. This conforms with how parsing is and should be used in
production.
 * Some const string references in arguments were changed to value passing, to
make
   use of the now allowed std::move.

[1] http://www.w3schools.com/xml/xml_syntax.asp

BUG=556433
TBR=isherman@chromium.org

Committed: https://crrev.com/efef7d0416f2797495e381a20358314b33b34eb0
Cr-Commit-Position: refs/heads/master@{#361862}
==========

[commit-bot: I haz the power, 2015-11-26 10:13:58]

Patchset 5 (id:??) landed as
https://crrev.com/efef7d0416f2797495e381a20358314b33b34eb0
Cr-Commit-Position: refs/heads/master@{#361862}

[vabr (Chromium), 2015-11-26 10:40:10]

On 2015/11/26 10:13:30, sdefresne wrote:
> On 2015/11/26 at 10:12:54, commit-bot wrote:
> > Committed patchset #5 (id:80001)
> 
> Thanks a lot for taking care of that migration!

My pleasure! :) Thanks for pointing this xml dependency out, and your
consultation on the bug!