Closed Bug 1060695 Opened 8 years ago Closed 5 years ago

switch sdk/selection to use a frame script for selection events

Categories

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

defect
Not set
normal

Tracking

(e10s+)

RESOLVED INCOMPLETE
Tracking Status
e10s + ---

People

(Reporter: zombie, Unassigned)

References

Details

Attachments

(1 file)

this module is not too high on the list of priorities (it's not used that much by addons), but it's bad because it doesn't just "fail to work" under e10s, it violently throws upon loading, so any addon that even requires it would probably fail to initialize properly..

at the very least (in the short run), we should guard against that.
Blocks: 1058538
This Add-on SDK exception also breaks the "Flip or Rotate Image" addon: bug 987937.
tracking-e10s: --- → +
Also Bug 1058538 - "DuckDuckGo Plus" add-on
Also Bug 1037198 - "SearchPreview" (Add-on SDK port, not yet in AMO) add-on
Assignee: nobody → tomica+amo
Priority: -- → P1
Duplicate of this bug: 1060099
Duplicate of this bug: 1099042
hey zombie, do you have time for this?  it's Dave's team is supposed to pick up the e10s work according to Joe, so I think we should push this bug to them since it's been broken for so long and anything using jpm & selection module is currently not working..
Flags: needinfo?(tomica+amo)
yeah, this slipped off my radar, will take a look soon.

but i'm not sure i understand how is this related to JPM? does it throw regardless if you use e10s or not? if so, than that's probably a similar-but-different (timing) issue, though likely solvable with the same fix (that's why i'm asking).
Flags: needinfo?(tomica+amo) → needinfo?(evold)
(In reply to Tomislav Jovanovic [:zombie] from comment #7)
> yeah, this slipped off my radar, will take a look soon.
> 
> but i'm not sure i understand how is this related to JPM? does it throw
> regardless if you use e10s or not? if so, than that's probably a
> similar-but-different (timing) issue, though likely solvable with the same
> fix (that's why i'm asking).

e10s is enabled by default on nightly, jpm only works on nightly, therefore the selection module does not work for jpm..
Flags: needinfo?(evold)
Hey Tom, if you aren't working on this could you please unassign yourself?
Flags: needinfo?(tomica+amo)
sure, sorry about that..
Assignee: tomica+amo → nobody
Flags: needinfo?(tomica+amo)
i'm gonna tackle this now..


(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #8)
> e10s is enabled by default on nightly, jpm only works on nightly, therefore
> the selection module does not work for jpm..

right makes sense, i was just being dim..
Assignee: nobody → tomica+amo
Status: NEW → ASSIGNED
this basically adds a selection-events.js frame script, where all the work of tracking windows & attaching selection listeners is done now, while the actual getting/setting of selection still uses CPOWs (with a slight update to the focus-related code). 

all the tests pass in e10s mode, with just a single line change to a helper function.


as discussed in irc last week, i'm still not clear and need to think/consult about:
  1) feasibility of making this sync api work without CPOWs (while not causing too much trouble), or 
  2) if it makes more sense to introduce a new async api here (in which case it's probably not worth trying to do 1).
Attachment #8546224 - Flags: review?(dtownsend)
Summary: sdk/selection.js throws immediately upon loading with e10s enabled → switch sdk/selection to use a frame script for selection events
note: just added a second commit to enable selection tests in an e10s addon, so again, it's probably easier to review the separate commits:

https://github.com/mozilla/addon-sdk/pull/1826/commits
Comment on attachment 8546224 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1826

r+ish. There are a couple of questions in there, if there are good reasons then please include them as code comments.
Attachment #8546224 - Flags: review?(dtownsend) → review+
ah, i was just about to ask if you started on this one, or if i could cancel the review?


(In reply to Tomislav Jovanovic [:zombie] from comment #12)
> while the actual getting/setting of selection still uses CPOWs 

as according to new development, this isn't good enough anymore. :\


>   1) feasibility of making this sync api work without CPOWs (while not
> causing too much trouble), 

turns out only one thing about this was "hard": setting the selection as html and then immediately trying to read it as text, or vice versa (not that i know why anybody would do that, but we have a test for it).
Duplicate of this bug: 1146540
+1

I'm encountering this on my own addon (Moo Later). Developed using JPM 1.0.2 and using only the high-level APIs. With the multiprocess shim turned on I see hundreds of CPOWs warnings on the console caused by requiring the selection API. The Addon does keep running though.

let selection = require('sdk/selection');

With the multiprocess shim turned off it throws an error and locks my addon up. They error reported is "TypeError: window is null".

Firefox version 43.0a2 (2015-10-13). Not a problem at all on Firefox 38.* where multiprocess support isn't enabled yet.
This is a high level SDK API and it's not e10s ready. Should we block on this one?
There's 269 add-ons on mxr using sdk/selection.
Priority: P1 → --
Comment on attachment 8546224 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1826

>Error retrieving Github pull request diff for https://github.com/mozilla/addon-sdk/pull/1826
42
Assignee: tomica → nobody
Status: ASSIGNED → NEW
https://bugzilla.mozilla.org/show_bug.cgi?id=1399562
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.