Closed Bug 1044108 Opened 7 years ago Closed 7 years ago

QR decoder needs cleanup and size reduction

Categories

(DevTools Graveyard :: WebIDE, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(2 files, 3 obsolete files)

The QR decoder file wastes lots of space with comments and massive whitespace.  Let's at least trim it and possibly minify.
Paul, this change is a one-time pass to reformat the QR decoder with a JS beautifier.  There is no real need to keep files in the style of the upstream project, as it doesn't have much activity.

This beautified file will be what we'd edit if we need to tweak something else in the future.  I've killed all comments, except the license headers and Mozilla specific notes.

The part takes us from 114kB to 90kB.  Also, the file is now possible to be read by humans.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Attachment #8463521 - Flags: review?(paul)
In this step, I've moved the human-readable version to |original.js|, and replaced |index.js| with a minified version.  The new README contains the Uglify command I used for future reference.  The decoder code is basically a black box for us, so I think it's fine to minify like this and ship that version.

This part takes us from 90kB to 46kB, so we're now at less the half the starting size.
Attachment #8463525 - Flags: review?(paul)
Paul, to support Fennec's goal of removing toolkit bits they don't need (bug 1044079), I've changed things to not ship it for that application.

gps, can you review the build changes here?
Attachment #8463529 - Flags: review?(paul)
Attachment #8463529 - Flags: review?(gps)
Comment on attachment 8463529 [details] [diff] [review]
Part 3: Disable QR decoder on Fennec for now

Review of attachment 8463529 [details] [diff] [review]:
-----------------------------------------------------------------

Build bits are fine.
Attachment #8463529 - Flags: review?(gps) → review+
(In reply to J. Ryan Stinnett [:jryans] from comment #2)
> Created attachment 8463525 [details] [diff] [review]
> Part 2: Minify QR decoder with Uglify
> 
> In this step, I've moved the human-readable version to |original.js|, and
> replaced |index.js| with a minified version.  The new README contains the
> Uglify command I used for future reference.  The decoder code is basically a
> black box for us, so I think it's fine to minify like this and ship that
> version.
> 
> This part takes us from 90kB to 46kB, so we're now at less the half the
> starting size.

Why do we minify this file specifically? What about all the other JS files we ship in B2G and Fennec?
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #6)
> (In reply to J. Ryan Stinnett [:jryans] from comment #2)
> > Created attachment 8463525 [details] [diff] [review]
> > Part 2: Minify QR decoder with Uglify
> > 
> > In this step, I've moved the human-readable version to |original.js|, and
> > replaced |index.js| with a minified version.  The new README contains the
> > Uglify command I used for future reference.  The decoder code is basically a
> > black box for us, so I think it's fine to minify like this and ship that
> > version.
> > 
> > This part takes us from 90kB to 46kB, so we're now at less the half the
> > starting size.
> 
> Why do we minify this file specifically? What about all the other JS files
> we ship in B2G and Fennec?

Mainly, I was trying to be nice to applications that don't currently minify globally (I believe b2g is the only one minifying globally currently), since I know that this file in particular would not be hurt too much by being minified, since it's external code we're using.

In any case, I don't feel too strongly about this, so we can also wait for Fennec to enable global chrome minification as part of size reduction (assuming they decide to do so).

Even without this, parts 1 and 3 still seem like good things to do.
Attachment #8463521 - Flags: review?(paul) → review+
Attachment #8463529 - Flags: review?(paul) → review+
Comment on attachment 8463525 [details] [diff] [review]
Part 2: Minify QR decoder with Uglify

I'm not sure we want to ship cryptic code. If we want to minify this code, let's do it from the makefile.
Attachment #8463525 - Flags: review?(paul) → review-
Summary: QR decoder needs cleanup and minifying → QR decoder needs cleanup and size reduction
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5c8555e5adfe
https://hg.mozilla.org/mozilla-central/rev/c1854c559c4f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.