msfeldstein / chrome-media-keys

Chrome extension that adds media keys to most web music players
GNU Affero General Public License v3.0
89 stars 33 forks source link

Frame selector #56

Closed nemchik closed 9 years ago

nemchik commented 9 years ago

This should work, I've tested spotify, but we could really use additional testing. I honestly don't know if there's even that many players with frames out there, so this might just be catering to spotify.

msfeldstein commented 9 years ago

It would probably be cleaner if you created a this.querySelector method that would then decide whether to run on this or a frame document, instead of checking it in every place

msfeldstein commented 9 years ago

Meaning put this in one place, instead of making each method need to know about the frame selector

 if (this.frameSelector) {
+    var el = document.querySelector(this.frameSelector).contentDocument.querySelector(key); 
+  } else {
+    var el = document.querySelector(key);
+  }
msfeldstein commented 9 years ago

And replace each call to document.querySelector with this.querySelector (or call it targettedQuerySelector or something to make it clear what it's doing)

nemchik commented 9 years ago

I tried what you recommended and it breaks spotify (the basic controller in this branch that is), but other controller still work. I'll try more later.

msfeldstein commented 9 years ago

Can you post a patch of the change you made? It really should work im willing to bet it was just a binding issue.

On Wed, May 20, 2015 at 3:01 PM Eric Nemchik notifications@github.com wrote:

I tried what you recommended and it breaks spotify, but other controller still work. I'll try more later.

— Reply to this email directly or view it on GitHub https://github.com/msfeldstein/chrome-media-keys/pull/56#issuecomment-104053054 .

nemchik commented 9 years ago

ok, here is what i have https://github.com/nemchik/chrome-media-keys/commit/130abd44241995f22053aba1f8312b3846042c14 it seems to be working just fine for existing controllers, but spotify won't pick up the player

msfeldstein commented 9 years ago

So you're saying that it works when you check frameTarget in each call, but when you move to targettedQuerySelector it stops working?

Also you mentioned somewhere some confusion about !!, !! converts any value to a boolean value. var s = "something"; !s // This is false !!s // this is true

this is in case you want to store the boolean value but don't want to store the actual value that evaluated to true/false.

On Wed, May 20, 2015 at 3:48 PM Eric Nemchik notifications@github.com wrote:

ok, here is what i have nemchik@130abd4 https://github.com/nemchik/chrome-media-keys/commit/130abd44241995f22053aba1f8312b3846042c14 it seems to be working just fine for existing controllers, but spotify won't pick up the player

— Reply to this email directly or view it on GitHub https://github.com/msfeldstein/chrome-media-keys/pull/56#issuecomment-104065305 .

nemchik commented 9 years ago

Ok, i found something. If I close all chrome and reopen it and load spotify with https://github.com/nemchik/chrome-media-keys/commit/130abd44241995f22053aba1f8312b3846042c14 it does NOT work. If i load soundcloud, or youtube (i guess any controller) and play music the controller works, and then if i load spotify in that tab the spotify controller works.

This is why i thought it was working when i made the PR, but i have no idea why this behaviour is happening. It's like the spotify website doesnt load something, but if i load another website that successfully loads a controller it passes that on to spotify. if i load a new tab with spotify it does NOT work.

nemchik commented 9 years ago

the behavior i described is the same with https://github.com/nemchik/chrome-media-keys/commit/5c5ef67882104182f3c6404019b6eaafb9f1e4a9 so having the code as you recommended is not the problem.

msfeldstein commented 9 years ago

That sounds like a horrible break of chrome's sandbox... it sounds more like the original music player is alerting the extension that it is playing, then spotify doesn't have to. If you do this, then open the popup does it show the actual music playing or the last players song?

On Wed, May 20, 2015 at 4:09 PM Eric Nemchik notifications@github.com wrote:

Ok, i found something. If I close all chrome and reopen it and load spotify with nemchik@130abd4 https://github.com/nemchik/chrome-media-keys/commit/130abd44241995f22053aba1f8312b3846042c14 it does NOT work. If i load soundcloud, or youtube (i guess any controller) and play music the controller works, and then if i load spotify in that tab the spotify controller works.

This is why i thought it was working when i made the PR, but i have no idea why this behaviour is happening. It's like the spotify website doesnt load something, but if i load another website that successfully loads a controller it passes that on to spotify. if i load a new tab with spotify it does NOT work.

— Reply to this email directly or view it on GitHub https://github.com/msfeldstein/chrome-media-keys/pull/56#issuecomment-104068070 .

nemchik commented 9 years ago

it shows the current song playing in spotify, and has the song info, album art, and the media keys work, as well as the buttons in the pop up.

msfeldstein commented 9 years ago

Hmm ok im still willing to bet observeStateChanges is what's not working

On Wed, May 20, 2015 at 4:12 PM Eric Nemchik notifications@github.com wrote:

it shows the current song playing in spotify, and has the song info, album art, and the media keys work, as well as the buttons in the pop up.

— Reply to this email directly or view it on GitHub https://github.com/msfeldstein/chrome-media-keys/pull/56#issuecomment-104068519 .

nemchik commented 9 years ago

Unfortunately nothing shows up in the log either. I'm stumped. This makes the quest for basic controllers a bit harder :(

On Wed, May 20, 2015, 6:14 PM Michael Feldstein notifications@github.com wrote:

Hmm ok im still willing to bet observeStateChanges is what's not working

On Wed, May 20, 2015 at 4:12 PM Eric Nemchik notifications@github.com wrote:

it shows the current song playing in spotify, and has the song info, album art, and the media keys work, as well as the buttons in the pop up.

— Reply to this email directly or view it on GitHub < https://github.com/msfeldstein/chrome-media-keys/pull/56#issuecomment-104068519

.

— Reply to this email directly or view it on GitHub https://github.com/msfeldstein/chrome-media-keys/pull/56#issuecomment-104068896 .

nemchik commented 9 years ago

ok so check out this latest commit. I was able to do some sleuthing and used the log A LOT and added a ton of console logs until I finally narrowed it down. The issue was that the iframe takes longer to load than the rest of the page. Using lazy observing makes the observer keep looking until it finds the element. I'm going to try and add a little more so that we can detect if the frame is fully loaded, so don't merge this just yet.

nemchik commented 9 years ago

ok i can't figure out anything beyond this. apparently the lazy observing is the right way to go for frames. it would be nice to know of another player that uses frames so i could test that it's not just spotify.

anyway i think this can merge unless you have any other recommendations.

edit: I squashed and rebased because it would have drove me crazy if i didnt.

msfeldstein commented 9 years ago

I will pull this but spotify is in a really bad state regardless, there's a UI Beta they're running that half of the users are having problems with (you'll know if you're seeing it, when the player buttons are on the bottom). If you are seeing it or have a friend who is i'd love to get that supported, spotify has been very unhelpful getting me into the beta.

nemchik commented 9 years ago

I haven't seen the beta yet, but im sure once we start seeing it we can update the controller to figure out which is which.