Closed Bug 552025 Opened 10 years ago Closed 6 years ago

Replace nsINavHistoryResultObserver.containerOpened and containerClosed with containerStateChanged

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: adw, Assigned: AMoz)

References

Details

(Whiteboard: [good first bug][mentor=mak][lang=js])

Attachments

(1 file, 2 obsolete files)

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&regexp=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...
Attached patch patchv1 (obsolete) — Splinter Review
Assignee: nobody → amod.narvekar
Attachment #8407671 - Flags: feedback?(mak77)
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
Attachment #8407671 - Flags: feedback?(mak77)
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
Attached patch patchv2 (obsolete) — Splinter Review
Thanks Marco for guiding me through it.
Attachment #8407671 - Attachment is obsolete: true
Attachment #8408035 - Flags: review?(mak77)
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...
Attached patch patchv2Splinter Review
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.
Attachment #8408035 - Attachment is obsolete: true
Attachment #8408035 - Flags: review?(mak77)
Attachment #8408328 - Flags: review?(mak77)
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b90918a83e15
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.