938829 - optimize attachmentWorker.onmessage to not construct the whole attachment notification bar when it is already shown

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[:aceman]

In MsgComposeCommands.js the attachmentWorker.onmessage function, if
(keywordsFound.length > 0) then all the elements of the notification bar are constructed even if the bar already exists. Then it is just unused when the keywords did not change from previous construction. It appears this could be optimized to skip the construction when not needed.

I also think the removing of the bar (nBox.removeNotification(notification)) could be consolidated into some common place. Also the function of this.lastMessage and CheckForAttachmentNotification.shouldFire should be commented.

Comment 1

[Thomas D. (:thomas8)]

The entire code relating to the attachment notification bar looks very complex, intertwined and poorly documented, so this initiative to start a cleanup and documentation exercise in this corner is more than welcome! :)

Comment 2

[Thomas D. (:thomas8)]

Judging from what I've seen in some of the old code which we streamlined in bug 521158, there was a lot of duplication, bugs and even wrong documentation, so I'd recommend close scrutiny of the entire attachment notification code, and I read Acemen's comment 0 having similar suspicions.

Comment 3

[Suyash Agarwal (:sshagarwal)]

Attachment: Preliminary patch

Sir, I have tried to reduce the work, and it may be visible as increased code duplication.
This is the first iteration, if this is approved, I will try to collect the common part in a small function, calling it wherever desired/required.

Thanks.

Comment 4

[Suyash Agarwal (:sshagarwal)]

To be applied over patches for bugs 938783 and 938759 respectively, for testing and reviewing.

Comment 5

[:aceman]

I do not see where you set this.lastMessage to the new keywords so I am not sure this can work.

I would rip this construction and destruction code into a separate function out of attachmentWorker.onmessage, that would autonomously determine what to do based on the states of the various variables (gManualReminder, number if keywords, maybe CheckForAttachmentNotification.shouldFire), similar to toggleAttachmentReminder .
This function would be called from all places that change any of these variables. The function would keep a global state of whether the bar is shown or not and act accordingly. It would know to determine whether to hide or show the bar according to the new situation. Currently you have to open-code the hide/show events at all possible places and it may get a mess.

Mkmelin, would you like such refactoring?

Comment 6

[Magnus Melin [:mkmelin]]

Honestly I don't see much value in this bug at all (apart from documenting things). I'd estimate the optimization, which will make the code more complex, is likely not even measurable. And stuff like nBox.removeNotification(notification) don't need much utility code - that just makes it one more thing you should remember, instead of using the the api directly.

Comment 7

[:aceman]

Have you seen that in attachmentWorker.onmessage the branch "if (keywordsFound.length > 0)" is executed (whole notifier bar constructed) every time if a keyword is found and the notifier is already shown and nothing changes? That code looks quite heavy.

I also think if this code would have been more consolidated, it could prevent bugs like bug 938759 and bug 938783 from happening.

Comment 8

[Magnus Melin [:mkmelin]]

Have you tried to measure it?

Comment 9

[Magnus Melin [:mkmelin]]

Also remember it's called probably only in very edge cases more than once, if ever, per message.

Comment 10

[:aceman]

The edge case being a body containing a keyword and the user not dismissing the notification until the message is done. In that case the code is rerun on a timer each several seconds.

Comment 11

[Magnus Melin [:mkmelin]]

Hmm, yes that does sound like unnecessary ;)

Comment 12

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #10)
> The edge case being a body containing a keyword and the user not dismissing
> the notification until the message is done. In that case the code is rerun
> on a timer each several seconds.

So it's not an edge case at all... ;)

A agree with :aceman that this code really needs close scrutiny and consolidation. Currently it's very intertwined ("non-consolidated"), undocumented and hard to read. Chances are that when we start with documentation/understanding of current reminder code, we might find that much of it could be rewritten more simple and efficient, both code and performance. Remember that in bug 521158 we've already removed several such duplicate loops and (wrongly documented) conditions which needlessly traversed the whole message again at send time (and the fact that I found them quite quickly with my current level of coding skills is also somewhat indicative...).

Hmmm, although at second thoughts, wrt timer, we should verify how often it runs, perhaps some of that final loop which we removed in bug 521158 was necessary as a final checkup (outside timed loops) for an edgecase when user sends just after last timer run and quickly typed or removed attachment keywords in last second...?

Comment 13

[:aceman]

It appears to me the timer runs the code every 0.5 seconds. And the scenario Thomas describes is real. You get the attachment dialog even if you removed the keywords half a second ago. But you must be real quick. Now that is an edge case :)

Comment 14

[:aceman]

Comment on attachment 833309 [details] [diff] [review]
Preliminary patch

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

OK, if you do not go the route of the "big intelligent state tracking function" then this does the main optimization of not constructing the notification neededlessly (haven't run it but the code shuffling looks semantically fine). I just do not see where you set this.lastMessage ...

And I do not see the comments added about the 2 meaning of the variables (lastMessage and shouldFire).

Comment 15

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

Okay, so this minimizes the no. of times the entire construction code is executed.

Comment 16

[Suyash Agarwal (:sshagarwal)]

lastMessage contains the keywords that were detected last time and shouldFire, I think, triggers the body scan again. As, I tried to remove that line and the body scan didn't happen after that.

Comment 17

[:aceman]

Comment on attachment 8376834 [details] [diff] [review]
Patch v1

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

This looks great:)
And now, can you just call the new function from CheckForAttachmentNotification and toggleAttachmentReminder that magically does the right thing (hides notification as needed) and remove all the opencoded notification removal logic from those 2 functions.

f+ for implementing the "big intelligent state tracking function" this time, but please now make actual use of it:)

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1802,5 @@
> +    nBox.removeNotification(notification);
> +    attachmentWorker.lastMessage = null;
> +  }
> +  if (removeNotification)
> +    return;

if(removeNotification) {
  if(notification) {
    nBox.removenotification ...
  }
  return;
}

@@ +1822,5 @@
> +
> +  // Construct the notification as we don't have one.
> +  Components.utils.reportError("construction kicks in");
> +  let msg = null;
> +  msg = document.createElement("hbox");

let msg = document.createElement("hbox");

@@ +1866,5 @@
> +                               "null",
> +                               nBox.PRIORITY_WARNING_MEDIUM,
> +                               [addButton, remindButton]);
> +  let buttons = notification.childNodes[0];
> +  notification.insertBefore(msg, buttons);

I see you copied this, but do you by chance know what do these 2 lines do? If they change the notification bar contents, could they be moved earlier before .appendNotification?

Comment 18

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

(In reply to :aceman from comment #17)
> > +  let buttons = notification.childNodes[0];
> > +  notification.insertBefore(msg, buttons);
> 
> I see you copied this, but do you by chance know what do these 2 lines do?
> If they change the notification bar contents, could they be moved earlier
> before .appendNotification?

Ya, I read on mdn that msg element is inserted in the notification before the buttons.

Comment 19

[:aceman]

Comment on attachment 8387631 [details] [diff] [review]
Patch v2

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

Looks better with each iteration. And even tests pass! :)

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1995,3 @@
>    if (!event)
>      attachmentWorker.lastMessage = null;
>    CheckForAttachmentNotification.shouldFire = false;

Would it be possible to fully manage CheckForAttachmentNotification.shouldFire and attachmentWorker.lastMessage in manageAttachmentNotification or here?
Does it need to have split handling there and here in CheckForAttachmentNotification ?

Comment 20

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.2

Let me know if any further changes need to be made.

Thanks.

Comment 21

[:aceman]

Comment on attachment 8388215 [details] [diff] [review]
Patch v2.2

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

Yes, seems to look better.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1815,5 @@
> +    return;
> +  }
> +
> +  if (!keywordsFound) {
> +    attachmentWorker.lastMessage = null;   

Trailing whitespace?

@@ +1998,2 @@
>    if (!ShouldShowAttachmentNotification(true)) {
> +    manageAttachmentNotification(event.data);

event may be null here too (according to old code).

@@ +2000,1 @@
>    }

'CheckForAttachmentNotification.shouldFire = true;' should be kept here, manageAttachmentNotification does not manage it.

Comment 22

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.5 (another step)

I have tried to eliminate CheckforattachmentNotification.shouldFire altogether.
Please let me know if this is fine, else we will revert to the previous patch.

Thanks.

Comment 23

[:aceman]

Comment on attachment 8390489 [details] [diff] [review]
Patch v2.5 (another step)

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

All my manual tests and the attachment-notification mozmill tests we have passed with this patch too.
However, I do not know what the .shouldFire does (that is why I wanted you to document it while you find out the purpose of it while working on this patch). So if you want to remove it you better ask its author (or search the bug where it was added, maybe there is some discussion about it).

Comment 24

[Suyash Agarwal (:sshagarwal)]

As far as I could figure out, shouldFire just makes sure an early return from checkForAttachmentNotification().

CheckForAttachmentNotification and related code comes from bug 498519 from :bwinton.
But, I couldn't figure out if .shouldFire remains false for more than one iteration.

http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1977
makes it false, and it may remain false only if this if condition isn't satisfied:
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1982

i.e. ShouldShowAttachmentNotification() returns true:
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1962

and, if that happens, attachmentWorker.postMessage() is called which triggers attachmentworker.onmessage() (said in https://bugzilla.mozilla.org/show_bug.cgi?id=498519#c29)
which makes sure CheckforAttachmentNotification.shouldFire is set to true:
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1882

So, since now I am making an early return from the function, I think we can eliminate shouldFire. Is this fine or am I making a mistake?
We should ask the patch author.


Thanks.

Comment 25

[Blake Winton (:bwinton) (:☕️)]

Well, that was a lot of thinking about stuff that happened a long time ago.  :)

Yes, it looks like you can eliminate shouldFire, as long as all the tests pass.

Thanks,
Blake.

Comment 26

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.7

(In reply to Blake Winton (:bwinton) from comment #25)
> Yes, it looks like you can eliminate shouldFire, as long as all the tests
> pass.

All the tests in composition/test-attachment-reminder.js pass.

Do we need more optimization or will this suffice?

Thanks.

Comment 27

[Blake Winton (:bwinton) (:☕️)]

That should be good.  :)

Comment 28

[:aceman]

Comment on attachment 8403391 [details] [diff] [review]
Patch v2.7

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

If all my nits from version 2.2 are adressed and you talked to bwinton about .shouldFire then I am happy:)

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 8403391 [details] [diff] [review]
Patch v2.7

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1789,5 @@
> +function manageAttachmentNotification(keywordsFound)
> +{
> +  // If the event isn't actually an event but a call to CheckForAttachmentNotification.
> +  if (keywordsFound == true)
> +    return;

fix the caller instead, this is an odd thing to do. generally having params being of different types makes for complicated code. 
(if there is such a caller, i didn't see one looking briefly)

@@ +1792,5 @@
> +  if (keywordsFound == true)
> +    return;
> +
> +  // If the user has chosen to be reminded later, return.
> +  if (gManualAttachmentReminder && keywordsFound)

keywordsFound doesn't matter, no?

@@ +1807,5 @@
> +
> +  // If there are no attachment specific keywords, we should remove the
> +  // notification.
> +  let removeNotification = ((!keywordsFound && gManualAttachmentReminder) ||
> +                            (keywordsFound && (keywordsCount == 0)));

how would the second part ever be true?

@@ +1839,5 @@
> +    return;
> +  }
> +
> +  // Construct the notification as we don't have one.
> +  Components.utils.reportError("construction kicks in");

remove debug stuff

@@ +1844,5 @@
> +  let msg = document.createElement("hbox");
> +  msg.setAttribute("flex", "100");
> +  msg.onclick = function(event)
> +  {
> +    openOptionsDialog("paneCompose", "generalTab",

brace on the earlier line, please. for the callbacks later too

@@ +1860,5 @@
> +  msgKeywords.id = "attachmentKeywords";
> +  msgKeywords.setAttribute("crop", "end");
> +  msgKeywords.setAttribute("flex", "1000");
> +  msgKeywords.setAttribute("value", keywords);
> +  var addButton = {

while you're moving this, please make them all let instead of var

Comment 30

[Suyash Agarwal (:sshagarwal)]

(In reply to Magnus Melin from comment #29)
> Comment on attachment 8403391 [details] [diff] [review]
> Patch v2.7
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +1789,5 @@
> > +function manageAttachmentNotification(keywordsFound)
> > +{
> > +  // If the event isn't actually an event but a call to CheckForAttachmentNotification.
> > +  if (keywordsFound == true)
> > +    return;
> 
> fix the caller instead, this is an odd thing to do. generally having params
> being of different types makes for complicated code. 
> (if there is such a caller, i didn't see one looking briefly)

Can't we exploit the fact that we don't have a datatype bound to the variable in JS? :)

Comment 31

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #30)
> Can't we exploit the fact that we don't have a datatype bound to the
> variable in JS? :)

Actually I think the JS engine tries to guess the datatype of a variable and if it does not change it enables it to optimize the code and run faster. I think they call it type inference: 
https://blog.mozilla.org/blog/2011/12/20/major-javascript-enhancements-make-firefox-speedy-up-to-30-faster/
I don't say this makes any difference in the code in question (it is not some compute intensive algorithm), just that keeping datatype consistent may have some advantages :)

Comment 32

[Magnus Melin [:mkmelin]]

Yeah It's not for performance resons, but for code clarity. If the input is something totally different then normal, you probably need to call something else. In this case we just do nothing for the "odd" param, so why involve this method in the first place.

Comment 33

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.9

(In reply to Magnus Melin from comment #29)
> Comment on attachment 8403391 [details] [diff] [review]
> Patch v2.7

> @@ +1792,5 @@
> > +  // If the user has chosen to be reminded later, return.
> > +  if (gManualAttachmentReminder && keywordsFound)
> 
> keywordsFound doesn't matter, no?
It is now required for the call in toggleAttachmentReminder(), which would otherwise make it return from here itself without returning the lastMessageState.

> @@ +1807,5 @@
> > +  let removeNotification = ((!keywordsFound && gManualAttachmentReminder) ||
> > +                            (keywordsFound && (keywordsCount == 0)));
> 
> how would the second part ever be true?

Whenever we type in any character/word that isn't attachment specific, this condition becomes true;

Comment 34

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #33)
> > @@ +1807,5 @@
> > > +  let removeNotification = ((!keywordsFound && gManualAttachmentReminder) ||
> > > +                            (keywordsFound && (keywordsCount == 0)));
> > 
> > how would the second part ever be true?
> 
> Whenever we type in any character/word that isn't attachment specific, this
> condition becomes true;

As keywordsFound and keywordsCount are dependent, Magnus means if keywordsFound is not empty (so "true"), how can keywordsCount be 0 ? (the part of the expression after '||' seems nonsensical)

Comment 35

[:aceman]

Comment on attachment 8405975 [details] [diff] [review]
Patch v2.9

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

So where is the attempt to use .lastMessage != keywords (instead of new variable) to solve the case we discussed on IRC?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1791,5 @@
> +function manageAttachmentNotification(keywordsFound, lastMessageState = null)
> +{
> +  // If the user has chosen to be reminded later, return.
> +  if (gManualAttachmentReminder && keywordsFound)
> +    return;

I wonder why this works. Where do we close the notification when manual reminder is set if not here?
I would expect this to fall down to the removeNotification determination and do what is needed there.

@@ +1805,5 @@
> +    attachmentWorker.lastMessage = lastMessageState;
> +
> +  // The user closed the notification, and we have nothing new to say.
> +  if (keywords && (keywords == attachmentWorker.lastMessage))
> +    return;

I know this comment is copied. Maybe it had sense at its original location but here it is strange.

@@ +1812,5 @@
> +  // notification.
> +  let bucket = document.getElementById("attachmentBucket");
> +  let removeNotification = ((!keywordsFound && gManualAttachmentReminder) ||
> +                            (keywordsFound && (keywordsCount == 0)) ||
> +                            bucket.itemCount);

It appears to me this would be more readable:
let removeNotification = (gManualAttachmentReminder ||
!keywordsFound || bucket.itemCount);

@@ +1826,5 @@
> +  }
> +
> +  if (!keywordsFound) {
> +    return;
> +  }

Is it really possible we do not have any keywords at this spot?

::: mail/test/mozmill/composition/test-attachment-reminder.js
@@ +315,5 @@
>    cwc.sleep(notificationSlackTime);
>    assert_automatic_reminder_state(cwc, false);
>  
> +  // Add some more text with a new keyword.
> +  setupComposeWin(cwc, "", "", " Do you find it attached?");

For the case we discussed on IRC I would update the test here to do:
add text without keywords and wait and notification should NOT show up.
Then add text with keyword and notification SHOULD show up. In the current state you do not test the behaviour you want to have.

Comment 36

[:aceman]

(In reply to :aceman from comment #35)
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +1791,5 @@
> > +function manageAttachmentNotification(keywordsFound, lastMessageState = null)
> > +{
> > +  // If the user has chosen to be reminded later, return.
> > +  if (gManualAttachmentReminder && keywordsFound)
> > +    return;
> 
> I wonder why this works. Where do we close the notification when manual
> reminder is set if not here?
> I would expect this to fall down to the removeNotification determination and
> do what is needed there.

Ah yes, from the manual reminder you send keywordsFound = null so this is not true and falls down to the removeNotification below. But then, if gManualAttachmentReminder == true then should hide the reminder and not care about checking any keywords.

Comment 37

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #34)
> > > > +  let removeNotification = ((!keywordsFound && gManualAttachmentReminder) ||
> > > > +                            (keywordsFound && (keywordsCount == 0)));
> > Whenever we type in any character/word that isn't attachment specific, this
> > condition becomes true;
> 
> As keywordsFound and keywordsCount are dependent, Magnus means if
> keywordsFound is not empty (so "true"), how can keywordsCount be 0 ? (the
> part of the expression after '||' seems nonsensical)

keywordsFound can be "" that is empty but not null and keywordsCount is 0. You can verify this, I tried it and so left it here.

(In reply to :aceman from comment #35)
> Comment on attachment 8405975 [details] [diff] [review]
> Patch v2.9
> So where is the attempt to use .lastMessage != keywords (instead of new
> variable) to solve the case we discussed on IRC?
I couldn't figure out how it would work. But not clearing lastMessage ever was posing a few issues which again needed another flag to decide when to clear lastMessage and when not, so I thought of saving the state.

> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +1791,5 @@
> > +function manageAttachmentNotification(keywordsFound, lastMessageState = null)
> > +{
> > +  // If the user has chosen to be reminded later, return.
> > +  if (gManualAttachmentReminder && keywordsFound)
> > +    return;
> 
> I wonder why this works. Where do we close the notification when manual
> reminder is set if not here?
> I would expect this to fall down to the removeNotification determination and
> do what is needed there.
Ya, it is handled in the toggleAttachmentReminder() now, the helper returns the state (keywords) to the caller (toggle..()) and hides the notification.

> @@ +1826,5 @@
> > +  if (!keywordsFound) {
> > +    return;
> > +  }
> 
> Is it really possible we do not have any keywords at this spot?
Ya, this is the first and the last time we are taking care of the fact that keywordsFound can be null.

> ::: mail/test/mozmill/composition/test-attachment-reminder.js
> > +  // Add some more text with a new keyword.
> > +  setupComposeWin(cwc, "", "", " Do you find it attached?");
> 
> For the case we discussed on IRC I would update the test here to do:
> add text without keywords and wait and notification should NOT show up.
> Then add text with keyword and notification SHOULD show up. In the current
> state you do not test the behaviour you want to have.
This is the same behavior, for the first time (after unchecking the remind later option) I am typing
the same keyword. That doesn't bring up the reminder, then I am typing in another different keyword, the reminder comes up. Isn't this what we meant?

(In reply to :aceman from comment #36)
> (In reply to :aceman from comment #35)
> > ::: mail/components/compose/content/MsgComposeCommands.js
> > @@ +1791,5 @@
> > > +function manageAttachmentNotification(keywordsFound, lastMessageState = null)
> > > +{
> > > +  // If the user has chosen to be reminded later, return.
> > > +  if (gManualAttachmentReminder && keywordsFound)
> > > +    return;
> > 
> > I wonder why this works. Where do we close the notification when manual
> > reminder is set if not here?
> > I would expect this to fall down to the removeNotification determination and
> > do what is needed there.
> 
> Ah yes, from the manual reminder you send keywordsFound = null so this is
> not true and falls down to the removeNotification below. But then, if
> gManualAttachmentReminder == true then should hide the reminder and not care
> about checking any keywords.

Ya, but we need the state, early return will not allow me to store the state so I am coming down to get it.

Comment 38

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #37)
> (In reply to :aceman from comment #34)
> > > > > +  let removeNotification = ((!keywordsFound && gManualAttachmentReminder) ||
> > > > > +                            (keywordsFound && (keywordsCount == 0)));
> > > Whenever we type in any character/word that isn't attachment specific, this
> > > condition becomes true;
> > 
> > As keywordsFound and keywordsCount are dependent, Magnus means if
> > keywordsFound is not empty (so "true"), how can keywordsCount be 0 ? (the
> > part of the expression after '||' seems nonsensical)
> 
> keywordsFound can be "" that is empty but not null and keywordsCount is 0.
> You can verify this, I tried it and so left it here.
!null and !"" may evaluate the same? You do not check for "null" specifically.

> > ::: mail/test/mozmill/composition/test-attachment-reminder.js
> > > +  // Add some more text with a new keyword.
> > > +  setupComposeWin(cwc, "", "", " Do you find it attached?");
> > 
> > For the case we discussed on IRC I would update the test here to do:
> > add text without keywords and wait and notification should NOT show up.
> > Then add text with keyword and notification SHOULD show up. In the current
> > state you do not test the behaviour you want to have.
> This is the same behavior, for the first time (after unchecking the remind
> later option) I am typing
> the same keyword. That doesn't bring up the reminder, then I am typing in
> another different keyword, the reminder comes up. Isn't this what we meant?
In the patch I see that immediatelly after unchecking manual reminder you type a new keyword. That is not what you say:
  setupComposeWin(cwc, "", "", " and look for your attachment!");
  // Now disable the manual reminder.
  click_manual_reminder(cwc, false);
  // Give the notification time to appear. It shouldn't.
  cwc.sleep(notificationSlackTime);
  assert_automatic_reminder_state(cwc, false);
  // Add some more text with a new keyword.
  setupComposeWin(cwc, "", "", " Do you find it attached?");

If the "attached" keyword was already in the text from previous typings that are not visible in the patch, then OK, but you then need to type another new keyword.

Comment 39

[:aceman]

What about calling manageAttachmentNotification(keywordsFound) only:
1. from attachmentWorker.onmessage when keywords change (manage attachmentWorker.lastMessage inside the worker where it would belong)
2. from toggleAttachmentReminder (so only when state of manual reminder is changed)

So only at these rare events we call the helper and it decides what to do.

Would that help?

Comment 40

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #38)
> > keywordsFound can be "" that is empty but not null and keywordsCount is 0.
> > You can verify this, I tried it and so left it here.
> !null and !"" may evaluate the same? You do not check for "null"
> specifically.
They aren't being evaluated to same in this case.

> > This is the same behavior, for the first time (after unchecking the remind
> > later option) I am typing
> > the same keyword. That doesn't bring up the reminder, then I am typing in
> > another different keyword, the reminder comes up. Isn't this what we meant?
> In the patch I see that immediately after unchecking manual reminder you
> type a new keyword. That is not what you say:
>   setupComposeWin(cwc, "", "", " and look for your attachment!");
>   // Now disable the manual reminder.
>   click_manual_reminder(cwc, false);
>   // Give the notification time to appear. It shouldn't.
>   cwc.sleep(notificationSlackTime);
>   assert_automatic_reminder_state(cwc, false);
>   // Add some more text with a new keyword.
>   setupComposeWin(cwc, "", "", " Do you find it attached?");
> 
> If the "attached" keyword was already in the text from previous typings that
> are not visible in the patch, then OK, but you then need to type another new
> keyword.

Ya, there isn't enough context in the patch.

(In reply to :aceman from comment #39)
But, why can't we accept this patch? :)

Comment 41

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #40)
> (In reply to :aceman from comment #39)
> But, why can't we accept this patch? :)

I find it hard to understand what the new variable does. And the point of this bug is to clean up the reminder handling :) You already removed one strange variable .shouldFire .

Comment 42

[Magnus Melin [:mkmelin]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #40)
> (In reply to :aceman from comment #38)
> > > keywordsFound can be "" that is empty but not null and keywordsCount is 0.
> > > You can verify this, I tried it and so left it here.
> > !null and !"" may evaluate the same? You do not check for "null"
> > specifically.
> They aren't being evaluated to same in this case.

null, "", 0, undefined and false are all falsy values - that's why our code usually don't check specifically just do if(x) not if(x===true). 

You may want to read http://www.sitepoint.com/javascript-truthy-falsy/

Comment 43

[Suyash Agarwal (:sshagarwal)]

(In reply to Magnus Melin from comment #42)
> null, "", 0, undefined and false are all falsy values - that's why our code
> usually don't check specifically just do if(x) not if(x===true). 
> 
> You may want to read http://www.sitepoint.com/javascript-truthy-falsy/
Nice link, thanks :)

Okay then maybe it isn't working and I got the illusion (due to other OR conditions) it was working, sorry.

(In reply to :aceman from comment #41)
> I find it hard to understand what the new variable does. And the point of
> this bug is to clean up the reminder handling :) You already removed one
> strange variable .shouldFire .

Okay, so allow me for the last time to explain what this does, if still unsatisfied, we're going to drop most of it :)

You type in words attach and attached
> Notification comes up.
You choose "Remind me later" either from the notification bar or the Attach
button's menu

> Notification goes away.
> (background)...
  > we store the words attach and attached. 
You un-check the "Remind me later" option
> (background)...
  > we re-scan the subject field and body to see if the keywords present
     are the same that were at the time of checking "Remind me later"
  IF_YES
  > we don't bring up the notification
  IF_NO (there are other new attachment specific keywords introduced)
  > we bring up the notification.

Also, I tried the patch after removing the (keywordsFound && !keywordsCount) condition, the patch fails.
If have introduced a debug print statement. Please see to it once.

Thanks.

Comment 44

[Suyash Agarwal (:sshagarwal)]

Attachment: problem demonstration patch

This patch shows the value of the controversial (keywordsFound and keywordsCount) condition. Also, please comment this particular OR operand from the removeNotification variable definition and then any keypress in the message body will bring up the notification.

Thanks.

Comment 45

[Magnus Melin [:mkmelin]]

Comment on attachment 8406778 [details] [diff] [review]
problem demonstration patch

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

So what's confusing us is that keywordsFound is null or an array (please document that!)

if(keywordsFound) is still true for an empty array. 

My original comment still applies though, you only need to check keywordsCount
And for the record, I do think numbers should be compared to numbers, falsy comparisons for numbers are just so very geeky, so if(keywordsCount == 0) like you have it is better than if(!keywordsCount)

Comment 46

[Magnus Melin [:mkmelin]]

Comment on attachment 8405975 [details] [diff] [review]
Patch v2.9

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

Clearing review until we have an updated patch

Comment 47

[:aceman]

Attachment: patch v3 (more separation of the state machine and the events affecting its state)

This builds on top of the existing patches. Makes the state machine even more robust and just receive events when some interesting things change (e.g. number of attachments, manual reminder, new keywords) and then decides what to do to the notification. This also adds some new tests. One fails due to bug 1024578 :)
I think in this state plugging in a new event affecting the notification (e.g. in bug 942436) should be easy.

Comment 48

[Magnus Melin [:mkmelin]]

Comment on attachment 8439332 [details] [diff] [review]
patch v3 (more separation of the state machine and the events affecting its state)

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

Looks ok to me. r=mkmelin with the debugging Components.utils.reportError lines removed

Comment 49

[:aceman]

Attachment: patch v3.1

Thanks.

Comment 50

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #49)
> Created attachment 8443757 [details] [diff] [review]
> patch v3.1
> 
> Thanks.

Did you mean to add checkin-needed?

Comment 51

[:aceman]

Possibly. It is a pity it can't be done in one step in bugzilla (add attachment and put keywords).

Comment 52

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/960663518ad6

Comment 53

[Mark Banner (:standard8)]

Backed out due to test failures:

https://hg.mozilla.org/comm-central/rev/447a5f8b564d

https://tbpl.mozilla.org/php/getParsedLog.php?id=43606071&tree=Thunderbird-Trunk

Comment 54

[:aceman]

Sorry, but the test failure is expected and we have bug 1024578 for it. Just that the patches are stacked so that bug 1024578 should be applied on top of the patch here, not the other way round as would be logical.

Could you r+ the other bug too and push both patches atomically, so that the test failure will not be seen on trunk at any time?

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Please set the request once bug 1024578 is ready to land.

Comment 56

[:aceman]

It is now. Please land this bug and bug 1024578 (in this order) in a single batch.

Comment 57

[Mark Banner (:standard8)]

I don't have time to land this atm, so I'll let checkin-needed deal with this.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/3ee90fde7ed3

Comment 59

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell and mozmill failures.
https://hg.mozilla.org/comm-central/rev/f89d51af2187

https://tbpl.mozilla.org/php/getParsedLog.php?id=43830757&tree=Thunderbird-Trunk
https://tbpl.mozilla.org/php/getParsedLog.php?id=43829477&tree=Thunderbird-Trunk

Please confirm that this is green on Try before requesting checkin again.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

But the bottom line is that after two failed landings now, the burden of proof is on your to prove that it won't fail again. Green Try or I won't be pushing it.

Comment 61

[:aceman]

Attachment: patch v3.2

Refreshed patch after the blocking bug got landed.
Aryx, can please push it to try? All tests needed.

Comment 62

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed as https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=4c2bb92a8d4d

Comment 63

[:aceman]

Thanks, seems to be passing fine!

Comment 64

[:aceman]

Comment on attachment 8490919 [details] [diff] [review]
patch v3.2

Carrying over review from patch v3.

Comment 65

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/db39511d70c5