854800 - provide some isReady(thing) method to take tabs, windows, and things

Status: RESOLVED INCOMPLETE

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

Description

[Erik Vold [:erikvold] (please needinfo? me)]

see https://bugzilla.mozilla.org/show_bug.cgi?id=832934#c5 in bug 832934

Comment 1

[shreyas]

Hi

I would like to work on this bug. Can you please give more info and how to go about it?

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

Ah sorry, we have to make sure that he module owner approves of this first.

I suggest we create a module, maybe `sdk/view/ready` which exports an `isReady` function, which is a dispatcher, like so:

    const { dispatcher } = require("sdk/util/dispatcher");
    const isReady = dispatcher("isReady");
    ...

As is done for `isPrivate` at the moment, and `isReady` can accept the same inputs which `isPrivate` currently supports.

https://github.com/mozilla/addon-sdk/blob/c983c8e7138475def3d120ed1b5dce8c6b5a5409/lib/sdk/private-browsing/utils.js#L49-L54

Does this sound ok to you for a contributor to work on Irakli?

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Sure that sounds fine by me.

Comment 4

[shreyas]

Can I work on it now?

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

ya! :)

Comment 6

[shreyas]

So basically what I need to do is create a ready.js which exports an isReady function based on the lines of 
isPrivate right?

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to shreyas from comment #6)
> So basically what I need to do is create a ready.js which exports an isReady
> function based on the lines of 
> isPrivate right?

yes, that's all correct, module would be at sdk/view/ready

Comment 8

[shreyas]

How can I test my patch?

Comment 9

[shreyas]

Attachment: bug854800.diff

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8557488 [details] [diff] [review]
bug854800.diff

I can take this one Irakli.

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to shreyas from comment #8)
> How can I test my patch?

So you will want to make a `test/test-view-ready.js` file, require your new module, and assert that the expected outputs are generated for things like `isReady()` (here there are no arguments), `isReady(window)` where window is a top level window, or a low level nsIDOMWindow, and other inputs.

Here is an example https://github.com/mozilla/addon-sdk/blob/master/test/test-private-browsing.js

Comment 12

[shreyas]

I am not getting it. Could you please elaborate? Does the first patch that I submitted look correct?

Comment 13

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to shreyas from comment #12)
> I am not getting it. Could you please elaborate?

Which part did you not understand?

> Does the first patch that I
> submitted look correct?

The structure looks correct, the logic appears to be a copy of the `isPrivate` logic, so that would need to be changed.  For instance we don't need `isReady.when(x => Ci.nsIPrivateBrowsingChannel && x instanceof Ci.nsIPrivateBrowsingChannel, x => x.isChannelPrivate);` or this line `isReady.when(isPermanentPrivateBrowsing, _=> true);`.

Let's also have the default case return `undefined` for now instead of `isReady.define(() => false);` and we should replace `isReady.when(x => x instanceof Ci.nsIDOMWindow, isWindowPrivate);` with `isReady.when(x => x instanceof Ci.nsIDOMWindow, x.document.readyState == "interactive" || x.document.readyState == "complete");`

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8557488 [details] [diff] [review]
bug854800.diff

Just getting this off my dashboard, f+ though, let me know if you have any questions about my previous comment.

Comment 15

[Andy McKay]

Because of the difficulty finding mentors and the expected life span of the SDK, we are removing [good first bug] from remaining SDK bugs.

Comment 16

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562