1420473 - Refactor and simplify the feed Subscribe dialog options updates

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, enhancement)

Description

[alta88]

1. Only show buttons that apply to the selected account, folder, or feed tree item.
2. Some options are instant apply and some need Update; this is confusing and double processing. All options will be instant apply and the Update button replaced by Verify (the url), except for..
3. An edit to an existing feed url must still require an Update. If the value is changed, the button will change from Verify to Update.
4. Buttons are only enabled if the url is focused (folders) or has a value (feeds).

The Update button currently verifies an existing stored feed url for network/cert/valid xml errors, so Update isn't accurate.

Comment 1

[alta88]

Attachment: subscribeTweaks.patch

Comment 2

[alta88]

Attachment: feedsImport.patch

Some tweaks for import. Remove cached feeds/feeditems db references, as they are already cached and cause stale problems for the account removal/recreation edge case as they aren't cleaned up on removal.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 8931741 [details] [diff] [review]
subscribeTweaks.patch

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

::: mail/locales/en-US/chrome/messenger-newsblog/feed-subscriptions.dtd
@@ +10,5 @@
>  <!ENTITY feedTitle.accesskey         "T">
>  
>  <!ENTITY feedLocation.label          "Feed URL:">
>  <!ENTITY feedLocation.accesskey      "F">
> +<!ENTITY feedLocation.placeholder    "Enter a valid feed url">

you need to bump the entity key, so that localizers will notice

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +1153,5 @@
>    },
>  
>    setPrefs: function(aNode)
>    {
> +FeedUtils.log.debug("setPrefs: aNode.id:disabled - " + aNode.id+":"+aNode.disabled);

remove

@@ +1299,5 @@
>    },
>  
>    updateButtons: function (aSelectedItem)
>    {
> +FeedUtils.log.debug("updateButtons: ");

remove
also debugging left in a few other places

@@ +1364,4 @@
>      if (isServer)
>      {
> +      let disable = addFeedButton != focusedElement &&
> +                    !locationValue.hasAttribute("focused") && !locationValue.value;

I'd not use this intermediate variable. It's just used once

@@ +1376,5 @@
> +      updateEnabled.disabled = autotagEnable.disabled = disable;
> +
> +      disable = addFeedButton != focusedElement &&
> +                !locationValue.hasAttribute("focused") && !locationValue.value;
> +      addFeedButton.disabled = disable;

here too just assign, without messing around with disable

@@ +1395,5 @@
> +      // Set button state.
> +      let disable = (focusedElement != updateFeedButton ||
> +                     updateFeedButton.disabled) &&
> +                    !locationValue.hasAttribute("focused");
> +      updateFeedButton.disabled = disable;

here too

Comment 4

[alta88]

Attachment: subscribeTweaks.patch

update for comments.

Comment 5

[alta88]

Attachment: feedsImport.patch

unbitrot.

Comment 6

[alta88]

Attachment: subscribeTweaks.patch

entity bump.

Comment 7

[alta88]

Attachment: subscribeTweaks.patch

and in the xul.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1c1e48b383bd
Refactor and simplify the feed Subscribe dialog options updates. r=mkmelin
https://hg.mozilla.org/comm-central/rev/1654a1adc838
Refactor and simplify the feed Subscribe dialog options updates, import/export part. r=mkmelin CLOSED TREE

Comment 9

[alta88]

Attachment: hide.patch

follow up, buttons hidden initially to prevent them showing briefly on load.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f44c101732e9
Follow-up: hide buttons initially. r=me

Comment 11

[alta88]

the only possible change in this patch that may remotely cause Bug 1439393 would be the FeedUtils.jsm hunk in
https://hg.mozilla.org/comm-central/rev/1654a1adc838

so that hunk could be reverted.  it was implemented long ago as an extra cache in case the even more ancient comment
"GetDataSourceBlocking has a cache, so it's cheap to do this again once we've already done it once." was
not true.

the cache was removed since the cached entry wasn't being deleted on account deletion, and absent a restart a new account would get the old account's cache, and assumed the above comment re rdf was true. it is possible something in core rdf was changed to make the old comment untrue, in fact.

it's entirely possible that processing huge feed files has nothing to do with rdf or this change, and something to do with creating a dom that large for parsing and is a side effect of some other core change.  

a solution might be to reject feeds with a file over a certain size, as defined in a pref.  there is also a pref to throttle concurrent feeds being processed (25) and people with big feed files need to lower this (as noted in  the guide).

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to alta88 from comment #11)
> the only possible change in this patch that may remotely cause Bug 1439393
> would be the FeedUtils.jsm hunk in
> https://hg.mozilla.org/comm-central/rev/1654a1adc838
You mean all hunks in FeedUtils.jsm?

Rctgamer3, if I do a try build with the partial backout, would you be able to test this and see whether the crash goes away? If so, which platform do you need? Linux32/64, Mac, Windows 32/64?

Comment 13

[rctgamer3]

(In reply to Jorg K (GMT+1) from comment #12)
> would you be able to test this and see whether the crash goes away? If so, which platform do
> you need? Linux32/64, Mac, Windows 32/64?
Sure. Windows 32 (even though i'm using W10 x64)

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: backout-1654a1adc838.patch

Backout patch for FeedUtils.jsm. Try coming for Win 32bit.

Comment 15

[Jorg K (CEST = GMT+2)]

Try build:
https://queue.taskcluster.net/v1/task/WQ8VMhVLT2uvvZOwuYnCsw/runs/0/artifacts/public/build/target.zip or
https://queue.taskcluster.net/v1/task/WQ8VMhVLT2uvvZOwuYnCsw/runs/0/artifacts/public/build/install/sea/target.installer.exe

Comment 16

[rctgamer3]

(In reply to Jorg K (GMT+1) from comment #15)

Doesn't crash, yay! - everything's working just fine with this build. It's been running for over an hour with no CPU spikes or memory leaks - stable at ~200MB memory.

Comment 17

[Jorg K (CEST = GMT+2)]

OK, let's go with this then.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9f58c4a6ec4a
Partially backed out changeset 1654a1adc838 (FeedUtils.jsm) to fix crash (bug 1439393). a=jorgk

Comment 19

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 2):
https://hg.mozilla.org/releases/comm-beta/rev/4d755f1cf38cbd870ffb3e2625a26c1d2ed51208