603307 - Add Context classes to context-menu

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Drew Willcoxon :adw]

Attachment: patch

As discussed in bug 578849, I'd like to create proper classes for context-menu's declarative contexts.  Right now there's no good way for example to declare a context that occurs when the page contains a selection.  This patch creates PageContext(), SelectorContext(selector), and SelectionContext() constructors.  If we think of other useful contexts, we'll be able to add them easily this way (assuming that's a good thing).

Comment 1

[Drew Willcoxon :adw]

Attachment: test patch

Test changes as a separate patch.

Comment 2

[Drew Willcoxon :adw]

Attachment: patch 2

I went ahead and added URLContext(matchPattern) rather than filing a new bug for `include`.  If you take context-menu alone, URLContext is more natural than include, but I also don't like how this diverges from page-mod.  I could go either way, and I think you are keeping a sharper lookout for cross-API consistency, so if you're OK with this then I am too.

Comment 3

[Drew Willcoxon :adw]

Attachment: test patch 2

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 482253 [details] [diff] [review]
patch 2

>+      <code>include</code> property.  Read more about patterns
>+      <a href="#module/jetpack-core/match-pattern">here</a>.

It would be better to link the imperative "read more about patterns" instead of the single word "here", because then the link is more closely tied to the referent, and the link target is bigger.


>+  Creates a page context.  See Specifying Contexts above.

Can "Specifying Contexts" be linked to the section?


>+  catch (err) {
>+    console.error("Error creating URLContext match pattern:");

Did you intend to concatenate "err" to the error message?

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 482254 [details] [diff] [review]
test patch 2

>+// URL contexts should cause items to appear on page that matches.

Nit: page that matches -> pages that match

Comment 6

[Myk Melez [:myk] [@mykmelez]]

> I went ahead and added URLContext(matchPattern) rather than filing a new bug
> for `include`.  If you take context-menu alone, URLContext is more natural than
> include, but I also don't like how this diverges from page-mod.  I could go
> either way, and I think you are keeping a sharper lookout for cross-API
> consistency, so if you're OK with this then I am too.

Yeah, it's a mixed bag, but I'm ok with it.

Comment 7

[Drew Willcoxon :adw]

(In reply to comment #4)
> It would be better to link the imperative "read more about patterns"

Good point!

> Can "Specifying Contexts" be linked to the section?

Unfortunately not.  There's no way to link inside pages. :(  We should fix that...

> >+    console.error("Error creating URLContext match pattern:");
> 
> Did you intend to concatenate "err" to the error message?

Actually this isn't concatenation: it just prints out that string on one line, and then the error is thrown, where it'll be later logged with a stacktrace, etc.

(In reply to comment #5)
> Nit: page that matches -> pages that match

Thanks!

http://hg.mozilla.org/labs/jetpack-sdk/rev/b9fe55a50b53
http://hg.mozilla.org/labs/jetpack-sdk/rev/62180c0cb5b5

Comment 8

[Myk Melez [:myk] [@mykmelez]]

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".