Last Comment Bug 721517 - convert mailnews/extensions/newsblog/content/ to Services.jsm and mailServices.js; fix global scope
: convert mailnews/extensions/newsblog/content/ to Services.jsm and mailService...
Product: MailNews Core
Classification: Components
Component: Feed Reader (show other bugs)
: Trunk
: All All
-- normal (vote)
: Thunderbird 15.0
Assigned To: alta88
Depends on: 340324 714570 716706 719456 792657
Blocks: 720356 720358 307724 747657 750292
  Show dependency treegraph
Reported: 2012-01-26 13:50 PST by :aceman
Modified: 2012-09-19 23:30 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch (166.27 KB, patch)
2012-04-17 14:29 PDT, alta88
no flags Details | Diff | Splinter Review
patch. (166.60 KB, patch)
2012-04-18 08:04 PDT, alta88
no flags Details | Diff | Splinter Review
updated patch. (166.61 KB, patch)
2012-04-26 08:40 PDT, alta88
mozilla: review+
Details | Diff | Splinter Review
address comments. (166.61 KB, patch)
2012-04-30 12:32 PDT, alta88
alta88: review+
Details | Diff | Splinter Review
address all comments. (166.59 KB, patch)
2012-04-30 12:33 PDT, alta88
alta88: review+
neil: review+
Details | Diff | Splinter Review
unbitrotted patch for checkin. (166.59 KB, patch)
2012-05-01 17:53 PDT, alta88
alta88: review+
standard8: approval‑comm‑aurora-
Details | Diff | Splinter Review

Description User image :aceman 2012-01-26 13:50:26 PST

Comment 1 User image :aceman 2012-01-30 05:52:58 PST
I'll work on this once other patches in that directory land.
Comment 2 User image :aceman 2012-02-13 13:53:20 PST
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 User image alta88 2012-02-13 14:32:37 PST
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 User image :aceman 2012-02-13 23:59:20 PST
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 User image :aceman 2012-02-24 15:50:02 PST
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 User image alta88 2012-04-17 14:29:23 PDT
Created attachment 615875 [details] [diff] [review]

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 User image Magnus Melin 2012-04-17 22:56:32 PDT
Comment on attachment 615875 [details] [diff] [review]

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

::: mailnews/extensions/newsblog/content/Feed.js
@@ +121,3 @@
>      if (!name)
> +      throw(" 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(" ") instead
Looks like there's a lot of these here...
Comment 8 User image alta88 2012-04-18 08:04:34 PDT
Created attachment 616145 [details] [diff] [review]

address comments.  file-utils.js isn't included in this, it will go away/be restructured separately.
Comment 9 User image alta88 2012-04-26 08:40:40 PDT
Created attachment 618664 [details] [diff] [review]
updated patch.

updated getNodeValue func: 1) textContent.trim() 2) this.getNodeValue
Comment 10 User image David :Bienvenu 2012-04-30 12:08:43 PDT
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 User image alta88 2012-04-30 12:32:07 PDT
Created attachment 619641 [details] [diff] [review]
address comments.
Comment 12 User image alta88 2012-04-30 12:33:45 PDT
Created attachment 619642 [details] [diff] [review]
address all comments.
Comment 13 User image Ian Neal 2012-04-30 12:58:12 PDT
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 User image 2012-04-30 16:23:31 PDT
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 User image alta88 2012-04-30 20:07:37 PDT
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 User image 2012-05-01 00:50:31 PDT
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 User image alta88 2012-05-01 06:20:13 PDT
Comment 18 User image Ryan VanderMeulen [:RyanVM] 2012-05-01 16:46:33 PDT
This is pretty badly bitrotted. Please submit an updated patch.
Comment 19 User image Ryan VanderMeulen [:RyanVM] 2012-05-01 16:47:52 PDT
Whoops, wrong bug. Ignore comment 18.
Comment 20 User image Ryan VanderMeulen [:RyanVM] 2012-05-01 16:53:58 PDT
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)
patch failed, rejects left in working dir
Comment 21 User image alta88 2012-05-01 17:53:29 PDT
Created attachment 620154 [details] [diff] [review]
unbitrotted patch for checkin.
Comment 22 User image Ryan VanderMeulen [:RyanVM] 2012-05-01 17:58:44 PDT
Comment 23 User image alta88 2012-05-02 11:05:43 PDT
Comment on attachment 620154 [details] [diff] [review]
unbitrotted patch for checkin.

[Approval Request Comment]

see bug 747657 comment 11.
Comment 24 User image Mark Banner (:standard8) 2012-06-12 03:09:15 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.