924571 - address review comments for Shumway integration patch

Status: RESOLVED FIXED

(Firefox Graveyard :: Shumway, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

See bug 904346 comment 14.

Comment 1

[Yury Delendik (:yury)]

Attachment: bug924571.diff

https://github.com/mozilla/shumway/pull/833

Comment 2

[Yury Delendik (:yury)]

(In reply to :Gavin Sharp (use [e-mail] for email) from bug 904346 comment 14)
> Comment on attachment 812355 [details] [diff] [review]
> Shumway extension core files (1/3)
> 
> Not critical issues, and I'm not sure how much of this was just copy/pasted
> from pdf.js. But here are the comments anyhow:
> 
> >diff --git a/browser/extensions/shumway/chrome.manifest b/browser/extensions/shumway/chrome.manifest
> 
> >+resource shumway content/
> >+resource shumway.components components/
> 
> Why do you need two separate resource packages? "components" implies an
> XPCOM component, and that's not what FlashStreamConverter.js is.

components/FlashStreamConverter.js is moved to content/ShumwayStreamConverter.jsm

> 
> >diff --git a/browser/extensions/shumway/components/FlashStreamConverter.js b/browser/extensions/shumway/components/FlashStreamConverter.js
> 
> >+var NSGetFactory1 = XPCOMUtils.generateNSGetFactory([FlashStreamConverter1]);
> >+var NSGetFactory2 = XPCOMUtils.generateNSGetFactory([FlashStreamConverter2]);
> 
> This is non-sensical - NSGetFactory (no "1" or "2") is a magical symbol that
> needs to be defined in JS XPCOM components, and this file is not a JS XPCOM
> component. It should probably be named .jsm and this gunk can be removed.

XPCOMUtils.generateNSGetFactory lines removed from the ShumwayStreamConverter.jsm

> 
> >diff --git a/browser/extensions/shumway/content/ShumwayUtils.jsm b/browser/extensions/shumway/content/ShumwayUtils.jsm
> 
> >+// Register/unregister a constructor as a component.
> >+function Factory() {}
> >+
> >+Factory.prototype = {
> >+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIFactory]),
> 
> This object is only used to call register() and unregister() on, so it
> doesn't actually need to be a full implementation of nsIFactory (as used by
> JS XPCOM components). You can strip this down to just an object with
> register/unregister functions.

Factories are created using XPCOMUtils._getFactory(targetConstructor).
 
> 
> >+let ShumwayUtils = {
> 
> >+  /**
> >+   * shumway is only enabled if it is both selected as the pdf viewer and if the 
> >+   * global switch enabling it is true.
> 
> Too much copy/pasting in this comment :)

Fixed

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to Yury Delendik (:yury) from comment #2)
> components/FlashStreamConverter.js is moved to
> content/ShumwayStreamConverter.jsm

You should use hg mv for this, rather than remove/add. It will make the diff much easier to read.

Comment 4

[Yury Delendik (:yury)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #3)
> (In reply to Yury Delendik (:yury) from comment #2)
> > components/FlashStreamConverter.js is moved to
> > content/ShumwayStreamConverter.jsm
> 
> You should use hg mv for this, rather than remove/add. It will make the diff
> much easier to read.

I used, git mv. However after sync'ing the built sources, I think hg will not be possible. (I was also hoping land it as combined patch during next shumway-mozcentral synchronization)

Comment 5

[Yury Delendik (:yury)]

Attachment: bug924571-2.diff

Comment 6

[Yury Delendik (:yury)]

Comment on attachment 818679 [details] [diff] [review]
bug924571-2.diff

(actually, hg mv before sync'ing with built sources helped)

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 818679 [details] [diff] [review]
bug924571-2.diff

thanks!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/93e324bc5330

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/93e324bc5330