Closed Bug 552025 Opened 10 years ago Closed 6 years ago
INav History Result Observer .container Opened and container Closed with container State Changed
Bug 536893 will add nsINavHistoryResultObserver.containerStateChanged(), which is intended to replace containerOpened and containerClosed (and maybe invalidateContainer?). Replacing all occurrences of containerOpened and containerClosed would increase the size of the patch in that bug, so let's spin that work out into this bug.
You cannot replace invalidateContainer with the new method. invalidateContainer is used for refreshing opened containers. However, the only case I know of is sortingChanged, thus you might be able to merge those.
looks like there is only a bit of cleanup left to do here, so taking just to do that. http://mxr.mozilla.org/mozilla-central/search?string=container%28Closed|Opened%29®exp=1&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central regarding invalidateContainer, it should be filed apart imo.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee: mak77 → nobody
Whiteboard: [good first bug][mentor=mak][lang=js]
Is this bug still open ? Since I have returned after a break, I prefer a simple bug to start with.
yes, this bug is still open, see comment 2 for the points to code points that should be fixed... should be quite trivial.
seeing the occurences, >>nsINavHistoryResultObserver.containerOpened/Closed are mere comments. So Whats the point in replacing those ?
(In reply to Amod [:greatwarrior] from comment #5) > seeing the occurences, >>nsINavHistoryResultObserver.containerOpened/Closed > are mere comments. So Whats the point in replacing those ? merely code coherence, we should not point to dead APIs in comments. those comments should just be removed. as well as the lines in head file... looks like the only thing to do here is to remove the found lines...
Assignee: nobody → amod.narvekar
Comment on attachment 8407671 [details] [diff] [review] patchv1 Review of attachment 8407671 [details] [diff] [review]: ----------------------------------------------------------------- you should also remove the instances in head_common.js see the search link in comment 2 ::: toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js @@ +145,5 @@ > do_check_true(resultObserver.inBatchMode); > } > }, null); > do_check_false(resultObserver.inBatchMode); > + trailing spaces @@ +212,5 @@ > do_check_true(resultObserver.inBatchMode); > } > }, null); > do_check_false(resultObserver.inBatchMode); > + trailing space @@ +228,5 @@ > var result = histsvc.executeQuery(query, options); > result.addObserver(resultObserver, false); > var root = result.root; > root.containerOpen = true; > + trailing space
2 very effective ways to avoid trailing spaces: 1. use a text editor with support to remove trailing spaces ONLY on lines you touched (for example Sublime Text does that, but I'm sure others do as well) 2. after attaching a patch, take a look at your patch through the review link, it will open Splinter (the review tool) that can hilight the trailing spaces making them more noticeable
Thanks Marco for guiding me through it.
I removed the trailing spaces which were demonstrated by red link in splinter review.
I think you may have attached again the previous version of the patch?
also, please remember the instances in head_common.js...
I am really very sorry. I was working on another bug and I forgot to pop the patch from the series. So the changes went into that patch.
Comment on attachment 8408328 [details] [diff] [review] patchv2 Review of attachment 8408328 [details] [diff] [review]: ----------------------------------------------------------------- no worries, that happened so many times to me!
Attachment #8408328 - Flags: review?(mak77) → review+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.