Moves manifest parsing to a new class, Reader.

Moves manifest parsing to a new class, Reader.

. Consolidate value analysis in Entry
. Catalog now just manages the various data structures that allow it to implement various flavors of Resolve().

BUG=

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

[Ben Goodger (Google), 2016-03-22 22:07:21]

ben@chromium.org changed reviewers:
+ sky@chromium.org

[Ben Goodger (Google), 2016-03-22 22:07:22]

https://codereview.chromium.org/1821383002/diff/40001/mojo/services/catalog/c...
File mojo/services/catalog/catalog.cc (right):

https://codereview.chromium.org/1821383002/diff/40001/mojo/services/catalog/c...
mojo/services/catalog/catalog.cc:123:
mojo::util::FilePathToFileURL(package_path_));
I'm going to convert the parameter to a filepath eventually, I just need to
figure out a good way to send those via mojo.

[sky, 2016-03-22 23:47:46]

https://codereview.chromium.org/1821383002/diff/40001/mojo/services/catalog/e...
File mojo/services/catalog/entry.cc (right):

https://codereview.chromium.org/1821383002/diff/40001/mojo/services/catalog/e...
mojo/services/catalog/entry.cc:205: return other.name_ < name_ ||
other.qualifier_ < qualifier_ ||
Don't you only want to compare subsequent values if the preceding one is equal?
eg:
if (other.name_ < name_)
  return true;
if (other.name_ > name_)
  return false;
if (other.qualifier_ < qualifier_)
  return true;
...

That said, I think if you use std::tie it'll do what you want:

std::tie(name_, qualifier_, display_name_, capabilities_) < std::tie(other...);

https://codereview.chromium.org/1821383002/diff/40001/mojo/services/catalog/r...
File mojo/services/catalog/reader.cc (right):

https://codereview.chromium.org/1821383002/diff/40001/mojo/services/catalog/r...
mojo/services/catalog/reader.cc:62: void
Reader::OnReadManifest(base::WeakPtr<Reader> reader,
Is there a reason you need this static? It doesn't seem like OnReadManifestImpl
needs to be a member function at all since it doesn't need any state from
Reader.

[Ben Goodger (Google), 2016-03-23 03:00:09]

On 2016/03/22 23:47:46, sky wrote:
>
https://codereview.chromium.org/1821383002/diff/40001/mojo/services/catalog/e...
> File mojo/services/catalog/entry.cc (right):
> 
>
https://codereview.chromium.org/1821383002/diff/40001/mojo/services/catalog/e...
> mojo/services/catalog/entry.cc:205: return other.name_ < name_ ||
> other.qualifier_ < qualifier_ ||
> Don't you only want to compare subsequent values if the preceding one is
equal?
> eg:
> if (other.name_ < name_)
>   return true;
> if (other.name_ > name_)
>   return false;
> if (other.qualifier_ < qualifier_)
>   return true;
> ...
> 
> That said, I think if you use std::tie it'll do what you want:
> 
> std::tie(name_, qualifier_, display_name_, capabilities_) <
std::tie(other...);
> 
>
https://codereview.chromium.org/1821383002/diff/40001/mojo/services/catalog/r...
> File mojo/services/catalog/reader.cc (right):
> 
>
https://codereview.chromium.org/1821383002/diff/40001/mojo/services/catalog/r...
> mojo/services/catalog/reader.cc:62: void
> Reader::OnReadManifest(base::WeakPtr<Reader> reader,
> Is there a reason you need this static? It doesn't seem like
OnReadManifestImpl
> needs to be a member function at all since it doesn't need any state from
> Reader.

Updated PTAL

[Ben Goodger (Google), 2016-03-23 15:14:33]

I realized i could get rid of the weakptr too, but only after I'd started
working on a subsequent CL so that will happen there.

On Tue, Mar 22, 2016 at 8:00 PM, <ben@chromium.org> wrote:

> On 2016/03/22 23:47:46, sky wrote:
> >
>
>
https://codereview.chromium.org/1821383002/diff/40001/mojo/services/catalog/e...
> > File mojo/services/catalog/entry.cc (right):
> >
> >
>
>
https://codereview.chromium.org/1821383002/diff/40001/mojo/services/catalog/e...
> > mojo/services/catalog/entry.cc:205: return other.name_ < name_ ||
> > other.qualifier_ < qualifier_ ||
> > Don't you only want to compare subsequent values if the preceding one is
> equal?
> > eg:
> > if (other.name_ < name_)
> > return true;
> > if (other.name_ > name_)
> > return false;
> > if (other.qualifier_ < qualifier_)
> > return true;
> > ...
> >
> > That said, I think if you use std::tie it'll do what you want:
> >
> > std::tie(name_, qualifier_, display_name_, capabilities_) <
> std::tie(other...);
> >
> >
>
>
https://codereview.chromium.org/1821383002/diff/40001/mojo/services/catalog/r...
> > File mojo/services/catalog/reader.cc (right):
> >
> >
>
>
https://codereview.chromium.org/1821383002/diff/40001/mojo/services/catalog/r...
> > mojo/services/catalog/reader.cc:62: void
> > Reader::OnReadManifest(base::WeakPtr<Reader> reader,
> > Is there a reason you need this static? It doesn't seem like
> OnReadManifestImpl
> > needs to be a member function at all since it doesn't need any state from
> > Reader.
>
> Updated PTAL
>
> https://codereview.chromium.org/1821383002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2016-03-23 15:45:45]

LGTM

[Ben Goodger (Google), 2016-03-23 15:58:48]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2016-03-23 15:58:59]

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

[commit-bot: I haz the power, 2016-03-23 16:13:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-23 16:15:11]

Description was changed from

==========
Moves manifest parsing to a new class, Reader.

. Consolidate value analysis in Entry
. Catalog now just manages the various data structures that allow it to
implement various flavors of Resolve().


BUG=
==========

to

==========
Moves manifest parsing to a new class, Reader.

. Consolidate value analysis in Entry
. Catalog now just manages the various data structures that allow it to
implement various flavors of Resolve().

BUG=

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

[commit-bot: I haz the power, 2016-03-23 16:15:12]

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