Last Comment Bug 726235 - break out XPath code into separate module
: break out XPath code into separate module
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Dietrich Ayala (:dietrich)
:
Mentors:
Depends on:
Blocks: sessionRestoreJank 697903
  Show dependency treegraph
 
Reported: 2012-02-10 16:53 PST by Dietrich Ayala (:dietrich)
Modified: 2012-03-26 07:12 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (11.50 KB, patch)
2012-02-10 16:55 PST, Dietrich Ayala (:dietrich)
paul: review+
Details | Diff | Splinter Review
break out XPath code into separate module ( (11.73 KB, patch)
2012-02-29 17:13 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Splinter Review
break out XPath code into separate module ( (11.93 KB, patch)
2012-02-29 17:44 PST, Dietrich Ayala (:dietrich)
no flags Details | Diff | Splinter Review
break out XPath code into separate module ( (11.97 KB, patch)
2012-03-01 15:26 PST, Dietrich Ayala (:dietrich)
paul: review+
Details | Diff | Splinter Review
for check-in (11.90 KB, patch)
2012-03-22 17:35 PDT, Dietrich Ayala (:dietrich)
dietrich: review+
Details | Diff | Splinter Review

Description Dietrich Ayala (:dietrich) 2012-02-10 16:53:59 PST
start moving code not directly related to session restore out into their own home.
Comment 1 Dietrich Ayala (:dietrich) 2012-02-10 16:55:44 PST
Created attachment 596230 [details] [diff] [review]
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 reading much bugmail, email directly) 2012-02-11 10:13:08 PST
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) 2012-02-29 17:13:37 PST
Created attachment 601827 [details] [diff] [review]
break out XPath code into separate module (
Comment 4 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-29 17:37:02 PST
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) 2012-02-29 17:44:55 PST
Created attachment 601833 [details] [diff] [review]
break out XPath code into separate module (
Comment 6 Dietrich Ayala (:dietrich) 2012-02-29 17:46:23 PST
Now with license header (thx zpao).
Comment 7 Dão Gottwald [:dao] 2012-03-01 05:00:36 PST
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) 2012-03-01 07:47:22 PST
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] 2012-03-01 10:18:20 PST
(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 reading much bugmail, email directly) 2012-03-01 10:30:24 PST
(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) 2012-03-01 10:48:35 PST
(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] 2012-03-01 10:56:41 PST
Sounds good to me.
Comment 13 Dietrich Ayala (:dietrich) 2012-03-01 15:26:18 PST
Created attachment 602155 [details] [diff] [review]
break out XPath code into separate module (
Comment 14 Dietrich Ayala (:dietrich) 2012-03-01 15:27:48 PST
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 reading much bugmail, email directly) 2012-03-01 17:12:49 PST
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) 2012-03-22 17:35:31 PDT
Created attachment 608545 [details] [diff] [review]
for check-in
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-03-23 19:04:09 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cbefc9ea3f
Comment 18 Ed Morley [:emorley] 2012-03-24 13:40:14 PDT
https://hg.mozilla.org/mozilla-central/rev/f6cbefc9ea3f
Comment 19 Philip Chee 2012-03-26 05:55:01 PDT
> 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 neil@parkwaycc.co.uk 2012-03-26 06:13:41 PDT
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] 2012-03-26 07:12:31 PDT
(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...

Note You need to log in before you can comment on or make changes to this bug.