Closed Bug 1360494 Opened 7 years ago Closed 7 years ago

[Mortar] 'stage-package' failed after adding '--enable-mortar' configuration

Categories

(Core Graveyard :: Plug-ins, enhancement)

Product:
Core Graveyard
Component:
Plug-ins
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: brsun, Assigned: ywu)

References

Details

Attachments

(3 files)

0004-Bug-1360494-remove-duplicate-image-files-and-referen.patch
133.31 KB, patch
lchang
: feedback+
Details | Diff | Splinter Review
bug 1360494 - part 1 remove duplicate image files and reference them from pdfjs.
59 bytes, text/x-review-board-request
Details
bug 1360494 - part 2 add mochitest to make sure the image files exist.
59 bytes, text/x-review-board-request
Details
Assignee: nobody → ywu
Errors:

[task 2017-04-28T08:37:07.190146Z] 08:37:07     INFO -  ERROR: The following duplicated files are not allowed:
[task 2017-04-28T08:37:07.190387Z] 08:37:07     INFO -  browser/features/ppapipdf.js@mozilla.org/chrome/content/style/images/toolbarButton-zoomOut.png
[task 2017-04-28T08:37:07.190590Z] 08:37:07     INFO -  browser/chrome/pdfjs/content/web/images/toolbarButton-zoomOut.png
[task 2017-04-28T08:37:07.190815Z] 08:37:07     INFO -  browser/features/ppapipdf.js@mozilla.org/chrome/content/style/images/grabbing.cur
[task 2017-04-28T08:37:07.191046Z] 08:37:07     INFO -  browser/chrome/pdfjs/content/web/images/grabbing.cur
[task 2017-04-28T08:37:07.191290Z] 08:37:07     INFO -  browser/features/ppapipdf.js@mozilla.org/chrome/content/style/images/toolbarButton-menuArrows.png
[task 2017-04-28T08:37:07.191599Z] 08:37:07     INFO -  browser/chrome/pdfjs/content/web/images/toolbarButton-menuArrows.png
[task 2017-04-28T08:37:07.191912Z] 08:37:07     INFO -  browser/features/ppapipdf.js@mozilla.org/chrome/content/style/images/toolbarButton-zoomOut@2x.png
[task 2017-04-28T08:37:07.192134Z] 08:37:07     INFO -  browser/chrome/pdfjs/content/web/images/toolbarButton-zoomOut@2x.png
[task 2017-04-28T08:37:07.192350Z] 08:37:07     INFO -  browser/features/ppapipdf.js@mozilla.org/chrome/content/style/images/treeitem-expanded.png
[task 2017-04-28T08:37:07.192579Z] 08:37:07     INFO -  browser/chrome/pdfjs/content/web/images/treeitem-expanded.png
[task 2017-04-28T08:37:07.192959Z] 08:37:07     INFO -  browser/features/ppapipdf.js@mozilla.org/chrome/content/style/images/grab.cur
[task 2017-04-28T08:37:07.193096Z] 08:37:07     INFO -  browser/chrome/pdfjs/content/web/images/grab.cur
[task 2017-04-28T08:37:07.193404Z] 08:37:07     INFO -  browser/features/ppapipdf.js@mozilla.org/chrome/content/style/images/treeitem-collapsed.png
[task 2017-04-28T08:37:07.193549Z] 08:37:07     INFO -  browser/chrome/pdfjs/content/web/images/treeitem-collapsed.png
[task 2017-04-28T08:37:07.193775Z] 08:37:07     INFO -  browser/features/ppapipdf.js@mozilla.org/chrome/content/style/images/annotation-noicon.svg
[task 2017-04-28T08:37:07.193976Z] 08:37:07     INFO -  browser/chrome/pdfjs/content/web/images/annotation-noicon.svg
[task 2017-04-28T08:37:07.194218Z] 08:37:07     INFO -  browser/features/ppapipdf.js@mozilla.org/chrome/content/style/images/toolbarButton-zoomIn.png
[task 2017-04-28T08:37:07.194419Z] 08:37:07     INFO -  browser/chrome/pdfjs/content/web/images/toolbarButton-zoomIn.png
[task 2017-04-28T08:37:07.194661Z] 08:37:07     INFO -  browser/features/ppapipdf.js@mozilla.org/chrome/content/style/images/treeitem-collapsed-rtl.png
[task 2017-04-28T08:37:07.194963Z] 08:37:07     INFO -  browser/chrome/pdfjs/content/web/images/treeitem-collapsed-rtl.png
[task 2017-04-28T08:37:07.195276Z] 08:37:07     INFO -  browser/features/ppapipdf.js@mozilla.org/chrome/content/style/images/treeitem-collapsed@2x.png
[task 2017-04-28T08:37:07.195576Z] 08:37:07     INFO -  browser/chrome/pdfjs/content/web/images/treeitem-collapsed@2x.png
Hey rhelmer,

We ran into this issue when we try to enable our pdf add-on on try server. It seems that we are not allowed to use same files as pdfjs. However, we want to try to let user feel as less change as possible when we shift from pdf.js to our add-on. Can you give me some advices to fix this? 

thank you very much :)
Flags: needinfo?(rhelmer)
(In reply to Ya-Chieh Wu from comment #3)
> Hey rhelmer,
> 
> We ran into this issue when we try to enable our pdf add-on on try server.
> It seems that we are not allowed to use same files as pdfjs. However, we
> want to try to let user feel as less change as possible when we shift from
> pdf.js to our add-on. Can you give me some advices to fix this? 
> 
> thank you very much :)

The build/packaging system does not allow duplicate files, so instead of copying these files I suggest:

1) let pdfjs continue to register these and reference them from mortar like: resource://pdf.js/web/images/treeitem-expanded.png

Or:

2) move these files to mortar, and change pdfjs to use resource://mortar/.../treeitem-expanded.png

Please let me know if that helps!
Flags: needinfo?(rhelmer) → needinfo?(ywu)
This is very helpful :) thanks a lot, Robert. we will discuss what is the better way for us :)
Flags: needinfo?(ywu)
Hey Luke,

Since this is the issue of our UI files, can I have your thoughts on this?
Flags: needinfo?(lchang)
If we're not allowed to add more exceptions to "browser/installer/allowed-dupes.mn", I personally prefer Robert's first suggestion as we don't need to modify the pdfjs' codebase at this moment. Besides, pdfjs won't be broken if, for some reason, mortar is disabled.
Flags: needinfo?(lchang)
I am going to check how many and how big the duplicate files are. btw Luke just tells me that we don't use all of the duplicate files so I will remove the unused files first.
We copied around 63 image files (exclude some which we don't use) from pdfjs and the total size is around 300k. I don't know if this is ok if we add them to "browser/installer/allowed-dupes.mn"? I would prefer to make one copy from pdfjs too because we basically are different add-ons and if we reference our image from pdfjs, there is no guarantee that pdfjs won't break us.  

How do you think, Robert?
thank you very much :)
Flags: needinfo?(rhelmer)
(In reply to Ya-Chieh Wu from comment #9)
> We copied around 63 image files (exclude some which we don't use) from pdfjs
> and the total size is around 300k. I don't know if this is ok if we add them
> to "browser/installer/allowed-dupes.mn"? I would prefer to make one copy
> from pdfjs too because we basically are different add-ons and if we
> reference our image from pdfjs, there is no guarantee that pdfjs won't break
> us.  
> 
> How do you think, Robert?
> thank you very much :)

If the intention is to remove the duplicates eventually (by removing pdfjs) then I am OK with this.

Please also file a bug to remove pdfjs (if it does not exist yet) and add a comment to allowed-dupes.mn along with your changes that mentions the new bug. If filing such a bug is inappropriate for some reason then mentioning the current bug is fine.
Flags: needinfo?(rhelmer) → needinfo?(ywu)
Well, hm. 300k is quite a bit to add... how soon can pdfs be removed? I think you might get pushback trying to ship this, unless pdfs can be removed at the same time mortar/pdfium ships.
Flags: needinfo?(ywu) → needinfo?(brsun)
As our team's previous discussion, we won't remove pdfjs when we ship pdfium because we want pdfjs as a fallback solution for pdf viewer. I can't say how soon can pdfjs be removed now. At least, there will be some overlap period until we prove pdfium is reliable.
Our plan is to ship our pdfium add-on disable with "pdfium.enabled" preference[1] off in the beginning. So my best guess is pdf.js and pdfium would co-exist for a while before pdfium can be enabled by default, at least.

[1] bug 1338476
Flags: needinfo?(brsun)
Due to the way pdf.js is included into the build (it gets packaged up into the omni jar and registered along with other built-in Firefox resources making them pretty safe to rely on), I think the best course is to use the pdfjs-registered resources until it's time to remove pdfjs from the packaged Firefox build.

This does require moving the files later and I think this would lead to a clearer VCS history.

I don't think pdfjs is getting significant changes anymore and shouldn't break you - one thing that would help here is to add a test to ensure that the resources are present just in case.
Ok. thank you, Robert for your valuable input.
Hey Luke,

In this patch, I remove all pdfjs image files from our mortar and change the reference in "viewer.css" to pdfjs. Are there any cases I miss to reference image to pdfjs in our mortar you can think of?

thank you, Luke. :p
Attachment #8866686 - Flags: feedback?(lchang)
Comment on attachment 8866686 [details] [diff] [review]
0004-Bug-1360494-remove-duplicate-image-files-and-referen.patch

Looks good. I think "viewer.css" is the only file containing image references. Thanks for cleaning up.
Attachment #8866686 - Flags: feedback?(lchang) → feedback+
Attachment #8867640 - Flags: review?(ehung)
Attachment #8867641 - Flags: review?(ehung)
Comment on attachment 8867641 [details]
bug 1360494 - part 2 add mochitest to make sure the image files exist.

https://reviewboard.mozilla.org/r/139230/#review142938

r+ with a few nits.

::: browser/extensions/mortar/test/mochitest/test_bug1360494.html:15
(Diff revision 2)
> +</head>
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1360494">Mozilla Bug 1360494</a>
> +<p id="display"></p>
> +<pre id="test">
> +<script class="testbody" type="text/javascript">

nit: it will be more readable by adding an indentation here.

::: browser/extensions/mortar/test/mochitest/test_bug1360494.html:102
(Diff revision 2)
> +function finishTest() {
> +  SimpleTest.finish();
> +}
> +
> +function fail(event) {
> +    ok(false);

nit: add a message for the failure.

::: browser/extensions/mortar/test/mochitest/test_bug1360494.html:118
(Diff revision 2)
> +function loadImage(uri, callback) {
> +    var img = document.createElement("img");
> +    img.onerror = fail;
> +    img.onload = success;
> +    img.callback = callback;
> +    img.src = "resource://pdf.js/web/images/"+ uri;

nit: space before '+'

::: browser/extensions/mortar/test/mochitest/test_bug1360494.html:119
(Diff revision 2)
> +    var img = document.createElement("img");
> +    img.onerror = fail;
> +    img.onload = success;
> +    img.callback = callback;
> +    img.src = "resource://pdf.js/web/images/"+ uri;
> +}

nit: the indentations above are inconsistent. We usually use two whitespces in JS code.
Attachment #8867641 - Flags: review?(ehung) → review+
Comment on attachment 8867640 [details]
bug 1360494 - part 1 remove duplicate image files and reference them from pdfjs.

https://reviewboard.mozilla.org/r/139228/#review142942

LGTM. Thanks for removing unused images.
Attachment #8867640 - Flags: review?(ehung) → review+
Comment on attachment 8867641 [details]
bug 1360494 - part 2 add mochitest to make sure the image files exist.

https://reviewboard.mozilla.org/r/139230/#review143306

carry r+ from ehung
Attachment #8867641 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1d0a2e2c167d
part 1 remove duplicate image files and reference them from pdfjs. r=evelyn
https://hg.mozilla.org/integration/autoland/rev/19c4f1bf59e3
part 2 add mochitest to make sure the image files exist. r=evelyn
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1d0a2e2c167d
https://hg.mozilla.org/mozilla-central/rev/19c4f1bf59e3
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: