Liberate URLRule (match patterns) from page-mod module

RESOLVED FIXED in 0.9

Status

Add-on SDK
General
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

unspecified

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 481675 [details] [diff] [review]
patch

context-menu will be using page-mod style match patterns to match URLs.  This patch breaks out URLRule from page-mod into its own jetpack-core module.  I renamed it match-pattern, since I think that's what these things are more commonly known as, and fleshed it out by making a class with a test(url) method.  Patch includes a unit test, expanded from page-mod's URLRule test, and a doc.

I'll attach a separate patch to this bug that converts page-mod to use this.
Attachment #481675 - Flags: review?(myk)
Irakli, is this line intentional?

http://mxr.mozilla.org/labs-central/source/jetpack-sdk/packages/addon-kit/lib/page-mod.js#170

I think it's supposed to be a for-loop, not a for-each-loop.  If so, I can fix it when I post the page-mod patch here.
Created attachment 481697 [details] [diff] [review]
page-mod patch
Attachment #481697 - Flags: review?(myk)
Attachment #481697 - Flags: feedback?(rFobic)
Created attachment 481698 [details] [diff] [review]
patch 2

Missed a couple of useful asserts from one of the page-mod tests that tests URLRule.
Attachment #481675 - Attachment is obsolete: true
Attachment #481698 - Flags: review?(myk)
Attachment #481698 - Flags: feedback?(rFobic)
Attachment #481675 - Flags: review?(myk)
Comment on attachment 481697 [details] [diff] [review]
page-mod patch

Looks good, works well, r=myk.
Attachment #481697 - Flags: review?(myk) → review+
Comment on attachment 481698 [details] [diff] [review]
patch 2

Looks good, works well, r=myk.
Attachment #481698 - Flags: review?(myk) → review+
I've noticed I had a bug introducing a global and you've repeated it.

-    for each (rule in RULES) {
+    for (rule in RULES) {

Instead should be be:
 
for (let rule in RULES) {

and yes you're right it's was supposed to be for in

The rest looks good to me!
Comment on attachment 481697 [details] [diff] [review]
page-mod patch

I've noticed I had a bug introducing a global and you've repeated it.

-    for each (rule in RULES) {
+    for (rule in RULES) {

Instead should be be:

for (let rule in RULES) {

and yes you're right it's was supposed to be for in

The rest looks good to me!
Attachment #481697 - Flags: feedback?(rFobic) → feedback-
Comment on attachment 481698 [details] [diff] [review]
patch 2

looks good
Attachment #481698 - Flags: feedback?(rFobic) → feedback+
Attachment #481697 - Flags: feedback- → feedback+
can you please fix that one before checking in!
Thanks
Thanks Irakli!  I made that change.  Also, I forgot to update the page-mod doc to point to the new match-pattern doc for pattern descriptions, so I made that change too.

http://hg.mozilla.org/labs/jetpack-sdk/rev/a9f51da42dd1
http://hg.mozilla.org/labs/jetpack-sdk/rev/72f3fd3f2cef
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: -- → 0.9
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.