Simplify B2G viewer #5947

yurydelendik · 2015-04-17T22:30:06Z

~~... also extracts PDFLinkService and PDFHistory APIs~~

Snuffleupagus · 2015-04-18T09:36:02Z

The (standard) preview and the extension is still not working: ReferenceError: PDFLinkService is not defined; it appears that viewer.js is missing the line //#include pdf_link_service.js.

timvandermeij · 2015-04-19T13:34:16Z

It seems like the locale stuff has been removed. Is that because of #3518? I think it would be nice if we could also use the localizations in the B2G viewer, but if that is out of scope for this refactoring we can also do that in a separate PR that addresses #3518.

timvandermeij · 2015-04-19T16:43:19Z

extensions/b2g/viewer.js

+    document.getElementById('activityTitle').textContent = title;
+  },
+
+  get pagesCount() {


Now that we have a link service here and this function (and the other two below this one) are also implemented there, can't we make use of the ones in the link service instead to avoid duplication?

It is nice to have those functions on the PDFViewerApplication level as well.

timvandermeij · 2015-04-19T16:58:46Z

This is a really nice refactoring! I think we can make the code in viewer.js even better by removing the B2G mentions now that we have extensions/b2g/viewer.js, but other than that and the nits above, this is looking really good. If the comments above are addressed, we should try running this on the actual device :)

Snuffleupagus · 2015-04-19T19:48:24Z

extensions/b2g/viewer.js

+    container.addEventListener('pagesinit', function () {
+      // we can use pdfViewer now, e.g. let's change default scale.
+      pdfViewer.currentScaleValue = 'page-width';
+    });


Just asking: did you intend to change the initial zoom level from auto to page-width, or is it a result of copy-pasting from the simpleviewer component?

Snuffleupagus · 2015-04-19T19:57:32Z

I'd still like to think through the class dependencies, between e.g. PDFLinkService/PDFHistory, that these patches introduces a bit more; but so far the general structure of the code looks good!

One small request on my part though: Can we please try and land PR #5699 before this one? That one has been open for quite some time, and it's already been bit-rotten a couple of times. This patch would cause even more merge conflicts, so I'd really appreciate if it could go first :-)

This PR removes both Preferences and ViewHistory from the B2G viewer, but I think that's mostly fine!

Regarding Preferences: Considering first of all the simplicity of the B2G viewer, and secondly the lack of any (easy) way for users to change them, I cannot see that we're loosing any functionality by removing it.
Regarding ViewHistory: This is included in the current B2G viewer, and I've just confirmed that it does work on an actual devices (a Flame, running Firefox OS 3.0 with the latest Nightly). Though I'm not sure if remembering/restoring the last position is a very important function in the B2G viewer. Thoughts/Opinions?

timvandermeij · 2015-04-20T18:12:36Z

extensions/b2g/viewer.js

+      this.pdfHistory.initialize(pdfDocument.fingerprint);
+    }.bind(this), function (error) {
+      // TODO handle and localize loading error?
+      console.error('PDF loading error: ' + error)


We're missing a semicolon here at the end (Travis also appears to be complaining about that).

timvandermeij · 2015-05-29T12:09:48Z

@yurydelendik Now that #5971 has landed, could you rebase the PR so I can play with it more, also on an actual Flame?

Snuffleupagus · 2015-05-29T19:44:11Z

A quick visual glitch that I noticed in the UI:
The loading bar is now constantly visible in the B2G viewer, and since there's no longer any JavaScript code for it, it will never be filled.
Considering the way that the B2G viewer works, and how files are opened with it, do we actually need a loading bar?

Snuffleupagus · 2015-05-29T19:45:52Z

Also, is the errorWrapper div, see https://github.com/mozilla/pdf.js/pull/5947/files#diff-5dba6a2f9a0235d9e5b2b75ef5a6f774L66, necessary any more?

timvandermeij · 2015-06-01T20:33:22Z

There appears to be a problem with the "zoom in" functionality. To reproduce, open http://107.21.233.14:8877/a5bae2087f5c8d3/extensions/b2g/content/web/viewer.html and click the Zoom In button (plus icon) in the bottom right. Notice the page not zooming in and notice that the following message is printed in the console:

"pdfViewSetScale: '0' is an unknown zoom value."

Zooming out zooms out to the minimal scale so it seems, which is also not correct, but after that zooming appears to work properly. Any idea what is causing this?

Snuffleupagus · 2015-06-05T16:02:19Z

Question: How about instead using https://github.com/gaia-components/gaia-progress, similar to the other gaia-components used in the B2G viewer, for the loading bar?
This way we'd have a more "native" look, and less JS/CSS code in the B2G viewer.

Snuffleupagus · 2015-06-05T16:08:37Z

extensions/b2g/viewer.js

+
+var DEFAULT_SCALE_DELTA = 1.1;
+var MIN_SCALE = 0.25;
+var MAX_SCALE = 10.0;


Would it make sense to move these constants into ui_utils.js instead (i.e. move them from web/viewer.js), to reduce duplication?

Actually, I'm expecting those to be different for B2G.

yurydelendik · 2015-06-05T16:24:18Z

How about instead using https://github.com/gaia-components/gaia-progress, similar to the other gaia-components used in the B2G viewer, for the loading bar?

Maybe. But at the moment I want to split viewer.js from B2G code. Supporting Firefox OS things shall fall on the shoulders of the professionals :) See also bug 1171998

timvandermeij · 2015-06-05T18:47:52Z

These commits still have the errorWrapper that @Snuffleupagus reported earlier. I have also found some accompanying CSS at https://github.com/yurydelendik/pdf.js/blob/fff086645b7a3fa5daeefe6e44f9ac5b5ab5abad/extensions/b2g/viewer.css#L215-L241. I also think that we do not need it on B2G. @yurydelendik What do you think?

On desktop I also notice that in the new preview the Gaia header is somewhat broken. It had a white background with black text and a 'x' sign, but now it is a gray background with white text and without the 'x' sign. I'm not yet sure what broke this though (probably we are not getting here: yurydelendik@fff0866#diff-af61da45d81c12dd8e6859e95eba2cc4R358). I have also not had time to test this on an actual Flame because I'm busy until the June 12th. I will do those tests after that time though.

yurydelendik · 2015-06-05T18:55:52Z

These commits still have the errorWrapper that @Snuffleupagus reported earlier.

I decided to keep them in case if PDF file will be corrupted. Therefore I added proper error handling.

yurydelendik · 2015-06-05T19:00:44Z

On desktop I also notice that in the new preview the Gaia header is somewhat broken.

It will be. The gaia project keep changing/reorganizing UI dependencies and l10n support. That's why we need to phase out the support of this viewer in this repo.

timvandermeij · 2015-06-30T18:38:00Z

/botio test

pdfjsbot · 2015-06-30T18:38:01Z

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/e4fdc7bfa4fb412/output.txt

pdfjsbot · 2015-06-30T18:38:02Z

From: Bot.io (Linux)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/f207f3bbe4aa073/output.txt

pdfjsbot · 2015-06-30T18:56:08Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/f207f3bbe4aa073/output.txt

Total script time: 18.11 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2015-06-30T18:56:15Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/e4fdc7bfa4fb412/output.txt

Total script time: 18.22 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

timvandermeij · 2015-06-30T20:53:20Z

extensions/b2g/viewer.html

-    <!-- <link rel="stylesheet" type="text/css" href="/shared/elements/gaia-icons/style.css" /> -->
-    <script type="text/javascript" src="/shared/elements/config.js"></script>
-    <script type="text/javascript" src="/shared/elements/gaia-header/dist/script.js"></script>
+    <link rel="stylesheet" type="text/css" href="/shared/elements/gaia-theme/gaia-theme.css" />


Remove / at the end: end it with gaia-theme.css">

Snuffleupagus · 2015-06-30T21:46:37Z

extensions/b2g/viewer.js

+  pdfViewer: null,
+  pdfHistory: null,
+  pdfLinkService: null,
+  loading: true,


Nit: this line is unused and can be removed (#6140).

timvandermeij · 2015-06-30T21:54:11Z

/botio-linux preview

pdfjsbot · 2015-06-30T21:54:12Z

From: Bot.io (Linux)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/397d75075264a93/output.txt

pdfjsbot · 2015-06-30T21:54:49Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/397d75075264a93/output.txt

Total script time: 0.62 mins

Published

timvandermeij · 2015-06-30T22:24:32Z

/botio lint

pdfjsbot · 2015-06-30T22:24:33Z

From: Bot.io (Windows)

Received

Command cmd_lint from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/d91f0eb88f9f40d/output.txt

pdfjsbot · 2015-06-30T22:24:33Z

From: Bot.io (Linux)

Received

Command cmd_lint from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/70f3339a1385d0b/output.txt

pdfjsbot · 2015-06-30T22:25:19Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/70f3339a1385d0b/output.txt

Total script time: 0.77 mins

Lint: Passed

pdfjsbot · 2015-06-30T22:25:35Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/d91f0eb88f9f40d/output.txt

Total script time: 1.04 mins

Lint: Passed

Simplify B2G viewer

timvandermeij · 2015-06-30T22:26:58Z

Great work!

yurydelendik force-pushed the b2g-viewer branch from c287dc8 to 40268e9 Compare April 17, 2015 22:52

Snuffleupagus added the viewer label Apr 18, 2015

yurydelendik force-pushed the b2g-viewer branch from 40268e9 to 0cb074d Compare April 18, 2015 16:47

timvandermeij reviewed Apr 19, 2015
View reviewed changes

Snuffleupagus reviewed Apr 19, 2015
View reviewed changes

yurydelendik force-pushed the b2g-viewer branch from 0cb074d to a892bc8 Compare April 20, 2015 16:20

timvandermeij reviewed Apr 20, 2015
View reviewed changes

yurydelendik mentioned this pull request Apr 27, 2015

Extracts PDFLinkService and PDFHistory APIs #5971

Merged

timvandermeij added this to the 2015 Q2 milestone May 26, 2015

yurydelendik force-pushed the b2g-viewer branch from a892bc8 to 2348cd7 Compare May 29, 2015 19:22

yurydelendik force-pushed the b2g-viewer branch from 2348cd7 to a61af41 Compare May 29, 2015 20:58

yurydelendik force-pushed the b2g-viewer branch 2 times, most recently from e144bd5 to a801cf0 Compare June 5, 2015 15:40

Snuffleupagus reviewed Jun 5, 2015
View reviewed changes

yurydelendik mentioned this pull request Jun 5, 2015

Bug 1171998 - Implements Firefox OS version specific PDF viewer. mozilla-b2g/gaia#30445

Closed

yurydelendik force-pushed the b2g-viewer branch from a801cf0 to 6e048f8 Compare June 5, 2015 16:57

timvandermeij reviewed Jun 30, 2015
View reviewed changes

yurydelendik force-pushed the b2g-viewer branch from c300427 to 3560f0f Compare June 30, 2015 21:40

Snuffleupagus reviewed Jun 30, 2015
View reviewed changes

yurydelendik force-pushed the b2g-viewer branch from 3560f0f to f3c3b1f Compare June 30, 2015 21:50

yurydelendik added 2 commits June 30, 2015 16:53

Using pdfjs-dist as a library for b2g viewer.

1b847df

Removes B2G preprocessing directives.

f3c3b1f

timvandermeij added a commit that referenced this pull request Jun 30, 2015

Merge pull request #5947 from yurydelendik/b2g-viewer

0a744fb

Simplify B2G viewer

timvandermeij merged commit 0a744fb into mozilla:master Jun 30, 2015

yurydelendik deleted the b2g-viewer branch January 28, 2016 21:04

calixteman mentioned this pull request Dec 7, 2022

Fit* values belong to view in URL parameters #15741

Closed

Simplify B2G viewer #5947

Simplify B2G viewer #5947

Conversation

yurydelendik commented Apr 17, 2015

Snuffleupagus commented Apr 18, 2015

timvandermeij commented Apr 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Apr 19, 2015

Choose a reason for hiding this comment

Snuffleupagus commented Apr 19, 2015

Choose a reason for hiding this comment

timvandermeij commented May 29, 2015

Snuffleupagus commented May 29, 2015

Snuffleupagus commented May 29, 2015

timvandermeij commented Jun 1, 2015

Snuffleupagus commented Jun 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented Jun 5, 2015

timvandermeij commented Jun 5, 2015

yurydelendik commented Jun 5, 2015

yurydelendik commented Jun 5, 2015

timvandermeij commented Jun 30, 2015

pdfjsbot commented Jun 30, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 30, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Jun 30, 2015

From: Bot.io (Linux)

Success

pdfjsbot commented Jun 30, 2015

From: Bot.io (Windows)

Success

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Jun 30, 2015

pdfjsbot commented Jun 30, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Jun 30, 2015

From: Bot.io (Linux)

Success

Published

timvandermeij commented Jun 30, 2015

pdfjsbot commented Jun 30, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 30, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Jun 30, 2015

From: Bot.io (Linux)

Success

pdfjsbot commented Jun 30, 2015

From: Bot.io (Windows)

Success

timvandermeij commented Jun 30, 2015