Closed
Bug 552025
Opened 14 years ago
Closed 10 years ago
Replace nsINavHistoryResultObserver.containerOpened and containerClosed with containerStateChanged
Categories
(Toolkit :: Places, defect)
Toolkit
Places
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)
5.25 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•14 years ago
|
||
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.
Comment 2•11 years ago
|
||
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
Updated•10 years ago
|
Assignee: mak77 → nobody
Whiteboard: [good first bug][mentor=mak][lang=js]
Assignee | ||
Comment 3•10 years ago
|
||
Is this bug still open ? Since I have returned after a break, I prefer a simple bug to start with.
Comment 4•10 years ago
|
||
yes, this bug is still open, see comment 2 for the points to code points that should be fixed... should be quite trivial.
Assignee | ||
Comment 5•10 years ago
|
||
seeing the occurences, >>nsINavHistoryResultObserver.containerOpened/Closed are mere comments. So Whats the point in replacing those ?
Comment 6•10 years ago
|
||
(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 | ||
Comment 7•10 years ago
|
||
Assignee: nobody → amod.narvekar
Attachment #8407671 -
Flags: feedback?(mak77)
Comment 8•10 years ago
|
||
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)
Comment 9•10 years ago
|
||
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
Assignee | ||
Comment 10•10 years ago
|
||
Thanks Marco for guiding me through it.
Attachment #8407671 -
Attachment is obsolete: true
Attachment #8408035 -
Flags: review?(mak77)
Assignee | ||
Comment 11•10 years ago
|
||
I removed the trailing spaces which were demonstrated by red link in splinter review.
Comment 12•10 years ago
|
||
I think you may have attached again the previous version of the patch?
Comment 13•10 years ago
|
||
also, please remember the instances in head_common.js...
Assignee | ||
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 16•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b90918a83e15
Flags: in-testsuite+
Keywords: checkin-needed
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b90918a83e15
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•