DocInfo: replace metaTags with viewport in API by mdmower · Pull Request #26687 · ampproject/amphtml

mdmower · 2020-02-09T21:29:23Z

Add getMeta to AmpDoc
Replace metaTags with viewport in DocInfo API and propagate change
Update documentLoaded message to viewer to include viewport property,
but still provide metaTags.viewport for backwards compatibility
Update amp-doc-viewer documentation
Update unit tests with phony document to include head

mdmower · 2020-02-09T21:34:37Z

Alternatives to this PR:

AmpDoc.getMeta and AmpDoc.get/setMetaByName could move out of AmpDoc and live in DocInfo instead
DocInfo.metaTags could be removed from DocInfo entirely

mdmower · 2020-02-09T22:23:14Z

src/service/ampdoc-impl.js

+
+      // Retain only the first meta content value for a given name, consistent
+      // with getMetaByName.
+      if (metaMap[name] === undefined) {


If metaMap[name] !== undefined), should we emit user().warn('AmpDoc', 'Only the first <meta> tag with a given name is retained');?

dreamofabear · 2020-02-11T16:49:17Z

src/consent.js

-    'amp-consent-blocking'
-  ];
-
+  let content = ampdoc.getMetaByName('amp-consent-blocking');


Nice! I see a lot of other dedupe opportunities when searching for "meta[name=" in the codebase. Would you mind filing a cleanup issue for this?

This? #26652

🥇Thanks! much cleaner

dreamofabear · 2020-02-11T16:49:27Z

src/service/ampdoc-impl.js

+      }
+
+      // Retain only the first meta content value for a given name, consistent
+      // with getMetaByName.


Not sure this is safe to do since there may be code that's relying on the existing "concat" behavior (instead of only retaining the first meta content).

I think it is safe, actually. There are very few uses of DocInfo.metaTags, and the array output is never used (in fact, in one place, the array behavior isn't even considered and I was surprised type checking didn't report an error).

Thanks for checking.

dreamofabear · 2020-02-11T18:53:03Z

There are very few uses of DocInfo.metaTags

Appears so. The only other one I saw was in link-rewriter-manager.js which probably should use AmpDoc.getMetaByName() anyways.

I'd be cool with this alternative:

DocInfo.metaTags could be removed from DocInfo entirely

dreamofabear · 2020-02-11T18:54:47Z

Actually looks like we send metaTags to the viewer in the "documentLoaded" message:

amphtml/src/service/resources-impl.js

Lines 783 to 793 in 082e2d6

    
           this.viewer_.sendMessage( 
        
             'documentLoaded', 
        
             dict({ 
        
               'title': doc.title, 
        
               'sourceUrl': getSourceUrl(this.ampdoc.getUrl()), 
        
               'serverLayout': doc.documentElement.hasAttribute('i-amphtml-element'), 
        
               'linkRels': documentInfo.linkRels, 
        
               'metaTags': documentInfo.metaTags, 
        
             }), 
        
             /* cancelUnsent */ true 
        
           );

Added in #14137. Let me follow up internally to see if this is actually used.

dreamofabear · 2020-02-11T22:15:46Z

It's only used for meta[name=viewport], so it's safe to remove if we send only the content for "viewport" in the "documentLoaded" viewer message in resources-impl.js.

We should also update amp-doc-viewer-api.md to reflect this change.

mdmower · 2020-02-20T05:05:30Z

It's only used for meta[name=viewport], so it's safe to remove if we send only the content for "viewport" in the "documentLoaded" viewer message in resources-impl.js.

We should also update amp-doc-viewer-api.md to reflect this change.

Let me know if the latest version of this PR matches your description, or if you had something else in mind. Thanks!

dreamofabear · 2020-02-20T19:29:19Z

Oh sorry for the delay, will take a look soon! 👍

dreamofabear · 2020-02-20T19:31:40Z

src/service/resources-impl.js

          'serverLayout': doc.documentElement.hasAttribute('i-amphtml-element'),
          'linkRels': documentInfo.linkRels,
-          'metaTags': documentInfo.metaTags,
+          'viewport': documentInfo.viewport,


Ah, sorry I meant we still need metaTags in the message contents, but it's OK if it only contains meta[name=viewport].

I.e. I think this should be:

'metaTags': {'viewport': documentInfo.viewport}, 'viewport': documentInfo.viewport,

Feel free to drop a TODO (or new issue) to rename this once the callers have been migrated to use viewport instead of metaTags.

I'm happy to do this, but I'm curious where this request to retain DocumentInfo.metaTags['viewport'] is coming from? After this PR, if you grep for metaTags in this repository, you won't find a single hit. So apparently there's a consumer of this API outside of AMPHTML?

Ohhh, oops. I see now. It's not that DocumentInfo.metaTags needs to be retained, it's that the documentLoaded message sent in src/service/resources-impl.js needs to avoid an API breaking change (between the document and the viewer). Got it, sorry, should have looked closer.

mdmower · 2020-02-21T08:44:53Z

FYI, it looks like the bundle size test on Travis is not going to complete unless I rebase, but all code comments will be wiped out if I force push. I'll wait for a "go ahead" before rebasing. Until then, code comments threads can continue.

- Add getMeta to AmpDoc - Replace metaTags with viewport in DocInfo API and propagate change - Update 'documentLoaded' message to viewer to include viewport property, but still provide metaTags.viewport for backwards compatibility - Update amp-doc-viewer documentation - Update unit tests with phony document to include head

mdmower · 2020-02-23T01:30:32Z

This is ready for review by an OWNER of extensions/amp-viewer-integration/amp-doc-viewer-api.md.

dreamofabear · 2020-02-24T22:07:59Z

@jridgewell Can you approve amp-doc-viewer-api.md for owners?

jridgewell · 2020-02-25T20:01:48Z

src/consent.js

-    .toUpperCase()
-    .replace(/\s/g, '')
-    .split(',');
+  content = content.toUpperCase().replace(/\s/g, '');


Nit: for faster execution

Suggested change

content = content.toUpperCase().replace(/\s/g, '');

content = content.toUpperCase().replace(/\s+/g, '');

jridgewell · 2020-02-25T20:08:31Z

src/service/ampdoc-impl.js

+   * @return {!Object<string, string>}
+   */
+  getMeta() {
+    const metaMap = map();


Can we cache this?

Commit 555d668 takes a stab at this. Let me know if you agree with the implementation.

jridgewell · 2020-02-25T20:09:58Z

src/service/ampdoc-impl.js

+  /** @override */
+  getMeta() {
+    const copy = map();
+    for (const k in this.meta_) {


Object.assign?

Thanks, adopted your suggestion by way of map(opt_initial)

jridgewell · 2020-02-25T20:10:58Z

src/service/document-info-impl.js

-  }
-  return metaTags;
+function getViewport(doc) {
+  const viewportEl = doc.head.querySelector('meta[name="viewport"]');


Why is this being specialized to viewport metas?

It's the only meta that's currently being used.

- Cache name/content pairs - Minor optimizations

Cached meta tags feature requires stubbing AmpDoc.getMeta to ensure old meta tags are reused in future tests.

* master: (54 commits) inabox-resources: Minor test improvement (ampproject#26916) DocInfo: replace metaTags with viewport in API (ampproject#26687) 🐛 SwG now uses AMP sendBeacon interface (ampproject#26970) 🏗 Allow array destructuring on preact hooks (ampproject#26901) Gulp Dep Check: fail on unused entries (ampproject#26981) Update no-import lint rule to forbid sub-paths (ampproject#26531) 🐛 amp-ad type blade - fix bladeOnLoad callback (ampproject#26627) 📖 Clarify when max-age is required (ampproject#26956) ♻️ Consolidate players as .i-amphtml-media-component (ampproject#26967) Add Preact Enzyme tests (ampproject#26529) Fixes `update_tests` flag on `gulp validator` (ampproject#26965) 📦 Update dependency google-closure-library to v20200224 (ampproject#26986) 🏗 Transform aliased configured components (ampproject#26541) ✨ InaboxResources: Observe intersections for some elements' viewportCallbacks (ampproject#26942) variable substitutions: Support allowlist lookup in AmpDocShadow (ampproject#26731) cl/297197875 Revision bump for ampproject#26877 (ampproject#26982) Json fix (ampproject#26971) 📦 Update dependency mocha to v7.1.0 (ampproject#26976) Add documentation for amp-access-scroll (ampproject#26782) make controls always shown in amp for email (ampproject#25714) ...

Cached meta tags feature requires stubbing AmpDoc.getMeta to ensure old meta tags are reused in future tests.

* master: Launch `amp-next-page` v2 & clean up experiment (ampproject#27035) ✨Implement Display Locking on amp-accordion (ampproject#25867) 📖<amp-next-page> documentation and validation (ampproject#26841) Improve visibility event tests (ampproject#26847) amp-geo: Fall back to API for country (ampproject#26407) ✨ Add customization options to <amp-story-quiz> (ampproject#26714) navigation: Minor test improvements (ampproject#26926) ♻️ Alias video.fullscreen action for symmetry with interface names (ampproject#27017) ✨ Implements `amp-analytics` support for `amp-next-page` (ampproject#26451) ✨ amp-video-iframe integration: global jwplayer() singleton by default (ampproject#26969) Fix unit tests broken by ampproject#26687 (ampproject#27000) Filter redirect info from emails (ampproject#27012) 📖 Make amp-bind doc valid, fix amp-form stories filter (ampproject#27003) url-replacements: Minor test improvement (ampproject#26930) viewport: Minor test improvement (ampproject#26931) amp-consent fullscreen warning (ampproject#26467) dep-check: USE CAPS FOR IMPORTANCE (ampproject#26993) fix img url (ampproject#26987) # Conflicts: # extensions/amp-next-page/amp-next-page.md

googlebot added the cla: yes label Feb 9, 2020

amp-owners-bot bot requested review from dreamofabear and zhouyx February 9, 2020 21:29

mdmower mentioned this pull request Feb 9, 2020

Evaluate uses of meta.getAttribute('content') #26652

Closed

33 tasks

mdmower commented Feb 9, 2020

View reviewed changes

dreamofabear reviewed Feb 11, 2020

View reviewed changes

mdmower mentioned this pull request Feb 12, 2020

ampdoc: Relax usage requirements of getMetaByName #26729

Closed

mdmower requested a review from dreamofabear February 12, 2020 07:00

mdmower changed the title ~~DocInfo: Use AmpDoc getMeta for meta name/content~~ DocInfo: replace metaTags with viewport in API Feb 12, 2020

dreamofabear reviewed Feb 20, 2020

View reviewed changes

dreamofabear approved these changes Feb 21, 2020

View reviewed changes

mdmower force-pushed the pr-docinfo branch from eb7c966 to 4746fcc Compare February 22, 2020 18:45

samouri requested a review from chenshay February 24, 2020 21:50

samouri assigned mdmower Feb 24, 2020

jridgewell reviewed Feb 25, 2020

View reviewed changes

Revions from review

555d668

- Cache name/content pairs - Minor optimizations

jridgewell approved these changes Feb 26, 2020

View reviewed changes

mdmower merged commit 88e75b8 into ampproject:master Feb 27, 2020

mdmower deleted the pr-docinfo branch February 27, 2020 02:56

mdmower added a commit to mdmower/amphtml that referenced this pull request Feb 27, 2020

Fix unit tests broken by ampproject#26687

b96bf70

Cached meta tags feature requires stubbing AmpDoc.getMeta to ensure old meta tags are reused in future tests.

rcebulko pushed a commit that referenced this pull request Feb 27, 2020

Fix unit tests broken by #26687 (#27000)

59d5517

Cached meta tags feature requires stubbing AmpDoc.getMeta to ensure old meta tags are reused in future tests.

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Mar 10, 2020

This was referenced May 10, 2022

🚨 Error: undefined is not a function ampproject/error-reporting#158

Open

🚨 Error: this.win.document.head.querySelectorAll(...).forEach is not a function ampproject/error-reporting#161

Open

	content = content.toUpperCase().replace(/\s/g, '');
	content = content.toUpperCase().replace(/\s+/g, '');

Conversation

mdmower commented Feb 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdmower commented Feb 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreamofabear commented Feb 11, 2020

Uh oh!

dreamofabear commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dreamofabear commented Feb 11, 2020

Uh oh!

mdmower commented Feb 20, 2020

Uh oh!

dreamofabear commented Feb 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdmower Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdmower commented Feb 21, 2020

Uh oh!

mdmower commented Feb 23, 2020

Uh oh!

dreamofabear commented Feb 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mdmower commented Feb 9, 2020 •

edited

Loading

dreamofabear commented Feb 11, 2020 •

edited

Loading

mdmower Feb 21, 2020 •

edited

Loading