fvogt added a comment.

  AFAICT it doesn't reload the mpris state in the content-script immediately 
when settings change, can that be implemented?
  
  IMO the blacklist should be more than the domain, it should test the same 
conditions as CORS, so protocol, domain and port.

INLINE COMMENTS

> action_popup.js:20
> +    // Gets the URL of the currently viewed tab
> +    static getCurrentUrl() {
> +        return new Promise((resolve, reject) => {

`getCurrentTabUrl()`

> action_popup.js:42
> +    // Gets the URLs of the currently viewed tab including all of its iframes
> +    static getCurrentUrls() {
> +        return new Promise((resolve, reject) => {

Currently the function name implies that it returns all tabs, so maybe rename 
to `getCurrentTabFramesUrls()` (or better)?

> action_popup.js:46
> +                allFrames: true, // so we also catch iframe videos
> +                code: `window.location.href`
> +            }, (result) => {

Maybe `runAt: "document_start"` to speed it up a bit? I'm not sure about the 
implications.

> action_popup.js:62
> +        return new Promise((resolve, reject) => {
> +
> +            Promise.all([

Whitespace?

> action_popup.js:118
> +
> +    set(domain, block) {
> +        return this.get().then((blockInfo) => {

Currently calling `set(domain, false);` twice has a different result from 
`set(domain, false);` once, so maybe split into `whitelist(domain)` and 
`blacklist(domain)`)?

> action_popup.js:120
> +        return this.get().then((blockInfo) => {
> +
> +            let whitelist = blockInfo.mprisSettings.whitelistedDomains;

Whitespace?

> content-script.js:68
>      if (mpris.enabled) {
> -        loadMpris();
> -        if (items.mprisMediaSessions.enabled) {
> -            loadMediaSessionsShim();
> +        const domain = window.location.hostname;
> +

Is this guaranteed to be identical to `new URL(window.location.href).hostname` 
as used in utils?

> content-script.js:70
> +
> +        const whitelist = items.mpris.whitelistedDomains || [];
> +        const blacklist = items.mpris.blacklistedDomains || [];

Use `mpris.` instead of `items.mpris`

REPOSITORY
  R856 Plasma Browser Integration

REVISION DETAIL
  https://phabricator.kde.org/D24194

To: broulik, #plasma, #vdg, fvogt, ognarb
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

Reply via email to