Browser Model for advanced analysis

Browser Model for advanced analysis

- Add BrowsingContext that models frames in the browser.
- Add ResourceFetch and ResourceEvaluation that model fetching / evaluating a
resource in a BrowsingContext.
- Add a resource.proto that includes the complete
resource/browsingcontext/fetch/eval description.
- Add an automatic browsing context creation out of the DOM.
- Add a Clone method to the DOM object to be able to retain a handle to a child
document.


BUG=
TEST=

[bmcquade]

comments so far. overall structure looks good - just want to get the subtleties
right here.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
File pagespeed/core/browsing_context.cc (right):

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:83: const BrowsingContext* candidate_context
= this;
do we expect the pagespeed_input_ to be finalized when it is passed in here? i
can't remember the exact order. if we do expect that, let's dcheck it here.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:83: const BrowsingContext* candidate_context
= this;
let's name this document_context

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:83: const BrowsingContext* candidate_context
= this;
let's add a one-line comment to explain that you are traversing up the parent
chain to find the first context with a document resource.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:96: context_document_url =
candidate_context->GetDocumentResourceOrNull()
I prefer not to wrap on the -> if it can be helped, so:

context_document_url =
    candidate_context->GetDocumentResourceOrNull()->GetRequestUrl();

would be better here.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:99:
action_uri_generator_->Create(uri_util::BROWSING_CONTEXT,
if this is a context without a document, will this URI be the same as the one
for its parent context? is that desired? i thought URIs were supposed to be
unique to a context. Or is the URI generator stateful and guaranteed to generate
a unique ID? in that case maybe call it GenerateUniqueUri() instead of Create()?

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:180: event_dom_content_msec_ = time_msec;
can we operate with only the tick data? what is the reason to store the times?
is it just for serialization purposes? i think that seems ok but let's note that
in the method comment in the header.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:213: if (!(*it)->IsFinalized() &&
!(*it)->Finalize()) {
i'm confused why we expect IsFinalized to return true here. I would expect
nothing to be finalized at this point. Should the check be
if ((*it)->IsFinalized() || !(*it)->Finalize()) {
  return false;
}

I think a
DCHECK(!(*it)->IsFinalized());
if (!(*it)->Finalize()) {
  return false;
}

is probably ok, or maybe Finalize() can just check that it's not already
finalized and return false, then you can just propagate that failure up here
when you call Finalize() and it returns false.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:224: if (!(*it)->IsFinalized() &&
!(*it)->Finalize()) {
same

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:259:
contexts->assign(nested_contexts_.begin(), nested_contexts_.end());
how about a DCHECK(contexts->empty()) on entry to the function (same for other
vector populating functions)

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:289: ResourceFetchVector* requests) const {
remind me in what contexts a resource can have multiple fetch objects associated
with it for a single browsing context?

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:493: RegisterBrowsingContext(this);
RegisterBrowsingContext DCHECKs that this is not the top-level context. It seems
like that would fire here.

Let's make the RegisterFoo methods protected. They are only supposed to be
called internally, right?

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
File pagespeed/core/browsing_context.h (right):

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:53: BrowsingContext*
CreateNestedBrowsingContext(const Resource* resource);
since ownership remains with the BrowsingContext I'd prefer to call these
AddNestedBrowsingContext instead of Create.... To me "Create" implies ownership
is being transferred to the caller. There is precedent for "add" in the protobuf
APIs which work the same way (ownership remains with the protobuf).

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:66: // Registers a resource that is referenced
in the DOM of this browsing
I think "that is referenced in the DOM" may be overly specific. Some things,
e.g. XHRs, would not be referenced in the DOM. Perhaps just "that is referenced
in this browsing context"?

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:90: bool IsFinalized() const {
The C++ style guide prefers that getter methods just have lowercase names, so
you can call this "finalized()" or "is_finalized()".

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:102: const std::string& GetUri() const;
maybe GetBrowsingContextUri to make it clearer that this is not an HTTP URL?

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:135: // BrowsingContext. However, in the case
of non-cachable resources, there will
instead of "there will be" I'd say "there may be" since some systems we
integrate with may lose the subsequent fetches.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:178: int64 GetEventDomContentSequence() const
{
I think these GetEventFooSequence names could be clearer. Perhaps
GetDomContentSequenceId and GetLoadSequenceId? The word "event" isn't conveying
any additional information to me so I am inclined to drop it.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:216: ActionUriGenerator*
action_uri_generator_;
my inclination is to never make members protected, and to instead expose a
protected getter. can you also make this:
ActionUriGenerator* const action_uri_generator_;

so it's clear that it can't be modified after the constructor? or can it be
modified after construction?

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:224: const PagespeedInput* pagespeed_input_;
let's go with const PagespeedInput* const pagespeed_input_;

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:228: TopLevelBrowsingContext*
top_level_context_;
* const top_level_context_; here as well.

[bmcquade]

a few more comments

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
File pagespeed/core/browsing_context_test.cc (right):

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context_test.cc:1: // Copyright 2011 Google Inc. All
Rights Reserved.
we need tests to verify that the freeze logic works as expected and to verify
that the redirect traversal works correctly. we should add a test that verifies
that we do correctly catch redirect chains as well.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context_test.cc:69: TEST(BrowsingContextTest,
SimpleContext) {
you should use a text fixture derived from PagespeedTest, which provides lots of
helper methods like NewResource, etc. Take a look at pagespeed_input_test.cc
which uses PagespeedTest.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/core...
File pagespeed/core/core.gyp (right):

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/core...
pagespeed/core/core.gyp:27:
'<(pagespeed_root)/pagespeed/html/html.gyp:pagespeed_external_resource_filter',
I'm wondering if the external resource filter should also move to core. Can you
add a TODO(bmcquade): move pagespeed_external_resource_filter to core for me to
do later?

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/core...
pagespeed/core/core.gyp:72:
'<(pagespeed_root)/pagespeed/proto/proto_gen.gyp:pagespeed_resource_pb',
it turns out that formatter.h also includes a pb.h for
pagespeed_proto_formatter. can you export that dependency here as well? it wont
change anything but it will more accurately reflect what we do need to export.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/page...
File pagespeed/core/pagespeed_input.cc (right):

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/page...
pagespeed/core/pagespeed_input.cc:49: class BrowsingContextFactory : public
ExternalResourceDomElementVisitor {
I'm inclined to expose this class in the header so we can test it.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/page...
pagespeed/core/pagespeed_input.cc:78: const Resource* resource =
pagespeed_input_->GetResourceWithUrlOrNull(
let's move everything after the equal sign onto the next line.

[MikeS]

PTAL.

Patch set 3 & 4 are the same - I uploaded twice because I got an error during
"git cl upload" but it seemed to work anyways.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
File pagespeed/core/browsing_context.cc (right):

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:83: const BrowsingContext* candidate_context
= this;
On 2012/01/20 23:49:21, bmcquade wrote:
> let's name this document_context

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:83: const BrowsingContext* candidate_context
= this;
On 2012/01/20 23:49:21, bmcquade wrote:
> do we expect the pagespeed_input_ to be finalized when it is passed in here? i
> can't remember the exact order. if we do expect that, let's dcheck it here.

It is in FINALIZE state, but if someone wants to set the BrowsingContext before
calling PagespeedInput::Freeze, it might be in the INIT state.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:83: const BrowsingContext* candidate_context
= this;
On 2012/01/20 23:49:21, bmcquade wrote:
> let's add a one-line comment to explain that you are traversing up the parent
> chain to find the first context with a document resource.

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:96: context_document_url =
candidate_context->GetDocumentResourceOrNull()
On 2012/01/20 23:49:21, bmcquade wrote:
> I prefer not to wrap on the -> if it can be helped, so:
> 
> context_document_url =
>     candidate_context->GetDocumentResourceOrNull()->GetRequestUrl();
> 
> would be better here.

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:99:
action_uri_generator_->Create(uri_util::BROWSING_CONTEXT,
On 2012/01/20 23:49:21, bmcquade wrote:
> if this is a context without a document, will this URI be the same as the one
> for its parent context? is that desired? i thought URIs were supposed to be
> unique to a context. Or is the URI generator stateful and guaranteed to
generate
> a unique ID? in that case maybe call it GenerateUniqueUri() instead of
Create()?

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:180: event_dom_content_msec_ = time_msec;
On 2012/01/20 23:49:21, bmcquade wrote:
> can we operate with only the tick data? what is the reason to store the times?
> is it just for serialization purposes? i think that seems ok but let's note
that
> in the method comment in the header.

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:213: if (!(*it)->IsFinalized() &&
!(*it)->Finalize()) {
On 2012/01/20 23:49:21, bmcquade wrote:
> i'm confused why we expect IsFinalized to return true here.

The code here is correct - however I added a comment to clarify that any number
of the fetches or evaluations are allowed to be finalized even before
BrowsingContext::Finalize is called.

Therefore, BrowsingContext::Finalize fails (returns false) if a resource is not
yet finalized and finalizing it fails.

The reason for this is as follows: the finalize phase is used for example in the
fetches to create the logical downloads (that is, hide the redirects). However,
the evaluation constraint calculation relies on the data produced by finalizing
the fetches. To accommodate the fact that the calculation is currently done in
the WebKit land, I allow finalizing of the fetches and evaluations prior to the
BrowsingContext finalization.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:224: if (!(*it)->IsFinalized() &&
!(*it)->Finalize()) {
On 2012/01/20 23:49:21, bmcquade wrote:
> same

same :)

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:259:
contexts->assign(nested_contexts_.begin(), nested_contexts_.end());
On 2012/01/20 23:49:21, bmcquade wrote:
> how about a DCHECK(contexts->empty()) on entry to the function (same for other
> vector populating functions)

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:289: ResourceFetchVector* requests) const {
On 2012/01/20 23:49:21, bmcquade wrote:
> remind me in what contexts a resource can have multiple fetch objects
associated
> with it for a single browsing context? 

Non-cachabe XMLHttpRequests for example.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.cc:493: RegisterBrowsingContext(this);
On 2012/01/20 23:49:21, bmcquade wrote:
> RegisterBrowsingContext DCHECKs that this is not the top-level context. It
seems
> like that would fire here.
> 
> Let's make the RegisterFoo methods protected. They are only supposed to be
> called internally, right?

RegisterFoo methods are protected, and overridden by TopLevelBrowsingContext.
This is why the DCHECK is in the BrowsingContext::RegisterFoo implementations.
The BrowsingContext::RegisterFoo implementations delegate the registration to
the TopLevelBrowsingContent::RegisterFoo implementations (via
BrowsingContext::top_level_context_). Only the top level context holds the
uri->Foo index.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
File pagespeed/core/browsing_context.h (right):

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:53: BrowsingContext*
CreateNestedBrowsingContext(const Resource* resource);
On 2012/01/20 23:49:21, bmcquade wrote:
> since ownership remains with the BrowsingContext I'd prefer to call these
> AddNestedBrowsingContext instead of Create.... To me "Create" implies
ownership
> is being transferred to the caller. There is precedent for "add" in the
protobuf
> APIs which work the same way (ownership remains with the protobuf).

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:66: // Registers a resource that is referenced
in the DOM of this browsing
On 2012/01/20 23:49:21, bmcquade wrote:
> I think "that is referenced in the DOM" may be overly specific. Some things,
> e.g. XHRs, would not be referenced in the DOM. Perhaps just "that is
referenced
> in this browsing context"?

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:90: bool IsFinalized() const {
On 2012/01/20 23:49:21, bmcquade wrote:
> The C++ style guide prefers that getter methods just have lowercase names, so
> you can call this "finalized()" or "is_finalized()".

I'm not a big supporter of this, as especially an IsFinalized method might
easily become a non-trivial method. Exposing an implementation detail in the API
name is a bad practice IMO. I'm willing to move the implementation to the cc
file though.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:102: const std::string& GetUri() const;
On 2012/01/20 23:49:21, bmcquade wrote:
> maybe GetBrowsingContextUri to make it clearer that this is not an HTTP URL?

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:135: // BrowsingContext. However, in the case
of non-cachable resources, there will
On 2012/01/20 23:49:21, bmcquade wrote:
> instead of "there will be" I'd say "there may be" since some systems we
> integrate with may lose the subsequent fetches.

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:178: int64 GetEventDomContentSequence() const
{
Dropped the "Event" and renamed "Sequence" to "Tick", as I think "SequenceId" is
misleading.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:216: ActionUriGenerator*
action_uri_generator_;
On 2012/01/20 23:49:21, bmcquade wrote:
> my inclination is to never make members protected, and to instead expose a
> protected getter. can you also make this:
> ActionUriGenerator* const action_uri_generator_;
> 
> so it's clear that it can't be modified after the constructor? or can it be
> modified after construction?

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:224: const PagespeedInput* pagespeed_input_;
On 2012/01/20 23:49:21, bmcquade wrote:
> let's go with const PagespeedInput* const pagespeed_input_;

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context.h:228: TopLevelBrowsingContext*
top_level_context_;
On 2012/01/20 23:49:21, bmcquade wrote:
> * const top_level_context_; here as well.

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
File pagespeed/core/browsing_context_test.cc (right):

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context_test.cc:1: // Copyright 2011 Google Inc. All
Rights Reserved.
see TEST(ResourceFetchTest, Propagate)

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/brow...
pagespeed/core/browsing_context_test.cc:69: TEST(BrowsingContextTest,
SimpleContext) {
On 2012/01/21 06:41:20, bmcquade wrote:
> you should use a text fixture derived from PagespeedTest, which provides lots
of
> helper methods like NewResource, etc. Take a look at pagespeed_input_test.cc
> which uses PagespeedTest.

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/core...
File pagespeed/core/core.gyp (right):

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/core...
pagespeed/core/core.gyp:27:
'<(pagespeed_root)/pagespeed/html/html.gyp:pagespeed_external_resource_filter',
On 2012/01/21 06:41:20, bmcquade wrote:
> I'm wondering if the external resource filter should also move to core. Can
you
> add a TODO(bmcquade): move pagespeed_external_resource_filter to core for me
to
> do later?

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/core...
pagespeed/core/core.gyp:72:
'<(pagespeed_root)/pagespeed/proto/proto_gen.gyp:pagespeed_resource_pb',
On 2012/01/21 06:41:20, bmcquade wrote:
> it turns out that formatter.h also includes a pb.h for
> pagespeed_proto_formatter. can you export that dependency here as well? it
wont
> change anything but it will more accurately reflect what we do need to export.

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/page...
File pagespeed/core/pagespeed_input.cc (right):

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/page...
pagespeed/core/pagespeed_input.cc:49: class BrowsingContextFactory : public
ExternalResourceDomElementVisitor {
On 2012/01/21 06:41:20, bmcquade wrote:
> I'm inclined to expose this class in the header so we can test it.

Done.

http://page-speed-codereview.appspot.com/677001/diff/2001/pagespeed/core/page...
pagespeed/core/pagespeed_input.cc:78: const Resource* resource =
pagespeed_input_->GetResourceWithUrlOrNull(
On 2012/01/21 06:41:20, bmcquade wrote:
> let's move everything after the equal sign onto the next line.

Done.

[bmcquade]

LGTM