Closed
Bug 1117771
Opened 9 years ago
Closed 9 years ago
Consolidate checks for the iframe package in fireplace
Categories
(Marketplace Graveyard :: Code Quality, defect, P4)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mat, Assigned: jainmukul1996, Mentored)
Details
(Whiteboard: [contribute][good first bug])
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•9 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]
Comment 2•9 years ago
|
||
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•9 years ago
|
||
replace instances of window.self/window.top checks with required('capabilities').iframed
Flags: needinfo?(kngo)
Comment 4•9 years ago
|
||
I mean require('core/capabilities')
Assignee | ||
Comment 5•9 years ago
|
||
I would like to take this bug
Assignee | ||
Comment 6•9 years ago
|
||
I would like to take this bug
Comment 7•9 years ago
|
||
Thanks Mukul, Assigning this bug to you.
Assignee: nobody → jainmukul1996
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•9 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•9 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•9 years ago
|
||
Sorry I will make it again.
Assignee | ||
Comment 11•9 years ago
|
||
new pull request link : https://github.com/mozilla/fireplace/pull/1417
Comment 12•9 years ago
|
||
Merged
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 13•9 years ago
|
||
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.
Description
•