Closed
Bug 1016817
Opened 10 years ago
Closed 10 years ago
[PDF Viewer] Update to use gaia-header
Categories
(Firefox OS Graveyard :: Gaia::PDF Viewer, defect)
Tracking
(ux-b2g:2.1)
RESOLVED
FIXED
ux-b2g | 2.1 |
People
(Reporter: yor, Assigned: yor)
References
Details
Attachments
(1 file)
No description provided.
Blocks: gaia-header
Comment 2•10 years ago
|
||
Comment on attachment 8429846 [details] Pull request Hey, you should probably use the modules page (https://wiki.mozilla.org/Modules/FirefoxOS) to find the right reviewer, or use the history in github of the repo. I don't think this app has a sole module owner, so perhaps either Vivien or Tim should take a look. Vivien - could you please take a look at this? If you don't have time, feel free to delegate to me. Thanks!
Attachment #8429846 -
Flags: review?(kgrandon) → review?(21)
Comment 3•10 years ago
|
||
Comment on attachment 8429846 [details]
Pull request
This app is own by Brendan.
Attachment #8429846 -
Flags: review?(21) → review?(bdahl)
Attachment #8429846 -
Flags: review?(bdahl) → feedback?(bdahl)
Comment 5•10 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #3) > Comment on attachment 8429846 [details] > Pull request > > This app is own by Brendan. Hi Vivien. PDF.js is definitely owned by bdahl, but he's on leave until September. I recommend you ask Yury for review, he is very familiar with PDF.js.
Comment on attachment 8429846 [details]
Pull request
I hope I have the right Yury.
Attachment #8429846 -
Flags: feedback?(bdahl) → review?(ydelendik)
Comment 7•10 years ago
|
||
Comment on attachment 8429846 [details] Pull request The code must the present in upstream repo at https://github.com/mozilla/pdf.js.
Attachment #8429846 -
Flags: review?(ydelendik) → feedback-
Opened bug 1054590 to track update to https://github.com/mozilla/pdf.js.
Comment 9•10 years ago
|
||
Marking this bug for 2.1 since the web components Header is the one committed web component for 2.1. Other web components for 2.2 depend on Header going first. Sorry for doing this late; Hema just brought this bug to my attention today, but it reflects agreed scope and is in the agreed 2.1 plan. Just updating the flags appropriately for release tracking.
ux-b2g: --- → 2.1
Assignee | ||
Comment 10•10 years ago
|
||
Bug 1054590 has landed and the changes has been uplifted to gaia in bug 1017328. Good to close.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Updated•10 years ago
|
Comment 11•10 years ago
|
||
The second commit in bug 1017328 breaks pdf.js on flame. See bug 1017328 and bug 1056989.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 12•10 years ago
|
||
Looks like pdf.js is not a certified app? and currently web component is enabled only for certified apps. This patch was probably tested with a device that has dom.webcomponents.enabled set to true. Do we need to drop this from 2.1?
Flags: needinfo?(wilsonpage)
Flags: needinfo?(kgrandon)
Comment 13•10 years ago
|
||
I think our solution depends on the roadmap of getting this turned on by default. It makes me sad to make it a certified app, but it may be a solution. William - what is the timeline of us being able to turn on web components by default? Is this something we can do in 2.1 or 2.2?
Flags: needinfo?(kgrandon) → needinfo?(wchen)
Comment 14•10 years ago
|
||
Kevin - Was there a patch backed out here by chance? Just want to make sure that we don't have the PDF viewer still broken here given comment 11.
Flags: needinfo?(kgrandon)
Comment 15•10 years ago
|
||
(In reply to Kevin Grandon :kgrandon from comment #13) > William - what is the timeline of us being able to turn on web components by > default? Is this something we can do in 2.1 or 2.2? There currently isn't a timeline to turn on web components by default. It's definitely not something we can do for 2.1, and I'm not sure about 2.2.
Flags: needinfo?(wchen)
Comment 16•10 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #14) > Kevin - Was there a patch backed out here by chance? Just want to make sure > that we don't have the PDF viewer still broken here given comment 11. This is for header implementation work, and it looks like no patches landed in master. I think we should verify if bug 1056989 is still an issue or not.
Flags: needinfo?(kgrandon)
Keywords: qawanted
Comment 17•10 years ago
|
||
Yury - is the current PDF.js code consumed elsewhere besides gaia? Can we make a temporary fix to make it a certified app until we have web components enabled for privileged apps?
Flags: needinfo?(ydelendik)
Comment 18•10 years ago
|
||
(In reply to Kevin Grandon :kgrandon from comment #17) > Yury - is the current PDF.js code consumed elsewhere besides gaia? Only viewer.html and its styles/images are specific to gaia. Small portion of the code in viewer.js is specific to gaia as well (see comments near `B2G` at https://github.com/mozilla/pdf.js/blob/master/web/viewer.js). > Can we make a temporary fix to make it a certified app until we have web components > enabled for privileged apps? Sure, it was a certified app in the past AFAIK.
Flags: needinfo?(ydelendik)
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to Kevin Grandon :kgrandon from comment #16) > (In reply to Jason Smith [:jsmith] from comment #14) > > Kevin - Was there a patch backed out here by chance? Just want to make sure > > that we don't have the PDF viewer still broken here given comment 11. > > This is for header implementation work, and it looks like no patches landed > in master. I think we should verify if bug 1056989 is still an issue or not. The patch was landed in bug 1017328 and has since been backed out.
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Yury Delendik (:yury) from comment #18) > (In reply to Kevin Grandon :kgrandon from comment #17) > > Yury - is the current PDF.js code consumed elsewhere besides gaia? > > Only viewer.html and its styles/images are specific to gaia. Small portion > of the code in viewer.js is specific to gaia as well (see comments near > `B2G` at https://github.com/mozilla/pdf.js/blob/master/web/viewer.js). > > > Can we make a temporary fix to make it a certified app until we have web components > > enabled for privileged apps? > > Sure, it was a certified app in the past AFAIK. So can we agree to make it a certified app? Do we need sign-off from anyone? We will also need to uplift to version 1.0.700 again.
Flags: needinfo?(kgrandon)
Comment 21•10 years ago
|
||
(In reply to Yan Or from comment #20) > So can we agree to make it a certified app? Do we need sign-off from anyone? I think you can just open up a pull request to do so and get a review on it. I would also open up a bug to track changing from certified back to priveleged. > We will also need to uplift to version 1.0.700 again. Don't we need to uplift the version that is certified?
Flags: needinfo?(kgrandon)
Comment 22•10 years ago
|
||
(In reply to Kevin Grandon :kgrandon from comment #16) > (In reply to Jason Smith [:jsmith] from comment #14) > > Kevin - Was there a patch backed out here by chance? Just want to make sure > > that we don't have the PDF viewer still broken here given comment 11. > > This is for header implementation work, and it looks like no patches landed > in master. I think we should verify if bug 1056989 is still an issue or not. bug 1053689 is no longer an issue - Actual results - on the latest 2.1 PDF files have a title and the X (to close) Device: Flame Master Build ID: 20140825085054 Gaia: a25ae14dbd2fe3e25144a7064efc0cc4e31042b8 Gecko: 4ca2bd0722d9 Version: 34.0a1 (Master) Firmware Version: v123 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0 I will go update that bug next.
Updated•10 years ago
|
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
Updated•10 years ago
|
Flags: needinfo?(wilsonpage)
Assignee | ||
Comment 23•10 years ago
|
||
Both bugs 1058285 and 1058288 have landed. Closing this one.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•