1037483 - update firefox microformats parser with microformats2 support

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

The microformats modules is v1, we should update to support v2.  Bug 926417 led my to try using the microformats-shiv project on github.  That could be used as a module, but should be written to take advantage of internal apis in firefox.

Comment 1

[Tantek Çelik]

For reference, the microformats2 parsing spec: http://microformats.org/wiki/microformats2-parsing

Comment 2

[Mike Kaply [:mkaply]]

I've been using microformats-shiv as well. It's a much better implementation than what I wrote. My microformats API didn't get any traction (as far as I know).

I'm not sure it should even be in Firefox anymore...

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: microformats

Patch that adds microformats v2 support, updates PageMetaData.jsm to also use microformats

Comment 5

[Mike Kaply [:mkaply]]

You are a gentleman and a scholar.

Comment 6

[Mike Kaply [:mkaply]]

We had some microformats tests. Does this pass them? I'm wondering how close the objects will be.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to Mike Kaply [:mkaply] from comment #6)
> We had some microformats tests. Does this pass them? I'm wondering how close
> the objects will be.

Glenn has done a lot of work on the test suite, I'm looking at how to integrate it into the codebase, but am not so familiar with mocha.  You can see his new branch here:

https://github.com/glennjones/microformat-shiv/tree/dev

update-tests pulls all the tests down from the test repo.

Comment 8

[Jet Villegas (inactive)]

(In reply to Mike Kaply [:mkaply] from comment #2)
> I've been using microformats-shiv as well. It's a much better implementation
> than what I wrote. My microformats API didn't get any traction (as far as I
> know).
> 
> I'm not sure it should even be in Firefox anymore...

Can we get a follow-up bug to pull it?

Comment 9

[Mike Kaply [:mkaply]]

(In reply to Jet Villegas (:jet) from comment #8)
> (In reply to Mike Kaply [:mkaply] from comment #2)
> > I've been using microformats-shiv as well. It's a much better implementation
> > than what I wrote. My microformats API didn't get any traction (as far as I
> > know).
> > 
> > I'm not sure it should even be in Firefox anymore...
> 
> Can we get a follow-up bug to pull it?

Well, it appears some components are starting to use it now (and we need to check add-ons). Maybe now's the time to actually try to get some traction.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: microformats-shiv

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: microformats-tests

Test infrastructure for microformats2 integrated to marionette.

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: microformats-share

PageMetaData and share support

Comment 13

[Mike Kaply [:mkaply]]

The results of the microformats-shiv are similar to what Microformats.js does.

Do you see any reason not to simply replace one with the other?

Comment 14

[Shane Caraveo (:mixedpuppy)]

(In reply to Mike Kaply [:mkaply] from comment #13)
> The results of the microformats-shiv are similar to what Microformats.js
> does.
> 
> Do you see any reason not to simply replace one with the other?

not really, there was only one addon on AMO that used the old parser.  removal can be a separate patch or bug.

Comment 15

[Tantek Çelik]

In addition to reviewing the current attachments, I am reviewing:
* the v1.3 update to microformats-shiv at https://github.com/glennjones/microformat-shiv and 
* the update to microformats-tests at https://github.com/microformats/tests

Comment 16

[Tantek Çelik]

r+ microformats-share
(still reviewing attachment + updates for microformats-shiv and microformats-tests)

Comment 17

[Shane Caraveo (:mixedpuppy)]

Attachment: microformats-tests

Comment 18

[Shane Caraveo (:mixedpuppy)]

Attachment: microformats-shiv

Comment 19

[Shane Caraveo (:mixedpuppy)]

Attachment: microformats-share

Comment 20

[Tantek Çelik]

(In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> Created attachment 8667997 [details] [diff] [review]
> microformats-tests

I don't seem to be able to review this attachment, e.g. when I go to https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1037483&attachment=8667997 I get the error message: "Attachment 8667997 [details] [diff] is not an attachment to bug 1037483" - Shane could you double-check the reviewer flag on this attachment?

Comment 21

[Tantek Çelik]

Still reviewing microformats-tests and microformats-shiv attachments and will be doing so this weekend til I've gotten through all the files in both!

Comment 22

[Tantek Çelik]

Comment on attachment 8667997 [details] [diff] [review]
microformats-tests

Review of attachment 8667997 [details] [diff] [review]:
-----------------------------------------------------------------

Very thorough number and combination of test files and harness. A few prose spelling errors in comments, but those can be fixed in a subsequent small patch.

Comment 23

[Tantek Çelik]

Comment on attachment 8667998 [details] [diff] [review]
microformats-shiv

Review of attachment 8667998 [details] [diff] [review]:
-----------------------------------------------------------------

A few prose spelling errors in comments, but those can be fixed in a subsequent small patch.

In addition, the following minor fixes can also be made in a subsequent small patch:
* Uses of ".indexOf(':')" should be more conservatively ".indexOf('://')" (no known examples where it would break either way)
* removeWhiteSpace should be renamed collapseWhiteSpace since it "leaves a single space" according to comments (and code)
* for modules.maps['h-recipe'] - 'tag': {} - should likely be like h-entry, h-review etc.
'category': {
				'map': 'p-category',
				'relAlt': ['tag']
},

I've updated the h-recipe spec accordingly to explicitly mention "p-category" in the properties list and for Parser Compat just like h-entry (omission was an oversight).

Comment 24

[Dietrich Ayala (:dietrich)]

(In reply to Tantek Çelik from comment #22)
> Very thorough number and combination of test files and harness. A few prose
> spelling errors in comments, but those can be fixed in a subsequent small
> patch.

A follow-up just to fix those will make it more difficult for future developers to dig through past changes. Let's just fix those now in this patch, while we're here. Should be super easy if it's not code change.

Comment 25

[Dietrich Ayala (:dietrich)]

There are many included bits of external code in the shiv patch. Prior to landing we need to get all the licensing for that code approved.

Mossop, what's the right way to do that? I usually email authors to get the code re-licensed under MPL, but want to confirm that's the correct path.

Comment 26

[Dave Townsend [:mossop]]

Looks like it is MIT licensed which I believe we can just include as-is but I defer licensing questions to someone who knows what they're talking about like Gerv.

Comment 27

[Dietrich Ayala (:dietrich)]

The shiv patch has a bunch of different things inside, pretty sure I saw some with different licenses than MIT.

Comment 28

[Glenn Jones]

dietrich and mossop - I put microformats-shiv under MIT, as it seems to be one of most widely know and understood of the family of permissive free licenses.  

If we can keep as MIT that would be great, I have nothing against MPL, but this code is used in other JavaScript projects. In some cases companies have a short list of pre-approved licenses of which MIT seems quite common. A dual license MIT/MPL would also be great, but I am not sure if that is possible? I would consider putting it under MPL only license as a last resort, but it would most likely reduce it uptake elsewhere which would be a shame.

There are two pieces of 3rd party code in microformats-shiv, one is under a BDS license the other has a public domain statement.  Interestingly neither of these two pieces of code is needed for Firefox over version 26.0; they are in the library as polyfills for missing/broken features in older browsers.

We could as dietrich suggests ask the authors of these two polyfills to relicense there code or I could change the project to produce a version of the microformats-shiv.js without the polyfills just for use in Firefox.  Updating the build to produce the version without the polyfills would be easy, about 30 mins work.

Comment 29

[Gervase Markham [:gerv]]

I would suggest updating the library to produce a non-polyfills version (why carry unused code). After that, MIT licensing is fine. You will need to add a copy of the MIT license to about:license (toolkit/content/license.html), in alphabetical order. It should be fairly obvious how to do that. Set r? to me on the patch.

Gerv

Comment 30

[Shane Caraveo (:mixedpuppy)]

Attachment: microformats-tests

Comment 31

[Shane Caraveo (:mixedpuppy)]

Attachment: microformats-shiv

Comment 32

[Glenn Jones]

Shanes last update to the patch hopefully addresses all current issues. As per Gerv comment, it now has no 3rd party polyfills and embedded licenses. It also addressed all of Tantek review points:

* Added p-category support to v1 hrecipe instead of tag{}
* Renamed removeWhiteSpace to collapseWhiteSpace
* Update test for protocol separator to indexOf('://')
* Corrected spelling errors in comments and try to make them clearer

Comment 33

[Tantek Çelik]

Comment on attachment 8671594 [details] [diff] [review]
microformats-shiv

Review of attachment 8671594 [details] [diff] [review]:
-----------------------------------------------------------------

Verified code fixes have been made. I didn't keep track of each specific comment typo - and this review tool does not show a diff from the previous patch. If any further typos are found in comments, they can be fixed in subsequent patches that will be trivial for future developers to look past (since such patches will actually show diffs, instead of this first patch of the code which even when updated here shows everything in its entirety as "new").

Comment 34

[Tantek Çelik]

Comment on attachment 8671593 [details] [diff] [review]
microformats-tests

Review of attachment 8671593 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't keep track of each specific comment typo - and this review tool does not show a diff from the previous patch. If any further typos are found in comments, they can be fixed in subsequent patches that will be trivial for future developers to look past (since such patches will actually show diffs, instead of this first patch of the code which even when updated here shows everything in its entirety as "new").

Comment 35

[Tantek Çelik]

(In reply to Dietrich Ayala (:dietrich) from comment #24)
> (In reply to Tantek Çelik from comment #22)
> > Very thorough number and combination of test files and harness. A few prose
> > spelling errors in comments, but those can be fixed in a subsequent small
> > patch.
> 
> A follow-up just to fix those will make it more difficult for future
> developers to dig through past changes. Let's just fix those now in this
> patch, while we're here. Should be super easy if it's not code change.

Dietrich, looks like all fixes and other reviews are in. Could you take a quick look to make sure your concerns are handled so we can land this?

Comment 36

[Dietrich Ayala (:dietrich)]

Comment on attachment 8667999 [details] [diff] [review]
microformats-share

r+, just a few questions below.

>-      gBrowser.selectedBrowser.messageManager.sendAsyncMessage("PageMetadata:GetMicrodata", null, { target });
>+      gBrowser.selectedBrowser.messageManager.sendAsyncMessage("PageMetadata:GetPageData", null, { target });

GetMicroData is removed entirely? If so, have you taken a look at how many consumers of this exist, eg in the wild in add-ons? (we can search source of all add-ons on AMO).

> var PageMetadataMessenger = {
>   init() {
>     addMessageListener("PageMetadata:GetPageData", this);
>-    addMessageListener("PageMetadata:GetMicrodata", this);
>   },
>   receiveMessage(message) {
>     switch(message.name) {
>       case "PageMetadata:GetPageData": {
>-        let result = PageMetadata.getData(content.document);
>+        let target = message.objects.target;
>+        let result = PageMetadata.getData(content.document, target);

Hm, is message.objects guaranteed to be not null?


>@@ -35,20 +36,21 @@ this.PageMetadata = {
>    * - title
>    * - Metadata specified in <meta> tags, including OpenGraph data
>    * - Links specified in <link> tags (short, canonical, preview images, alternative)
>    * - Content that can be found in the page content that we consider useful metadata
>    * - Microdata, as defined by the spec:
>    *   http://www.whatwg.org/specs/web-apps/current-work/multipage/microdata-2.html
>    *
>    * @param {Document} document - Document to extract data from.
>+   * @param {Element} [target] - Optional element to restrict microdata lookup to.
>    * @returns {Object} Object containing the various metadata, normalized to
>    *                   merge some common alternative names for metadata.
>    */
>-  getData(document) {
>+  getData(document, target = null) {

why nullifying that?

>@@ -63,21 +65,26 @@ this.PageMetadata = {
>           shentry.value && shentry.value.URIWasModified) {
>         return result;
>       }
>     }
> 
>     this._getMetaData(document, result);
>     this._getLinkData(document, result);
>     this._getPageData(document, result);
>-    result.microdata = this.getMicrodata(document);
>+    result.microdata = this.getMicrodata(document, target);
>+    result.microformats = this.getMicroformats(document, target);
>

Those are sync operations - can they block UI?

Comment 37

[Dietrich Ayala (:dietrich)]

Comment on attachment 8671593 [details] [diff] [review]
microformats-tests

These are tests for the shiv. Are there tests for the priv js code that you've changed in these patches?

Comment 38

[Dietrich Ayala (:dietrich)]

(In reply to Dietrich Ayala (:dietrich) from comment #37)
> Comment on attachment 8671593 [details] [diff] [review]
> microformats-tests
> 
> These are tests for the shiv. Are there tests for the priv js code that
> you've changed in these patches?

To clarify: Are there pre-existing tests for the chrome microformat code in Firefox to make sure that 1) it works as expected and 2) it doesn't get regressed by other future changes in Firefox?

If not, this is the time to write them. Should be trivial to do so - mozilla-central contains many examples of writing tests that check that chrome JS handles web content in the expected way.

Comment 39

[Shane Caraveo (:mixedpuppy)]

(In reply to Dietrich Ayala (:dietrich) from comment #37)
> Comment on attachment 8671593 [details] [diff] [review]
> microformats-tests
> 
> These are tests for the shiv. Are there tests for the priv js code that
> you've changed in these patches?

There are pagedata tests in the socialapi stuff

Comment 40

[Dietrich Ayala (:dietrich)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #39)
> (In reply to Dietrich Ayala (:dietrich) from comment #37)
> > These are tests for the shiv. Are there tests for the priv js code that
> > you've changed in these patches?
> 
> There are pagedata tests in the socialapi stuff

Do those run per-push, and do they exercise this code? And how thoroughly?

Comment 41

[Shane Caraveo (:mixedpuppy)]

(In reply to Dietrich Ayala (:dietrich) from comment #40)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #39)
> > (In reply to Dietrich Ayala (:dietrich) from comment #37)
> > > These are tests for the shiv. Are there tests for the priv js code that
> > > you've changed in these patches?
> > 
> > There are pagedata tests in the socialapi stuff
> 
> Do those run per-push, and do they exercise this code? And how thoroughly?

They are mochitests, and do excercise this code pretty well, but not specifically the microformats part.  I can easily add a simple test for that.

On the comments on the other patch, I can leave GetMicroData in, it's no big deal.  Blocking UI may be an issue with the call to getMicroformats.

Comment 42

[Tantek Çelik]

Shane, Dietrich, we're all here at Mozlando this week - let's set aside an hour to resolve any outstanding questions and land this!


Regarding:

> but not specifically the microformats part.  I can easily add a simple test for that.

Shane, do you think you can take care of this in the next day?


> On the comments on the other patch, I can leave GetMicroData in, it's no big deal.

I'd rather drop if there's no good reason to keep. Let's discuss in person to understand why/why not.


> Blocking UI may be an issue with the call to getMicroformats.

Can we resolve this "may" with an actual or not? Blocking UI is a pretty bad thing in any situation, and especially bad as anything to be introduce. I'd like to understand *how* "Blocking UI may be an issue", and preferably falsify that assertion so that we can respond to any questions in the future with a "no" and "here's why".

Comment 43

[Dietrich Ayala (:dietrich)]

Perf code-review IRL at Orlando: Nothing blocks page load by default. Only action occurs upon user interaction with Share. There's some cost on microformat detection, but is in response to that user action only. As long as tryserver run is clean, r=me for landing.

Comment 44

[Shane Caraveo (:mixedpuppy)]

TODO:

1. add microformats test to PageMetaData module tests.
2. pull out microdata support (for bug 909633)

note: microformats will block UI if called, this only happens when a user specifically takes action to share.  [performance data to follow]

Performance results

Averaged over 3 page renders. Laptop used was a MacBook Pro (Retina, Mid 2014) and phone used was a HTC M8. Used latest browsers 2015-07-21

Microformats.count()
site                        Firefox Fennec  Chrome  Chrome for Andriod  Opera  Safari
bbc.co.uk (no micorformats)     4ms   33ms     3ms                45ms    4ms     2ms
glennjones.net/notes            5ms   60ms     3ms                28ms    2ms     3ms
tantek.com                      5ms   64ms     7ms                52ms    4ms     3ms

Microformats.getParent()
site                        Firefox Fennec  Chrome  Chrome for Andriod  Opera  Safari
bbc.co.uk (no micorformats)     2ms    7ms     1ms                 5ms    1ms     1ms
glennjones.net/notes           18ms  192ms    14ms               101ms   10ms     7ms
tantek.com                     16ms  237ms    12ms               116ms   10ms     9ms

Microformats.get()
site                        Firefox Fennec  Chrome  Chrome for Andriod  Opera  Safari
bbc.co.uk (no micorformats)     5ms   61ms     6ms                42ms*   4ms     2ms
glennjones.net/notes           91ms  790ms    68ms               503ms   76ms    46ms
tantek.com                    163ms 1481ms   116ms               980ms  120ms    59ms
* Random results in 170ms range seen

Comment 45

[Shane Caraveo (:mixedpuppy)]

note: socialapi share is the only current user of microformats, performance notes for desktop are relevant there, fennec will only be relevant if microformats is used there in the future.

Comment 46

[Glenn Jones]

Sorry I missed the remoting into code-review at Orlando. Was offline for a couple of days and did not get Tanteks invite at the time. Its great to see this moving again.

Comment 47

[Shane Caraveo (:mixedpuppy)]

Attachment: microformats-share

update the share patch replacing microdata with microformats (updates tests as well).

Comment 48

[Shane Caraveo (:mixedpuppy)]

Glenn,
The shiv is used from a content script where we do not have the URL class.  We need to address that in modules.url.resolve somehow.  Sorry, this only showed up once I revamped all the chrome side tests.

Comment 49

[Glenn Jones]

Shane, I think we have a couple of options. The first is to back track and reinstate Gary Court uri-js http://github.com/garycourt/uri-js which works fine, but has a custom license which is one of the reasons we took it out. 

The second is to make use of the DOMParser object create a very small html doc with a <base> and <a> tag. The resulting tiny DOM object will allows to get a the resolved URL. On one hand this is great as it uses the browsers internal URL parsing, but could be slower if there are lots a URLs to revolve. The code for resolving URLs is only called for URL data in a discovered microformat and then only if the URL does not start with a protocol schema i.e. http://

In terms a getting this out I would go with DOMParser options, will not legal review. Happy to knock up the code

Comment 50

[Shane Caraveo (:mixedpuppy)]

Lets go with the domparser.  Could it be cached and only replaced if the baseurl changes?  Maybe also do the check for typeof(URL) and use it if it is available?

Comment 51

[Glenn Jones]

OK, just about to go out for evening will have a look at in the morning. Caching it so we only create a single instance pre a parse is a good idea. I known from past work you can change the <base> in an existing and it updates a absolute paths you can get from <a> tag.  

The use of typeof(URL) is a bit more of an issue, I wrote about while testing shiv. Short answer is IE team only built one of the two URL object spec so you cannot use testing without breaking the whole script in IE 
In depth - http://glennjones.net/articles/the-problem-with-window-url

Comment 52

[Shane Caraveo (:mixedpuppy)]

Ok, lets just do what will work best.

Comment 53

[Glenn Jones]

OK, microformats shiv is now updated to use the DOMParser method when the URL API is not available. I have tested the performance which does not seem to be an issue at all.

Using DOMParser Method – With cached DOM object and links to nodes:

Desktop - Avg time to resolve URL 0.014 milliseconds for 400k calls
Android - Avg time to resolve URL 0.080 milliseconds for 400k calls  (HTC OneX Android 4)

Shane if you use the upgrade script it should pull the new version.

Any problems let me know

Comment 54

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a211b21510a3

Comment 55

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96fe24b9aaf0

Comment 56

[Shane Caraveo (:mixedpuppy)]

Attachment: microformats v2

consolidated microformats v2 patch.  carry forward r+ from tantek.  addresses jsm specific issues with URL and DOMParser.

Comment 57

[Shane Caraveo (:mixedpuppy)]

Attachment: replace microdata with microformats in socialapi

removes microdata, adds microformats, updates tests for microformats.  carry forward r+ from dietrich

Comment 58

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40b398cb67de

Comment 59

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b78689f8c98

Comment 60

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b03f780a4fc

Comment 61

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ce5f71ae533

Comment 62

[Shane Caraveo (:mixedpuppy)]

Small update...

try tests are failing (win/lin) because of some difference in test packaging.  On osx the entire toolkit/components/microformats directory is packaged, allowing the tests to include the shiv.  On win/lin only toolkit/components/microformats/test is packaged, and all tests fail because the shiv is unavailable.  Will continue to track down a fix for this.

Comment 63

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc97ebf71fd4

Comment 64

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac6088cdfab7

Comment 65

[Shane Caraveo (:mixedpuppy)]

Attachment: microformats v2

The fix was to move the manifest.ini file into the microformats directory.  Looks like we're passing now.  carry forward the r+

Comment 66

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/c697940b6f5c7e2ea8a422c1bf13911dc380ba43
Bug 1037483 adopt microformats-shiv for microformats v2 support, r=tantek

https://hg.mozilla.org/integration/fx-team/rev/de364648e24c790ce7e342e17df41a6ec00dd759
Bug 1037483 replace microdata with microformats in share, r=dietrich

Comment 67

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This caused several ESLint failures: https://treeherder.mozilla.org/logviewer.html#?job_id=6857389&repo=fx-team

Backed out in https://hg.mozilla.org/integration/fx-team/rev/1a56a997f5d5

Comment 68

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/ed69cf9c7396135de51ae9f620e851308fd4a34b
Bug 1037483 adopt microformats-shiv for microformats v2 support, r=tantek

https://hg.mozilla.org/integration/fx-team/rev/96f4549cab327edecf945365a6c2637caef1eecf
Bug 1037483 replace microdata with microformats in share, r=dietrich

Comment 69

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ed69cf9c7396
https://hg.mozilla.org/mozilla-central/rev/96f4549cab32

Comment 70

[Tantek Çelik]

:mixedpuppy, the fix for this bug did not remove the old microformats parser AFAIK, should we create another bug to remove the old one? does it have any remaining dependencies?
:mkaply - any objections?

Comment 71

[Shane Caraveo (:mixedpuppy)]

Tantek, removal would be blocked by bug 1237480.  We should do a new bug for removal.