726235 - break out XPath code into separate module

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Dietrich Ayala (:dietrich)]

start moving code not directly related to session restore out into their own home.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: v1

move xpath generator code to a module, and rename to be more explicit.

the only change made to the xpath code itself was a s/Ci/Components.interfaces/.

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

There was some similar stuff happening in bug 697903, but that stalled and included the rest of the form restoration. But then e10s concerns stalled it and it didn't pick back up.

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: break out XPath code into separate module (

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 601827 [details] [diff] [review]
break out XPath code into separate module (

>diff --git a/browser/components/sessionstore/src/XPathGenerator.js b/browser/components/sessionstore/src/XPathGenerator.js
>new file mode 100644

Don't forget a license header!

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: break out XPath code into separate module (

Comment 6

[Dietrich Ayala (:dietrich)]

Now with license header (thx zpao).

Comment 7

[Dão Gottwald [:dao]]

Do we expect this module to be useful for non-sessionstore code?
If so, can we put it in toolkit?
Otherwise, can the file be named SessionstoreXPathGenerator.js or put in resource:///modules/sessionstore/?

Comment 8

[Dietrich Ayala (:dietrich)]

Sure, this code has nothing specific to sessionstore in it anymore (note to self: other than the "sss_", which should be removed).

Where in toolkit do you suggest? We don't have /toolkit/modules yet. Could add one, I suppose.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Dietrich Ayala (:dietrich) from comment #8)
> Sure, this code has nothing specific to sessionstore in it anymore

Yeah, but do we actually expect other code to use it?

> Where in toolkit do you suggest? We don't have /toolkit/modules yet. Could
> add one, I suppose.

toolkit/content/ contains some JS modules.

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to Dietrich Ayala (:dietrich) from comment #8)
> > Sure, this code has nothing specific to sessionstore in it anymore
> 
> Yeah, but do we actually expect other code to use it?

Not for the forseeable future. Maybe if 697903 get's picked up again but we can always move it to a more general location then if need be.

Personally, I like resource:///modules/sessionstore/, especially as we move towards a compatmentalized sessionstore

Comment 11

[Dietrich Ayala (:dietrich)]

(In reply to Paul O'Shannessy [:zpao] from comment #10)
> Personally, I like resource:///modules/sessionstore/, especially as we move
> towards a compatmentalized sessionstore

Sure. Dao?

Comment 12

[Dão Gottwald [:dao]]

Sounds good to me.

Comment 13

[Dietrich Ayala (:dietrich)]

Attachment: break out XPath code into separate module (

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 602155 [details] [diff] [review]
break out XPath code into separate module (

* renamed to jsm
* now in modules/sessionstore/

Comment 15

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 602155 [details] [diff] [review]
break out XPath code into separate module (

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

::: browser/components/sessionstore/src/Makefile.in
@@ +48,5 @@
>  	nsSessionStartup.js \
>  	$(NULL)
>  
> +libs::
> +	 $(NSINSTALL) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/sessionstore

Nit, you have a space after your tab in there

Comment 16

[Dietrich Ayala (:dietrich)]

Attachment: for check-in

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cbefc9ea3f

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f6cbefc9ea3f

Comment 19

[Philip Chee]

> Yeah, but do we actually expect other code to use it?
Well yes. SeaMonkey would like to use the module too in our session restore code, rather than forking it unnecessarily.

Comment 20

[[e-mail]]

Comment on attachment 608545 [details] [diff] [review]
for check-in

When you move this to toolkit, please use EXTRA_JS_MODULES...

Comment 21

[Dão Gottwald [:dao]]

(In reply to Philip Chee from comment #19)
> > Yeah, but do we actually expect other code to use it?
> Well yes. SeaMonkey would like to use the module too in our session restore
> code, rather than forking it unnecessarily.

To move this to toolkit, I think we want more demand than from a sessionstore fork. Obviously it would be easier for SeaMonkey if way more sessionstore code was shared with a stable API...