[Telemetry] Add script to snapshot page's HTML

[Telemetry] Add script to snapshot page's HTML

To try this out:
./telemetry/bin/snap_page --browser=system --url=http://cnn.com

BUG=chromium:717218
Review-Url: https://chromiumcodereview.appspot.com/3010063002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/c9667ecd29cbb003b7fbdcd6f2da0f02bf0e7257

[nednguyen, 2017-09-02 12:55:05]

Description was changed from

==========
[Telemetry] Add script to snapshot page's HTML
==========

to

==========
[Telemetry] Add script to snapshot page's HTML

To try this out:
./telemetry/bin/snap_page --browser=system --url=http://cnn.com

COMMIT=false (wait for license info of telemetry/third_party/snap-it/)
==========

[nednguyen, 2017-09-02 13:30:59]

nednguyen@google.com changed reviewers:
+ pdr@chromium.org, wkorman@chromium.org

[wkorman, 2017-09-02 13:50:06]

lgtm

Sweet!

https://codereview.chromium.org/3010063002/diff/1/telemetry/bin/snap_page
File telemetry/bin/snap_page (right):

https://codereview.chromium.org/3010063002/diff/1/telemetry/bin/snap_page#new...
telemetry/bin/snap_page:21: """ Save the HTML snapshot of the page whose address
|url| to |snapshot_path|.
"is" after "address"

https://codereview.chromium.org/3010063002/diff/1/telemetry/bin/snap_page#new...
telemetry/bin/snap_page:49: print 'Successfuly saved snapshot to file://%s' %
snapshot_path
'lly' vs 'ly'

https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
File telemetry/third_party/snap-it/README.md (right):

https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
telemetry/third_party/snap-it/README.md:1: # snap-it
Re: importing a third party extension, is there somewhere in a README or
elsewhere that we're supposed to include a link to the hosting page or the git
clone url, or something to help future eng find the canonical source repo and
re-import/update the extension if needed?

[nednguyen, 2017-09-05 13:38:23]

I filed a bug about LICENSE of snipit here:
https://github.com/stevenbfine/snap-it/issues/72

Hopefully the author will respond quickly :-/

https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
File telemetry/third_party/snap-it/README.md (right):

https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
telemetry/third_party/snap-it/README.md:1: # snap-it
On 2017/09/02 13:50:06, wkorman wrote:
> Re: importing a third party extension, is there somewhere in a README or
> elsewhere that we're supposed to include a link to the hosting page or the git
> clone url, or something to help future eng find the canonical source repo and
> re-import/update the extension if needed?

Yes, I will add README.chromium once the LICENSE file is created.

[pdr., 2017-09-05 16:32:10]

On 2017/09/05 at 13:38:23, nednguyen wrote:
> I filed a bug about LICENSE of snipit here:
https://github.com/stevenbfine/snap-it/issues/72
> 
> Hopefully the author will respond quickly :-/
> 
>
https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
> File telemetry/third_party/snap-it/README.md (right):
> 
>
https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
> telemetry/third_party/snap-it/README.md:1: # snap-it
> On 2017/09/02 13:50:06, wkorman wrote:
> > Re: importing a third party extension, is there somewhere in a README or
> > elsewhere that we're supposed to include a link to the hosting page or the
git
> > clone url, or something to help future eng find the canonical source repo
and
> > re-import/update the extension if needed?
> 
> Yes, I will add README.chromium once the LICENSE file is created.

I will ping the author directly. If we need to, I can takeover the repo.

[nednguyen, 2017-09-05 16:32:50]

On 2017/09/05 16:32:10, pdr. wrote:
> On 2017/09/05 at 13:38:23, nednguyen wrote:
> > I filed a bug about LICENSE of snipit here:
> https://github.com/stevenbfine/snap-it/issues/72
> > 
> > Hopefully the author will respond quickly :-/
> > 
> >
>
https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
> > File telemetry/third_party/snap-it/README.md (right):
> > 
> >
>
https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
> > telemetry/third_party/snap-it/README.md:1: # snap-it
> > On 2017/09/02 13:50:06, wkorman wrote:
> > > Re: importing a third party extension, is there somewhere in a README or
> > > elsewhere that we're supposed to include a link to the hosting page or the
> git
> > > clone url, or something to help future eng find the canonical source repo
> and
> > > re-import/update the extension if needed?
> > 
> > Yes, I will add README.chromium once the LICENSE file is created.
> 
> I will ping the author directly. If we need to, I can takeover the repo.

+1

[pdr., 2017-09-05 16:34:16]

On 2017/09/05 at 16:32:10, pdr. wrote:
> On 2017/09/05 at 13:38:23, nednguyen wrote:
> > I filed a bug about LICENSE of snipit here:
https://github.com/stevenbfine/snap-it/issues/72
> > 
> > Hopefully the author will respond quickly :-/
> > 
> >
https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
> > File telemetry/third_party/snap-it/README.md (right):
> > 
> >
https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
> > telemetry/third_party/snap-it/README.md:1: # snap-it
> > On 2017/09/02 13:50:06, wkorman wrote:
> > > Re: importing a third party extension, is there somewhere in a README or
> > > elsewhere that we're supposed to include a link to the hosting page or the
git
> > > clone url, or something to help future eng find the canonical source repo
and
> > > re-import/update the extension if needed?
> > 
> > Yes, I will add README.chromium once the LICENSE file is created.
> 
> I will ping the author directly. If we need to, I can takeover the repo.

This was developed as part of a Chrome intern project btw, so I'm pretty sure
this is okay to import as-is.

[pdr., 2017-09-05 17:22:38]

On 2017/09/05 at 16:34:16, pdr. wrote:
> On 2017/09/05 at 16:32:10, pdr. wrote:
> > On 2017/09/05 at 13:38:23, nednguyen wrote:
> > > I filed a bug about LICENSE of snipit here:
https://github.com/stevenbfine/snap-it/issues/72
> > > 
> > > Hopefully the author will respond quickly :-/
> > > 
> > >
https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
> > > File telemetry/third_party/snap-it/README.md (right):
> > > 
> > >
https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
> > > telemetry/third_party/snap-it/README.md:1: # snap-it
> > > On 2017/09/02 13:50:06, wkorman wrote:
> > > > Re: importing a third party extension, is there somewhere in a README or
> > > > elsewhere that we're supposed to include a link to the hosting page or
the git
> > > > clone url, or something to help future eng find the canonical source
repo and
> > > > re-import/update the extension if needed?
> > > 
> > > Yes, I will add README.chromium once the LICENSE file is created.
> > 
> > I will ping the author directly. If we need to, I can takeover the repo.
> 
> This was developed as part of a Chrome intern project btw, so I'm pretty sure
this is okay to import as-is.

A license file has been added. Does this unblock you?

Steven later converted to full-time at Google but doesn't work in this area.
Would you like me to fork the project so we have control over it?

[nednguyen, 2017-09-05 17:25:37]

On 2017/09/05 17:22:38, pdr. wrote:
> On 2017/09/05 at 16:34:16, pdr. wrote:
> > On 2017/09/05 at 16:32:10, pdr. wrote:
> > > On 2017/09/05 at 13:38:23, nednguyen wrote:
> > > > I filed a bug about LICENSE of snipit here:
> https://github.com/stevenbfine/snap-it/issues/72
> > > > 
> > > > Hopefully the author will respond quickly :-/
> > > > 
> > > >
>
https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
> > > > File telemetry/third_party/snap-it/README.md (right):
> > > > 
> > > >
>
https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
> > > > telemetry/third_party/snap-it/README.md:1: # snap-it
> > > > On 2017/09/02 13:50:06, wkorman wrote:
> > > > > Re: importing a third party extension, is there somewhere in a README
or
> > > > > elsewhere that we're supposed to include a link to the hosting page or
> the git
> > > > > clone url, or something to help future eng find the canonical source
> repo and
> > > > > re-import/update the extension if needed?
> > > > 
> > > > Yes, I will add README.chromium once the LICENSE file is created.
> > > 
> > > I will ping the author directly. If we need to, I can takeover the repo.
> > 
> > This was developed as part of a Chrome intern project btw, so I'm pretty
sure
> this is okay to import as-is.
> 
> A license file has been added. Does this unblock you?
> 
> Steven later converted to full-time at Google but doesn't work in this area.
> Would you like me to fork the project so we have control over it?

Yes please. Once you fork the project, I will use your instead.

[pdr., 2017-09-05 17:31:24]

On 2017/09/05 at 17:25:37, nednguyen wrote:
> On 2017/09/05 17:22:38, pdr. wrote:
> > On 2017/09/05 at 16:34:16, pdr. wrote:
> > > On 2017/09/05 at 16:32:10, pdr. wrote:
> > > > On 2017/09/05 at 13:38:23, nednguyen wrote:
> > > > > I filed a bug about LICENSE of snipit here:
> > https://github.com/stevenbfine/snap-it/issues/72
> > > > > 
> > > > > Hopefully the author will respond quickly :-/
> > > > > 
> > > > >
> >
https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
> > > > > File telemetry/third_party/snap-it/README.md (right):
> > > > > 
> > > > >
> >
https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
> > > > > telemetry/third_party/snap-it/README.md:1: # snap-it
> > > > > On 2017/09/02 13:50:06, wkorman wrote:
> > > > > > Re: importing a third party extension, is there somewhere in a
README or
> > > > > > elsewhere that we're supposed to include a link to the hosting page
or
> > the git
> > > > > > clone url, or something to help future eng find the canonical source
> > repo and
> > > > > > re-import/update the extension if needed?
> > > > > 
> > > > > Yes, I will add README.chromium once the LICENSE file is created.
> > > > 
> > > > I will ping the author directly. If we need to, I can takeover the repo.
> > > 
> > > This was developed as part of a Chrome intern project btw, so I'm pretty
sure
> > this is okay to import as-is.
> > 
> > A license file has been added. Does this unblock you?
> > 
> > Steven later converted to full-time at Google but doesn't work in this area.
> > Would you like me to fork the project so we have control over it?
> 
> Yes please. Once you fork the project, I will use your instead.

Done! https://github.com/progers/snap-it

[nednguyen, 2017-09-05 17:49:08]

https://codereview.chromium.org/3010063002/diff/1/telemetry/bin/snap_page
File telemetry/bin/snap_page (right):

https://codereview.chromium.org/3010063002/diff/1/telemetry/bin/snap_page#new...
telemetry/bin/snap_page:21: """ Save the HTML snapshot of the page whose address
|url| to |snapshot_path|.
On 2017/09/02 13:50:06, wkorman wrote:
> "is" after "address"

Done.

https://codereview.chromium.org/3010063002/diff/1/telemetry/bin/snap_page#new...
telemetry/bin/snap_page:49: print 'Successfuly saved snapshot to file://%s' %
snapshot_path
On 2017/09/02 13:50:06, wkorman wrote:
> 'lly' vs 'ly'

Done.

https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
File telemetry/third_party/snap-it/README.md (right):

https://codereview.chromium.org/3010063002/diff/1/telemetry/third_party/snap-...
telemetry/third_party/snap-it/README.md:1: # snap-it
On 2017/09/05 13:38:23, nednguyen wrote:
> On 2017/09/02 13:50:06, wkorman wrote:
> > Re: importing a third party extension, is there somewhere in a README or
> > elsewhere that we're supposed to include a link to the hosting page or the
git
> > clone url, or something to help future eng find the canonical source repo
and
> > re-import/update the extension if needed?
> 
> Yes, I will add README.chromium once the LICENSE file is created.

Done. See README.chromium

[nednguyen, 2017-09-05 17:49:50]

https://codereview.chromium.org/3010063002/diff/20001/telemetry/bin/snap_page
File telemetry/bin/snap_page (right):

https://codereview.chromium.org/3010063002/diff/20001/telemetry/bin/snap_page...
telemetry/bin/snap_page:65: parser.add_option('--interactive',
I added this flag since the original review.

[nednguyen, 2017-09-05 18:13:02]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-09-05 18:13:06]

Dry run: CQ is trying da patch.

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

[nednguyen, 2017-09-05 18:27:50]

Description was changed from

==========
[Telemetry] Add script to snapshot page's HTML

To try this out:
./telemetry/bin/snap_page --browser=system --url=http://cnn.com

COMMIT=false (wait for license info of telemetry/third_party/snap-it/)
==========

to

==========
[Telemetry] Add script to snapshot page's HTML

To try this out:
./telemetry/bin/snap_page --browser=system --url=http://cnn.com

BUG=chromium:717218
==========

[commit-bot: I haz the power, 2017-09-05 19:23:56]

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

[commit-bot: I haz the power, 2017-09-05 19:23:58]

Dry run: This issue passed the CQ dry run.

[nednguyen, 2017-09-05 20:11:38]

On 2017/09/05 17:49:50, nednguyen wrote:
> https://codereview.chromium.org/3010063002/diff/20001/telemetry/bin/snap_page
> File telemetry/bin/snap_page (right):
> 
>
https://codereview.chromium.org/3010063002/diff/20001/telemetry/bin/snap_page...
> telemetry/bin/snap_page:65: parser.add_option('--interactive',
> I added this flag since the original review.

Phillip, Walter?

[pdr., 2017-09-05 21:30:41]

LGTM

https://codereview.chromium.org/3010063002/diff/40001/telemetry/bin/snap_page
File telemetry/bin/snap_page (right):

https://codereview.chromium.org/3010063002/diff/40001/telemetry/bin/snap_page...
telemetry/bin/snap_page:20: def SnapPage(finder_options, url, interactive,
snapshot_path):
WDYT of adding a simple end-to-end integration test of this? I.e., just snap a
simple static hello world page?

https://codereview.chromium.org/3010063002/diff/40001/telemetry/bin/snap_page...
telemetry/bin/snap_page:51: serialized_dom =
''.join(tab.EvaluateJavaScript('serializedDomArray'))
What is ''.join for? Is this converting the result to a string? Is that
necessary?

[wkorman, 2017-09-05 21:34:19]

lgtm

[nednguyen, 2017-09-05 21:41:46]

On 2017/09/05 21:30:41, pdr. wrote:
> LGTM
> 
> https://codereview.chromium.org/3010063002/diff/40001/telemetry/bin/snap_page
> File telemetry/bin/snap_page (right):
> 
>
https://codereview.chromium.org/3010063002/diff/40001/telemetry/bin/snap_page...
> telemetry/bin/snap_page:20: def SnapPage(finder_options, url, interactive,
> snapshot_path):
> WDYT of adding a simple end-to-end integration test of this? I.e., just snap a
> simple static hello world page?

Great idea. Though I will write one in subsequent CL so folks can start using
this script right away.

> 
>
https://codereview.chromium.org/3010063002/diff/40001/telemetry/bin/snap_page...
> telemetry/bin/snap_page:51: serialized_dom =
> ''.join(tab.EvaluateJavaScript('serializedDomArray'))
> What is ''.join for? Is this converting the result to a string? Is that
> necessary?

yes
serializedDomArray is a list of string. We want to turn it to a single string to
write to file.

[nednguyen, 2017-09-05 21:41:52]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2017-09-05 21:41:57]

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, 2017-09-05 22:49:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1504647712643420,
"parent_rev": "2fb10a1470faea49892fca673c4cf898fc9264d2", "commit_rev":
"c9667ecd29cbb003b7fbdcd6f2da0f02bf0e7257"}

[commit-bot: I haz the power, 2017-09-05 22:49:22]

Description was changed from

==========
[Telemetry] Add script to snapshot page's HTML

To try this out:
./telemetry/bin/snap_page --browser=system --url=http://cnn.com

BUG=chromium:717218
==========

to

==========
[Telemetry] Add script to snapshot page's HTML

To try this out:
./telemetry/bin/snap_page --browser=system --url=http://cnn.com

BUG=chromium:717218

Review-Url: https://chromiumcodereview.appspot.com/3010063002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-09-05 22:49:23]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

[chrishtr, 2017-09-07 20:39:16]

chrishtr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2017-09-07 20:39:17]

Does this work for cross-domain iframes? I think you'll need to inject the
script into
all of them. Is there a method similar to ExecuteJavaScript that does this for
all such
iframes? snap-it's Chrome extension did this.

[nednguyen, 2017-09-07 20:41:04]

On 2017/09/07 20:39:17, chrishtr wrote:
> Does this work for cross-domain iframes? I think you'll need to inject the
> script into
> all of them. Is there a method similar to ExecuteJavaScript that does this for
> all such
> iframes? snap-it's Chrome extension did this.

The logic of serialzing document of this is copied from snap-it (see line 40 ->
48 in telemetry/bin/snap_page)

[nednguyen, 2017-09-07 20:42:41]

On 2017/09/07 20:41:04, nednguyen wrote:
> On 2017/09/07 20:39:17, chrishtr wrote:
> > Does this work for cross-domain iframes? I think you'll need to inject the
> > script into
> > all of them. Is there a method similar to ExecuteJavaScript that does this
for
> > all such
> > iframes? snap-it's Chrome extension did this.
> 
> The logic of serialzing document of this is copied from snap-it (see line 40
->
> 48 in telemetry/bin/snap_page)

Err nvm, I just find that snap-it does something extras in
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/third_par...

[nednguyen, 2017-09-07 20:51:23]

On 2017/09/07 20:39:17, chrishtr wrote:
> Does this work for cross-domain iframes? I think you'll need to inject the
> script into
> all of them. Is there a method similar to ExecuteJavaScript that does this for
> all such
> iframes? snap-it's Chrome extension did this.

chrishtr@ how does snap-it's Chrome extension package all the serialized form of
the top level document & the iframes together? 

Telemetry does support injecting script in iframe, but I am not sure how to
package the iframes' snapshot together with the host snapshot.

[nednguyen, 2017-09-07 20:52:04]

On 2017/09/07 20:51:23, nednguyen wrote:
> On 2017/09/07 20:39:17, chrishtr wrote:
> > Does this work for cross-domain iframes? I think you'll need to inject the
> > script into
> > all of them. Is there a method similar to ExecuteJavaScript that does this
for
> > all such
> > iframes? snap-it's Chrome extension did this.
> 
> chrishtr@ how does snap-it's Chrome extension package all the serialized form
of
> the top level document & the iframes together? 
> 
> Telemetry does support injecting script in iframe, but I am not sure how to
> package the iframes' snapshot together with the host snapshot.

nvm, Move the discussion to crbug/763119