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)

x86
macOS
defect
Not set
normal

Tracking

(ux-b2g:2.1)

RESOLVED FIXED
ux-b2g 2.1

People

(Reporter: yor, Assigned: yor)

References

Details

Attachments

(1 file)

46 bytes, text/x-github-pull-request
yury
: feedback-
Details | Review
No description provided.
Blocks: gaia-header
Assignee: nobody → yor
Status: NEW → ASSIGNED
Attached file Pull request
Kevin, Please redirect r? as needed. Thanks.
Attachment #8429846 - Flags: review?(kgrandon)
Comment on attachment 8429846 [details] [review] 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 on attachment 8429846 [details] [review] Pull request This app is own by Brendan.
Attachment #8429846 - Flags: review?(21) → review?(bdahl)
Cool, thanks Vivien! Can we update the modules page with that info? :)
Attachment #8429846 - Flags: review?(bdahl) → feedback?(bdahl)
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #3) > Comment on attachment 8429846 [details] [review] > 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] [review] Pull request I hope I have the right Yury.
Attachment #8429846 - Flags: feedback?(bdahl) → review?(ydelendik)
Comment on attachment 8429846 [details] [review] Pull request The code must the present in upstream repo at https://github.com/mozilla/pdf.js.
Attachment #8429846 - Flags: review?(ydelendik) → feedback-
Depends on: 1054590
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
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
See Also: → 1017328
See Also: → 1056989
The second commit in bug 1017328 breaks pdf.js on flame. See bug 1017328 and bug 1056989.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
Depends on: 1056989
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)
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)
(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)
(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
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)
(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)
(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.
(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)
(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)
Depends on: 1058285
Depends on: 1058288
(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.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga)
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
Flags: needinfo?(wilsonpage)
Both bugs 1058285 and 1058288 have landed. Closing this one.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: