338830 - Multiple license blocks in safe browsing JS components

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement)

Description

[Régis Caspar]

Since bug 338754 landing Safe Browsing js components are living in Firefox components directory, however, all these JS files are containing multiple license blocks (from 'grep -ce "/* ***** BEGIN LICENSE BLOCK *****" *.js'):
nsSafebrowsingApplication.js:13
nsUrlClassifierLib.js:17
nsUrlClassifierListManager.js:3
nsUrlClassifierTable.js:6

According to bug 338754 one license block is ~1.7KB so here there are (13 + 17 + 3 + 6 - 4) * ~1.7 = ~59.5 useless KB. It would be good to have one license block per file at least.

Kinda related to bug 309418.

 -> ALL/ALL (this is related to js components)
 -> making this one blocking bug 171082

Comment 1

[Benjamin Smedberg]

We should preprocess out all the license blocks.

Comment 2

[Régis Caspar]

Attachment: part1 (nsSafeBrowsingApplication.js)

before: 143Ko / After: 124Ko

Comment 3

[Régis Caspar]

Attachment: part2 (nsUrlClassifier*.js) (checked in)

for nsUrlClassifier*.js before: 201Ko / after: 198Ko
NB: toolkit.jar size is affected too

The ~1.7Ko per license seems over-estimated

Comment 4

[Régis Caspar]

(In reply to comment #3)
> The ~1.7Ko per license seems over-estimated
Note that I made my comparison between a debug and a non debug (current nightly) build, this could explain the little amount of size gained.

Comment 5

[u88484]

(In reply to comment #1)
> We should preprocess out all the license blocks.
> 

that would be bug 309418 correct?

Comment 6

[Phil Ringnalda (:philor)]

You might need to push a little bit harder for checkin, at least to the extent of adding "[checkin needed]" to the Status Whiteboard.

I'm guessing this hasn't bitrotted, since it's only at the start of files?

Comment 7

[Régis Caspar]

(In reply to comment #6)
> You might need to push a little bit harder for checkin, at least to the 
> extent of adding "[checkin needed]" to the Status Whiteboard.
OK, to be honnest, I kinda forgot about this bug.

> I'm guessing this hasn't bitrotted, since it's only at the start of files?
I will check that later today and submit  modified patch if needed.

Thanks.

Comment 8

[Régis Caspar]

Attachment: part1 (nsSafeBrowsingApplication.js) revised

Updated patch including changes for "sb-loader.js" and corrections to changes for "globalstore.js"

Checked against trunk tree (2006092816)

Comment 9

[Robert Sayre]

I don't see license in nsSafeBrowsingApplication.js in either patch.

Comment 10

[Régis Caspar]

(In reply to comment #9)
> I don't see license in nsSafeBrowsingApplication.js in either patch.
'nsSafeBrowsingApplication.js' doesn't have a license block, but include 13 files handled by part1 patch.

Comment 11

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

Comment on attachment 222879 [details] [diff] [review]
part2 (nsUrlClassifier*.js) (checked in)

Checked in on the trunk.

Comment 12

[Simon Bünzli]

Is there any reason the nsSafeBrowsingApplication patch hasn't been checked in yet?

Comment 13

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

Not that I know of.

Comment 14

[Kenneth Herron]

The patch "part1 (nsSafeBrowsingApplication.js) revised" doesn't apply for me. One failed hunk and three missing files. Could someone update it?

Comment 15

[Simon Bünzli]

While you're at it, why not doing it yourself? The missing files (eventregistrar.js, listdictionary.js, tabbedbrowserwatcher.js) have been permanently deleted from the tree and the failed hunk should just be due to the addition of a contributor name to the license block.

Comment 16

[Régis Caspar]

Attachment: part1 (nsSafeBrowsingApplication.js) revised and updated

Here's the updated version.

$ patch --dry-run -p0 < patch.patch 
patching file `browser/components/safebrowsing/content/application.js'
patching file `browser/components/safebrowsing/content/browser-view.js'
patching file `browser/components/safebrowsing/content/controller.js'
patching file `browser/components/safebrowsing/content/firefox-commands.js'
patching file `browser/components/safebrowsing/content/globalstore.js'
patching file `browser/components/safebrowsing/content/list-warden.js'
patching file `browser/components/safebrowsing/content/phishing-afterload-displayer.js'
patching file `browser/components/safebrowsing/content/phishing-warden.js'
patching file `browser/components/safebrowsing/content/reporter.js'
patching file `browser/components/safebrowsing/content/sb-loader.js'
patching file `browser/components/safebrowsing/content/tr-fetcher.js'

Removed 3 obsolete files, and added Simon in the contributor list of controller.js.

Comment 17

[Michael Wu [:mwu]]

This patch causes an error in the JS console:

Error: illegal character
Source File: chrome://browser/content/safebrowsing/sb-loader.js
Line: 1
Source Code:
# ***** BEGIN LICENSE BLOCK ***** 

sb-loader.js is not going through the preprocessor.

Comment 18

[Michael Wu [:mwu]]

Attachment: patch as checked in

I made the change to jar.mn so sb-loader.js would go through the preprocessor.

Checking in browser/components/safebrowsing/jar.mn;
/cvsroot/mozilla/browser/components/safebrowsing/jar.mn,v  <--  jar.mn
new revision: 1.5; previous revision: 1.4
done
Checking in browser/components/safebrowsing/content/application.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/application.js,v  <--  application.js
new revision: 1.12; previous revision: 1.11
done
Checking in browser/components/safebrowsing/content/browser-view.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/browser-view.js,v  <--  browser-view.js
new revision: 1.6; previous revision: 1.5
done
Checking in browser/components/safebrowsing/content/controller.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/controller.js,v  <--  controller.js
new revision: 1.10; previous revision: 1.9
done
Checking in browser/components/safebrowsing/content/firefox-commands.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/firefox-commands.js,v  <--  firefox-commands.js
new revision: 1.3; previous revision: 1.2
done
Checking in browser/components/safebrowsing/content/globalstore.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/globalstore.js,v  <--  globalstore.js
new revision: 1.16; previous revision: 1.15
done
Checking in browser/components/safebrowsing/content/list-warden.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/list-warden.js,v  <--  list-warden.js
new revision: 1.13; previous revision: 1.12
done
Checking in browser/components/safebrowsing/content/phishing-afterload-displayer.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/phishing-afterload-displayer.js,v  <--  phishing-afterload-displayer.js
new revision: 1.16; previous revision: 1.15
done
Checking in browser/components/safebrowsing/content/phishing-warden.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/phishing-warden.js,v  <--  phishing-warden.js
new revision: 1.23; previous revision: 1.22
done
Checking in browser/components/safebrowsing/content/reporter.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/reporter.js,v  <--  reporter.js
new revision: 1.5; previous revision: 1.4
done
Checking in browser/components/safebrowsing/content/sb-loader.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/sb-loader.js,v  <--  sb-loader.js
new revision: 1.17; previous revision: 1.16
done
Checking in browser/components/safebrowsing/content/tr-fetcher.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/tr-fetcher.js,v  <--  tr-fetcher.js
new revision: 1.11; previous revision: 1.10
done

Comment 19

[Régis Caspar]

Thanks Michael.