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

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Comment 1

[:aceman]

I'll work on this once other patches in that directory land.

Comment 2

[:aceman]

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?

Comment 3

[alta88]

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.

Comment 4

[:aceman]

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.

Comment 5

[:aceman]

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:)

Comment 6

[alta88]

Attachment: 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

Comment 7

[Magnus Melin [:mkmelin]]

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...

Comment 8

[alta88]

Attachment: patch.

address comments.  file-utils.js isn't included in this, it will go away/be restructured separately.

Comment 9

[alta88]

Attachment: updated patch.

updated getNodeValue func: 1) textContent.trim() 2) this.getNodeValue

Comment 10

[David :Bienvenu]

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();
+      }

Comment 11

[alta88]

Attachment: address comments.

Comment 12

[alta88]

Attachment: address all comments.

Comment 13

[Ian Neal]

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.

Comment 14

[[e-mail]]

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.

Comment 15

[alta88]

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.

Comment 16

[[e-mail]]

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...

Comment 17

[alta88]

there

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This is pretty badly bitrotted. Please submit an updated patch.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Whoops, wrong bug. Ignore comment 18.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[alta88]

Attachment: unbitrotted patch for checkin.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/98cb113060ef

Comment 23

[alta88]

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

[Approval Request Comment]

see bug 747657 comment 11.

Comment 24

[Mark Banner (:standard8)]

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.

Comment 25

[[e-mail]]

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

>+      newSubject = mailServices.mimeConverter.encodeMimePartIIStr(
The file might be mailServices.js but the symbols is MailServices... unfortunately this is in a try/catch so it's never worked since but nobody's noticed.