1131911 - Extract page metadata extraction from Social.jsm to its own JSM

Status: RESOLVED FIXED

(Toolkit :: General, defect, P1)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

We currently have two (that I know of) implementations of page metadata extraction - primarily relying on OpenGraph. These are:

* OpenGraphBuilder in browser/modules/Social.jsm (also handles microdata)
* _getExcerpt in toolkit/components/reader/content/Readability.js (only excerpt, but also handles Twitter cards)

(This is of course ignoring Microformats, which is another thing again)

And we have the following uses for metadata extraction:
* Social API (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API/Bookmarks#OpenGraphData_DOM_Event)
* ReaderMode
* ReadingList

To clean that up, we should extract the relevant parts from Social.jsm and Readability.js into a JSM in Toolkit, which gives us one API for all three consumers to use.


Non-exhaustive list of relevant noodles of spaghetti:

https://hg.mozilla.org/mozilla-central/file/2cb22c058add/browser/base/content/browser-social.js#l640
https://hg.mozilla.org/mozilla-central/file/2cb22c058add/browser/base/content/content.js#l1008
https://hg.mozilla.org/mozilla-central/file/2cb22c058add/browser/modules/Social.jsm#l490
https://hg.mozilla.org/mozilla-central/file/2cb22c058add/browser/modules/Social.jsm#l671
https://hg.mozilla.org/mozilla-central/file/2cb22c058add/toolkit/components/reader/content/Readability.js#l743
https://hg.mozilla.org/mozilla-central/file/2cb22c058add/mobile/android/chrome/content/Reader.js#l218
https://hg.mozilla.org/mozilla-central/file/2cb22c058add/browser/base/content/content.js#l520

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Additional notes:

This won't just be a JSM - that'll just be the public API. The actual metadata extraction should be in a frame script. And ideally, this should be lazily done, and cached for the lifetime of the page so it isn't extracted twice.

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

Adding Shane who wrote the Social API code.  Shane, any thoughts?

Comment 3

[Shane Caraveo (:mixedpuppy)]

There is probably some useful combination of the two.  I do eventually also need to add microformats2 support, so we'd need to be able to expand on what would be in the jsm.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Wasn't sure what to do with generateEndpointURL - it doesn't really fit in the new module. So I've left it as-is.

Also haven't touched Readability.js, which does some extraction too. Figured that would be best left to a separate Fennec bug, since only Fennec uses that part.

Otherwise:
* Split out into separate toolkit module
* Added an initial basic test, which is a start for more in the future
* Updated the code style a bit
* Added documentation
* Fixed a couple of latent bugs. IIRC, around adding extra sanity checks

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, and Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=18e2ee243829

Customary unrelated intermittent oranges included for free.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8570988 [details] [diff] [review]
Patch v1

The ftp support (_validateURL) could probably be removed.  It might also be worthwhile move the tests over (would require a little reworking of them) from browser_share.js (see testSharePage and testShareMicrodata).

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> Comment on attachment 8570988 [details] [diff] [review]
> Patch v1
> 
> The ftp support (_validateURL) could probably be removed.

Done.

>  It might also be
> worthwhile move the tests over (would require a little reworking of them)
> from browser_share.js (see testSharePage and testShareMicrodata).

These looked like useful integration tests for Social, so I've left them as-is. I've filed bug 1140347 for more unit test coverage.



https://hg.mozilla.org/integration/fx-team/rev/5b40873e617b

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b40873e617b

Comment 9

[Shane Caraveo (:mixedpuppy)]

Blair, do we need any follow up bugs?  The original comment feels more extensive than what is done so far.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Thanks for the reminder. So many balls in the air, hard to keep track of it all. Filing stuff now.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/115541937926