convert mailnews/extensions/newsblog/content/ to Services.jsm and mailServices.js; fix global scope

RESOLVED FIXED in Thunderbird 15.0

Status

MailNews Core
Feed Reader
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: alta88)

Tracking

(Blocks: 2 bugs)

Trunk
Thunderbird 15.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

5 years ago
Component: Filters → Feed Reader
QA Contact: filters → feed.reader
(Reporter)

Comment 1

5 years ago
I'll work on this once other patches in that directory land.
Depends on: 719456, 340324
(Reporter)

Comment 2

5 years ago
alta88, do you plan any new big patches in feeds? Can you tell me when you have some pause so I can do this bug and our patches do not clash?
(Assignee)

Comment 3

5 years ago
well, one could have endless patches in feed-land ;)

i'm intending to fix the opml import/export folder hierarchy problem next, but won't look at it until you do this work and bug 716706, which david is looking at now, is checked in.

newsblogOverlay.js is converted partly by bug 716706 and partly by bug 714570 (which is looking like an orphan), so other than that one the others are all yours.
(Reporter)

Comment 4

5 years ago
Thanks, so I will wait on bug 716706. Why is bug 714570 an orphan? It looks like you are working on it.

If you mean bug 307724, you can surely take that one, I can't do it so far.
Blocks: 307724
Depends on: 716706, 714570
(Reporter)

Comment 5

5 years ago
It looks like you are revamping the feeds quite thoroughly and even removing calls to services and making some common utility functions. So it probably doesn't make sense for me to do this bug. I would just keep the current state. You can take it and do the services your better way.

I'll at least find out how to resurrect the patch in bug 719456 now that you killed it:)
Assignee: acelists → alta88
(Assignee)

Comment 6

5 years ago
Created attachment 615875 [details] [diff] [review]
patch


probably reviewing a patch like this is the one thing less fun than writing it..

goal has been to avoid changing (much) logic, just to move everything into the FeedUtils object, format/whitespace/wrap, get rid of strict js/deprecated warnings, etc.  the few minor functional changes would be:
1. remove block of unnecessary code in newsblog.js downloadFeed,
2. make feeds.js xhr test schemes using generic function,
3. test feeds.js xhr request.status if http scheme,
4. change feeds.js If-Modified-Since logic slightly
Attachment #615875 - Flags: review?(dbienvenu)

Comment 7

5 years ago
Comment on attachment 615875 [details] [diff] [review]
patch

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

::: mailnews/extensions/newsblog/content/Feed.js
@@ +121,3 @@
>      if (!name)
> +      throw("Feed.name: couldn't compute name, as feed has no title, " +
> +            "description, or URL.");

While you're at it - shouldn't throw strings directly. Use throw new Error("Feed.name.... ") instead
Looks like there's a lot of these here...
(Assignee)

Comment 8

5 years ago
Created attachment 616145 [details] [diff] [review]
patch.


address comments.  file-utils.js isn't included in this, it will go away/be restructured separately.
Attachment #615875 - Attachment is obsolete: true
Attachment #615875 - Flags: review?(dbienvenu)
Attachment #616145 - Flags: review?(dbienvenu)
(Assignee)

Updated

5 years ago
Blocks: 747657
(Assignee)

Comment 9

5 years ago
Created attachment 618664 [details] [diff] [review]
updated patch.


updated getNodeValue func: 1) textContent.trim() 2) this.getNodeValue
Attachment #616145 - Attachment is obsolete: true
Attachment #616145 - Flags: review?(dbienvenu)
Attachment #618664 - Flags: review?(dbienvenu)
(Assignee)

Updated

5 years ago
Blocks: 750292
Severity: trivial → normal
Priority: P5 → --

Comment 10

5 years ago
Comment on attachment 618664 [details] [diff] [review]
updated patch.

this looks like a lot of good cleanup. A few nits:

I saw "return ;" should lose the space before the ;

+    for (let i=0; i < items.length; i++)

should have spaces around =.

this pattern occurs several times

+    let parsedItems = new Array(), tags;

I find that syntax a bit hard to read, and I think it would be better to declare tags where it's first used, e.g.,

+   let tags = this.childrenByTagNameNS(channel, FeedUtils.ATOM_IETF_NS, "title");

(assuming the scoping works out)

don't need braces here:

+      if (yeardiff >= 0 && yeardiff < 3)
+      {
+        // It's quite likely the correct date.
+        return d.toString();
+      }
Attachment #618664 - Flags: review?(dbienvenu) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 619641 [details] [diff] [review]
address comments.
Attachment #618664 - Attachment is obsolete: true
Attachment #619641 - Flags: review+
(Assignee)

Comment 12

5 years ago
Created attachment 619642 [details] [diff] [review]
address all comments.
Attachment #619641 - Attachment is obsolete: true
Attachment #619642 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed

Comment 13

5 years ago
As you are changing SM as well as TB and Mail Core files, you do need a review from an SM mail news peers as well.
Status: NEW → ASSIGNED
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Attachment #619642 - Flags: review?(neil)
Comment on attachment 619642 [details] [diff] [review]
address all comments.

Sorry, I was expecting a patch that converts newsblog to (mail)Services.js(m) (as the bug title suggests) but this has all sorts of other changes such as an apparent wholesale s/var/let/ which makes it almost impossible to review.
Attachment #619642 - Flags: review?(neil)
(Assignee)

Comment 15

5 years ago
Comment on attachment 619642 [details] [diff] [review]
address all comments.


there is only one /suite file in this patch, for only a few lines, needing SM review.  if that is what comment 13 was all about.  otherwise a more clear explanation of the SM blocker preventing checkin is necessary.
Attachment #619642 - Flags: review?(neil)
Comment on attachment 619642 [details] [diff] [review]
address all comments.

Well all I can say is that it's very confusing to have a review request for a (part of a) patch which bears no relation to the title of the bug...
Attachment #619642 - Flags: review?(neil) → review+
(Assignee)

Comment 17

5 years ago
there
Keywords: checkin-needed
Summary: convert mailnews/extensions/newsblog/content/ to Services.jsm and mailServices.js → convert mailnews/extensions/newsblog/content/ to Services.jsm and mailServices.js; fix global scope
This is pretty badly bitrotted. Please submit an updated patch.
Keywords: checkin-needed
Whoops, wrong bug. Ignore comment 18.
Actually, yes, this does need an updated patch. The patch currently doesn't apply:

patching file mail/base/content/mailWindowOverlay.js
patching file mailnews/extensions/newsblog/content/Feed.js
patching file mailnews/extensions/newsblog/content/FeedItem.js
patching file mailnews/extensions/newsblog/content/debug-utils.js
patching file mailnews/extensions/newsblog/content/feed-parser.js
bad hunk #1 @@ -29,518 +29,679 @@
 (520 518 679 679)
mail/base/content/mailWindowOverlay.js
mailnews/extensions/newsblog/content/Feed.js
mailnews/extensions/newsblog/content/FeedItem.js
patch failed, rejects left in working dir
(Assignee)

Comment 21

5 years ago
Created attachment 620154 [details] [diff] [review]
unbitrotted patch for checkin.
Attachment #619642 - Attachment is obsolete: true
Attachment #620154 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/98cb113060ef
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
(Assignee)

Comment 23

5 years ago
Comment on attachment 620154 [details] [diff] [review]
unbitrotted patch for checkin.

[Approval Request Comment]

see bug 747657 comment 11.
Attachment #620154 - Flags: approval-comm-aurora?
Comment on attachment 620154 [details] [diff] [review]
unbitrotted patch for checkin.

Sorry for not getting to this earlier. Unfortunately we also missed the test-day. Therefore I think we'll not advance this forward, but let it get testing through the release trains.
Attachment #620154 - Flags: approval-comm-aurora? → approval-comm-aurora-

Updated

5 years ago
Depends on: 792657
You need to log in before you can comment on or make changes to this bug.