Note: There are a few cases of duplicates in user autocompletion which are being worked on.

address review comments for Shumway integration patch

RESOLVED FIXED in Firefox 27

Status

Firefox Graveyard
Shumway
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Gavin, Assigned: yury)

Tracking

Trunk
Firefox 27

Details

Attachments

(1 attachment, 1 obsolete attachment)

See bug 904346 comment 14.
(Assignee)

Comment 1

4 years ago
Created attachment 818537 [details] [diff] [review]
bug924571.diff

https://github.com/mozilla/shumway/pull/833
Attachment #818537 - Flags: review?(gavin.sharp)
(Assignee)

Comment 2

4 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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
(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.
(Assignee)

Comment 4

4 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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)
(Assignee)

Comment 5

4 years ago
Created attachment 818679 [details] [diff] [review]
bug924571-2.diff
(Assignee)

Updated

4 years ago
Attachment #818537 - Attachment is obsolete: true
Attachment #818537 - Flags: review?(gavin.sharp)
(Assignee)

Comment 6

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

(actually, hg mv before sync'ing with built sources helped)
Attachment #818679 - Flags: review?(gavin.sharp)
Comment on attachment 818679 [details] [diff] [review]
bug924571-2.diff

thanks!
Attachment #818679 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
Depends on: 904346
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/93e324bc5330
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/93e324bc5330
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.