1301715 - Extract website metadata while browsing websites (for Activity Stream)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

See bug 1288330.

For activity stream we require some metadata about the websites visited. While browsing we already download the document and images, so it makes sense to extract and store some of that. This bug is just about the extracting part.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1301715 - Extract website metadata and make it available.

This patch introduces WebsiteMetadata.jsm which imports fathom and page-metadata-parser.
The code has been slightly modified to not depend on more node libraries.

On DOMContentLoaded the module will extract the metadata asynchronously and send it with
a 'Website:Metadata' event.

Review commit: https://reviewboard.mozilla.org/r/77928/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77928/

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1301715 - BrowserApp: Receive metadata from event.

Review commit: https://reviewboard.mozilla.org/r/77930/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77930/

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8789874 [details]
Bug 1301715 - Extract website metadata and make it available.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77928/diff/1-2/

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8789875 [details]
Bug 1301715 - BrowserApp: Receive metadata from event.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77930/diff/1-2/

Comment 5

[Andrzej Hunt :ahunt]

Comment on attachment 8789874 [details]
Bug 1301715 - Extract website metadata and make it available.

https://reviewboard.mozilla.org/r/77928/#review79526

::: mobile/android/chrome/content/browser.js:3946
(Diff revision 2)
>            }.bind(this);
>  
>            this.browser.addEventListener("pagehide", listener, true);
>          }
> +
> +        WebsiteMetadata.parseAsynchronously(this.browser.contentDocument);

I wonder if we should make this conditional on AS being enabled, as to avoid unnecessary page processing?

On a similar note: can we add performance telemetry? (I'm not quite sure what we should be measuring though. Any significant performance impacts should hopefully be visible on perfherder, but we could maybe also measure parsing time directly?)

Comment 6

[Andrzej Hunt :ahunt]

Comment on attachment 8789875 [details]
Bug 1301715 - BrowserApp: Receive metadata from event.

https://reviewboard.mozilla.org/r/77930/#review79528

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Andrzej Hunt :ahunt from comment #5)
> I wonder if we should make this conditional on AS being enabled, as to avoid
> unnecessary page processing?

That's what I was wondering too. Enabling this for all now would have the advantage that we start with data as soon as we enable Activity Stream. Eventually I was thinking about replacing our icon extraction code with this too.

> On a similar note: can we add performance telemetry? (I'm not quite sure
> what we should be measuring though. Any significant performance impacts
> should hopefully be visible on perfherder, but we could maybe also measure
> parsing time directly?)

This is a good point. This shouldn't affect page loading time though.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1301715 - Only extract metadata if the Activity Stream or Nightly flag is enabled.

Review commit: https://reviewboard.mozilla.org/r/82392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82392/

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #7)
> (In reply to Andrzej Hunt :ahunt from comment #5)
> > I wonder if we should make this conditional on AS being enabled, as to avoid
> > unnecessary page processing?
> 
> That's what I was wondering too. Enabling this for all now would have the
> advantage that we start with data as soon as we enable Activity Stream.
> Eventually I was thinking about replacing our icon extraction code with this
> too.

I added another patch to only enable this if the AS or Nightly flag is enabled. Nightly only so that this code is run already and we can find issues with it already.

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8796589 [details]
Bug 1301715 - Only extract metadata if the Activity Stream or Nightly flag is enabled.

https://reviewboard.mozilla.org/r/82392/#review81780

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/479f64142010
Extract website metadata and make it available. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/a8e194e02cce
BrowserApp: Receive metadata from event. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/db7f17f568db
Only extract metadata if the Activity Stream or Nightly flag is enabled. r=ahunt

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/479f64142010
https://hg.mozilla.org/mozilla-central/rev/a8e194e02cce
https://hg.mozilla.org/mozilla-central/rev/db7f17f568db