Closed Bug 726235 Opened 8 years ago Closed 8 years ago

break out XPath code into separate module

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 14

People

(Reporter: dietrich, Assigned: dietrich)

References

Details

Attachments

(1 file, 4 obsolete files)

start moving code not directly related to session restore out into their own home.
Attached patch v1 (obsolete) — Splinter Review
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/.
Assignee: nobody → dietrich
Attachment #596230 - Flags: review?(paul)
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.
Blocks: 697903
Attachment #596230 - Attachment is obsolete: true
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
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!
Keywords: checkin-needed
Attachment #601827 - Attachment is obsolete: true
Now with license header (thx zpao).
Keywords: checkin-needed
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/?
Keywords: checkin-needed
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.
(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.
(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
(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?
Sounds good to me.
Attachment #601833 - Attachment is obsolete: true
Comment on attachment 602155 [details] [diff] [review]
break out XPath code into separate module (

* renamed to jsm
* now in modules/sessionstore/
Attachment #602155 - Flags: review?(paul)
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
Attachment #602155 - Flags: review?(paul) → review+
Attached patch for check-inSplinter Review
Attachment #602155 - Attachment is obsolete: true
Attachment #608545 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cbefc9ea3f
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/f6cbefc9ea3f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
> 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 on attachment 608545 [details] [diff] [review]
for check-in

When you move this to toolkit, please use EXTRA_JS_MODULES...
(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...
You need to log in before you can comment on or make changes to this bug.