974718 - The Downloads Indicator may stay highlighted after "Clear Downloads" is selected in the Library window

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Frank]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140212131424

Steps to reproduce:

1. Open the download manager (Ctrl+J) and leave it open
2. Start a download, i.e. an image from Google search
3. Once the download is complete the download arrow will turn green
4. In the already opened download manager select "Clear Downloads"
5. There are no longer any downloads, but the arrow is still green


Actual results:

The download arrow stays green when "Clear Downloads" is pressed and there are no active downloads.


Expected results:

The download arrow should return to black if "Clear Downloads" is selected.

Comment 1

[Manish Goregaokar [:manishearth]]

Reproduced on Nightly.

The relevant code is probably here[1], and we need to set the "attention" value of the indicator to false[2], which we probably can do by changing a variable somewhere since there are tons of getters and setters involved here.


 [1]: http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/DownloadsCommon.jsm#600
 [2]: http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/indicator.js#467

Comment 2

[Manish Goregaokar [:manishearth]]

(A note on the reproduction steps Ctrl-J doesn't work for me on Nightly, but clicking the downloads arrow and "show all downloads" does work)

Comment 3

[Manish Goregaokar [:manishearth]]

Ah, the class

Comment 4

[Manish Goregaokar [:manishearth]]

Marking as good first bug for now. Already put the mxr links out there, might as well :)


If you want to take this bug, just leave a comment saying so :)

The links above should help you get started, we basically need to clear the attention value of all indicators somehow. Note that this bug applies to all open windows of the same privacy level, not just the RecentWindow, since the color change applies to all windows. 

For this, you will probably have to use |Services.wm.getEnumerator("navigator:browser");|[1] along with |PrivateBrowsingUtils.isWindowPrivate| to get all windows of the same privacy levels, and then use |DownloadsCommon.getIndicatorData|.

Ask me if you have any further queries!


 [1]: https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIWindowMediator

Comment 5

[Himani Agrawal]

i would like to work on this

Comment 6

[Manish Goregaokar [:manishearth]]

OK, So I looked into the code, and there's a cleaner way of doing it.



The DownloadsDataCtor object (where the removeDownloads[1] function is) has a "_views" property (this._views), which contains all views registered with it. A view is basically an object that displays download information, it can be the indicator or something else. All these views extend the DownloadsViewPrototype[2] base class, so they inherit all the methods defined there.

So we need to update the view associated with the indicator. This turns out to be a DownloadsIndicatorDataCtor object. While not all views monitor the attention, DownloadsIndicatorDataCtor has a setter registered for "attention"[3] that lets it monitor a change in the attention value and propagate it to all the views. Wonderful, because it's able to handle the "find all relevant indicators" on its own.

So basically, we have to set the attention property of this one to true.


There are three ways to do this. While you only need to implement one of them, I suggest you understand all three so that you can learn :)

##Method 1: The laziest way, and not good coding practice

In the removeDownloads[1] function, just do a for...of loop over all the _views (look at the code of the nearby functions to see how to do this), and set the "attention" attribute for them to false.

This is bad coding since we'll be adding useless "attention" properties to the views that don't have them. We can improve this by only doing it when the setter for "attention" is defined.

##Method 2: Less lazy, and good coding

First, add a getter to DownloadsIndicatorDataCtor for "attention", right below the corresponding setter[3]. Just have it return the value.

Now do the same as Method 1, except you should check if the property "attention" exists, and only then set it.

Alternatively, you can avoid touching DownloadsIndicatorDataCtor and use getOwnPropertyDescriptor[5] to check if the setter is defined.


##Method 3: Keeping in line with the nearby coding style, making this easily extendable

This method makes the attention property a property of *all* DownloadsViewPrototype subclasses. All we have to do here is to add a dummy _attention variable to DownloadsViewPrototype, and a basic setter for attention that just sets _attention to the given parameter.[4] This setter will be automatically overridden by the DownloadsIndicatorDataCtor class.

Now, do the same as method 1.

This has the advantage of exposing the attention variable as an API for all Downloads views to use, even new ones written by addons.

However, to make it consistent, we also will have to propagate attention=true to all these views when necessary, which makes this unnecessarily tedious.

-------------

Hope this helps, if not, leave a comment, or you know where to find me in IRC :)


 [1]: http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/DownloadsCommon.jsm#600
 [2]: http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/DownloadsCommon.jsm#1085
 [3]: http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/DownloadsCommon.jsm#1385
 [4]: Read https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Working_with_Objects#Defining_getters_and_setters for more info on getters and setters.
 [5] http://stackoverflow.com/questions/8591873/determine-if-a-javascript-property-has-a-getter-or-setter-defined

Comment 7

[Manish Goregaokar [:manishearth]]

Whoops, forgot to mention this: Preferably do some variant of Method #2 above.

Comment 8

[:Paolo Amadini]

Actually, this probably boils down to setting the "attention" property [3] to false at the right time.

There is no need to read the property before setting it, also because the final attribute is not changed if the property has not been modified, and this is handled at the view level.

Comment 9

[Manish Goregaokar [:manishearth]]

(In reply to :Paolo Amadini from comment #8)
> Actually, this probably boils down to setting the "attention" property [3]
> to false at the right time.

Yep, that works (method #1), however not all of the registered views *have* an "attention" property. So it ends up creating a superfluous property for those views that does nothing and is false all the time. IMO that's not really a clean way of doing it.

If we're okay with that, then we can just do it that way.

Comment 10

[Manish Goregaokar [:manishearth]]

Just a note, with the patch if you want to trigger the UI without downloading anything, try

    let v=DownloadsCommon.getIndicatorData(window)._views[0];v.hasDownloads=true;
    //I think this bit is async, so wait a second before entering the next part
    v.hasActiveDownloads=true;v.percentComplete=-1;v.isUndetermined=true;

Comment 11

[Manish Goregaokar [:manishearth]]

Oops, commented on wrong bug.

I had discussed this with mconley in #fx-team as well. I didn't exactly propose method #1, but he seemed OK with method #2. http://logbot.glob.com.au/?c=mozilla%23fx-team#c106293

I understand that the check is done internally by the setter, however the issue here is that we're tacking on an extra property "attention", to objects that don't have it as well if we blindly iterate through this._views. (So checking for existence of the setter avoids this)

Comment 12

[Himani Agrawal]

@Manishearth: I have patch to show you. I couldn't push it, as i don't have any rights.

Comment 13

[Manish Goregaokar [:manishearth]]

(In reply to himani93 from comment #12)
> @Manishearth: I have patch to show you. I couldn't push it, as i don't have
> any rights.

Use the "add attachment" link above and upload it. In the flag section, ask me for feedback (Set feedback: to "?" and enter :manishearth into the textbox and select from the dropdown). You can also ask :paolo or :mconley for review if you think that the patch is a final one. Or you can ask them for feedback.

Comment 14

[Himani Agrawal]

Attachment: name.patch

Comment 15

[Manish Goregaokar [:manishearth]]

Comment on attachment 8379743 [details] [diff] [review]
name.patch

Review of attachment 8379743 [details] [diff] [review]:
-----------------------------------------------------------------

Your commit message is a bit strange, I see 

Bug 974718 - 1 patch
/** puesudo-code
    iterate over each view.
    check if attention property exists
    if yes set it to false.
*/

but we probably want something like

Bug 974718 - Clear downloads indicator when downloads are cleared from the panel

You can edit the commit message with |hg qrefresh -m "new commit messaage"| if you generated the patch with mq.

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +596,5 @@
>  
>    /**
>     * Asks the back-end to remove finished downloads from the list.
> +   *
> +   /

You've accidentally put an extra newline here, this will break the comment and subsequently the script.

@@ +603,5 @@
>      let promiseList = Downloads.getList(this._isPrivate ? Downloads.PRIVATE
>                                                          : Downloads.PUBLIC);
>      promiseList.then(list => list.removeFinished())
>                 .then(null, Cu.reportError);
> +  

Nit: Extra whitespace

@@ +604,5 @@
>                                                          : Downloads.PUBLIC);
>      promiseList.then(list => list.removeFinished())
>                 .then(null, Cu.reportError);
> +  
> +    /** puesudo-code

This could be a one line comment of the form "Clear indicator attention status" or something like that.

@@ +610,5 @@
> +      check if attention property exists
> +      if yes set it to false.
> +    */
> +    let allWindows = Iterator(this._viwes);
> +    for (var windows in allWindows)

We don't need an iterator here. We can just do |for (let view of this._views)| since this._views is a simple array.

@@ +614,5 @@
> +    for (var windows in allWindows)
> +    {
> +      if (windows.hasOwnProperty(attention))
> +        {
> +          windows.attention(false);

windows.attention=false should do the trick, attention is a setter, which doesn't behave like a function.

Javascript setters are functions which behave like variables, i.e. they get called when you _set_ the value of the variable.

So when you do |view.attention=false|, the |set attention()| function is called.

Comment 16

[Manish Goregaokar [:manishearth]]

Additionally, I myself can't grant review+ on a patch; you should just set the feedback flag to ?manishearth if you want my review. We'll probably ask :paolo (or :mconley) for review on this; they have the necessary rights.

(If you decide a reviewer, add an ";r=reviewerusername" to the patch commit message)

Are you testing this locally? If you've got a build set up all you need to do is run ./mach build browser/components/downloads/src from the root mozilla-central directory; and after that ./mach run.

If you're unable to test it, let me know, I can try it out on my system (and we can try to solve any build system issues over IRC)

Comment 17

[:Paolo Amadini]

(In reply to Manish Goregaokar [:manishearth] from comment #11)
> I understand that the check is done internally by the setter, however the
> issue here is that we're tacking on an extra property "attention", to
> objects that don't have it as well if we blindly iterate through
> this._views. (So checking for existence of the setter avoids this)

We're talking about two different things here!

As part of method #2, you mentioned adding a getter to DownloadsIndicatorDataCtor for "attention" below the existing setter. I said that this is unnecessary.

With regard to the fact that we set the "attention" property on view objects that don't have it, I agree that this is not ideal. We could do two things here, but since that is more of a "correctness" issue independent from the reported problem, we should do that in a different bug.

In the other bug, we could either simply add the setter to all the views (considering that as a mandatory part of the "interface" of registered views) or consider it optional, though in the latter case I'd check to see if there are other setters that we might want to consider optional. Feel free to file a new bug if you want to discuss this further!

Comment 18

[Manish Goregaokar [:manishearth]]

(In reply to :Paolo Amadini from comment #17)
> (In reply to Manish Goregaokar [:manishearth] from comment #11)
> > I understand that the check is done internally by the setter, however the
> > issue here is that we're tacking on an extra property "attention", to
> > objects that don't have it as well if we blindly iterate through
> > this._views. (So checking for existence of the setter avoids this)
> 
> We're talking about two different things here!
> 
> As part of method #2, you mentioned adding a getter to
> DownloadsIndicatorDataCtor for "attention" below the existing setter. I said
> that this is unnecessary.


Ah, I see :)
 
> With regard to the fact that we set the "attention" property on view objects
> that don't have it, I agree that this is not ideal. We could do two things
> here, but since that is more of a "correctness" issue independent from the
> reported problem, we should do that in a different bug.

So we should just do view.attention=true and worry about the extra properties in a different bug? Seems OK.

====

@Himani, so we probably don't need to do the check for the attention setter, at least not in this bug*. So the necessary bit is reduced to a simple for..of loop with no ifs inside. Leave a comment or ping me in IRC if you need further help on this :)



*Once this one is resolved, I'll file a separate one, let me know if you're interested! It would be a good way to move on to slightly more complicated code in the same module.

Comment 19

[:Paolo Amadini]

Resetting to false the "attention" property on the DownloadsDataCtor object (whose instances are DownloadsData and PrivateDownloadsData) should suffice - the object already has the logic to iterate over the views, which will result in the "attention" properties of all the view objects being set to false.

Since the function to be modified already is a method declared on DownloadsDataCtor, you could probably just do "this.attention = false;".

Comment 20

[Himani Agrawal]

Attachment: name1.patch

As discussed, this patch iterates over _views array and set attention to false for each array.

A new bug will be filled and that set make attention a property for all views.

Comment 21

[Manish Goregaokar [:manishearth]]

Comment on attachment 8380378 [details] [diff] [review]
name1.patch

Review of attachment 8380378 [details] [diff] [review]:
-----------------------------------------------------------------

No need to ask for superreview for this, though you can ask for an additional review by editing the patch.

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +604,5 @@
>      promiseList.then(list => list.removeFinished())
>                 .then(null, Cu.reportError);
> +  /**
> +  * Sets attention property false for every view
> +  */

Nit : this could probably be a one line comment, or maybe not there at all, since the code below it is pretty small. Something like `// Reset attention for all views` may be better, also.

Also, `/**`  is for documentation, usually.

@@ +606,5 @@
> +  /**
> +  * Sets attention property false for every view
> +  */
> +  for (let window of _views)
> +    window.attention = false;

You need to use this._views here, otherwise the patch won't work.

Also, maybe use `view` instead of `window` here?

Comment 22

[Himani Agrawal]

Attachment: patch3.patch

Comment 23

[Manish Goregaokar [:manishearth]]

Comment on attachment 8380381 [details] [diff] [review]
patch3.patch

Review of attachment 8380381 [details] [diff] [review]:
-----------------------------------------------------------------

The patch applies to the older patch; you probably need to qfold it as discussed in IRC.


I'm doing the same on my system, will test it in a moment while you work on finalizing the patch.

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +602,5 @@
>      let promiseList = Downloads.getList(this._isPrivate ? Downloads.PRIVATE
>                                                          : Downloads.PUBLIC);
>      promiseList.then(list => list.removeFinished())
>                 .then(null, Cu.reportError);
> +    

Nit: There is some extra whitespace (spaces) on this line.

Comment 24

[Himani Agrawal]

Attachment: patch1.patch

Comment 25

[Manish Goregaokar [:manishearth]]

Comment on attachment 8380382 [details] [diff] [review]
patch1.patch

Review of attachment 8380382 [details] [diff] [review]:
-----------------------------------------------------------------

This patch works, thanks :)

Comment 26

[:Paolo Amadini]

Comment on attachment 8380382 [details] [diff] [review]
patch1.patch

Actually, the DownloadsIndicatorData object should be used here, so that its internal state is updated correctly. Can you check if the following alternative solution works?

    let indicatorData = this._isPrivate ? PrivateDownloadsIndicatorData
                                        : DownloadsIndicatorData;
    indicatorData.attention = false;

nit: please leave a blank line before the new code in the function.

Comment 27

[:Paolo Amadini]

I see how the previous patch would also update the DownloadsIndicatorData internal state correctly, but this alternative solution avoids the issue of forcing an "attention" property on DownloadsData consumers entirely, because only DownloadsIndicatorData consumers will need that property.

Comment 28

[Manish Goregaokar [:manishearth]]

Ah, that would be better.


FWIW there is no "clear downloads" button for the private browsing view (which is a simple incontent list), but that isn't much of an issue; we can keep the check for _isPrivate here for future-proof-ness.

Comment 29

[Manish Goregaokar [:manishearth]]

Any updates on this? Comment 26 is pretty clear on what the final patch should look like, feel free to ask for more help either here or in IRC.

Comment 30

[Saurabh Anand [:sawrubh]]

Clearing the assigned flag.

Comment 31

[[e-mail]]

Attachment: v1

Comment 32

[Manish Goregaokar [:manishearth]]

Comment on attachment 8399082 [details] [diff] [review]
v1

Review of attachment 8399082 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +602,5 @@
> +     let promiseList = Downloads.getList(this._isPrivate ? Downloads.PRIVATE
> +                                                         : Downloads.PUBLIC);
> +     promiseList.then(list => list.removeFinished())
> +                .then(null, Cu.reportError);
> +    for (let view of this._views){

This for loop is unnecessary, the *DownloadsIndicatorData objects propagate stuff to the views on their own.

Comment 33

[[e-mail]]

Attachment: v2.patch

Comment 34

[Manish Goregaokar [:manishearth]]

Comment on attachment 8400648 [details] [diff] [review]
v2.patch

Review of attachment 8400648 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, though there is a small nit:

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +604,5 @@
> +     promiseList.then(list => list.removeFinished())
> +                .then(null, Cu.reportError);
> +    let indicatorData = this._isPrivate ? PrivateDownloadsIndicatorData
> +                                        : DownloadsIndicatorData;
> +    indicatorData.attention = false;

Nit: This should be indented the same as the above code.

Comment 35

[:Paolo Amadini]

Comment on attachment 8400648 [details] [diff] [review]
v2.patch

Review of attachment 8400648 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, the patch looks good. I will set the review flag on a new version with the code indentation aligned to four spaces. Also, please insert a blank line before "let indicatorData".

More importantly, have you tested again locally that this change fixes the reported issue?

Comment 36

[Manish Goregaokar [:manishearth]]

Abhishek, are you still working on this? Feel free to ask me for help if you need any.

Comment 37

[Mike Hoye [:mhoye]]

This has been hanging for a long time. Should I ask around for somebody to carry it over the line?

Comment 38

[Manish Goregaokar [:manishearth]]

If this needs fixing I can do it, but I'd prefer if a newbie did it. If you can find one, that would be great :)

Comment 39

[nithin[:lazybug]]

hi,
if nobody is working on this, can you assign it to me please ?

Comment 40

[Manish Goregaokar [:manishearth]]

Sure, could you start work on it? Contact me if you have any questions, comment 26 and existing patches might help you here.

Comment 41

[nithin[:lazybug]]

thanks ,
the link you gave to the code http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/DownloadsCommon.jsm .... says file not found , though i can access the code in my local repository . is there something i need to care about this ??

Comment 42

[Marco Bonardo [:mak]]

we have flattened the src directory there, so whatever was in downloads/src/ is now in downloads/
http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/DownloadsCommon.jsm

Comment 43

[nithin[:lazybug]]

Attachment: downIoadIndicator.patch

Comment 44

[Manish Goregaokar [:manishearth]]

Comment on attachment 8489113 [details] [diff] [review]
downIoadIndicator.patch

Review of attachment 8489113 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but this still applies to the old location of the file (and old line numbers).

Dont just edit the existing patch and submit it, test it out with a full build yourself :)

These links might help: https://developer.mozilla.org/en-US/docs/Simple_Firefox_build , https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch , https://developer.mozilla.org/en-US/docs/Mercurial_Queues

(Or step 3,4 here: http://inpursuitoflaziness.blogspot.in/2014/02/getting-started-with-bug-squashing.html)

Comment 45

[nithin[:lazybug]]

Attachment: downloadIndicator_v2.patch

Comment 46

[Manish Goregaokar [:manishearth]]

Comment on attachment 8489269 [details] [diff] [review]
downloadIndicator_v2.patch

Review of attachment 8489269 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, though the comment should be r=paolo instead of r=Manishearth

Comment 47

[nithin[:lazybug]]

Attachment: downloadIndicator_v3.patch

Comment 48

[:Paolo Amadini]

Comment on attachment 8489322 [details] [diff] [review]
downloadIndicator_v3.patch

Looks good to me, I assume you've taken care of all the relevant testing.

Comment 49

[Manish Goregaokar [:manishearth]]

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eea77f2f7e2b

Comment 50

[Manish Goregaokar [:manishearth]]

Oops, https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=92fe889d1ed7

Comment 51

[nithin[:lazybug]]

the path works fine in normal-mode. but in private-mode, even though there is no 'clear-downloads' after deleting all listed downloads using 'delete' key the indicator is not clearing. do we need to fix that in this bug ??

Comment 52

[:Paolo Amadini]

(In reply to nithin from comment #51)
> the path works fine in normal-mode. but in private-mode, even though there
> is no 'clear-downloads' after deleting all listed downloads using 'delete'
> key the indicator is not clearing. do we need to fix that in this bug ??

This looks like a separate bug, thanks for finding it! Feel free to file it in case a duplicate doesn't already exist. If you also want to try and fix it next, just request needinfo from me there if you need information on the relevant code.

Comment 53

[Manish Goregaokar [:manishearth]]

Okay, Nithin, could you file a followup bug on that issue? Marking this as checkin-needed.

I suspect that that has to do with the delete key not being hooked up correctly to the rest of the DownloadsIndicator interfaces.

Comment 54

[nithin[:lazybug]]

i have filed a new bug for that issue, https://bugzilla.mozilla.org/show_bug.cgi?id=1068052

Comment 55

[Ryan VanderMeulen [:RyanVM]]

(In reply to Manish Goregaokar [:manishearth] from comment #50)
> Oops, https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=92fe889d1ed7

That looks like a lot of mochitest-bc fail, no?

Comment 56

[Manish Goregaokar [:manishearth]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #55)
> (In reply to Manish Goregaokar [:manishearth] from comment #50)
> > Oops, https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=92fe889d1ed7
> 
> That looks like a lot of mochitest-bc fail, no?

Seemed unrelated and only for e10s. I tried to trigger a rebuild but somehow the button on treeherder wasn't working.

Re pushed: https://tbpl.mozilla.org/?tree=Try&rev=495cf62606be

Comment 57

[nithin[:lazybug]]

Attachment: downloadind_v5.patch

updated patch, as the source has been changed

Comment 58

[Tooru Fujisawa [:arai]]

pushed to try, using same tests as previous one.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=705cb490c041

Comment 59

[:Paolo Amadini]

This bug seems ready for check-in, but slipped through the cracks.

I've added the "checkin-needed" keyword.

Comment 60

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/47dff2b61b7f

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/47dff2b61b7f

Comment 62

[Catalin Varga [QA][:VarCat]]

Verified as fixed on:

FF 40
Build Id: 20150602004005
OS: Win 7 x64