Consolidate checks for the iframe package in fireplace

RESOLVED FIXED

Status

Marketplace
Code Quality
P4
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mat, Assigned: Mukul Jain, Mentored)

Tracking

x86_64
Linux
Points:
---

Details

(Whiteboard: [contribute][good first bug])

(Reporter)

Description

3 years ago
There are several places in fireplace and marketplace-core-modules that check if we are in an iframed app, using window.self !== window.top. We should expose this in capabilities.js or some other module instead of repeating the same check everywhere.

Spotted by cvan when reviewing https://github.com/mozilla/fireplace/pull/877
(Reporter)

Comment 1

3 years ago
marketplace-core-modules core/capabilities module now exposes "packaged" and "iframed". We should find and replace the manual checks by these properties everywhere in fireplace (it's already used in fireplace/src/media/js/update_banner.js)
Assignee: mpillard → nobody
Whiteboard: [contribute][good first bug]
Hi Kevin/MP-Team, it looks like this might not be very much clear to the newbie how to fix this. Can you put some information here for newbies so that they can pick this bug and fix it.

Thanks,
Ram
Flags: needinfo?(kngo)

Comment 3

3 years ago
replace instances of window.self/window.top checks with required('capabilities').iframed
Flags: needinfo?(kngo)

Comment 4

3 years ago
I mean require('core/capabilities')
(Assignee)

Comment 5

3 years ago
I would like to take this bug
(Assignee)

Comment 6

3 years ago
I would like to take this bug
Thanks Mukul, Assigning this bug to you.
Assignee: nobody → jainmukul1996
Status: NEW → ASSIGNED
(Assignee)

Comment 8

3 years ago
Sorry for double same comment(spam?!) it's by mistake, unable to remove!!!

i found "window.self !== window.top", only in one file which is webactivities.js, so do I have to replace that with "capabilities.iframed" ... that's what I have tried... I made a commit and pull request.
(Reporter)

Comment 9

3 years ago
I can't see your pull request at the moment (are you sure you submitted it ?) but that sounds right, there used to be one or two other files but they got cleaned up during other modifications.
(Assignee)

Comment 10

3 years ago
Sorry I will make it again.
(Assignee)

Comment 11

3 years ago
new pull request link : https://github.com/mozilla/fireplace/pull/1417

Comment 12

3 years ago
Merged
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Mentor: trishul.goel
Many congratulations Mukul on fixing your good first bug :) As next steps

- Feel free to pick more good first bugs and work on them - https://bugzilla.mozilla.org/buglist.cgi?cmdtype=runnamed&namedcmd=mp-good-first-bugs
- We also encourage you to add your contribution to this month's recognition wiki - https://wiki.mozilla.org/Marketplace/Contributing/Aug2015#Contributor_Recognition
You need to log in before you can comment on or make changes to this bug.