1090135 - use string overload of nsIDOMWindowUtils::loadSheet method for sdk/stylesheet/

Status: RESOLVED FIXED

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

Description

[Tomislav Jovanovic :zombie]

from bug 1066685: the temporary CPOW solution for our sdk/stylesheet APIs is now broken from some recent changes, and unlikely to get fixed.

the problem is that those functions we expose in stylesheet/* are synchronous. the alternative implementation is not hard, but can't be sync (without even more egregious CPOW hacks).

Irakli, what do you think about converting those to async, and returning a Promise from them? specifically util::loadSheet() and mod::attach() methods?

https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/stylesheet_style
https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/stylesheet_utils
https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/content_mod

it's somewhat convenient because they are not specified to return anything now, so returning a Promise (that can be ignored) should not break much, unless an addon depends on effects applying immediately..

the only alternative would be to completely break/remove those APIs and introduce async ones under a different name.. :\

Comment 1

[Tomislav Jovanovic :zombie]

> from bug 1066685

it's bug 1089230

Comment 2

[Tomislav Jovanovic :zombie]

Dave is adding an overload in bug 1089230.

Comment 3

[Tomislav Jovanovic :zombie]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1693

obviously not tested, and that may not even be the final name..  ;)

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

You want probably update also the direct usage of `loadSheet` and `removeSheet` in `sdk/l10n`: https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/l10n/html.js

For the tests, it should passes the previous ones to be OK – that are mainly on high level API tests that uses such methods, like page-mod and tabs –; maybe you could add some specific tests to check that it behaves like the previous method – as I expect so –: specifically, that calling multiple times `loadSheet` with the same URI, will load the sheet just once, and therefore one call of `removeSheet` would be enough.

Comment 5

[Tomislav Jovanovic :zombie]

> You want probably update also the direct usage of `loadSheet` and `removeSheet` in `sdk/l10n`:
good catch, thanks.. i only searches for uses of stylesheet/ modules, not direct DOM calls. updated the PR to use methods from stylesheet/util there as well..

> For the tests, it should passes the previous ones to be OK – that are mainly
we don't already have tests for these modules, so i'm not even sure that's how it's supposed to work -- or at least, there is no guarantee that it does..

i'd be happy add those tests, as a separate bug, since there are still things not working in e10s, which are a higher priority..

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Tomislav Jovanovic [:zombie] from comment #5)

> > For the tests, it should passes the previous ones to be OK – that are mainly
> we don't already have tests for these modules, so i'm not even sure that's
> how it's supposed to work -- or at least, there is no guarantee that it
> does..

As said, we have plenty of tests for those methods, especially in page-mod but one also in tabs: we don't test directly the usage of `loadSheet` and `removeSheet` – also because the tests you could do there would be limited, and makes more sense on platform side – instead, we tests that they behave properly when used – see `contentStyle*`, `Style` / `attach`, they are based on them.

So, if we have those tests passing, I think we're OK. My only concern is the scenario I explained above, (multiple calls of `loadSheet` shouldn't add the style multiple times); but it's not a priority definitely, as soon as the tests we already have are passing.

Comment 7

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/1fa27c8af4f935cb437fa249af42fc7aa61e1bb9
bug 1090135 - use nsIDOMWindowUtils::loadSheetUsingURIString

https://github.com/mozilla/addon-sdk/commit/bf1598106812621be9f91a639270aacb41c8c618
Merge pull request #1693 from zombie/1090135-loadSheet

bug 1090135 - use nsIDOMWindowUtils::loadSheetFromURIString(), r=@Mossop