1016817 - [PDF Viewer] Update to use gaia-header

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::PDF Viewer, defect)

Comment 1

[Yan Or]

Attachment: Pull request

Kevin,

Please redirect r? as needed.  Thanks.

Comment 2

[Kevin Grandon :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!

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8429846 [details] [review]
Pull request

This app is own by Brendan.

Comment 4

[Kevin Grandon :kgrandon]

Cool, thanks Vivien! Can we update the modules page with that info? :)

Comment 5

[Bill Walker [:bwalker] [@wfwalker]]

(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 6

[Yan Or]

Comment on attachment 8429846 [details] [review]
Pull request

I hope I have the right Yury.

Comment 7

[Yury Delendik (:yury)]

Comment on attachment 8429846 [details] [review]
Pull request

The code must the present in upstream repo at https://github.com/mozilla/pdf.js.

Comment 8

[Yan Or]

Opened bug 1054590 to track update to https://github.com/mozilla/pdf.js.

Comment 9

[Stephany Wilkes]

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.

Comment 10

[Yan Or]

Bug 1054590 has landed and the changes has been uplifted to gaia in bug 1017328.

Good to close.

Comment 11

[yifan [:yifan][:yliao]]

The second commit in bug 1017328 breaks pdf.js on flame. See bug 1017328 and bug 1056989.

Comment 12

[Yan Or]

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?

Comment 13

[Kevin Grandon :kgrandon]

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?

Comment 14

[Jason Smith [:jsmith]]

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.

Comment 15

[William Chen [:wchen]]

(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.

Comment 16

[Kevin Grandon :kgrandon]

(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.

Comment 17

[Kevin Grandon :kgrandon]

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?

Comment 18

[Yury Delendik (:yury)]

(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.

Comment 19

[Yan Or]

(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.

Comment 20

[Yan Or]

(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.

Comment 21

[Kevin Grandon :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?

Comment 22

[Joshua Mitchell (Inactive)]

(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.

Comment 23

[Yan Or]

Both bugs 1058285 and 1058288 have landed.  Closing this one.