move hasHtml to AnnotationElement #6856

xlc · 2016-01-12T03:44:58Z

Required by #6172

Snuffleupagus · 2016-01-12T10:59:01Z

src/display/annotation_layer.js

@@ -199,6 +199,16 @@ var AnnotationElement = (function AnnotationElementClosure() {
    },

    /**
+     * Check does this annotation need HTML element.


This sentence sounds a bit strange, how about e.g. Check if the annotation needs a HTML element. instead?

timvandermeij · 2016-01-12T12:43:21Z

I agree with the comments above. Aside from that, I think it's quite a lot of code compared to the current situation, with no clear advantage. We need to find a way to only call render on the element if it exists, meaning that in case of https://github.com/mozilla/pdf.js/pull/6856/files#diff-18ca06e2bd5be4c9132b51f78388b8f0L793 we should not create the annotation in the first place.

xlc · 2016-01-12T21:55:27Z

It is AnnotationElement's responsibility to check is render needed to be called. If hasHtml() returns false, render won't be called. The main advantage for this PR is hasHtml code moved from model (Annotation and its data object) to renderer (AnnotationElement). It is the renderer's responsibility to decide it want's to render anything or not.
Another way is simply let render return null if it doesn't want to render anything.
If you want to avoid create 'AnnotationElement' at first place, then you need a big complicated function hasHtml(parameter) to make the decision that contains multiple switch cases.

timvandermeij · 2016-01-13T12:27:32Z

src/display/annotation_layer.js

@@ -199,6 +199,16 @@ var AnnotationElement = (function AnnotationElementClosure() {
    },

    /**
+     * Check if the annotation needs a HTML element.


Nit: an HTML element (here and below).

Snuffleupagus · 2016-01-13T12:47:40Z

Aside from that, I think it's quite a lot of code compared to the current situation, with no clear advantage.

I agree with this sentiment. It seems like a lot of code to add, and I think that a "shorter" solution would be preferable.

timvandermeij · 2016-01-13T12:57:16Z

How about, instead of a method, use a member variable for this instead? I think hasHtml is a slightly bad name because each annotation element has associated HTML, it's just a matter of if we want to show it or not, so I think we should call it something like isRenderable instead. We can set isRenderable in the constructor for the base class, overwrite it for subclasses after https://github.com/mozilla/pdf.js/pull/6856/files#diff-1097e3b131ba22f8e8a6c2f20e3ab5ecR710 and change https://github.com/mozilla/pdf.js/pull/6856/files#diff-1097e3b131ba22f8e8a6c2f20e3ab5ecR852 to if (element.isRenderable) {. That should give us the same functionality but with much less code (much more like how it is right now, just one line per subclass).

Snuffleupagus · 2016-01-13T13:05:31Z

@timvandermeij I was actually thinking something similar, but preferably I'd also avoid creating the container in this case. Similar to your idea, but could we maybe do something along these lines https://gist.github.com/Snuffleupagus/206b52763a250f6ba811?

timvandermeij · 2016-01-13T13:12:24Z

Yes, that looks like a clean solution to me!

xlc · 2016-01-15T00:32:34Z

Updated per @Snuffleupagus's suggestion.

timvandermeij · 2016-01-15T15:54:25Z

src/display/annotation_layer.js

@@ -310,7 +314,9 @@ var LinkAnnotationElement = (function LinkAnnotationElementClosure() {
 */
 var TextAnnotationElement = (function TextAnnotationElementClosure() {
  function TextAnnotationElement(parameters) {
-    AnnotationElement.call(this, parameters);
+    var isRenderable = !!(parameters.data.hasPopup||


Nit: add a space after hasPopup and before ||

timvandermeij · 2016-01-20T21:23:14Z

@Snuffleupagus Do you have any ideas to further improve this code (regarding the above)?

timvandermeij · 2016-02-09T20:37:12Z

@Snuffleupagus Ping for the above as I think this comment got lost in the pile of GitHub messages.

This looks good as-is, but perhaps you have an idea to improve this even further (see above, about returning in the constructor).

Snuffleupagus · 2016-02-10T13:17:13Z

src/display/annotation_layer.js

@@ -86,7 +86,8 @@ AnnotationElementFactory.prototype =
        return new StrikeOutAnnotationElement(parameters);

      default:
-        throw new Error('Unimplemented annotation type "' + subtype + '"');
+        warn('Unimplemented annotation type "' + parameters.data.subtype + '"');
+        return null;


Since we already warn in annotation.js for this case, I think that we can avoid duplicate warnings here.
Would return AnnotationElement(parameters); make sense here, instead of returning null?

Snuffleupagus · 2016-02-10T13:17:54Z

Ping for the above as I think this comment got lost in the pile of GitHub messages.

Yeah, unfortunately that happened here (the problem with getting notified about everything), sorry for the lag!
This seems OK to me, with two minor comments, provided the tests pass.

xlc · 2016-02-11T01:01:04Z

@timvandermeij @Snuffleupagus Updated.

timvandermeij · 2016-02-13T15:11:39Z

/botio-linux preview

pdfjsbot · 2016-02-13T15:11:40Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2016-02-13T15:12:32Z

From: Bot.io (Linux)

Success

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

Total script time: 0.87 mins

Published

timvandermeij · 2016-02-13T15:12:41Z

/botio test

pdfjsbot · 2016-02-13T15:12:41Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/7d57e20a57e6ddf/output.txt

pdfjsbot · 2016-02-13T15:12:41Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/33aa3d9766f82fc/output.txt

pdfjsbot · 2016-02-13T15:32:52Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/33aa3d9766f82fc/output.txt

Total script time: 20.17 mins

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

pdfjsbot · 2016-02-13T15:34:21Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/7d57e20a57e6ddf/output.txt

Total script time: 21.66 mins

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

move hasHtml to AnnotationElement

timvandermeij · 2016-02-13T19:12:16Z

Looks good, thank you!

Snuffleupagus reviewed Jan 12, 2016
View reviewed changes

timvandermeij added annotations 4-work-in-progress labels Jan 12, 2016

xlc force-pushed the remove-has-html branch 2 times, most recently from ed5003a to b0f8492 Compare January 12, 2016 22:04

timvandermeij reviewed Jan 13, 2016
View reviewed changes

xlc force-pushed the remove-has-html branch from b0f8492 to 1cfcaf8 Compare January 15, 2016 00:28

timvandermeij removed the 4-work-in-progress label Jan 15, 2016

timvandermeij reviewed Jan 15, 2016
View reviewed changes

xlc force-pushed the remove-has-html branch 2 times, most recently from 5a274e4 to 89ebcbd Compare January 17, 2016 21:31

Snuffleupagus reviewed Feb 10, 2016
View reviewed changes

xlc force-pushed the remove-has-html branch from 89ebcbd to 5f75cb1 Compare February 11, 2016 00:57

move hasHtml to AnnotationElement

e42da0f

xlc force-pushed the remove-has-html branch from 5f75cb1 to e42da0f Compare February 11, 2016 00:58

timvandermeij mentioned this pull request Feb 11, 2016

Investigate intermittent bot failures for Firefox #6944

Closed

timvandermeij added a commit that referenced this pull request Feb 13, 2016

Merge pull request #6856 from KamiHQ/remove-has-html

addc4a3

move hasHtml to AnnotationElement

timvandermeij merged commit addc4a3 into mozilla:master Feb 13, 2016

xlc deleted the remove-has-html branch February 14, 2016 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move hasHtml to AnnotationElement #6856

move hasHtml to AnnotationElement #6856

xlc commented Jan 12, 2016

Snuffleupagus Jan 12, 2016

timvandermeij commented Jan 12, 2016

xlc commented Jan 12, 2016

timvandermeij Jan 13, 2016

Snuffleupagus commented Jan 13, 2016

timvandermeij commented Jan 13, 2016

Snuffleupagus commented Jan 13, 2016

timvandermeij commented Jan 13, 2016

xlc commented Jan 15, 2016

timvandermeij Jan 15, 2016

timvandermeij commented Jan 20, 2016

timvandermeij commented Feb 9, 2016

Snuffleupagus Feb 10, 2016

Snuffleupagus commented Feb 10, 2016

xlc commented Feb 11, 2016

timvandermeij commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

timvandermeij commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

timvandermeij commented Feb 13, 2016

move hasHtml to AnnotationElement #6856

move hasHtml to AnnotationElement #6856

Conversation

xlc commented Jan 12, 2016

Snuffleupagus Jan 12, 2016

Choose a reason for hiding this comment

timvandermeij commented Jan 12, 2016

xlc commented Jan 12, 2016

timvandermeij Jan 13, 2016

Choose a reason for hiding this comment

Snuffleupagus commented Jan 13, 2016

timvandermeij commented Jan 13, 2016

Snuffleupagus commented Jan 13, 2016

timvandermeij commented Jan 13, 2016

xlc commented Jan 15, 2016

timvandermeij Jan 15, 2016

Choose a reason for hiding this comment

timvandermeij commented Jan 20, 2016

timvandermeij commented Feb 9, 2016

Snuffleupagus Feb 10, 2016

Choose a reason for hiding this comment

Snuffleupagus commented Feb 10, 2016

xlc commented Feb 11, 2016

timvandermeij commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 13, 2016

From: Bot.io (Linux)

Success

Published

timvandermeij commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 13, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 13, 2016

From: Bot.io (Windows)

Success

pdfjsbot commented Feb 13, 2016

From: Bot.io (Linux)

Success

timvandermeij commented Feb 13, 2016