Skip to content
This repository has been archived by the owner on Aug 10, 2022. It is now read-only.

Add WIP Media Session API updates article #4057

Merged
merged 10 commits into from
Jan 31, 2017

Conversation

beaufortfrancois
Copy link
Member

This PR is still WIP and is used to get feedback from engineers.

WDYT @mounirlamouri @xxyzzzq?

Copy link

@xxyzzzq xxyzzzq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I only found some minor issues.

In the image:

  • The second MediaImage has no "{".
  • Action handlers currently take no arguments.
  • Disable spell check when making the final version?

}

function getNetworkArtwork(request) {
// Fetch network artwork.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indent 2 spaces

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is fine as is.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is a 3-space indent and the other functions have a 2-space indent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


{# wf_updated_on: 2017-02-06 #}
{# wf_published_on: 2017-02-06 #}
{# wf_tags: news,mediasession,play,pause #}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build Errors for uncommon tags. You can add these tags in src/data/commonTags.json. Be sure to review the file first to make sure you're not creating similar tags.

  • src/content/en/updates/2017/02/media-session.md Uncommon tag (mediasession) found.
  • src/content/en/updates/2017/02/media-session.md Uncommon tag (pause) found.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know this file! Thank you ;)
Fixed.

{# wf_updated_on: 2017-02-06 #}
{# wf_published_on: 2017-02-06 #}
{# wf_tags: news,mediasession,play,pause #}
{# wf_featured_image: /web/updates/images/2017/02/featured.png #}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build Error:

  • src/content/en/updates/2017/02/media-session.md WF Tag wf_featured_image found, but couldn't find image - /web/updates/images/2017/02/featured.png

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

{# wf_published_on: 2017-02-06 #}
{# wf_tags: news,mediasession,play,pause #}
{# wf_featured_image: /web/updates/images/2017/02/featured.png #}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly encourage also including a {# wf_featured_snippet: blah blah blah #}.

The featured snipped is used on the index pages as the primary description. It CAN include HTML and is your "hook" to get someone to read your article. The description at the top of the document is used as the meta description and is only used as the primary description if wf_featured_snippet isn't provided.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. Thanks!


{# wf_updated_on: 2017-02-06 #}
{# wf_published_on: 2017-02-06 #}
{# wf_tags: news,mediasession,play,pause #}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add chrome57 to the list of tags.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

If you don't want to play audio right after the first interaction, I recommend
you use the `load()` method of the audio element.

<pre class="prettyprint">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of wrapping in pretty print, the preferred method is to indent four spaces. I think this works, but there may also be some weirdness around it if I remember right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this particular code sample, I'm using <strong>audio.load()</strong> which is not parsed correctly when using four spaces indentation. If there's a way to get it without prettyprint, I'm all for it ;)

<div class="clearfix"></div>
<div class="attempt-left">
<figure>
<img src="https://placehold.it/350x350?text=With no Media Session" alt="TODO">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alt="TODO"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

</div>
<div class="attempt-right">
<figure>
<img src="https://placehold.it/350x350?text=With Media Session" alt="TODO">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alt="TODO"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

<div class="clearfix"></div>
<div class="attempt-left">
<figure>
<img src="https://placehold.it/350x350?text=Lock screen" alt="TODO">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alt="TODO"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

</div>
<div class="attempt-right">
<figure>
<img src="https://placehold.it/350x350?text=Wear notification" alt="TODO">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alt="TODO"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

}

function getNetworkArtwork(request) {
// Fetch network artwork.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is fine as is.

@petele petele requested a review from jpmedley January 18, 2017 15:13

As you may know, `autoplay` is disabled on Chrome for Android which
means we have to use the `play()` method of the audio element there. This
method must be triggered by [a user gesture] such as a touch or a mouse click.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does [ ] do here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It automagically adds URLs when they're defined at the very bottom of the file.

events. In other words, the user must click on a button before your web app can
actually make some noise.

playButton.addEventListener('pointerup', function(event) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not 'click'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pointerup is what we promote nowadays.

events. In other words, the user must click on a button before your web app can
actually make some noise.

playButton.addEventListener('pointerup', function(event) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/function (event)/e =>/ ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to use arrow function for non addEventListener stuff as it creates confusion when it's not needed.


let audio = document.querySelector('audio');

welcomeButton.addEventListener('pointerup', function(event) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same answer.

});

If you don't want to play audio right after the first interaction, I recommend
you use the `load()` method of the audio element.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can explain that the reason is that Blink will keep track of whether the user interacted with the element in some ways and load() is one of them. It will also help the playback be more smooth because the content will be loaded.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Explanation added.

## Implementation nits

- Chrome Media notifications show only if media files duration is [at least 5 seconds].
- Muted media files won't trigger Chrome media notifications.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not entirely true. <video autoplay muted> will not. Anything else muted will. Even the former case will if it gets unmuted then re-muted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I'll remove this nit.


- Chrome Media notifications show only if media files duration is [at least 5 seconds].
- Muted media files won't trigger Chrome media notifications.
- Media notifications use the favicon if no artworks are defined.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rephrase:
- If no artwork is define and there is a favicon at a desirable size, media notifications will use it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, the full logic is:
If no artwork is defined, no suitable image is available, or image download failed, media notification will fallback to a favicon at a desirable size if available.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xxyzzzq If image download failed, it doesn't always fallback to favicon right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We fallback to favicon in this case. If not then it should be a bug :)


- Chrome Media notifications show only if media files duration is [at least 5 seconds].
- Muted media files won't trigger Chrome media notifications.
- Media notifications use the favicon if no artworks are defined.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might also want to check your "artwork" vs "artworks".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you're right! Sorry for that ;)

[low-end devices], it is `256x256`.
- Dismiss media notifications with `audio.src = ''`.
- Hook up an `<audio>` element as the input source to the [Web Audio API] makes
it work with the Media Session API.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you have a short paragraph/section about audio focus? This might sound weird but if we were to explain that Media Session API only applies when Chrome will take audio focus could help some readers understand more deeply what's happening.

Copy link
Member Author

@beaufortfrancois beaufortfrancois Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mounirlamouri Do you mind sharing an abstract?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


At the time of writing, Chrome for Android is the only platform that supports
the Media Session API. More up-to-date information on browser implementation
status can be found on [chromestatus.com].

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact link to the entry maybe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is available at the very bottom of the page.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this should be a link to the exact entry.

@beaufortfrancois
Copy link
Member Author

@xxyzzzq The hero image was still WIP as well. Next patch will include the "final" one.

@beaufortfrancois
Copy link
Member Author

I've just addressed @petele @xxyzzzq and @mounirlamouri feedback in bd09396

@beaufortfrancois
Copy link
Member Author

@jakearchibald Do you mind having a look at this Media Session article? I'd love to get your feedback on the Service Worker / Cache part.

@petele petele requested a review from jakearchibald January 19, 2017 15:00
@jakearchibald
Copy link
Contributor

Does the developer have to do anything to release the session once playback is done?

With the brand new [Media Session API], you can now **customize media
notifications** by providing metadata information such as the title, artist,
album name, and artwork of the media (audio or video) your web app is playing
in Chrome 57 (beta in February 2017, stable in March 2017). It also
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty long first sentence with a lot of commas and parenthesis. I'd lose "such as the title, artist,
+album name, and artwork" and maybe "(audio or video)" too.

You can cover the details later in the article. In fact, the image below covers a lot of the details really well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

## Gimme What I Want

You already know all about the Media Session API and are simply coming back to
copy and paste with no shame some boilerplate code? So here it is.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

<source src="audio.ogg" type="audio/ogg"/>
</audio>

Note: I could have used a `<video>` tag as well there to showcase the Media
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As well, or instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Note: I could have used a `<video>` tag as well there to showcase the Media
Session API.

As you may know, `autoplay` is disabled on Chrome for Android which
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It work for some types of media doesn't it? Dunno if that's worth clarifying.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added for audio elements. Thanks!

Session API.

As you may know, `autoplay` is disabled on Chrome for Android which
means we have to use the `play()` method of the audio element there. This
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"there" where?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nowhere ;)

playAudio();
});

The Media Session API allows you as well to show "Seek Backward" and "Seek
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Remove "as well"

It's kinda suggested here, but is it worth being explicit that these buttons won't be shown unless you add a handler?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

.then(cache => cache.matchAll())
.then(responses => Promise.all(responses.map(r => r.blob())))
.then(blobs => blobs.map(blob => blob.size).reduce((acc, cur) => acc + cur, 0))
.then(cacheSize => { console.log('Artworks cache is ' + cacheSize + ' bytes.'); })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to bring all the artworks into memory. Alternatively you could add up the content-length headers, and make estimates where this header is missing.

Copy link
Member Author

@beaufortfrancois beaufortfrancois Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the code below I've used to add up content-length headers. It gives me different results than using blob.size()...

caches.open('artworks-cache-v1')
.then(cache => cache.matchAll())
.then(responses => {
  let cacheSize = 0;
  let queue = Promise.resolve();
   responses.forEach(response => {
    let responseSize = response.headers.get('content-length');
    if (responseSize) {
      queue = queue.then(_ => cacheSize += Number(responseSize));
    } else {
      queue = queue.then(_ => response.blob())
          .then(blob => cacheSize += blob.size);
    }
  });
  return queue.then(_ => {
    console.log('Artworks cache is ' + cacheSize + ' Bytes.'); 
  });
})
.catch(error => { console.log(error); });

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakearchibald What do you think of using this code?

caches.open('artworks-cache-v1')
.then(cache => cache.matchAll())
.then(responses => {
  let cacheSize = 0;
  let queue = Promise.resolve();
  responses.forEach(response => {
    queue = queue.then(_ => response.blob())
          .then(blob => { cacheSize += blob.size; blob.close(); });
  });
  return queue.then(_ => {
    console.log('Artworks cache is ' + cacheSize + ' Bytes.'); 
  });
})
.catch(error => { console.log(error); });

Copy link
Member Author

@beaufortfrancois beaufortfrancois Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


At the time of writing, Chrome for Android is the only platform that supports
the Media Session API. More up-to-date information on browser implementation
status can be found on [chromestatus.com].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this should be a link to the exact entry.


## Samples & Demos

Check out our official [Media Session Chrome samples].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broken link

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The media session sample has not been reviewed yet.

album name, and artwork of the media (audio or video) your web app is playing
in Chrome 57 (beta in February 2017, stable in March 2017). It also
allows you to **handle media related events** such as seeking or track
changing which may come from notifications or media keys.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe link to a demo here? As in, show it working, then go on to explain how it works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@jakearchibald
Copy link
Contributor

Really enjoyed this article!

@jakearchibald
Copy link
Contributor

However, dare I say it… is the service worker stuff a little off-topic? (didn't want to say this, because, of course I want everything to be about service worker)

@beaufortfrancois
Copy link
Member Author

beaufortfrancois commented Jan 20, 2017

Thanks for reviewing @jakearchibald.
I've addressed all your feedback except for the blob's one yet as I'd like to find a way to not rely on content-length.

Re: Service Worker, I think the offline story needs to be told "there". It makes sense especially when you play with the Media Session API on a lie-fi connection.

@beaufortfrancois
Copy link
Member Author

@petele Is there a staging server for PRs where we can tell people to look at?

@beaufortfrancois
Copy link
Member Author

@jpmedley It is not yet finalized but I think it is in a "good shape" for you to have a look. Thank you.

@petele
Copy link
Member

petele commented Jan 24, 2017

@beaufortfrancois - sadly no staging server for folks to look at right now. :(

{% include "web/_shared/contributors/beaufortfrancois.html" %}

With the brand new [Media Session API], you can now **customize media
notifications** by providing metadata information of the media your web app is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think this should be 'for the media' not 'of the media'.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


With the brand new [Media Session API], you can now **customize media
notifications** by providing metadata information of the media your web app is
playing in Chrome 57 (beta in February 2017, stable in March 2017). It also
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should probably add a full stop after 'playing' and then start a new sentence with 'MediaSession is supported in Chrome 57 ...'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a full stop and moved the "supported" sentence. WDYT?

<source src="audio.ogg" type="audio/ogg"/>
</audio>

Note: I could have used a `<video>` tag instead to showcase the Media Session
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: `the MediaSession API.'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually the "Media Session API", not "MediaSession API" according to https://wicg.github.io/mediasession/


### Let's play 🎷

Add a simple `<audio>` tag to your web page and assign several media sources so
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: should probably say '<audio> element' and '<video> element'.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Done.

// Do something more than just pausing current audio...
});

Note: As the browser may consider the web app is not be playing when media
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'The browser may consider that the web app is not playing media when files are seeking or loading. You can override ...'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Note: As the browser may consider the web app is not be playing when media
files are seeking or loading, you can override this behaviour by setting
`navigator.mediaSession.playbackState` to `"playing"` or `"paused"`. This
comes handy when you want to make sure your web app UI stays in sync with the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'comes in handy'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

### The Service Worker caching strategy

Regarding media files, I would recommend a simple "[Cache, falling back to
network]" strategy as illustrated by Jake Archibald.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe 'as described by Jake Archibald', with a link.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Cache, falling back to network] is already a link.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah — thanks.

Regarding media files, I would recommend a simple "[Cache, falling back to
network]" strategy as illustrated by Jake Archibald.

For artworks though, I'd be a little bit more specific and choose the approach
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think 'artwork' is better — 'artworks' sounds like works of art :).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops ;)
Done.


### Let user control cache

As user will consume content from your web app, media files and artworks may
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'As the user consumes content from your web app, media files and artwork may take a lot of space on their device.'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

## Implementation nits

- Chrome for Android requests "full" audio focus to show media notifications
only when media files duration is [at least 5 seconds].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'media file duration'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops.


- Chrome for Android requests "full" audio focus to show media notifications
only when media files duration is [at least 5 seconds].
- If no artwork is defined and there is a favicon at a desirable size, media
Copy link
Contributor

@samdutton samdutton Jan 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really mean favicon (i.e. like https://simpl.info/favicon.ico)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

icon image will be ;)

@beaufortfrancois
Copy link
Member Author

@samdutton I've addressed all your feedback. Thanks again!

@jpmedley
Copy link
Contributor

@beaufortfrancois Just after you asked me to edit was when Sam put in all of his comments. I want to double-check. Are you expecting feedback from ANYONE else?

@jakearchibald I'll need you to review and approve your requested changes as well.

@beaufortfrancois
Copy link
Member Author

As discussed offline, @jakearchibald is happy for it to launch as it.
Therefore, I'd love your final review @jpmedley.

@jakearchibald
Copy link
Contributor

Yep! The article was great before I reviewed it. Any fixes are a bonus 😄

@petele
Copy link
Member

petele commented Jan 27, 2017

@jakearchibald - can you please "approve" the changes in git so that I don't have to "dismiss" your review before I merge? :)

alt="Media Session TL;DR;"/>
</figure>

## Gimme What I Want
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All titles level 2 and lower should be sentence case. Only the article title should be title case. Please change throughout.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

});

Note: If the `<audio>` element has the `controls` attribute, you can simply set
up the Media Session in the audio `play` event listener instead which occurs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rules are these:

  • When referring to the API proper, capitalize. Media Session API
  • When referring to a generic media session, do not capitalize. media session
  • When referring to an instance of the MediaSession interface, like this: mediaSession (or something similar)
  • When referring to the MediaSession interface itself, like this: MediaSession

Please verify throughout.

is why you need to update it to make sure you're always showing relevant
information in the media notification.

#### Previous Track / Next Track
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sentence case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

True but first and foremost, you want to make sure **all items in this
checklist are checked**:

1. All media and artwork files are served with the appropriate `Cache-Control`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Items that aren't required to be in a particular order should be bulleted, not numbered.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

`Allow-Control-Allow-Origin: *` HTTP header. This will allow third-party web
apps to fetch and consume HTTP responses from your web server.

### The Service Worker caching strategy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The term service worker is lower case unless referring to the 'Service Worker API'. See my rules for media source, above. Please check throughout.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


### The Service Worker caching strategy

Regarding media files, I would recommend a simple "[Cache, falling back to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete the word 'would'.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

.then(cache => Promise.all(artworkFilesToDelete.map(artwork => cache.delete(artwork))))
.catch(error => { console.log(error); });

## Implementation nits
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 'Implementation notes' would be a better title.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


- Chrome for Android requests "full" audio focus to show media notifications
only when media file duration is [at least 5 seconds].
- If no artwork is defined and there is an icon image at a desirable size, media
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean appropriate size?

@beaufortfrancois
Copy link
Member Author

@jpmedley Thank you very much. I've addressed your valuable feedback at 460bfb3

@beaufortfrancois
Copy link
Member Author

If @jpmedley is OK, @petele please merge and push live.

@petele petele merged commit 1c93aac into google:master Jan 31, 2017
@beaufortfrancois beaufortfrancois deleted the mediaSessionArticle branch February 1, 2017 07:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants